Index backfills resume from their checkpoint and yield the event loop, so large tables converge (harper#2536)

[kriszyp]

A secondary-index backfill on a large table could never converge: runIndexing discarded its own resume checkpoint (ordered-binary sorts undefined lowest, so the running-minimum guard never fired and every retrigger rescanned from the first record), and on a plain RocksDB index it never yielded the event loop (RocksIndexStore.put is synchronous, so the outstanding counter the yields were keyed to never left 0 and a 19M-row build ran as one ~18-minute turn until the worker was terminated). Backfills now resume from the minimum persisted checkpoint across the attributes being built (used as the scan's start, clearing every index when a full scan is needed), yield every 100 scanned entries regardless of write-completion timing (deletion entries count too and still pace the checkpoints). Because the checkpoint is now actually consumed, it has to be trustworthy: persistCheckpoint writes it only after the index writes it covers have settled and the RocksDB store has been flushed (flushIndexStores: index stores have no WAL), at most once per 5s and never before 10k more records, never advances after any index write fails (every put's rejection is tracked, not only a record's last), and stamps each checkpoint with its own key so the trigger ignores a checkpoint left by an earlier release (which advanced past failed and unflushed writes) and rebuilds instead of resuming past a gap. The completion path flushes the tail before persisting the ready descriptor and parks the index if that flush fails; the interrupt path awaits the in-flight checkpoint and persists synchronously. Where to look hardest: persistCheckpoint, flushIndexStores and the trigger's uncertifiedCheckpoint — everything else is plumbing around them. Fixes #2536 (the per-thread isIndexing divergence and the dead-backfill marker are #2537, whose PR #2543 edits lines adjacent to the trigger change here; expect a trivial rebase on whichever lands second).

For the human reviewer

Step-6 planning review returned Framing-Verdict: better-alternative-exists twice and both were adopted: round 1 made the checkpoint certify a successfully indexed prefix; after the task owner's ruling on the two open questions (1b, 2a) and a teammate's rig repro, the recheck accepted the layer and all four rejected alternatives (WAL-on index column families, colocating the checkpoint in the index CF, a trigger-side discard on process change, and accept-and-document) and asked for three corrections, all in: never join a flush already in flight, bind the stamp to its key, flush before readiness. Nine pre-push rounds; the last two produced no new actionable finding.

1. Flush-gated, rate-limited checkpoints (ruling 1b). RocksDB data and index stores open with disableWAL: true but the descriptor store with the WAL on, so a checkpoint written every 100 records could outlive the entries it certifies after a SIGKILL/OOM (proven on a child-process kill: 99 of the first 100 entries lost, checkpoint intact). A checkpoint is now persisted only after a flush, at most once per 5s and never before 10k more records (setIndexingCheckpointPeriod() is the test seam) — the flush seals every column family in the database (atomic_flush), so the record floor keeps a slow backfill from imposing the 5s rate on unrelated tables. At most one flush in flight and one queued per root store; a caller never joins one already running because a flush only covers writes issued before it started. Consequences to weigh: a table under 10k rows never checkpoints mid-scan (a crash costs a sub-second rescan; the final barrier still flushes), each interruption reworks ≤10k records or ≤5s, and a crash loop shorter than that cannot converge. LMDB commits one environment in order and needs no flush.
2. Only a key-stamped checkpoint resumes (ruling 2a, in the form the teammate's comment asked for). checkpointCertified repeats the checkpoint key; the trigger resumes only when it equals lastIndexedKey and otherwise rebuilds (logged as reindex reason uncertified-checkpoint). That covers both legacy exits — indexingFailed and the marker-less interrupted return — so no field cleanup is needed before deploying; the one-time cost is that an in-progress legacy backfill restarts from record 0 on upgrade. This is the one descriptor schema addition in the PR (a field the old trigger drops, since it rebuilds the descriptor from the schema definition).
3. The completion barrier. A flush before the ready descriptor closes a hole that exists on main too: a kill right after completion lost 0 of 10,000 index entries with it and all 10,000 without it (child-process test). A flush failure parks the index (indexingFailed) rather than announcing it complete.
4. Checkpoint safety rests on index writes settling in dispatch order: RocksDB puts throw synchronously, LMDB puts settle FIFO in one environment, so an earlier rejection is always visible before a later checkpoint fires. An explicit per-interval barrier over every put is a contained change if an engine ever settles out of order. The per-put rejection handler allocates one derived promise per LMDB put (bounded by the promise lmdb-js already creates per put; the RocksDB path never allocates).
5. Checkpoint freezes at the first per-record error for the rest of the pass (follow-up): a table with recurring transient errors advances one error-gap per retrigger — monotone, strictly better than main's full rescan, not single-pass convergence. An in-process retry of the failed keys would close it.
6. One start at the minimum checkpoint across attributes, re-putting already-indexed rows for attributes that were further ahead: index puts are idempotent and the redundant range is at most one interval. Reversible.
7. Custom (HNSW) indexes still yield once per record — unchanged from main's didSynchronousIndexing branch; reviewers flagged it as a throughput ceiling, but it is not this change's to alter.
8. resumeStartKey and setIndexingCheckpointPeriod are exported solely for the unit tests; internal, trivially reversible.

Verification

Route (b): new in-process end-to-end tests through table() → runIndexing → the real primary store on both engines, plus two real process kills. unitTests/resources/indexBackfillConvergence.test.js (with indexBackfillConvergence-crash.js as the child fixture): the four resumeStartKey cases; an interrupted two-attribute backfill resumes with its scan opened at the persisted, key-stamped checkpoint (asserted on the start option and the first key visited, and that the stamp is cleared on completion); a record whose index write throws, rejects asynchronously on a non-last value, or rejects while a later attribute's put resolves freezes the checkpoint before it and the retry revisits and indexes that record; unequal persisted checkpoints resume at the lower one, an absent one forces a full scan, and an unstamped legacy checkpoint is rebuilt from the first record; a child process SIGKILLs itself at its first persisted checkpoint and the parent resumes from that key through the PID-mismatch trigger; a child killed right after the ready descriptor leaves an index every entry of which survives; a rejecting flush parks the index and the retry completes; checkpoints are never closer than the record floor; and a 2000-row plain-index backfill yields every 100 records on RocksDB (a setImmediate ticker stamps every visited key).

Fails-on-base (base dist built from origin/main's databases.ts): the resumed scan's start was undefined (expected the checkpoint) and all 2000 records were visited in one event-loop turn; on the parent commit of the completion barrier, the kill-after-complete test kept 0 of 10,000 index entries. With the fix: 14/14 on RocksDB and 13/13 (+1 skipped) under HARPER_STORAGE_ENGINE=lmdb.

Gates on the final head: npm run test:unit:resources RocksDB 2266 passing / 0 failing; LMDB 1763 passing / 0 failing; npm run test:unit:main 5461 passing / 2 failing (the configValidator domain-socket path-length case, a local worktree-path artifact, and the rsa_keys token-authentication flake, both known on this box and unrelated). Earlier runs on this branch hit the pre-existing longLivedTransactions chain-link flake once per run; it passes alone. oxlint and prettier --check clean on the changed files. Measured: 0.8µs per setImmediate yield; a 100k-row RocksDB backfill ran at 216k rec/s on base and 219–244k rec/s with the fix; an await at the checkpoint instead of the deferred write made the LMDB backfill 8x slower, which is why the checkpoint write is attached to the last put's settlement.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EVCC8xu4zk7eirYMg1ZHPb

Complexity: complicated

_{Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=9 @ 763ba6f}

_{Human-Review-Need: 3 @ 763ba6f}

[github-actions]

Release cherry-pick v5.2: conflict

Cherry-pick onto v5.2 produced conflicts on commit(s): 2e8171fa95598c2c9b6507176d80d8a364533e88

The conflict markers are committed on branch cherry-pick/v5.2/pr-2539.
A pull request has been opened to land this patch: #2551

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to resume secondary-index backfills from a persisted checkpoint rather than scanning from the beginning, and yields the event loop at bounded intervals to prevent event-loop starvation. It also adds comprehensive unit tests to cover crash-resume scenarios and event-loop yielding. The reviewer recommends improving the crash-resume test robustness by wrapping the child process execution in a try/finally block with a Promise.race watchdog timer to prevent deadlocks and resource leaks. Additionally, the reviewer suggests using the codebase's loose equality idiom '== null' instead of '== undefined' for null-or-undefined checks.

[gemini-code-assist]

The setup and execution of the child process, as well as the subsequent assertions, are not fully protected by the try/finally block. If the child process hangs, the test suite will deadlock. If an assertion fails or an error is thrown before the try block is entered, the global database configuration (terms.CONFIG_PARAMS.DATABASES) will not be restored, polluting subsequent tests. Additionally, the child process and the temporary crashDir directory may leak.

Wrapping the entire setup and execution in a try/finally block with a Promise.race watchdog timer ensures robust cleanup and prevents test suite hangs.

		let child;
		let exitTimeout;
		try {
			child = spawn(
				process.execPath,
				[
					path.join(__dirname, 'indexBackfillConvergence-crash.js'),
					path.join(crashDir, 'child-root'),
					path.join(crashDir, 'shared'),
					DATABASE,
					TABLE,
					markerPath,
					String(N),
				],
				{ stdio: ['ignore', 'ignore', 'pipe'] }
			);
			let stderr = '';
			child.stderr.on('data', (chunk) => (stderr += chunk));
			const [code, signal] = await Promise.race([
				new Promise((resolve, reject) => {
					child.once('error', reject);
					child.once('exit', (code, signal) => resolve([code, signal]));
				}),
				new Promise((_, reject) => {
					exitTimeout = setTimeout(() => reject(new Error('Child process exit timeout')), 10000);
				}),
			]);
			clearTimeout(exitTimeout);
			assert.strictEqual(
				signal,
				'SIGKILL',
				`the child should have killed itself at its first checkpoint (exit ${code}): ${stderr}`
			);
			const checkpoint = readFileSync(markerPath, 'utf8');
			assert.match(checkpoint, /^c-\d{6}$/, 'the child should have recorded a durable checkpoint');

			// The dead process's PID on the descriptor is the crash-recovery trigger.
			const Tbl = table({
				table: TABLE,
				database: DATABASE,
				attributes: [
					{ name: 'id', isPrimaryKey: true },
					{ name: 'tag', indexed: true },
				],
			});
			assert.ok(Tbl.indexingOperation, 'reopening after the crash should retrigger the backfill');
			const resumed = observeRange(Tbl);
			try {
				await Tbl.indexingOperation;
			} finally {
				resumed.restore();
			}
			assert.strictEqual(
				resumed.start,
				checkpoint,
				"the resumed scan should start at the crashed process's checkpoint"
			);
			assert.strictEqual(resumed.keys[0], checkpoint);
			assert.strictEqual(
				findDescriptor(Tbl, 'tag').value.indexingPID,
				undefined,
				'the resumed backfill should complete'
			);
			let total = 0;
			for (let i = 0; i < 7; i++) {
				total += (await collect(Tbl.search({ conditions: [{ attribute: 'tag', value: 't-' + i }] }))).length;
			}
			assert.strictEqual(total, N, 'every row should be indexed after the resumed backfill');
		} finally {
			if (exitTimeout) {
				clearTimeout(exitTimeout);
			}
			if (child && !child.killed) {
				child.kill('SIGKILL');
			}
			closeDatabase(DATABASE);
			env.setProperty(terms.CONFIG_PARAMS.DATABASES, databasesConfig);
			rmSync(crashDir, { recursive: true, force: true });
		}

References

1. Ensure temporary test resources (such as directories created for child processes) are cleaned up in a finally block to prevent resource leaks in the event of assertion failures, non-zero exit statuses, or malformed output.
2. When testing operations or promises that may never settle, use Promise.race with a setTimeout watchdog timer as a race guard to prevent hanging the test suite, rather than condition-polling helpers like waitFor which cannot bound an unsettled promise.

[gemini-code-assist]

To adhere to the codebase's loose equality idiom for null-or-undefined checks, please use == null instead of == undefined.

Suggested change

	if (attribute.lastIndexedKey == undefined) return undefined;
	if (attribute.lastIndexedKey == null) return undefined;

References

1. Use loose equality checks ('!= null' or '== null') for null-or-undefined checks. This complies with the style guide and prevents runtime errors (such as 'TypeError') when a value can be 'null' rather than 'undefined' at runtime. ^(link)

[heskew]

TL;DR: this PR fixes convergence (confirmed on a rig), but as written it can bake a permanent silent index gap into the exact nodes it's meant to recover — and there's a second, independent WAL durability gap. Both matter before this recovers a stuck cluster.

✔ Convergence fix — confirmed. 1M-row SIGKILL crash-loop: base resumes at record 0 every cycle; with this PR the checkpoint advances each cycle and the final run gate-verifies complete.

Blocker — resuming a legacy checkpoint can silently drop rows.

• What: the new persist path is correct going forward, but checkpoints already on disk were written by the old path, which advanced lastIndexedKey every 100 records with no hadIndexingErrors gate — so a checkpoint can sit past a record whose put threw.
• Effect: resume starts past that record → completes → clears markers → row stays unindexed, index reads "complete." The exact production failure, baked in.
• Confirmed 3 ways: code trace + Codex + rig (checkpoint at record 299 despite a failure at 20 → this PR leaves 20 missing; clearing lastIndexedKey first recovers 300/300).
• Fix: stamp new-path checkpoints (checkpointCertified), treat unstamped legacy as undefined. The indexingFailed → undefined guard alone misses the interrupted-return path (no marker set there).
• Field recovery: clear lastIndexedKey on all affected nodes unconditionally before deploy.

Separate issue — the index store is WAL-off.

• What: openRocksDatabase defaults disableWAL: true; the checkpoint (descriptor) store overrides to false at every call site, the index store open (~L2213) doesn't.
• Effect: a hard kill can durably advance the checkpoint past index writes never fsync'd → a gap independent of the error-gating fix.
• Fix: flush-gate the checkpoint (persist after flush()), or open the index store WAL-on.

Both surfaced this session; rig harness available if useful.

🤖 Findings from a session rig repro + cross-model (Codex) review.

[kriszyp]

Thanks — the rig repro was exactly the evidence this needed, and both findings are now addressed on this branch.

Legacy checkpoints. Adopted your stamp, bound to the key rather than a boolean: every checkpoint written by the new path carries checkpointCertified = lastIndexedKey, and the trigger resumes only when the two match. That covers both legacy exits — the indexingFailed path and the marker-less interrupted return you pointed at — so no lastIndexedKey cleanup is needed before deploying; an unstamped checkpoint is a full rebuild (logged as reindex reason uncertified-checkpoint). The key binding also means an older binary that round-trips the field while advancing the key cannot produce a false certificate. Covered by the unstamped-legacy case in indexBackfillConvergence.test.js.

WAL-off index store. Went with the flush gate: a checkpoint is persisted only after rootStore.flush() on RocksDB, at most once per 5s and never before 10k more records (one flush seals every column family, so a slow backfill must not impose the rate on unrelated tables), with at most one flush in flight and one queued per database (a flush only covers writes issued before it started, so a caller never joins one already running). The completion path flushes the tail before persisting the ready descriptor and parks the index if that flush fails — a kill right after completion lost every unflushed entry before, on main too. Both are proven by child-process SIGKILL tests with no manual flush.

Your harness would be a good cross-check on the shipping policy (5s / 10k); the unit tests exercise it only at a 10k floor.

— Claude Fable 5.1

[claude]

Suggestion (non-blocking): env.setProperty(terms.CONFIG_PARAMS.DATABASES, ...) (line 441) mutates process-wide config before the try that restores it starts at line 472. runCrashChild's own 60s watchdog now bounds the child-hang case gemini flagged on this thread, but the restore gap itself is still open: if runCrashChild rejects, or either assert on lines 455/461 throws, env.setProperty(terms.CONFIG_PARAMS.DATABASES, databasesConfig) never runs, leaking the test's DATABASES override into every later test in this worker. The sibling test at lines 518-538 ("flushes the tail...") has the same shape. Moving the config swap inside the try (or wrapping the whole body in an outer try/finally) would make the restore unconditional.

[claude]

Reviewed; no blockers found. One non-blocking suggestion posted inline.