cockroachdb/code-review

Pebble Code Review

Use this skill when asked to review code, a PR, diff, or changes in the Pebble codebase.

Priority: Critical Correctness Issues

These issues cause bugs, crashes, or data corruption. Flag immediately.

1. Resource Leaks

Get() returns a closer that MUST be closed:

// WRONG - memory leak
val, _, err := db.Get(key)

// CORRECT
val, closer, err := db.Get(key)
if err != nil {
    return err
}
defer closer.Close()

Iterators must be closed: This refers to database iterators or internal key-value iterators.

iter, _ := db.NewIter(nil)
defer iter.Close()  // Required

2. Concurrency Bugs

One iterator per goroutine:

// WRONG - race condition
iter := db.NewIter(nil)
go func() { iter.Next() }()  // Goroutine 1
iter.First()                  // Goroutine 2

// CORRECT - separate iterators
go func() {
    iter := db.NewIter(nil)
    defer iter.Close()
    // use exclusively
}()

Lock release on panic:

// WRONG - deadlock if panic
mu.Lock()
doWork()
mu.Unlock()

// CORRECT
mu.Lock()
defer mu.Unlock()
doWork()

3. Iterator Misuse

This section refers to key-value iterators (including base.InternalIterator), not to Go loop iterators.

Must position before accessing Key/Value:

// WRONG - undefined behavior
iter := db.NewIter(nil)
key := iter.Key()  // Not positioned!

// CORRECT
iter := db.NewIter(nil)
if iter.First() {
    key := iter.Key()
}

Must check Error() after iteration:

// WRONG - silent failures
for iter.First(); iter.Valid(); iter.Next() {
    process(iter.Key())
}
// What if I/O error occurred?

// CORRECT
for iter.First(); iter.Valid(); iter.Next() {
    process(iter.Key())
}
if err := iter.Error(); err != nil {
    return err
}

4. Double-Close Panics

Many types panic on double-close. Use invariants.CloseChecker for new types:

type MyResource struct {
    closeCheck invariants.CloseChecker
}

func (r *MyResource) Close() error {
    r.closeCheck.Close()  // Panics on double-close in invariant builds
    return r.underlying.Close()
}

5. Bounds Checking

Use invariants package for safety:

// WRONG in hot paths
if i >= len(slice) {
    panic("out of bounds")
}

// CORRECT - only panics in invariant builds
invariants.CheckBounds(i, len(slice))

Priority: Required Patterns (Lint-Enforced)

These are caught by go test -tags invariants ./internal/lint but flag them in review.

Error Handling

// WRONG
return fmt.Errorf("failed: %v", err)

// CORRECT - preserves stack traces
return errors.Errorf("failed: %v", err)
return errors.Wrap(err, "context")

Atomic Operations

// WRONG - alignment issues, not type-safe
var count uint64
atomic.StoreUint64(&count, 10)

// CORRECT
var count atomic.Uint64
count.Store(10)

Finalizers

// WRONG
runtime.SetFinalizer(obj, cleanup)

// CORRECT - controlled, testable
invariants.SetFinalizer(obj, cleanup)

OS Error Checks

// WRONG
if os.IsNotExist(err) { ... }

// CORRECT
if oserror.IsNotExist(err) { ... }

Forbidden Imports

• errors → use github.com/cockroachdb/errors
• pkg/errors → use github.com/cockroachdb/errors
• golang.org/x/exp/slices → use slices (builtin)
• golang.org/x/exp/rand → use math/rand/v2

Priority: Corruption Prevention

Corruption Errors

Mark errors appropriately so they can be detected:

// For data corruption
return base.CorruptionErrorf("invalid key: %s", key)

// To check
if base.IsCorruptionError(err) {
    // Handle corruption
}

Invariant Checks

Use invariants for assertions that should hold:

if invariants.Enabled {
    if unexpectedCondition {
        panic("invariant violated: ...")
    }
}

CockroachDB Conventions

Error Wrapping

Always add context when wrapping:

// WRONG
return err

// CORRECT
return errors.Wrap(err, "loading manifest")
return errors.Wrapf(err, "reading block %d", blockNum)

Redact Safety

Use redact.SafeFormat for types that may contain user data:

func (k Key) SafeFormat(w redact.SafePrinter, _ rune) {
    w.Print(redact.SafeString(k.String()))
}

TODO Attribution

Always attribute TODOs:

// TODO(username): description of future work

Review Checklist

When reviewing, verify in order:

1. Resource Management

   • [ ] All Get() callers close the returned closer
   • [ ] All iterators are closed
   • [ ] No double-close potential

2. Concurrency

   • [ ] Iterators not shared across goroutines
   • [ ] Locks released via defer
   • [ ] Atomic types used (not raw atomic ops)

3. Error Handling

   • [ ] Errors not silently ignored
   • [ ] iter.Error() checked after loops
   • [ ] Using errors.Errorf not fmt.Errorf

4. Safety

   • [ ] No potential nil dereferences
   • [ ] Bounds checked where needed
   • [ ] Corruption marked appropriately

5. Lint Compliance

   • [ ] No forbidden imports
   • [ ] oserror not os.Is*
   • [ ] invariants.SetFinalizer not runtime.SetFinalizer

Commit Message Format

When reviewing commit messages:

• Subject: package: short description (lowercase after colon)
• Body: Explain "what existed before" and "why"
• No test plan unless explicitly requested

Good examples:

bloom: improve simulation output
db: add progressive bloom filter policy
internal/manifest: rename LevelFile to LevelTable