Repository navigation
Conversation
FlushBulk and EndCoordinatedBulkLoad unlocked writeMu by hand on each return path. A panic inside the dense-index seal (or anything else under the gate) skipped the unlock, so every later writer, the owner's retry and Close all blocked on the gate forever. Split both into a write-gate half that releases with a defer and an outer half that runs what the exit still owes after the release, in the same order as before: the residue gate for the coordinated final PASSIVE, then the durability TRUNCATE for a standalone window. A panic now unwinds with the gate free and skips the post-release work. EndCoordinatedBulkLoad clears the coordinated flag before its FTS merges and the seal. A panic there left the pinned writer live behind a window nobody owned: the owner's next End returned nil and the store kept synchronous=OFF until Close. A deferred guard now restores the flag, the same retryable state the seal-error branch already keeps. SetBulkSealHookForTest is a per-store hook the seal calls before each dense-index DDL; an error stands in for that DDL failing and a panic unwinds the seal. TestFlushBulkSealPanicDoesNotDeadlock drives a non-error panic through both exits, probes the gate with a deadline so a regression fails instead of hanging, and checks the retry seals, closes the window and restores synchronous.
A cold bulk window that closes with its seal still owed (a failed seal at FlushBulk or Close, or AbortCoordinatedBulkLoad) restored durability and released the writer, but closeBulkConnectionLocked then cleared bulkIndexesDeferred and nothing remembered which dense indexes were gone. The planner probe reads an absent index as nothing to judge, so the store reported clean while lookups ran without their indexes. closeBulkConnectionLocked now reads the schema on the pinned connection (after the PRAGMA restores, before it closes) and records the droppable indexes that are absent, falling back to the whole droppable set when that read fails. A successful seal clears the record. It is not persisted: Open's createGraphCoreIndexes rebuilds the indexes on restart. PlannerStatsHealth reports the new reason dense_indexes_missing with the names in PlannerStatsFreshness.DenseIndexesMissing, shaped like bulk_window_active: not stale, nothing seeded, no ANALYZE attempted. TestFailedSealReportsDenseIndexesMissing covers a flush whose seal fails on two named indexes and an abort before any seal.
A cold window that closed unsealed now records which dense indexes it left missing, but nothing rebuilt them before the next restart, so a long-running daemon kept serving lookups without them. The flush and abort exits now post a dense_index_repair job after they release the write gate. The lane worker runs it after any owed WAL drain and before the statistics pass, as a quiescing whole-database job. Each attempt first checks free space on the store's volume and skips while it is below the store's used bytes (64 MiB floor): a rebuild needs about twice the index size at its peak and the dense set is a fraction of the used pages, so a full volume is never asked to rebuild while a volume with the store's own size free always can. A skipped or failed attempt re-posts itself after 30 s, doubling up to 15 min. The rebuild runs the same DDL as createGraphCoreIndexes (both come from bulkDroppableIndexes), but takes the write gate once per index instead of once for the set, so a writer waits behind at most one CREATE INDEX. Like VACUUM it defers while a bulk window pins the writer. A completed repair clears the record and asks the lane for a statistics pass for the rebuilt indexes. The job stays out of the lane's settle point so a long rebuild cannot hold AwaitMaintenanceIdle callers. TestFailedSealReportsDenseIndexesMissing now also checks that no rebuild is attempted while the volume reads as full, and that one attempt rebuilds every listed index and clears the reason once space is back.
index_health already reports the planner verdict's reason, so a store that lost dense indexes to an interrupted bulk load would show dense_indexes_missing with no way to tell which lookups now scan. The planner_stats block now carries dense_indexes_missing with the index names, and the recommendation says what they cost and how they come back (the background repair once the volume has room, or a restart). The subtest fixes the verdict through a wrapper over the real store, because the real state repairs itself on the maintenance lane as soon as the test volume has room and could not be observed without a race.
drainAddBatch wrote every shadow chunk through the void AddBatch, so a
storage failure during a first-index drain (SQLITE_FULL, an I/O error,
a failed deferred-index seal at a row limit) panicked out of the drain
instead of failing the build, and only the backstop flush stood between
that panic and a bulk window left open.
The drain now writes through AddBatchCheckedContext when the target has
it and falls back to AddBatch otherwise. The write runs under
context.WithoutCancel: cancellation is still observed only between
sub-batches, so an ordinary drain never stops inside a chunk. The node
loop now stops at the first refused write (the edge loop already did),
and the result names a refused write ("persist bulk graph batch")
apart from a cooperative stop. The builtin re-assert after the edge
drain also goes through the checked write, and only while the drain is
still succeeding: a failed drain publishes nothing and is evicted before
its retry.
One visible change: errors the void AddBatch used to swallow (a store
closed under the drain) now fail the build instead of dropping rows.
SetBatchWriteHookForTest is a per-store hook every AddBatch-family write
calls inside its transaction, so a test can make the store refuse a
batch with a real driver error. TestDrainAddBatchReturnsStorageFailure
hands it a genuine SQLITE_FULL and checks both drain shapes return a
*StorageError with nothing committed. The first-index probe intercepts
the checked write as well as AddBatch.
The first-index backstop skipped FlushBulk after the explicit flush had panicked, because that panic could leave the store's write gate held and re-entering would deadlock. The store now releases the gate on every exit and keeps an unfinished seal retryable, so skipping the retry only left the standalone window open: a pinned writer at synchronous=OFF with the dense indexes dropped, until the store closed. After a panicking explicit flush the backstop now calls FlushBulk once more while the panic unwinds. The retry recovers its own panic and only logs what it hit, so the original panic still reaches the caller. Under an outer coordinated or generation window FlushBulk is a store no-op, so the retry is too. TestFirstIndexFlushPanicPropagates now runs on the real store with a seal that panics once: the panic propagates, the retry seals and closes the window, and the writer's synchronous, the schema's index list, the receipts, the disk target and the memory lease are all back where they started. It inverts the old assertion that the backstop must not retry. WriterPragmaForTest reads a PRAGMA off the writer connection under a deadline, which is how indexer tests check durability restoration and a free write gate.
TestFirstIndexFailureRestoresStore ran its four cases through a wrapper around the store. The wrapper is not a *store_sqlite.Store, so the drain took the generic eviction rather than the shadow-replacement retirement a real daemon takes, and its injected faults sat outside the store's write gate, so they could not show what a failure inside the store leaves behind. The test also could not check the writer's durability, because PRAGMA synchronous is connection-local. The test now runs on the real store with seven cases: an FTS error in the drain, a cancel before the drain, a panic in the body, a panic in a drain write, a genuine SQLITE_FULL from a drain write, a seal panic, and a seal DDL error. Store faults come from the store's own seal and batch hooks. Every case checks that the write gate is free and the writer's synchronous is back, that the window is closed and the schema's index list is restored, that no receipt was written and that the indexer handed its disk target back. The seal-error case also waits for the maintenance lane to rebuild the missing index. Cases that reach the drain assert that it took the retirement branch, through a test hook beside drainSubBatchHook, and that the window was engaged when they wrote. The wrapper's FTS and drain-panic injectors are gone; the replacement test shares the FTS trigger helper.
…and on retry TestFirstIndexDrainUsesBulkWindow covered only IndexCtx, through a wrapper that counted BeginBulkLoad and FlushBulk calls. That shows the calls were made, not that the store's window was open while the drain wrote, and it said nothing about the multi-repository route or a retry. It now runs on the real store and watches the window from the store's batch hook: the planner probe reports bulk_window_active and the dense indexes are absent from the schema while drain batches land. Three routes: - IndexCtx on a fresh store. - TrackRepoCtx on an empty multi-repository store. - A retry: the first IndexCtx has its first edge batch refused, leaving nodes behind and no receipt. The second IndexCtx evicts them, finds the store cold again, engages the window and succeeds. Each route also checks that no receipt precedes the drain (at the eviction, just before the window opens), that the drain took the retirement branch, and that the writer's synchronous, the schema's index list and the receipts end up right.
…corded The missing-index record held only names, and the maintenance lane looked each name up in bulkDroppableIndexes when it got to the repair. That list is fixed in production, but in-package tests replace it to force a seal failure and put it back when they finish. A failed seal now schedules a repair, so the lane can read the list while such a test restores it. TestBulkRebuildFailureStillRestoresCheckpointsAndLeavesWriterUsable restores the list before its deferred Close joins the lane, and nothing orders that write against the lane's read, which is a data race under -race whenever the lane takes the write gate after the test's last write. A lookup that finds no entry also dropped the name from the record as if it had been built. The window now records each missing index together with its DDL, copied from the list under the write gate as it closes, and the repair runs that DDL. The lane no longer reads the list. The existing test keeps its injection and its defer order unchanged. TestDenseIndexRepairBuildsFromTheRecordedDDL records an index whose DDL cannot succeed, restores the list before the lane may run, and checks that every attempt still runs the recorded DDL and fails, instead of reporting a repair it never made.
…abort AbortCoordinatedBulkLoad closed whatever bulk connection was pinned. Its only caller so far opened a coordinated window and aborted that same window, so this never mattered. The next change makes the coordinated owners abort after a final EndCoordinatedBulkLoad fails. If that end failed after it had already closed the window (planner statistics, a PRAGMA restore), a payload generation can have opened its own window by the time the abort runs. A generation window and a coordinated window refuse each other, so a pinned generation writer is never the window being aborted. Closing it would strand the window's checkpoint lease, which only EndGenerationBulkLoad releases, and turn that end into a no-op. The abort now returns without touching a generation window. TestAbortCoordinatedBulkLoadLeavesAGenerationWindowToItsOwner checks that the window survives the abort, that its owner's end succeeds, and that the next generation can then open.
…fails twice indexMultiRepo ends its coordinated window explicitly and again in a defer when the explicit end fails. A final seal that fails keeps the window open by design, so its owner can retry. When the deferred retry failed as well, the pass logged it and returned, and the window stayed open until the store closed: one pinned writer at synchronous=OFF with automatic checkpoints off, carrying every later write from every repository. The dense indexes the seal could not build were never recorded, so neither the planner probe nor the maintenance repair saw them. CoordinatedBulkLoader now includes AbortCoordinatedBulkLoad. The only implementation, the SQLite store, already had it, so no backend changes. EndOrAbortCoordinatedBulkLoad ends the window and aborts it if the end fails. The defer uses it, so the explicit end still gets one retry and a second failure closes the window. The abort restores the writer and records the missing indexes, which the maintenance lane repairs once the volume has room. TestIndexMultiRepoFailedFinalSealGivesUpTheWindow runs the cold pass on a real store with a final seal that fails every time. It checks that the error reaches the caller, the writer's synchronous level is back, the planner probe no longer reports a live window, and the schema's index list is whole again after the repair.
…eal fails Daemon warmup ends its coordinated cold-load window once, after the go.mod drain. On failure it logged the error and cleared its active flag, which also skipped the deferred end, and never retried. A final seal that fails keeps the window open so its owner can retry, so a failed seal at warmup, typically a full disk, left the daemon running on one pinned writer at synchronous=OFF until shutdown. The indexes the seal could not build were never recorded, so index_health reported bulk_window_active instead of the missing indexes, and the maintenance repair could not run while the writer stayed pinned. The finalize and the deferred cleanup now go through EndOrAbortCoordinatedBulkLoad. A failed end is followed by an abort, which restores the writer and records the missing indexes for the maintenance lane to rebuild once there is room. TestWarmupFailedFinalSealGivesUpTheCoordinatedWindow runs a real warmup on a SQLite store with a failing final seal. It checks that the failure is logged, that the writer's synchronous level is back, that the planner probe no longer reports a live window, and that the repair then clears dense_indexes_missing.
repairDenseIndex refuses with ErrMaintenanceBusy while a bulk window pins the writer, as VACUUM does, so the CREATE INDEX never runs under a window that owns the connection. No test covered that branch: a cold window cannot reopen on the populated store that a failed seal leaves behind. A generation window can, so this test uses one. A flush whose seal fails on nodes_by_kind records the index as missing. While a generation window is open, every repair attempt is refused: nothing is reported as repaired, the record keeps the name, and the index stays absent. Once the window's owner ends it, the next retry builds the index and clears the record.
A rebuilt index has no sqlite_stat1 row, so until ANALYZE runs on it the planner costs it from defaults. runDenseIndexRepair asks the maintenance lane for a statistics pass when it completes, and nothing else does at that point: the window that left the index missing scheduled its own pass long before the repair ran. No test covered that request. TestFailedSealReportsDenseIndexesMissing now waits, in both of its cases, for nodes_by_kind to gain a sqlite_stat1 row after the repair. Without the request both cases time out.
…nic row TestFirstIndexFlushPanicPropagates repeated the "seal panic" row of TestFirstIndexFailureRestoresStore: the same injected seal panic, the same backstop retry, synchronous, index-list, disk-target and receipt assertions. Only its memory-lease checks were its own, so they move into that row and the standalone test goes.
When the store refused a node batch, the node drain stopped but the edge drain still went on writing: at least one more batch that would be refused the same way, or, if the refusal was transient or specific to that node batch, the full edge set of a build that had already failed. The failed build publishes nothing and the retry evicts what it wrote, so those writes were wasted I/O against a store that had just reported a storage failure, often a full volume. A failed node write now ends the whole drain. A symbol FTS failure still lets the graph rows drain, as before.
The first-index backstop retried a FlushBulk that panicked only when it was the explicit flush after a completed drain. When a drain write panicked instead, the deferred FlushBulk ran while that panic unwound, and if its seal panicked as well, nothing retried it: the standalone window stayed pinned at synchronous=OFF until Close. The deferred flush now marks itself in progress the same way, so the earlier-registered defer gives it the same single retry while the later panic unwinds. No recover is involved, so that panic keeps its stack.
When TestFirstIndexDrainUsesBulkWindow moved to the real store, it lost the probe assertions that the drain opened and flushed exactly one window. The witness only counted batches written inside an open window, so a drain that opened a second window would still pass. The witness now also counts seals per dense index through the store's seal hook and requires each index to be sealed exactly once per IndexCtx, on every route the test covers, the retry included.
The repair's free-space gate sizes an attempt by the store's used bytes from CompactStats, which reads 0, 0 when any of its PRAGMAs fails. The need then fell to the 64 MiB floor, so a multi-gigabyte store could start rebuilding its dense indexes on a nearly full volume, the case the gate exists to block. An unreadable free-space figure was already a skip; an unreadable store size is now one too, and the backoff retries later.
The comment said a seal panic leaves the window open "exactly as a seal error does". It does not: after a seal error FlushBulk still restores and releases the pinned writer, and records the indexes it could not rebuild for the maintenance-lane repair. Only a coordinated End keeps its window open after a seal error.
index_health's planner_stats can now report the reason dense_indexes_missing together with a dense_indexes_missing list, but the index_health entry only described bulk_window_active. Say what the new verdict means, what it costs, and what clears it.
The drain now asserts drainCheckedWriter on its target so a storage failure comes back as an error instead of a panic out of the void AddBatch. TestDeltaWriterCapabilityChecklistIsComplete requires every interface the engine asserts to be implemented by *graph.DeltaWriter or listed with a reason, and this one was neither, so the checklist test failed from that change on. The delta does not need it: the checked write belongs to the whole-repository shadow drain into the durable store, not to a file delta's per-save path, and a store without it still takes AddBatch.
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.
A first index into an empty SQLite store opens a cold bulk window. While it is open, writes go through one pinned writer at
synchronous=OFFand the dense indexes are dropped. On a panic or a full disk this window could deadlock, or close without its indexes and leave nothing to report or repair the gap. This draft makes the cold window fail-safe on the store, indexer and daemon sides.Draft status: the store, indexer and daemon fixes are done and tested. The observability items are held until #1005 lands, the performance gates have not been measured, and the last review-fix round was interrupted (see "Not done").
Changes
Store (
internal/graph/store_sqlite)FlushBulkandEndCoordinatedBulkLoadnow release the write gate with adefer. A panic inside the dense-index seal no longer leaves every later writer blocked.Enddoes not return nil on a window nobody owns.PlannerStatsHealthreports a new reason,dense_indexes_missing, with the index names.Indexer
AddBatchused to swallow, such as a store closed under the drain, now fail the build instead of dropping rows.FlushBulk, the first-index backstop retries the flush once. This covers both the explicit flush and the deferred flush that runs while a drain panic unwinds. The original panic still reaches the caller.EndOrAbortCoordinatedBulkLoad). A failed final seal no longer leaves one pinned writer atsynchronous=OFFuntil the store closes.AbortCoordinatedBulkLoadjoins theCoordinatedBulkLoaderinterface; the SQLite store, its only implementation, already had it.Daemon
MCP
index_healthplanner_statslistsdense_indexes_missingand says what the missing indexes cost and how they come back.docs/mcp.mddescribes the new verdict.Tests
First-index failure cases run on the real store, using the store's own seal and batch hooks. The cases:
Each case asserts which eviction branch ran, a free write gate, restored
synchronous, a closed window, the restored index list, no receipt and the returned disk target.TestFirstIndexDrainUsesBulkWindowcoversIndexCtx,TrackRepoCtxon an empty multi-repository store, and a retry after a failed first drain. It checks that each dense index is sealed exactly once.New store tests cover the seal-panic deadlock, missing-index reporting and repair, the repair waiting for a pinned writer, a repair built from recorded DDL, the unreadable-size skip, and the abort leaving a generation window alone. Real-store tests cover the failed final seal in the multi-repository pass and in daemon warmup.
Tested
go test -race ./internal/graph/store_sqlite/...at the final store code passes in 371 s, with no data race.go test -race ./internal/indexer/...was run one commit before the head. It had one failure:TestDeltaWriterCapabilityChecklistIsComplete, which the drain's new checked-write assertion broke. The last commit fixes it, and a focused race run of that test plusTestFirstIndexFailureRestoresStorepasses at the head. The full indexer suite was not re-run at the head; CI runs it.FirstIndex|DrainAddBatch, three times;cmd/gortexwarmup tests.index_healthand capability-table tests ininternal/mcppass.internal/graph,internal/indexer,internal/mcpandcmd/gortexreports 0 issues.TestDeltaWriterCapabilityChecklistIsCompletered. History was not rewritten.Not done
Held until Measure the edit loop: per-phase daemon laps, CPU per edit, WAL reclaim cancels, edit_and_probe #1005 (PR feat: measure edit-loop phases and add edit-and-probe diagnostics #1169) lands, so they reuse its fields:
BeginBulkLoad/BeginCoordinatedBulkLoad, with its test;windowandseal_elapsedon theFlushBulk completeline;dense_indexes_missingandlast_first_index_drainindaemon status;last_first_index_draininindex_health.Performance gates, not measured:
BenchmarkPersistFixturewithin ±10% of main.They will be measured at the final base. The preliminary benchmark run was interrupted and produced nothing.
Manual full-disk check: not run.
Interrupted review-fix round: the preliminary benchmark and the independent check of the last fix round did not run.
Branch is behind main. It is 37 commits behind and merges cleanly. Per the issue, it lands after Replace the full FTS5 optimize after each repo index with a bounded merge #1021 (PR perf(sqlite): bound live FTS finalization to the cold-path merge budget #1209).
Out of scope: an owned window and backstop for the direct and streaming index routes (separate issue).
Refs #1027