Pebble Code Review

Name: code-review
Rating: 92
Author: cockroachdb

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:

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:

code

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