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:**
```go
// 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.
```go
iter, _ := db.NewIter(nil)
defer iter.Close()  // Required
```

### 2. Concurrency Bugs

**One iterator per goroutine:**
```go
// 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:**
```go
// 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:**
```go
// 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:**
```go
// 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:
```go
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:
```go
// 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
```go
// WRONG
return fmt.Errorf("failed: %v", err)

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

### Atomic Operations
```go
// WRONG - alignment issues, not type-safe
var count uint64
atomic.StoreUint64(&count, 10)

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

### Finalizers
```go
// WRONG
runtime.SetFinalizer(obj, cleanup)

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

### OS Error Checks
```go
// 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:
```go
// 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:
```go
if invariants.Enabled {
    if unexpectedCondition {
        panic("invariant violated: ...")
    }
}
```

## CockroachDB Conventions

### Error Wrapping
Always add context when wrapping:
```go
// 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:
```go
func (k Key) SafeFormat(w redact.SafePrinter, _ rune) {
    w.Print(redact.SafeString(k.String()))
}
```

### TODO Attribution
Always attribute TODOs:
```go
// 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
```