Wait for every index write a backfill checkpoint covers, so a failed write cannot be skipped

[kriszyp]

A secondary-index backfill could persist a resume checkpoint past a record whose index write later failed, or declare the index complete over one, leaving that record silently missing from the index.

runIndexing issues one index put per indexed value per record, plus a drop per removed index and a clear per full rebuild, but tracked only the last put in lastResolution. Everything else was fire-and-forget: a rejection set hadIndexingErrors whenever it happened to settle, while the checkpoint read that flag once on entry and completion waited a single event turn. Whether a failed index write froze the checkpoint was an accident of scheduling. Varying only how long the failing write takes to reject, on one 600-record table whose record at k-0250 fails:

rejection delay	checkpoint after the first pass	parked	record indexed after the retry
one event turn	k-0199, correct	yes	yes
30 ms	k-0499, past the failed record	yes	no
300 ms	cleared, index declared complete	no	no

Every index mutation the build issues is now registered in a per-build set that absorbs its own rejection, covering the per-value puts, the drops for removed indexes and the LMDB clear that a full rebuild runs first. A checkpoint drains a snapshot of that set and refuses to certify when any mutation it covers failed, and completion drains it in place of the one-setImmediate heuristic, so "no errors" means every write settled rather than none had failed yet. The set also bounds what stays in flight, waiting on the next settlement of any mutation rather than a chosen one, because the drop and clear are inserted before the scan starts and faster writes overtake them.

Checkpoints carry a CHECKPOINT_ALGORITHM stamp. A descriptor without the current stamp resumes as uncertified and rebuilds, so a checkpoint the previous code certified past a failed record cannot be resumed from. A clean completion keeps the stamp rather than clearing it with the other checkpoint fields, and it is carried across a later descriptor rewrite; without that, indexing a second attribute on the table dropped the stamp and made the first index indistinguishable from a pre-fix build.

For the human reviewer

1. Completed indexes an affected release already built are not repaired, and this PR does not repair them. v5.2.10 was tagged 2026-09-10 on origin/v5.2 and contains the defective code. A build it declared complete over a failed write cleared every marker, so on upgrade its descriptor has no checkpoint and no stamp, table() treats it as ready, and the record stays missing from that index. The alternative is forcing one rebuild of every unstamped completed index on upgrade, whose cost depends on how far v5.2.10 was distributed — a fact I do not have. I chose to make the later decision possible instead of making it now: stamping completions means every index built from here on is distinguishable, which is what any remediation has to key on. Changing this later needs another version bump and that same expensive rebuild; leaving it means any already-gapped index stays wrong in the field. This is the call I most want ruled on.
2. The algorithm stamp is compared with !==, not >=. A downgrade-then-upgrade cycle therefore forces a full rebuild of any in-flight checkpoint. That is the conservative direction and cheap to change while there is one consumer, but it is a persisted on-disk shape, so changing it later needs its own compatibility story.
3. Backpressure and the yield hint now share one unit. MAX_OUTSTANDING_INDEXING (1000) and MIN_OUTSTANDING_INDEXING (10) were tuned against a per-record counter and now count individual index writes, so a fan-out table reaches both sooner. Two reviewers called this a throughput risk; I measured it rather than argue it, and there is no regression (numbers in Verification). Splitting the yield hint back onto a per-record counter is one line if you disagree with the reading.
4. The bound is checked once per record, so a single row's fan-out can overshoot it. A row with a very large indexed array issues all its puts before the cap applies. I left it: it is strictly tighter than the per-record counter it replaces, which allowed that same row's fan-out plus 1000 records' worth beyond it, and moving the check inside the value loop adds a suspension point mid-record for a pathological input.
5. A checkpoint that fails to persist still only warns. A sustained catalog write failure leaves hadIndexingErrors false and the scan continuing, so progress is under-recorded and a crash rescans further back than necessary. That predates this change; I did not widen it into an indexing error here, because parking a build that is writing correct index data is its own harm.
6. The barriers are proven at module level with patched mutation promises, not end to end. Nothing here drives a real engine rejection through the operations API. Adding that later is additive.

I also declined one review suggestion on evidence: re-reading hadIndexingErrors after the flush and before writing descriptors. I tried it first, and it made the existing freeze cases checkpoint at k-0099 instead of k-0199, because a failure at a later key invalidated a checkpoint whose own records had all succeeded. The drain's scoped result is the correct guard; the global flag is not.

Changes

resources/databases.ts carries the mechanism above. Beyond the links in the summary: track returns the original result untouched and registers nothing when a put resolves synchronously, so the RocksDB path allocates as it did before; drainMutations reports whether any member of its snapshot failed, which is what lets a failure on a later key leave an earlier checkpoint valid; and nextSettlement resolves on the next settlement of anything tracked. The when() import is gone with its last use.

unitTests/resources/indexBackfillConvergence.test.js adds five cases; see Verification.

DESIGN.md described the removed when() error handler, the removed final await lastResolution try-catch, and a checkpoint written every 100 records. It now describes the mutation set, the drain barriers, and both checkpoint stamps.

Verification

Five new cases, each of which fails on a dist built from origin/main's resources/databases.ts:

case	on base
freezes the checkpoint when an index write rejects only after a later checkpoint was reached	checkpoint k-0299 must not pass the failed record k-0250 — the nightly symptom, now deterministic
does not declare the index complete while an index write is still in flight	index declared complete
parks the build when the clear that precedes a full rebuild rejects (LMDB)	build completed over a failed clear
bounds how many index writes it leaves in flight	3900 writes in flight
rebuilds instead of resuming a checkpoint stamped by an earlier checkpoint algorithm	resumed at k-0200

A sixth case covers the stamp surviving a descriptor rewrite; it fails on the first two commits of this branch rather than on base, since it guards the stamp those commits introduced.

The rejection in the first two cases is released by an observed scan position, not a timer, so the interleaving is the same on a fast and a slow runner.

npx mocha unitTests/resources/indexBackfillConvergence.test.js          19 passing, 1 pending
HARPER_STORAGE_ENGINE=lmdb  (same file)                                 19 passing, 1 pending
npm run test:unit:resources                                             2357 passing
HARPER_STORAGE_ENGINE=lmdb npm run test:unit:resources                  1838 passing
npm run test:unit:main                                                  5466 passing, 2 failing
npm run test:integration:all                                            2058 passing, 0 failing

The two test:unit:main failures are environmental and reproduce on this box without the diff: a token test that depends on a shared system database, and a config-validator case that asserts on the length of the checkout path. The integration run's only failures are the ollama-backend suite, which fails at module load with ERR_IMPORT_ATTRIBUTE_MISSING for systemSchema.json and needs a live Ollama instance.

Throughput, since this changes what the backpressure thresholds count. 50,000 records, two indexed attributes of two-element arrays, four index writes per record, RocksDB, same box, dist built from each side:

build	backfill
origin/main	699 ms, 727 ms
this branch	636 ms, 647 ms

Refs #2536

Complexity: complicated

🤖 Generated with Claude Code

https://claude.ai/code/session_01892ogD1y5AdZrV42zWpMXb

_{Review-Coverage: authored=claude; ran=gemini,codex; declined=cursor-grok,cursor-composer,domain; rounds=3 @ 33dfa28}

_{Human-Review-Need: 4 @ 33dfa28}

[github-actions]

Release cherry-pick v5.2: merged

Cherry-picked onto v5.2.

[gemini-code-assist]

Code Review

This pull request improves the reliability of the index backfill process by tracking all pending mutations in a set, preventing checkpoints from certifying if any write fails, and bounding the number of concurrent in-flight writes. It also introduces a checkpoint algorithm versioning mechanism to prevent resuming from incompatible or corrupt checkpoints, and adds comprehensive tests to cover these scenarios. The reviewer noted that the lastResolution variable is now unused and should be removed.

[claude]

File: resources/databases.ts:3463-3473 and :3481-3500
What: Both tail loops in runIndexing — the hadIndexingErrors branch (~3463-3473) and this completion branch — loop over attributes, reassigning a single lastResolution = Table.dbisDB.put(attribute.key, attribute) each iteration, then await lastResolution only once after the loop. When attributes.length > 1 (any multi-attribute backfill, e.g. the tag+group cases this PR's own tests exercise), only the last attribute's put is awaited — the earlier ones are fired with no .catch()/error handler attached anywhere.
Why it matters: This is the exact anti-pattern this PR just replaced everywhere else in this function (a single lastResolution var overwritten in a loop, only the last one awaited) — but these two loops still have it. Table.dbisDB.put() can genuinely reject in production: this function's own outer catch explicitly anticipates "a worker shutting down closes its stores mid-backfill" causing a put to throw "Database not open" (see the comment at ~3509-3514). If an earlier attribute's put rejects in that scenario here, nothing ever attaches a handler to that promise — an unhandled rejection, which by default terminates the Node process, undermining the very graceful-shutdown handling this function goes out of its way to provide a few lines below.
Suggested fix: Mirror the pattern already used 30 lines further down in this same function's outer catch block (harper#843, ~3528-3537): collect the puts into an array and await Promise.all(puts) after the loop, in both tail loops.

[claude]

One blocker: the two tail descriptor-persist loops in runIndexing (resources/databases.ts, ~3463-3473 and ~3481-3500) still await only the last Table.dbisDB.put() per multi-attribute loop instead of every one — the exact pattern this PR fixes elsewhere in the same function. See inline comment for detail and the working sibling pattern nearby.