structrually remove deadlock potential - #1058
Conversation
General PR Review: structrually remove deadlock potentialBlocking Issues: 0 | Suggestions: 7 | Threads Resolved: 0 Review SummaryThe full PR diff (84 files) was re-scanned for security and correctness; no security issues and no confident correctness bugs were found. The single new commit ( Risk triage (
Security IssuesNone found. Correctness IssuesNone found. The prior high-confidence exported-API break is repeated under Suggestions since it is a compatibility, not a runtime, defect. Suggestions
Prompt for AI agents |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if s.admission == pebbleStoreClosing { | ||
| attempt := s.closeAttempt | ||
| for s.admission == pebbleStoreClosing && s.closeAttempt == attempt { | ||
| s.cond().Wait() | ||
| } | ||
| err := s.lastCloseErr | ||
| s.closeMu.Unlock() | ||
| return err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): a waiter here inherits lastCloseErr from whatever operation held Closing, even when that operation failed and reset admission back to Open — so the waiter returns an error without ever performing its own close. Because Close and CloseEngineOnly share lastCloseErr, a Close that waits behind a failed CloseEngineOnly returns "pebble CloseEngineOnly: refusing to discard dirty writable store" and leaves the store open and unsaved, even though a real save would have succeeded. Consider having waiters re-loop and re-attempt their own close when admission returns to pebbleStoreOpen, rather than adopting a different operation's verdict.
| if os.Getenv("BATON_FULL_TESTS") == "" { | ||
| t.Skip("scheduler soak runs in make scheduler-soak and make test-full") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: BATON_FULL_TESTS is set only by the test-full, race-check, and scheduler-soak Makefile targets, and none of those run in .github/workflows/ci.yaml (CI invokes go test ... ./... directly plus make chaos-check). So this 6-seed randomized scheduler soak now runs in no automated workflow, where it previously ran on every PR. That's a notable coverage loss to take in the same PR that restructures store close/mutation concurrency. Consider keeping 1–2 seeds unconditional, or wiring a race-check/test-full job into CI.
| // build failure anywhere — it just silently stops the capability from being | ||
| // discovered. See pebble_store_capabilities_test.go, which asserts each one. | ||
|
|
||
| func (s *pebbleStore) CurrentDBSizeBytes() (int64, error) { |
There was a problem hiding this comment.
🟠 Bug: GrantsForEntitlementPrincipalSorted() (engine/pebble/adapter_reader.go:183) was promoted by the old embed and is not forwarded here — exactly the silent-optional-capability failure this file's header warns about. pkg/sync/syncer.go:427-432 discovers it by inline assertion on a.store (the *pebbleStore), so it now returns false for every Pebble sync: Expander.RunSingleStep (expand/expander.go:186) stops selecting RunTopologicalMergeProjection and falls through to the legacy source-batched expander, and newPrincipalGroupStream/buildProjectionDB drop to the buffer-and-sort path. Nothing fails — pebble_expansion_fastpath_test.go calls RunTopologicalMergeProjection directly, bypassing the gate. Add a forwarder here and an assertion in pebble_store_capabilities_test.go.
| func (s *pebbleStore) CurrentDBSizeBytes() (int64, error) { | |
| func (s *pebbleStore) GrantsForEntitlementPrincipalSorted() bool { | |
| return s.Engine.GrantsForEntitlementPrincipalSorted() | |
| } | |
| func (s *pebbleStore) CurrentDBSizeBytes() (int64, error) { |
| var mutatingEnginePrefixes = []string{ | ||
| "Abort", "Add", "Begin", "Checkpoint", "Clear", "Delete", "Drop", "End", | ||
| "Ensure", "Finish", "Ingest", "Mark", "New", "Persist", "Put", "Replace", | ||
| "Reset", "Resume", "Set", "Stash", "Start", "Store", "Unsafe", "Wipe", | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the verb list misses several mutating prefixes the engine actually uses, so those methods are invisible to this guard if a hand-written passthrough ever appears: Flush, CompactAllRanges, Save, InitCurrentSync, BuildGrantDigests / BuildDeferredGrantIndexes, InvalidateGrantDigestPartitions, RepairMissingGrantDigests. TestPebbleStoreEngineIsNotEmbedded covers re-embedding, but a single forwarder for e.g. Flush escapes both tests.
| var mutatingEnginePrefixes = []string{ | |
| "Abort", "Add", "Begin", "Checkpoint", "Clear", "Delete", "Drop", "End", | |
| "Ensure", "Finish", "Ingest", "Mark", "New", "Persist", "Put", "Replace", | |
| "Reset", "Resume", "Set", "Stash", "Start", "Store", "Unsafe", "Wipe", | |
| } | |
| var mutatingEnginePrefixes = []string{ | |
| "Abort", "Add", "Begin", "Build", "Checkpoint", "Clear", "Compact", "Delete", | |
| "Drop", "End", "Ensure", "Finish", "Flush", "Ingest", "Init", "Invalidate", | |
| "Mark", "New", "Persist", "Put", "Replace", "Repair", "Reset", "Resume", | |
| "Save", "Set", "Stash", "Start", "Store", "Unsafe", "Wipe", | |
| } |
| // WithEngineMutation runs a direct engine mutation under the owning store's | ||
| // admission guard. A bare *Engine has no envelope lifecycle to coordinate, so | ||
| // it executes the callback directly and relies on the Engine's own write guard. | ||
| func WithEngineMutation(ctx context.Context, target any, fn func(context.Context, *Engine) error) error { |
There was a problem hiding this comment.
🔴 Security/Compat — 🟠 Bug (high confidence): this replaces two exported functions that shipped in the released SDK. pebble.MarkStoreDirty and pebble.AddFoldDeadBytes both exist at tag v0.21.0 (merge_accessor.go:171,186) and are deleted here with no deprecated shim, while pkg/sdk/version.go still reads v0.21.0 and the PR description doesn't mention the removal. Any out-of-tree caller of github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble fails to compile on upgrade with no migration signal.
Per the repo's SDK criteria (deprecate-before-remove; breaking changes must be reflected in pkg/sdk/version.go and described in the PR), either keep thin // Deprecated: wrappers that delegate to WithEngineMutation / WithEngineFoldMutation for one release, or bump the 0.x minor in pkg/sdk/version.go and call the removal out explicitly in the PR body. Note the new TestPebbleStoreMutationWrapperInventory AST guard forbids the identifier MarkStoreDirty anywhere under pkg/, so a shim would need that guard narrowed to the store's own call sites.
| if ts := rec.GetEndedAt(); ts != nil && ts.AsTime().After(maxEnded) { | ||
| maxEnded = ts.AsTime() | ||
| var newSyncID string | ||
| err = enginepkg.WithEngineFoldMutation(ctx, c.compactedC1z, func(ctx context.Context, destEng *enginepkg.Engine) (int64, error) { |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): wrapping the whole fold in WithEngineFoldMutation moves the dirty flip from after success to before any work. withMutation sets s.dirty = true at admission (pkg/dotc1z/pebble_store.go:276-278), whereas the old code only called MarkStoreDirty on the last line after the provenance write. So every fold failure path now leaves the dest dirty: merge error, digest invalidate/repair error, PutSyncRunRecord error, and — the common one in production — ctx.Err() from the run-duration deadline at compactor_pebble.go:517.
Compact's deferred c.compactedC1z.Close(ctx) (compactor.go:362-370) then runs the full save(): CheckpointTo plus a whole-envelope encode over the entire inherited base keyspace, for an artifact that is never published (cpFile is skipped on the error return). At whale scale — the only scale fold is selected for — that is an O(base) checkpoint+encode charged to every aborted fold, and on the deadline path it runs after the budget already expired, which is exactly what runDuration exists to bound. RunPebbleFoldMutation also accumulates foldDeadBytes when the callback returns an error (pebble_store.go:523-530), so that discarded envelope carries an inflated cumulative counter too.
Suggest restoring the success-only semantics: have the fold callback report dirtiness explicitly (e.g. only bump dirty/foldDeadBytes when mutationErr == nil), or open the fold with an admission mode that does not pre-mark dirty and mark it once the provenance write lands.
|
|
||
| interruptCases := append(parityCases(), cyclicCases()...) | ||
| if testing.Short() { | ||
| if testing.Short() || !fullTestSuite() { |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): this is a coverage reduction on Linux CI, not just a Windows-gating rename. The old condition was testing.Short(), and .github/workflows/ci.yaml:39 runs the Linux job without -short, so Linux PR CI previously swept the full parityCases() + cyclicCases() matrix here (and at :190, and at topological_merge_layer_interrupt_test.go:99). With || !fullTestSuite() it drops to two representative cases everywhere, because no workflow sets BATON_FULL_TESTS — ci.yaml only runs bare go test ./... plus make chaos-check.
Same pattern in three other places in this PR, all previously full on Linux CI:
topological_merge_differential_test.go:479-484—fuzzSeedRangenow returnsshortCountby default instead oflongCount.pkg/sync/checkpoint_cut_test.go:509,512,519— cut points halved 16→8, 16→8, 12→6 unconditionally (no env escape;BATON_CUT_SWEEP=fullstill uncaps, but only in the manual tier).pkg/dotc1z/race_test.go:36-40— WAL checkpoint race 100 → 20 iterations.
That is a real reduction in the interruption/resume permutation coverage this repo's own bug-catching guidance names as the instrument for checkpoint/schedule-dependent risk, taken in the same PR that restructures store close/mutation concurrency. If the runtime cost is the motivation, consider adding a scheduled (nightly/weekly) workflow job that runs make test-full / make race-check so the full tier actually executes somewhere, rather than existing only as a Makefile target.
| func (e *Engine) saveToC1z(ctx context.Context, dest string) error { | ||
| return errors.New("pebble engine: Save requires the dotc1z.Save shim (envelope write); use CheckpointTo for direct directory access") | ||
| } | ||
|
|
||
| // DBDir returns the on-disk path the engine writes to. Exported so | ||
| // databaseDir returns the on-disk path the engine writes to. Exported so | ||
| // the Adapter can implement OutputFilepath / CurrentDBSizeBytes. | ||
| func (e *Engine) DBDir() string { | ||
| func (e *Engine) databaseDir() string { | ||
| return e.dbDir |
There was a problem hiding this comment.
🟠 Bug: This commit unexports ~47 methods on *pebble.Engine (plus deletes 4 and renames Save→saveToC1z, DBDir→databaseDir) in pkg/dotc1z/engine/pebble, a public package already published at v0.21.0. pkg/sdk/version.go is unchanged, there are no // Deprecated: shims, and the PR description does not mention the removal — so any out-of-tree caller that reached the engine (via pebble.AsEngine) breaks at compile time with no signal. Per .claude/skills/ci-review.md, breaking SDK changes need a pkg/sdk/version.go minor bump and a migration note; deprecate-then-remove is the expected shape for symbols that shipped.
Also, databaseDir's comment still reads "Exported so the Adapter can implement…", which is no longer true, and saveToC1z now has no caller outside engine_test.go — worth deleting alongside the other callerless functions this commit removed.
| if !s.readOnly && s.dirty { | ||
| err := errors.New("pebble CloseEngineOnly: refusing to discard dirty writable store") | ||
| s.admission = pebbleStoreOpen | ||
| s.lastCloseErr = err | ||
| s.cond().Broadcast() | ||
| s.closeMu.Unlock() | ||
| return errors.New("pebble CloseEngineOnly: refusing to discard dirty writable store") | ||
| return err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: a failed CloseEngineOnly publishes its refusal into the shared lastCloseErr and reopens admission, so a Close(ctx) already parked in the waiter at :915-922 wakes, sees admission != Closing, and returns "pebble CloseEngineOnly: refusing to discard dirty writable store" — without ever running save. On base the two serialized on closeMu, so the pending Close acquired the lock afterwards and saved normally; here the dirty store is silently left unsaved in tmpDir. Consider giving CloseEngineOnly its own error slot (or not touching lastCloseErr on the refusal path, and having waiters re-run the close instead of inheriting a foreign result). No in-tree caller hits this today — the compactor only calls CloseEngineOnly on read-only sources — hence suggestion rather than blocking.
| @@ -226,7 +227,9 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op | |||
| if err = c.convertGrants(ctx, bi, syncID, cfg, &stats.Grants); err != nil { | |||
| return nil, fmt.Errorf("to-pebble: grants: %w", err) | |||
| } | |||
| if err = bi.Finish(ctx); err != nil { | |||
| if err = pebble.WithEngineMutation(ctx, dest, func(ctx context.Context, _ *pebble.Engine) error { | |||
| return bi.Finish(ctx) | |||
| }); err != nil { | |||
| return nil, fmt.Errorf("to-pebble: ingest: %w", err) | |||
There was a problem hiding this comment.
🟡 Suggestion: only StartBulkSyncImport and bi.Finish are admitted here — every actual record write (convertResourceTypes / convertResources / convertGrants through bi, lines 213-228) runs outside any admission window, and TestPebbleStoreAbandonedSessionsDoNotBlockClose explicitly asserts Close does not wait for an open bulk import. So a concurrent Close can close the engine mid-import while the code reads as guarded. ToPebble owns dest and closes it itself, so nothing races today; consider either holding one admission across the whole import or noting in a comment that the guard here only covers the lifecycle calls, not the writes.
| if e, ok := target.(*Engine); ok && e != nil { | ||
| n, err := fn(ctx, e) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if n > 0 { | ||
| // fold_dead_bytes lives in the envelope manifest, which only the | ||
| // store writes. Accepting the count here would drop it, understating | ||
| // accumulated waste and deferring the rebuild that reclaims it, so | ||
| // a fold that shadows bytes has to target a store. A fold that | ||
| // shadowed nothing has nothing to record and is allowed through. | ||
| return errors.New("pebble WithEngineFoldMutation: bare engine cannot record fold dead bytes; fold into a store") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🟡 Suggestion: (new, medium confidence) The bare-engine guard rejects after fn has already applied the fold to the engine, so the caller gets an error describing a mutation that did happen and cannot be undone — the engine now carries the merged records while the return value says the operation failed. n is only knowable post-hoc, so the check can never be a real pre-flight. Since the whole point is that a bare engine has no envelope to record fold_dead_bytes into, consider rejecting bare-engine targets up front (before invoking fn) rather than after; if the n == 0 pass-through is needed for the test-only callers, gate it on an explicit opt-in instead of on the outcome.
| .PHONY: test-full | ||
| test-full: ## Run complete matrices and timing-sensitive soak iterations. | ||
| BATON_FULL_TESTS=1 BATON_CUT_SWEEP=full go test -tags=baton_lambda_support -count=1 -timeout=30m ./... |
There was a problem hiding this comment.
🟡 Suggestion: (carried over, unaddressed, medium confidence) test-full is the only thing that sets BATON_FULL_TESTS, and no workflow invokes it. Verified at 13226dc: .github/workflows/ci.yaml:39 runs go test ./... on Linux with no -short and no BATON_FULL_TESTS, and neither ci.yaml nor main.yaml calls make test-full / race-check / scheduler-soak. So the cases this PR moved behind the gate now run nowhere in CI: pkg/sync/expand/topological_merge_resume_test.go:39,190, topological_merge_layer_interrupt_test.go:99, topological_merge_differential_test.go:483, pkg/sync/scheduler_soak_test.go:158 (skips outright), and pkg/dotc1z/race_test.go:37 (100→20 WAL-race attempts). Given this PR is entirely about a concurrency lifecycle, losing the race/soak tier is the coverage you most want.
Fix: add a scheduled (or nightly) workflow that runs make test-full and ideally make race-check, otherwise the full tier is dead configuration.
| for s.activeWrites > 0 { | ||
| s.cond().Wait() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: (residual, low confidence) The rationale added above for the non-cancellable drain is sound — abandoning it would leave admitted writes running against an engine about to close. What is left is observability: compactPebble (pkg/synccompactor/compactor_pebble.go:1000) and compactPebbleFold (:490) each hold a single admission for an entire compaction, so a wedged merge or a Lambda freeze/thaw turns Close(ctx) into a silent unbounded block with no log line and no ctx.Err(). Consider a periodic warn from a watchdog goroutine naming activeWrites and elapsed time, so the wedge is diagnosable from logs rather than only from a stack dump.
| var err error | ||
| defer func() { uotel.EndSpanWithError(span, err) }() | ||
| if c.rawDb == nil { | ||
| if c.rawDb == nil || !c.dbClosed.CompareAndSwap(false, true) { |
There was a problem hiding this comment.
🟡 Suggestion: closeRawDB no longer nils c.rawDb, which silently breaks the closed-handle guard in finalize at line 737 (if c.rawDb != nil) and its comment at 733–736 ("callers ... manually close c.rawDb ... that path skips both operations here"). That skip is now unreachable via closeRawDB: a C1File whose handle was released this way and then reaches finalize will run PRAGMA wal_checkpoint(TRUNCATE) on a closed *sql.DB, get sql.ErrConnDone, and take the cleanupDbDir(c.dbFilePath, finalizeErr) branch — deleting the working database instead of proceeding to saveC1z. No live caller hits this today (every closeRawDB site either returns immediately or sets c.closed), so this is latent, but the guard should be c.rawDb != nil && !c.dbClosed.Load() and the comment updated. (medium confidence)
| if err == nil { | ||
| srcFile = f | ||
| defer srcFile.Close() | ||
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return stats, fmt.Errorf("c1z v3: open splice source: %w", err) | ||
| } | ||
| srcFile = f | ||
| defer srcFile.Close() | ||
| // Reuse is an optimization, not a durability dependency. The | ||
| // extracted payload is complete, so if its source envelope was | ||
| // removed while the store was open, encode every frame afresh. | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the ErrNotExist fallback is silent — it emits no log and no stat distinguishable from "nothing was reusable". A wrong or stale reuse.srcPath (path bug, source rotated under the store, caller passing a PayloadReuse from a different envelope) now converts an O(changed-frames) splice save into a full O(payload) re-encode of a whale-scale file with no signal anywhere; the caller in pebbleStore.save discards SpliceStats, so nothing else notices either. Log at warn with reuse.srcPath before falling through so the regression is diagnosable in the field. (high confidence that no signal is emitted; low severity)
| } | ||
| var id string | ||
| var started bool | ||
| err := s.withMutation(func(e *pebble.Engine) error { |
There was a problem hiding this comment.
🟡 Suggestion (new, medium confidence): withMutation sets dirty on admission, which widens the dirty condition versus the targeted markDirty calls it replaced. StartOrResumeSync used to flip dirty only when err == nil && started — a pure resume left the store clean. It now marks dirty unconditionally, so an open that resumes a sync and then does nothing (or fails immediately) pays a full CheckpointTo + envelope re-encode at Close, which is the dominant cost on a whale-scale c1z. Same widening applies to BeginExpandedGrantLayer / AddExpandedGrantLayerContributions / AbortExpandedGrantLayer (lines 917-958), which previously left dirty alone and relied on FinishExpandedGrantLayer — an abandoned, never-finished layer's already-ingested segments now reach the saved envelope. If admission-time dirty is deliberate for the ordinary write paths, consider keeping the started-conditional flip here (and skipping it on Abort) so a no-op resume does not force a save.
| s.closeMu.Lock() | ||
| if s.closed { | ||
| s.closeMu.Unlock() | ||
| if !s.beginClose() { |
There was a problem hiding this comment.
🟡 Suggestion (new, low confidence): CloseEngineOnly enters the same uncancellable beginClose drain as Close, but has no warnWhileBlocked watchdog. This is the teardown the k-way compactor uses for every source chunk (pkg/synccompactor/pebble/kway.go:324), so a wedged admission here blocks with no ctx.Err(), no log line, and — unlike Close — nothing naming activeWrites. Wrapping this call the same way (it would need a ctx, which CloseEngineOnly does not currently take) would make both blocking teardowns diagnosable from logs rather than only from a stack dump.
| stopWarning := s.warnWhileBlocked(ctx) | ||
| owned := s.beginClose() | ||
| stopWarning() | ||
| if !owned { | ||
| return nil | ||
| } | ||
|
|
||
| if !s.readOnly && s.dirty { | ||
| if !s.readOnly && s.isDirty() { | ||
| if err := s.save(ctx); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: the drain watchdog is stopped at line 1032, before save() runs, so only one of Close's two long phases is instrumented. save() is CheckpointTo + a full envelope encode, and this PR's own new SpliceStats.ReuseMissingPath signal exists precisely because that encode can silently degrade from O(changed frames) to O(payload) on a whale-scale file. An operator looking at logs for a Close that has been running for ten minutes gets periodic "still waiting to tear down" lines if it is stuck in the drain, and complete silence if it is stuck in the save. Consider deferring stopWarning() until after save() (with the message distinguishing drain from save), or emitting the save phase at Info rather than Debug. (medium confidence, non-blocking)
…gine surface A Close could race the writes it was supposed to be waiting for, in both storage paths, and the Pebble store's engine was reachable in ways that skipped whatever guard was in place. The Pebble store now admits every mutation through an explicit open/closing/closed gate, so a teardown drains the writes it admitted before it touches the engine, and a second Close reports success rather than replaying the first one's terminal error or inheriting a foreign teardown's failure. The drain is deliberately uncancellable, so a watchdog logs what it is waiting on while it blocks. C1File tracks closed-ness with an atomic flag instead of nil-ing the handle out from under concurrent readers, which was a live data race. The save path keys off whether the handle is open rather than whether the pointer is non-nil, so releasing it out of band can no longer make finalize discard the database. pebbleStore no longer embeds *pebble.Engine. It forwards what it needs, so no engine mutator reaches a caller without passing the gate; a test walks the AST to keep that true, and compile-time assertions pin the optional capability interfaces that callers discover by type assertion, which an un-embedding would otherwise drop silently into a slow path. The engine's exported surface goes from 179 methods to 128, four of which had no callers at all. Folds mark the store dirty and account dead bytes only when they succeed, and a fold aimed at a bare engine is refused before it runs, since there is nowhere to record the count afterwards. Co-authored-by: Cursor <cursoragent@cursor.com>
| for _, n := range enumerateCutPoints(baseline.checkpoints, 8) { | ||
| cuts = append(cuts, cut{name: fmt.Sprintf("checkpoint-%02d", n), checkpoint: n, cause: errInjectedCut}) | ||
| } | ||
| for _, m := range enumerateCutPoints(baseline.responses, 16) { | ||
| for _, m := range enumerateCutPoints(baseline.responses, 8) { | ||
| cuts = append(cuts, cut{name: fmt.Sprintf("response-%02d", m), response: m, cause: errInjectedCut}) | ||
| } | ||
| // Expiry cuts take the run-duration deadline path, which force-writes | ||
| // a checkpoint of the MID-BATCH stack (spawned cursors in flight) | ||
| // before exiting — the token shape hard cuts never persist, and the | ||
| // one resume bugs have historically hidden in. | ||
| for _, m := range enumerateCutPoints(baseline.responses, 12) { | ||
| for _, m := range enumerateCutPoints(baseline.responses, 6) { |
There was a problem hiding this comment.
🟡 Suggestion: the sampled cut sweep is halved here (16→8, 16→8, 12→6) in the same PR that moves the heavyweight matrices behind BATON_FULL_TESTS. enumerateCutPoints only ignores the limit when BATON_CUT_SWEEP=full, which is set by make checkpoint-cut-check (nightly) — so nightly stays exhaustive, but ordinary PR CI now exercises half as many checkpoint/response/expiry resume points. Given this PR restructures store teardown and the save/dirty decision, resume coverage is exactly the axis worth keeping wide on the PR path; consider leaving the sampled limits at 16/16/12 or stating why halving is acceptable.
Confidence: medium.
The list asked a question names cannot answer: whether a method writes. It classified by prefix, so it missed any mutator whose name starts with a verb nobody thought of, saw nothing on the facade types, and reported false confidence in between — while reading like the thing standing between the store and an unguarded write. What stays is the part that is structural rather than inferential: the engine sits in a named field, so the store's method set is exactly what this package declares, and TestPebbleStoreEngineIsNotEmbedded fails if that changes. The AST inventory still proves every wrapper we declare takes admission. The residual gap — a hand-written passthrough that writes through the engine without admission — is not covered by naming either way, and closing it belongs with the gate unification rather than with a longer list of verbs. Co-authored-by: Cursor <cursoragent@cursor.com>
| // guardedMutationWrappers is the set of pebbleStore methods that mutate. | ||
| // TestPebbleStoreMutationWrapperInventory proves, via the AST, that every | ||
| // declaration bearing one of these names calls withMutation, so a wrapper | ||
| // that stops taking admission fails there rather than at runtime. | ||
| // | ||
| // What this list is not: proof that the store exposes no other writes. It | ||
| // covers the wrappers we know about. The store's method set being closed — | ||
| // the engine is a named field, not embedded, so nothing is promoted — is what | ||
| // keeps the unknown set small enough to review; see | ||
| // TestPebbleStoreEngineIsNotEmbedded. |
There was a problem hiding this comment.
🟡 Suggestion: dropping TestPebbleStoreDoesNotPromoteEngineMutators leaves no automated check on the reverse direction, and the two surviving guards do not cover it. TestPebbleStoreMutationWrapperInventory skips any decl whose name is not already in guardedMutationWrappers (if _, isTracked := tracked[fn.Name.Name]; !isTracked { continue }), and TestPebbleStoreEngineIsNotEmbedded only proves nothing is promoted. A hand-written passthrough such as func (s *pebbleStore) PutSyncRunRecord(ctx, r) error { return s.Engine.PutSyncRunRecord(ctx, r) } added to pebble_store_reads.go now compiles and passes every test in this package with no admission and no dirty bit — the exact escape the deleted test existed to catch (still reachable via PutSyncRunRecord, PutGrantRecord, SetCurrentSync, Flush, CompactAllRanges, PersistSyncStats, DropAllGrantDigestState, … which stayed exported on the engine).
The verb heuristic was indeed the weak half, but a non-heuristic replacement is available: snapshot the store's full method set with reflect and require it to equal guardedMutationWrappers ∪ a pinned list of the read forwarders in pebble_store_reads.go. That fails for any new store method — mutating verb or not — and forces the author to classify it, rather than guessing from the name. (medium confidence)
1 participant
Summary
open → closing → closedmutation admission for Pebble stores.Closewait for admitted mutations before deciding whether to save.Performance
Admission adds two brief mutex operations per synchronous mutation. Mutations remain concurrent and no lock is held while the underlying write executes.
Verification
go test -race ./pkg/synccompactor/...go test -tags=baton_lambda_support ./pkg/dotc1z/... ./pkg/synccompactor/...-racegolangci-lint run --timeout=3m ./pkg/dotc1z/... ./pkg/synccompactor/...go test -race ./pkg/dotc1z/...— Pebble packages passed; the command failed on the existing SQLiteTestC1ZConcurrentCloserace and later timed out.Remaining risks
AsEngine; production mutation paths now use guarded callbacks.CloseEngineOnlyagainst fullCloseretains its existing unsupported lifecycle semantics.origin/main, so this change does not introduce or depend on them.