fix(store): acquire reindex writer gate unbounded to stop warmup crash#328
Open
zzet wants to merge 1 commit into
Open
fix(store): acquire reindex writer gate unbounded to stop warmup crash#328zzet wants to merge 1 commit into
zzet wants to merge 1 commit into
Conversation
reindexEdgesSetOriented acquired writeMu with a bounded LockContext (sqliteBusyRetryWindow, 15s default). A resolver reindex is a mandatory graph.Store mutation with no error channel, so ReindexEdges routes any failure through panicOnFatal, which does not swallow context.DeadlineExceeded. When a sibling writer (a warmup checkpoint or a deferred-enrich mutation) held writeMu past the window, the gate timed out and crashed the whole daemon over a recoverable lock-wait. Acquire the gate with a plain unbounded Lock, matching the sibling mandatory-write paths (ReindexEdge, setEdgeProvenanceBatchSetOriented, RemoveEdge). Waiting cannot hang: beginWriteContext never re-locks writeMu, Compact's unbounded VACUUM runs before the warmup resolve in the same goroutine, and the lock order is always resolver mutex then writeMu. Per-transaction SQLITE_BUSY contention remains bounded by withSQLiteBusyRetry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The daemon panics during warmup:
Root cause
reindexEdgesSetOrientedacquiredwriteMuwith a boundedLockContext(sqliteBusyRetryWindow)— 15s by default. A resolver reindex is a mandatorygraph.Storemutation with no error channel (the interface mirrors the in-memory store's "everything succeeds" contract), soReindexEdgesroutes any failure throughpanicOnFatal, which only swallowsErrNoRows/ErrConnDone/ store-closed — notcontext.DeadlineExceeded.When a sibling writer (a warmup WAL checkpoint, or a mutation from the deferred-enrichment pool started ahead of the resolve) held
writeMupast the 15s window, the gate timed out and the recoverable lock-wait was escalated into a whole-daemon panic.reindexEdgesSetOrientedwas the only mandatory-write path using a bounded gate; the siblingsReindexEdge,setEdgeProvenanceBatchSetOriented, andRemoveEdgeall use a plain unboundedwriteMu.Lock().Fix
Acquire the gate with a plain unbounded
Lock(), matching the sibling mandatory-write paths. A mandatory graph mutation must wait for the writer gate, not abandon the batch and crash.Waiting cannot hang:
beginWriteContextnever re-lockswriteMu;writeMuholder —Compact's VACUUM — runs sequentially before the warmup resolve in the same goroutine, never concurrent;writeMu, no inversion (the siblings already prove unboundedLockis safe from these resolver contexts).Per-transaction
SQLITE_BUSYcontention remains bounded bywithSQLiteBusyRetry. The checkpoint's boundedLockContextis left untouched — a checkpoint is retryable maintenance whose caller treats failure as skip-and-continue.Verification
go build ./...andgo vet ./internal/graph/store_sqlite/clean.store_sqlitebusy / reindex / checkpoint / provenance tests pass with-raceand without:TestCheckpointSerializesBehindWriterGate,TestReindexEdgesRetriesWholeTransactionAfterBusy,TestReindexEdgesPersistentBusySurfacesAndRollsBack,TestLongWALReaderDoesNotBlockReindexWriter. No test asserted the gate-timeout path, so behavior is unchanged for covered cases.