fix: TTL eviction/delete no longer orphans secondary-index entries (F-149)

[kriszyp]

Resolves #1894.

Problem

Record removal — the background TTL/expiration eviction sweep and plain deletes — calls updateIndices(id, existingRecord, null) to clear that record's secondary-index entries. With record === null, the line const value = record && (...) resolved the new indexed value to null rather than "absent". Because indexNulls defaults to true for every @indexed attribute, getIndexedValues(null, indexNulls) returns [null], so updateIndices removed the real [value, id] index entry and then immediately re-added a [null, id] entry — a dangling index entry (raw key null, value = the id of a now-deleted record).

On RocksDB an @indexed TTL table left 100% of evicted rows dangling in the raw index (unbounded index bloat / phantom hits under a null-valued query). The classic search_by_value oracle can't observe it — it joins each index hit back through the primary record and drops the hit when the record is gone — which is why "phantom=0" read as clean for so long.

Note on the root cause vs the issue's trace. The issue (F-149) traced this to the eviction batcher's transaction being bound to primaryStore.store. That trace is incorrect — normal deletes use the same primaryStore.store-bound raw transaction and persist fine, and the leak reproduces on the per-record evict() path too, single- and multi-worker. It's engine-agnostic, in shared updateIndices(), and — because updateIndices is shared — it also affects plain deletes, not just eviction.

Fix

A removed record has no values to index, so resolve value to undefined (not null) when record == null → nothing is re-added after the real entry is removed. A record that is present but whose attribute is genuinely null is a different case (record is a truthy object) and still indexes under null, so the indexNulls feature is preserved.

-			const value = record && (resolver ? resolver(record) : record[key]);
+			const value = record == null ? undefined : resolver ? resolver(record) : record[key];

Test

Adds integrationTests/database/eviction-index-null-reindex.test.ts (RocksDB) with a direct raw-index-DBI oracle — reads t.indices[field].getRange() with no join, so the [null, id] orphan is visible — plus a guard that a present record with a genuinely-null field stays indexed under null. Fails without the fix, passes with it.

Verification

• P-398 (plain @indexed) and P-399 (FK-target) reproducers: index fully cleared, 0 dangling (was 750 / 241).
• Green: eviction-secondary-index, indexed-update-churn, compound-index-conditions, delete-update-race, indexed-numeric-range, bigint-indexed-range, vector-index (unit + integration), longtxn-secondary-index; LMDB eviction path unaffected.

Cross-model review

Codex + Gemini + Harper domain pass. Core fix confirmed correct across all updateIndices callers and the HNSW custom-index path. One Gemini "blocker" on the adjacent existingValue line was refuted — that line's null result for a null existingRecord is intentional asymmetry (it's what makes removal clean up the [null,id] entry), it's unchanged by this PR, and the described skip never triggers on the real insert path. Test-file lint/format/flake findings were addressed.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request fixes a regression (harper#1894 / F-149) where TTL eviction or record deletion left dangling secondary-index entries. In resources/Table.ts, when a record is removed entirely, the indexed value now correctly resolves to undefined instead of null, preventing the incorrect re-addition of a [null, id] entry. A new integration test suite has been added to verify this fix. The reviewer feedback suggests replacing a fixed 4-second sleep in the integration test with a converging poll to avoid potential test flakiness on CI.

[gemini-code-assist]

Instead of using a fixed 4-second sleep to wait for background operations (such as database reindexing) to complete, wrap the verification checks in a converging poll (retrying until the expected state is reached). This prevents race conditions and test flakiness on CI.

References

1. When verifying background operations (such as index backfills or database reindexing) after a service restart in integration tests, wrap the verification checks in a converging poll (retrying until the expected state is reached) to prevent race conditions and test flakiness.

[claude]

Reviewed; no blockers found.

[claude]

Suggestion (non-blocking): the fixed await sleep(4_000) here waits a hardcoded window for the background sweep to (not) touch these permanent rows, unlike the polling loop used just above in the eviction test (lines 95-102). A slow CI runner could still be mid-sweep when the assertions fire, causing intermittent flakiness. Consider a bounded poll (e.g. checking primaryIds/rawIndex stabilize at 8 for N consecutive checks) instead of a single fixed delay, consistent with the convergence-poll pattern already used elsewhere in this file.

[cb1kenobi]

Nit: regression coverage exercises only eviction, not the two claims the fix also makes

The fixed line in updateIndices is shared, so proving it on the TTL-eviction path transitively covers explicit delete and composite/multi-attribute indexes. But the PR title says "eviction/delete" and the writeup claims composite coverage, and neither is asserted directly here — a future change that diverges the delete path or a resolver-backed (composite/computed) index would slip through. Two cheap additions would lock in both claims against the same raw-DBI oracle:

• an explicit DELETE /Expiring/<id> (or t.delete(id)) case asserting the [value,id] entry is gone with no [null,id] orphan;
• a table with a composite/multi-attribute (resolver-backed) @indexed field, confirming removal clears the composite entry too.

Not blocking — the core fix is correct and the eviction oracle fails without it.

—
Generated by Barber AI