Fix two engine deadlocks, and make the next one loud - #1088
Conversation
Both are reachable on main without unusual concurrency. writeMu is not reentrant, so calling an exported write from inside another write's body wedges that goroutine permanently: one goroutine, no output, no stack. Nothing at the call site says so, and the engine offers no way to spell "these two writes go together", so a contributor reaching for atomicity reaches for the exported method. lockWriteBarrier now records the holding goroutine under `go test` and panics on re-entry. Close and CheckpointTo drain writeWG one step before the mutex, where that check cannot see them, so they ask explicitly. CurrentSyncStep took lifecycleMu, which EndSync holds across a finalize whose steps take the write barrier. Any write whose body read its own progress therefore took writeMu then lifecycleMu while EndSync took them in the opposite order, and the pair hung — from a method that reads like a plain getter. It now reads the binding, reads the record, then re-reads a binding generation to confirm nothing moved underneath it, so the lock order has one edge instead of a cycle. Regression tests drive both interleavings; each was confirmed to hang without its fix. Two meta-tests keep the invariants from regrowing: the lifecycleMu takers stay the five sync-lifecycle transitions, and every path that locks writeMu or drains writeWG goes through the ownership check. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const writers = 8 | ||
| errs := make(chan error, writers) | ||
| for i := 0; i < writers; i++ { | ||
| go func() { errs <- e.CheckpointSync(ctx, "concurrent") }() | ||
| } | ||
| for i := 0; i < writers; i++ { | ||
| require.NoError(t, <-errs) |
There was a problem hiding this comment.
🟡 Suggestion: the 8 goroutines can never contend for the barrier, so the stated purpose ("two writers contending for the barrier hand ownership back and forth ... where a bookkeeping bug would report the next holder as re-entrant") is not exercised. CheckpointSync holds lifecycleMu for its whole body (adapter.go:259-273) and only reaches lockWriteBarrier inside PutSyncRunRecord, so writeMu is acquired by at most one goroutine at a time here. Use a write path that takes the barrier without lifecycleMu — e.g. concurrent PutGrants/PutResources — if you want the handoff covered. Confidence: high.
General PR Review: Fix two engine deadlocks, and make the next one loudBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the Risk triage (per Security IssuesNone found. Correctness IssuesNone found (no blocking issues; see Suggestions). Suggestions
Prompt for AI agents |
Reads took no part in the engine's close barrier. Writers have the closing flag, writeWG participation, and a re-check after Add; readers had at most a bare `e.db == nil` check. A read in flight when Close ran panicked with "pebble: closed", and a retained handle used after Close nil-dereferenced rather than returning the ErrEngineClosing the Engine doc promises for every method. pinRead pins the handle for the duration of one read, and Close now drains readWG alongside writeWG. The WaitGroup is what does the work: it both keeps the teardown from running under a live read and orders the read's view of e.db against Close's write to it. Applied to the 30 self-contained read methods -- the paginate and iterate families, the invariant scans, and the read predicates -- each of which opens an iterator and closes it before returning, so a call-scoped pin covers the whole read. The merge surface is excluded on purpose: NewIter and Get hand a live iterator and closer back to the caller, so a pin that releases at return would protect nothing, and listing them would let a useless pin satisfy the check. Three tests, all of which failed before the change: the documented after-close contract across 26 entry points, a Close-vs-read hammer over both read shapes, and an AST check that a read method cannot skip the pin or reach for e.db directly. That last one earns its place -- it caught six functions missing from a hand-written enumeration on its first run. Also deletes TestC1ZConcurrentClose. It asserted a concurrent-close contract the SQLite path never implemented, reported the resulting race nondeterministically, and cost 175s of every nightly dotc1z shard. SQLite is in maintenance mode and the engine now holds that contract where it matters. Co-authored-by: Cursor <cursoragent@cursor.com>
| if err != nil { | ||
| if errors.Is(err, pebble.ErrNotFound) { | ||
| return "", nil | ||
| } | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the ErrNotFound branch returns without the generation re-check the success path does, so it doesn't uphold the doc's "the value still belongs to a sync that was bound at a real instant" claim. startNewSync binds via MarkFreshSync (gen++) before PutSyncRunRecord, so a reader that sampled the previous binding can read the record after the swap, see a different sync_id, and get pebble.ErrNotFound from GetSyncRunRecord's id-mismatch check (sync_runs.go:63) — reporting "no step" for a sync that was never unbound. The locked version could not observe that. Re-check currentSyncBinding() before returning on not-found and retry if the generation moved. (Medium confidence on practical reach — the single-record layout makes the window narrow — but the asymmetry is real.)
| // rawdb.DB.NewIter on a nil receiver instead — the paginate methods had | ||
| // no guard, and the Iterate family had neither a guard nor the nil check | ||
| // the invariant-scan surface carried | ||
| // (TestIngestScanSurfaceAfterCloseReturnsClosing covers that one). |
There was a problem hiding this comment.
🟡 Suggestion: TestIngestScanSurfaceAfterCloseReturnsClosing does not exist anywhere in the repo, so this comment asserts coverage that isn't there. The ingest-scan surface changed in this PR (ingest_facts.go, ingest_repair.go swapped their if e.db == nil guards for pinRead) and ForEachDistinct*, ForEachDanglingGrant*, HasResourceRecord, and GrantsFor*CarryInsertFact have no after-Close assertion — those methods used to only guard against a nil handle and now also refuse on the closing flag, which is a behavior change worth pinning. Same drift at engine.go:715, where the pinRead doc names TestPaginateReadsArePinned (actual name: TestScanReadsArePinned).
…silently Sync joins ErrSyncNotComplete onto whatever error it returns once the deadline has fired, and handleOperationError joins it onto a failed checkpoint's error too, so require.ErrorIs(err, ErrSyncNotComplete) passed whether or not the checkpoint on expiry was actually written. Assert no context error survives in the chain instead: handleOperationError drops the batch error, and these tests keep the caller's context live, so a context error can only come from a checkpoint that ran under a dead one. Verified by rewiring the checkpoint to the expired runCtx, which the new assertion catches in both tests and the old one did not. Co-authored-by: Cursor <cursoragent@cursor.com>
Follow-ups on the deadlock work, each verified against the code first and each with a mutation showing the test earns its place: - assertNotWaitingOnOwnWrite only recognized goroutines that took the barrier, but CompactAllRanges and Flush join writeWG without it, and it is writeWG membership — not barrier ownership — that hangs Close. Track membership. Under the old check, a Close from that state hangs forever instead of panicking. - ResumeSync and SetCurrentSync reach lifecycleMu without ever touching writeMu, so the re-entrancy check that the meta-test's comment credited for making them unreachable from a write body cannot fire. They assert directly now, and the meta-test pins which takers rely on which. - CurrentSyncStep's seqlock retry had no coverage: the existing test is sequential, so every call returned on the first pass. A test seam moves the binding inside the read window, plus a -race soak. Gutting the retry now fails a test; it did not before. - The ownership checks are gated on testing.Testing(), true in a benchmark binary too, so the write benchmarks were reporting a runtime stack format (~2us against a ~7us grant write) and only on the Pebble side, skewing the SQLite comparisons in the same file. - lockWriteBarrier returned its release, which is a heap allocation on every production write. Paired lock/unlock instead, back to 28 allocs/op. Also fixes the barrier admission test's concurrent phase, which used CheckpointSync: it holds lifecycleMu across its whole body, so the writers reached the barrier one at a time and never contended. Co-authored-by: Cursor <cursoragent@cursor.com>
| func requireCheckpointedOnExpiry(t *testing.T, err error) { | ||
| t.Helper() | ||
| require.NotErrorIs(t, err, context.DeadlineExceeded, | ||
| "run-duration expiry checkpoint failed against a dead context; the sync is not resumable") | ||
| require.NotErrorIs(t, err, context.Canceled, | ||
| "run-duration expiry checkpoint failed against a cancelled context; the sync is not resumable") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this is a negative oracle, so it only catches the context-flavored subset of checkpoint failures. handleOperationError joins checkpointErr unconditionally, so a checkpoint that fails for a non-context reason (store already closed, engine sealed, disk error) still leaves err free of DeadlineExceeded/Canceled and this passes — while the message claims "the sync is not resumable" has been ruled out. Consider asserting the joined tree contains nothing but ErrSyncNotComplete, or reopening the c1z and asserting the persisted sync token actually advanced, so the helper matches its name. (medium confidence)
| // IterateGrants iterates all grants in primary-key order. yield returns | ||
| // false to stop iteration. | ||
| func (e *Engine) IterateGrants(ctx context.Context, yield func(*v3.GrantRecord) bool) error { | ||
| db, release, err := e.pinRead() |
There was a problem hiding this comment.
🟡 Suggestion: the pin makes Close wait for this scan, but the loop below (line 1030) never checks ctx.Err() — unlike the Paginate* loops, which check it per iteration. A full-keyspace IterateGrants at whale scale, or a yield callback that blocks on IO, now pins Close for an unbounded and non-cancellable duration where it previously raced ahead. Same for the other Iterate*/ForEach* scans. Adding the per-iteration ctx.Err() check the paginate family already has would bound it.
Second review round on the deadlock work. Each finding verified against the code first, each fix carrying a mutation that shows the test earns its place. - Close drains readWG but only writeWG membership was tracked, so a Close reached from inside a pinned read hung with no output — the failure mode the check exists to report. Iterate* and ForEach* hold the pin across a caller-supplied callback, which is how you get there. readWG participation is now tracked the same way, Close asserts on it, and the meta-test pins readWG.Add/Done to the enter/exit pair. - The ownership assertions sat behind closeMu, so they missed the overlap they most needed to catch: one Close parked in the drain, and the goroutine holding the work it waits for calling Close behind it. That caller blocked on closeMu and never reached the diagnostic. They run before the lock now. - pinRead and withWriteAllowSealed checked the closing flag and then joined the WaitGroup as two steps. Re-checking after the Add narrows the window rather than closing it: an Add that lands after Close has parked at a zero counter is "WaitGroup misuse: Add called concurrently with Wait", a panic instead of the ErrEngineClosing the re-check was reaching for. Admission and the flip are now mutually exclusive. - The Iterate* scans never checked ctx.Err(), which the paginate family does per iteration. Once the pin makes Close wait for a scan, a full-keyspace read holds the teardown open for as long as it takes. The pin meta-test now requires the check in any iterator loop, keyed on the loop rather than the method so a bounded page walk over an already-read page does not have to carry one. - CurrentSyncStep's not-found branch returned without the generation re-check its hit path does. startNewSync bumps the generation before writing the record, so a reader that sampled the old binding could be told "no such sync" about a sync that never unbound. Also strengthens requireCheckpointedOnExpiry from the previous commit: it ruled out context-flavored checkpoint failures only, and a checkpoint that failed on a sealed engine or a disk error passed it while claiming resumability had been proven. It now requires every leaf of the joined tree to be ErrSyncNotComplete, which is the same claim for any cause. No allocation change on the write path (28 allocs/op). Co-authored-by: Cursor <cursoragent@cursor.com>
| rec, err := e.GetSyncRunRecord(ctx, syncID) | ||
| if err != nil { | ||
| if errors.Is(err, pebble.ErrNotFound) { | ||
| for { |
There was a problem hiding this comment.
🟡 Suggestion: the retry loop has no ctx.Err() check and no bound, so a caller that cancelled cannot call it off — the same gap the previous round just closed in every Iterate* scan loop. Each pass costs a Pebble Get, so a steady stream of SetCurrentSync/clearCurrentSync (each bumps currentSyncGen) keeps a reader looping with cancellation ignored. A if err := ctx.Err(); err != nil { return "", err } at the top of the loop makes the termination argument in the comment below a guarantee rather than a statistical one.
| if e.test.currentSyncStepPreReadHook != nil { | ||
| e.test.currentSyncStepPreReadHook() | ||
| } | ||
| rec, err := e.GetSyncRunRecord(ctx, syncID) |
There was a problem hiding this comment.
🟡 Suggestion: GetSyncRunRecord (sync_runs.go:48) reads e.db.Get(...) with neither a pin nor a nil guard, so CurrentSyncStep after Close nil-dereferences instead of returning ErrEngineClosing — the exact contract the new TestReadSurfaceAfterCloseReturnsClosing pins for the 26 scan entry points. It survives here because pinnedReadPrefixes is Paginate/Iterate/ForEach, and this is the sync-run path. Since this PR rewrote CurrentSyncStep, pinning the record read (or at minimum guarding it) would close the read-after-close hole on the one read every write body reaches for.
| return nil | ||
| } | ||
| joined, ok := err.(interface{ Unwrap() []error }) | ||
| if !ok { |
There was a problem hiding this comment.
🟡 Suggestion: flattenJoined uses a direct type assertion for Unwrap() []error, so it stops at the first fmt.Errorf("...: %w", joined) wrapper and returns that whole subtree as one leaf. require.ErrorIs(leaf, ErrSyncNotComplete) then matches anywhere in that subtree, silently degrading the new "every leaf is ErrSyncNotComplete" oracle back to the plain errors.Is it was written to replace. Unwrapping single-error Unwrap() before checking the multi-error form would keep the flatten total.
The checks were gated on testing.Testing(), which is also true in benchmark binaries: every barrier acquisition formatted a runtime stack (~2µs against a ~7µs grant write), on the Pebble side only of every Pebble-vs-SQLite comparison, and the per-benchmark opt-out helper covered five call sites and nothing else. A runtime-flippable gate also leaves the participant bookkeeping's consistency at the mercy of when it flips. writeBarrierOwnerChecks is now a constant set by -tags=baton_lockchecks or -race (cmd/go defines the race tag automatically), so unarmed builds carry none of the bookkeeping and armed ones cannot change mid-run. make test and the CI workflows supply the tag; the race-based targets are armed for free, and benchmarks are uninstrumented by default rather than by opt-out. Silent de-arming is the failure mode of any opt-in, so two tripwires: TestLockChecksCompiledIn fails any test run whose binary was built unarmed, and TestLockChecksSuppliedByTestInvocations fails when a whole-tree go test invocation in the Makefile or a workflow stops supplying the tag. Both verified by mutation. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| workflowHits := 0 | ||
| for path, n := range wholeTreeInvocations { | ||
| if strings.HasPrefix(path, ".github/") { |
There was a problem hiding this comment.
🟠 Bug: filepath.Rel returns OS-native separators, so on Windows rel is .github\workflows\ci.yaml and this HasPrefix(path, ".github/") never matches. workflowHits stays 0 and the floor assertion below fatals — the windows-latest job in ci.yaml/main.yaml runs the full suite (only -short, which this test doesn't honor), so this fails CI on every PR. Use filepath.ToSlash(rel) when storing the key (or compare against filepath.Join(".github", "")).
| rel, _ := filepath.Rel(root, path) | ||
| wholeTreeInvocations[rel]++ | ||
| if !strings.Contains(line, "baton_lockchecks") && !race.MatchString(line) { | ||
| violations = append(violations, rel+":"+strings.TrimSpace(line)) |
There was a problem hiding this comment.
🟡 Suggestion: lineNo is tracked but never read — the violation message reports rel:<trimmed line> without it. Either fold it in (fmt.Sprintf("%s:%d: %s", rel, lineNo, strings.TrimSpace(line))), which makes the failure directly navigable, or drop the counter.
| // `make test` and the CI workflows supply; TestLockChecksCompiledIn and | ||
| // TestLockChecksSuppliedByTestInvocations exist to make forgetting that | ||
| // loud. | ||
| const writeBarrierOwnerChecks = true |
There was a problem hiding this comment.
🟡 Suggestion: .golangci.yml sets run.build-tags: [baton_lambda_support] only, so with the checks now behind baton_lockchecks || race this file and the newly tag-gated write_barrier_reentry_test.go are invisible to every linter — the previous testing.Testing() gate kept them in the lint build. Adding baton_lockchecks to run.build-tags restores that coverage (the disabled variant still compiles in make build/baton-demo-test).
| @@ -1,163 +0,0 @@ | |||
| package dotc1z | |||
There was a problem hiding this comment.
🟡 Suggestion: TestC1ZConcurrentClose is deleted with no replacement and no mention in the PR description. It covered the SQLite C1File path — concurrent Close/PutGrants converging on ErrDbNotOpen, plus the WAL file being drained to empty on close. The new TestConcurrentCloseWithPaginatedReads covers the Pebble engine's read side, not either of those properties. If the deletion is deliberate (flaky, superseded), say so in the PR body; otherwise this is a coverage regression on the still-default engine.
The open/close path had grown nine pieces of synchronization state on the Engine struct (closing, closeMu, admitMu, two WaitGroups, two participant sets, plus the assertions over them), with the invariants holding them together living in comments and AST enumerations. All of it implements one concept — operations enter, Close shuts the gate and drains everyone inside — so it now lives behind one admission type with five entry methods, unit-tested directly with no Engine or DB behind it. The meta-test shrinks to what AST checks are good at: nobody reaches past the methods, and the two drains keep exactly one caller each. Consolidating also fixed a latent crash: CompactAllRanges and Flush still joined the write group with the bare Add-then-check pattern, outside the admission lock that closes the check-then-Add window, so either racing a draining Close could still die with sync.WaitGroup's "Add called concurrently with Wait" fatal. Both now enter through the gate. Verified by mutation: stripping the admission lock fails the new hammer test under -race in milliseconds, and an engine-side read of gate internals fails the confinement meta-test naming the line.
| if self := trackedGoroutineID(); self != 0 && a.writerIDs.holds(self) { | ||
| panic(writeBarrierWaitFromWritePanic) | ||
| } | ||
| a.writers.Wait() |
There was a problem hiding this comment.
🟡 Suggestion: drainWrites waits without the atomicity the type comment says is load-bearing. closeAndDrain is safe because the flip under mu.Lock means no writers.Add can follow it; drainWrites never flips, so a concurrent enterWrite can Add(1) while this Wait is registered with the counter dropping to zero — the exact sync: WaitGroup misuse: Add called concurrently with Wait fatal the type comment cites, on the one path (CheckpointTo → save) whose whole purpose is quiescing writes that may still be arriving. The shape is inherited from the old writeWG.Wait(), but the gate's contract now claims to cover it. Consider a generation/epoch or a mu.Lock-held counter snapshot so the drain observes a state no Add can straddle. (confidence: high on the asymmetry, medium on how often the interleaving is hit)
| // this to turn the same mistake into a panic instead of a hang. | ||
| func (e *Engine) assertNotTakingLifecycleFromWrite() { | ||
| if !writeBarrierOwnerChecks { | ||
| return |
There was a problem hiding this comment.
🟡 Suggestion: "they take the barrier themselves, so lockWriteBarrier's re-entrancy check fires first" only holds when the lifecycleMu acquisition doesn't block. CheckpointSync (adapter.go:275), startNewSync (adapter.go:99) and EndSync (adapter.go:298) take lifecycleMu before reaching PutSyncRunRecord/the barrier — so if a goroutine calls one from inside a write body while another goroutine's EndSync already holds lifecycleMu and is waiting on writeMu, this one parks on lifecycleMu and never reaches lockWriteBarrier. That is precisely the ABBA deadlock, and it hangs silently rather than panicking; the sequential regression tests pass because the lock is free there. Calling assertNotTakingLifecycleFromWrite at the top of all five takers (and widening wantGuarded in lifecycle_lock_meta_test.go:47, which currently forbids it) would close the gap. (confidence: high on the mechanism)
| // release while the caller still holds the handle, so they need the | ||
| // release tied to the returned object instead, and listing them here | ||
| // would let a useless pin satisfy the check. | ||
| var pinnedReadPrefixes = []string{"Paginate", "Iterate", "ForEach"} |
There was a problem hiding this comment.
🟡 Suggestion: the point-read surface is left out of the "After Close, all methods return ErrEngineClosing" contract this file pins. GetSyncRunRecord (sync_runs.go:49), GetResourceRecord (resources.go:115), GetEntitlementRecordByIdentity (entitlements.go:98), GetAssetRecord (assets.go:31) and friends still do a bare e.db.Get(...) with no nil guard, so they nil-deref inside rawdb.DB.Get after Close rather than returning the error. CurrentSyncStep — rewritten in this PR — reaches one of them whenever Close happens without a preceding EndSync (the binding is still set), which turns a documented error into a panic. Worth either pinning Get* here too or giving those a cheap pinRead/nil check. (confidence: high that the deref is reachable, medium that it matters in-tree today)
| // Deliberately "any iterator loop" rather than "every" one: these shapes | ||
| // nest, an outer index walk feeding an inner primary-key fetch, and one | ||
| // check per scan is what bounds the pin. | ||
| func scanLoopCancellation(fn *ast.FuncDecl) (bool, bool) { |
There was a problem hiding this comment.
🟡 Suggestion: the loop detector keys on a for whose condition calls Valid(), but the package's seek-skip scans are written for valid := iter.First(); valid; { ... valid = iter.SeekGE(...) } — ingest_facts.go:91/140/192 and ingest_repair.go:113. Those are full-keyspace scans holding the pin, and hasLoop is false for every one of them, so the cancellation requirement silently doesn't apply. They all happen to check ctx.Err() today, so nothing is broken now, but the next scan written in that shape passes the fence with an unbounded pin holding Close open. Also note callsMethod(loop.Body, "Err") matches any .Err() receiver, not specifically ctx. (confidence: high)
The gate itself: counters and a condition variable replace the WaitGroups, so draining tolerates concurrent enter attempts (the Add-vs-Wait misuse is unexpressible), and all five lifecycle transitions run as admitted writes with their state validated under lifecycleMu (ResumeSync's check-then-bind race is gone). The read surface: every point read pins, and the resolve/digest/repair helper chains take the admitted handle instead of re-reading e.db, so one admission covers a whole operation. The racy e.db==nil pseudo- checks are deleted; the merge surface's exclusion from the gate is now documented where it lives. Enforcement now keys on the field access itself: a new meta-test requires every e.db touch to sit inside withWrite or a justified allowlist, pinRead releases must be deferred, seek-driven iterator loops join the ctx-check rule, and the correctness-focused Make targets compile the lock checks in. Verification packet under docs/verification/engine-close-gate/. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Both are reachable on main without unusual concurrency.
writeMu is not reentrant, so calling an exported write from inside another write's body wedges that goroutine permanently: one goroutine, no output, no stack. Nothing at the call site says so, and the engine offers no way to spell "these two writes go together", so a contributor reaching for atomicity reaches for the exported method. lockWriteBarrier now records the holding goroutine under
go testand panics on re-entry. Close and CheckpointTo drain writeWG one step before the mutex, where that check cannot see them, so they ask explicitly.CurrentSyncStep took lifecycleMu, which EndSync holds across a finalize whose steps take the write barrier. Any write whose body read its own progress therefore took writeMu then lifecycleMu while EndSync took them in the opposite order, and the pair hung — from a method that reads like a plain getter. It now reads the binding, reads the record, then re-reads a binding generation to confirm nothing moved underneath it, so the lock order has one edge instead of a cycle.
Regression tests drive both interleavings; each was confirmed to hang without its fix. Two meta-tests keep the invariants from regrowing: the lifecycleMu takers stay the five sync-lifecycle transitions, and every path that locks writeMu or drains writeWG goes through the ownership check.