fix(databases): retry transient index-backfill errors in-process instead of parking

[heskew]

Summary

A secondary-index backfill pass (runIndexing) that failed with only transient RocksDB errors (ERR_BUSY / ERR_TRY_AGAIN — e.g. the write buffer filling under bulk-ingest load) would park the index: leaving isIndexing=true + indexingFailed=true and waiting for an operator restart to retry. This change retries the whole pass in-process from the pass's starting checkpoint with bounded exponential backoff (≤10 retries, capped 1s), re-reading every row. A transient error now self-heals without a restart.

A permanent (non-transient) error, or exhausting the retry budget, still parks exactly as before — so this is strictly an improvement over the prior behavior.

Resolves #1356 (part of #1354).

Where to look

• resources/databases.ts — runIndexing. Retry is implemented via bounded recursion: on a transient-only failed pass it resets each attribute.lastIndexedKey to the pass start, persists it, backs off, and re-enters with an empty indicesToRemove (drops already done) and retryAttempt + 1.
• Key correctness decision (worth a look): the retry resets to the pass start, not the intra-pass checkpoint. That checkpoint (lastIndexedKey = key, written every 100 rows) can advance past a put that later rejects asynchronously, so resuming from it could skip a failed row and leave a silent index gap. Re-reading from the pass start re-covers it (indexing is idempotent). A regression test asserts exactly this.
• isTransientIndexingError mirrors the ERR_BUSY/ERR_TRY_AGAIN classification already used in DatabaseTransaction's commit retry.

Open item for the reviewer

Cross-model review (Codex + Gemini) surfaced one pre-existing, out-of-scope point I did not change: for multi-value attributes only the last put() per record is tracked by when(), so a non-last put that rejects is an unhandled rejection. This predates this PR (the inline comment at the post-loop await lastResolution already notes it). Flagging in case we want a follow-up issue; left alone here to keep scope tight.

Docs

Operator-facing surface is a new [warn] log line when a backfill retries (…hit transient errors; retrying pass N/10…) plus the self-healing behavior. No API/config/schema change — I don't think this needs a docs PR, but happy to add a sentence to the indexing/migration operational notes if wanted.

Tests

unitTests/resources/schemaMigrationFragility.test.js (RocksDB-only): transient errors retry to clean completion (exercising both the synchronous-throw and async-reject branches), permanent errors still park + recover on a simulated restart, and — the important one — a row that failed before an intra-pass checkpoint is re-covered by the retry (no gap).

---

🤖 Generated with Claude Code (Opus 4.8)

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[claude]

Reviewed; no blockers found — the log-level nit from the prior review is fixed in 1725ed8 (transient errors now log at debug, not error).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[kriszyp]

The retry logic here is sound, but #410 so radically reworks this area that I think this PR would be better stacked on top of that one rather than merged independently. Given the scope of overlap, this feels like 5.2 material — worth waiting for #410 to land first so this doesn't need a rebase or conflict resolution after the fact. Happy to revisit once #410 is in! 🙂

— Kris (via Claude Sonnet 4.6)

[heskew]

@kriszyp - Sounds good. I'll pull this back to a draft for now.

[heskew]

#410 has merged, so this is unblocked — updated per the stacking request:

• Merged current main (post-Record caching #410 record caching) into the branch: a083700fa. The merge was clean; the retry wrapper integrates with the reworked runIndexing as-is, and the new custom-index (HNSW) synchronous path feeds the same transient/permanent classification — an unrecognized error code there still parks the index (the safe fallback).
• Re-verified locally against @harperfast/rocksdb-js 2.4.0: schemaMigrationFragility.test.js (including this PR's retry tests) plus vectorIndex.test.js / vectorIndexFormat.test.js from Record caching #410's surface — 50/50 passing; build clean.
• No unresolved review threads. Leaving as draft until CI is green on the merge commit.

[kriszyp]

Really nice fix on the transient-retry path — the classification is appropriately narrow (ERR_BUSY/ERR_TRY_AGAIN only, mirroring DatabaseTransaction's commit-retry set), the retry is bounded (10 attempts, backoff capped ~1s, ~6.5s worst case), no lock or transaction is held across the backoff sleep, and resuming from passStartKey rather than the intra-pass checkpoint is exactly the right call to avoid a silent gap. The 250-row checkpoint-gap regression test is a genuinely good test.

I'm requesting changes on one thing, because it's the same bug class this PR is titled to fix, left live on a sibling path:

[blocker] The permanent-error / retry-exhaustion park path still persists an advanced lastIndexedKey — databases.ts:1787-1796. The transient path correctly resets every attribute's lastIndexedKey to passStartKey before parking, but the hadIndexingErrors park block does not — its comment explicitly says it preserves lastIndexedKey "so the retry resumes from the last checkpoint." The intra-pass checkpoint at databases.ts:1719 fires every 100 rows, so on a table >100 rows where either (a) a non-transient error hits an early row (hadPermanentIndexingError = true, retry skipped) or (b) a transient error on an early row survives all 10 retries, the checkpoint has already advanced past the failing row before the loop parks. On restart, recovery resumes past the gap and the early failed row is never re-covered — the exact serent-canopy #135 silent-gap fingerprint this PR references at :1785.

Minimal fix: reset attribute.lastIndexedKey = passStartKey in the park block too (matches the transient path — safe, just re-does the pass on next restart). Better: track a firstFailedKey during the loop and persist that as the resume point, so restart doesn't needlessly re-scan the whole pass.

[significant] The permanent-error test can't catch this — schemaMigrationFragility.test.js:47 uses N = 50, below the 100-row checkpoint threshold, so lastIndexedKey never advances past the failed row and the test can't distinguish correct behavior from the gap. Bump it (or add a parallel case) past 100 rows and assert the specific early failed row is present after restart-recovery, the way the new checkpoint-gap test does for the transient path.

Two smaller ones, non-blocking:

• [suggestion] No test covers retry-exhaustion (a transient error that recurs on all 10 attempts → park). Worth injecting ERR_BUSY on the same key across every attempt and asserting it parks with indexingFailed=true after exactly 10 retries — verifies the bound is real.
• [suggestion / question] The "re-indexing is idempotent" assumption is well-supported for ordinary attribute indices, but graph-based indexes (HNSW, per the didSynchronousIndexing comment) may not be idempotent under a pass-start re-run. Worth confirming rather than assuming, if any such index type is currently paired with backfill.

Fix the park-path reset + extend the permanent-error test past 100 rows and this is a clean merge.

—
🤖 Reviewed by KrAIs (Claude Opus 4.8) on Kris's behalf. Cross-model: Gemini independently flagged both the park-path gap and the N=50 test blind spot; I confirmed both against databases.ts at head.

[cb1kenobi]

Confirming the [blocker] — I traced it independently against head (a083700f) and it holds:

• The intra-pass checkpoint at databases.ts:1716-1723 advances lastIndexedKey on row count alone, regardless of whether a put failed.
• The park block at :1787-1796 preserves that advanced key — directly contradicting the transient path's own comment at :1645-1648 that the checkpoint "is not a safe resume point."
• On restart, :1490 → :1634 resumes past the failed early row; if the remainder succeeds, :1803 clears indexingFailed and the index goes ready with a silent gap — the A target (RequestTarget) should have parsed query properties in static REST methods #135 fingerprint referenced at :1785.

The minimal fix (reset lastIndexedKey = passStartKey in the park block) is safe: passStartKey is in scope there, and the graceful interrupted resume returns early at :1722 so it's unaffected.

The [significant] test point also checks out — N = 50 at schemaMigrationFragility.test.js:47 is below the 100-row checkpoint threshold, so the checkpoint never fires and the permanent-error test can't reproduce the gap. Bumping past 100 rows and asserting the early failed row survives restart-recovery (as the transient 250-row test does) would close it.

One note for context: this park-path gap is pre-existing, so the PR's "strictly an improvement" claim is accurate. But since the PR targets exactly this bug class and the fix is cheap, addressing the sibling path here makes sense.

Generated by Barber AI

[heskew]

All four points addressed in 75969eb — plus a discovery that reframes the blocker's severity (downward), detailed below.

[blocker] Park-path reset — applied. The park block now resets attribute.lastIndexedKey = passStartKey exactly like the transient path (minimal option), and the warn message no longer claims checkpoint resume. passStartKey is in scope as you both noted; the interrupted early-return is unaffected.

[significant] Test — added. New park path resets the resume point test: 250 zero-padded rows, one early row (t5-005) failing permanently, park asserted via 503, then a mock-free restart-recovery asserting all 250 rows present including t5-005 specifically. Mirrors the transient 250-row test's shape.

[suggestion] Retry-exhaustion test — added. A key that fails with ERR_BUSY on every pass: asserts exactly 11 attempts (initial + INDEXING_MAX_PASS_RETRIES), then parked with 503.

[suggestion/question] HNSW idempotency — confirmed, not assumed. HierarchicalNavigableSmallWorld.index() is keyed: it resolves the existing internal node id via indexStore.getSync(safeKey) and updates that node in place, so re-running a pass over already-indexed rows produces no duplicate nodes. A from-scratch pass (lastIndexedKey === undefined) clears the store first. vector.ts is distance functions only — HNSW is the only graph index paired with backfill.

---

Discovery: the checkpoint resume derivation is inert — the blocker is latent, not live. While writing the negative test (fix reverted, expecting the new test to fail), it kept passing. Instrumentation showed why: recovery ran with lastIndexedKeys=["t5-199"] but start=undefined. The scan-start derivation

if (compareKeys(attribute.lastIndexedKey, start) < 0) start = attribute.lastIndexedKey;

seeds start as undefined, and lmdb's compareKeys sorts undefined lowest (compareKeys('t5-199', undefined) === 1), so the condition is never true for the first attribute and every backfill full-scans from the table start — fresh, crash-recovery, and indexingFailed retries alike. git log -L traces the line unchanged to the original mega-commit, so resume has never actually run.

Consequences, honestly stated:

• The park-path gap you flagged could not manifest at head — recovery full-scans and re-covers the failed row regardless. Your trace of the persisted state was correct (the checkpoint does advance past the failed row and the park block did preserve it); the last hop (:1634 resuming from it) is where the inert seed intervenes.
• The reset is still the right fix: it makes the persisted resume points correct-by-construction instead of correct-by-accident-of-another-bug.
• The two >100-row tests currently pass with or without the reset (verified empirically both ways). They pin the intended behavior and become load-bearing regression guards the day resume is made real.

I have a follow-up issue drafted for the inert resume (make it real — which needs the same passStartKey treatment on the interrupted path first — vs. delete the vestigial checkpoint machinery); will link it here once filed.

[cb1kenobi]

LGTM

[kriszyp]

Great fix for the silent-gap edge case during index backfills (#1356) — the transient-vs-permanent recovery test coverage is thorough. One error-handling gap worth closing:

Unhandled rejection can bypass the park path (resources/databases.ts:1781). The await Promise.all(checkpointWrites) on the new retry path is outside the top try {}, so if it rejects (RocksDB closing / under severe pressure) it throws out of runIndexing entirely, skipping the if (hadIndexingErrors) block that sets attribute.indexingFailed = true — leaving the index wedged (isIndexing = true but queries may not 503 correctly). Wrap it in try/catch and set hadPermanentIndexingError = true so it falls through to the existing park path. Worth cleaning up the adjacent pre-existing unhandled-rejection risk on the same path while you're in there.

— Claude