Harden reindex and KNN serving against crash residue (travsr#735 follow-up) - #21
Conversation
Abhishek5517
left a comment
Abhishek5517
left a comment
There was a problem hiding this comment.
Review: empirical + E2E (before/after on the real load path)
Built this branch, ran the suite, and reproduced the corrupt-index recovery end to end against a real index (the travsr repo's own graph.db / embed.db, arctic-embed-m-v1.5, copied to scratch).
Tests
- Full suite: 64 passed / 0 failed. Verified the new tests run and pass:
try_load_rejects_truncation_residue_cleanly,failed_review_keeps_serving_the_old_index,
quarantine_moves_corrupt_file_aside,reload_due_throttles_attempts, and the NULL/mistyped pending-row decode tests.
E2E: defect 3 (corrupt index on the --reindex load path)
Seeded 800 pending nodes, overwrote the code .hnsw.usearch with residue, and ran the identical input through both binaries:
- Shipped v1.4.0:
reindex failed: load existing HNSW index: load usearch index from disk: Failed to read vectors, exit 1, and the corrupt file was left in place. On a daemon this is the respawn-every-tick poison loop this PR targets. Reproduced. - This branch: detected
100 bytes, below the 512 byte minimum any published index can have, quarantined the file to.hnsw.usearch.corrupt, rebuilt a valid 23 MB index from embed.db, embedded all 800 pending, exit 0.
Also confirmed:
- Re-quarantine replaces the previous
.corruptcopy (only the latest is kept: md5 rotated, one file on disk). - The rebuilt index is queryable (a follow-up reindex reports "up to date", 0 pending).
- The
.usearch.tmp/.usearch.corruptnames match the realsave()path (with_extension("usearch.tmp")/("usearch.corrupt")), so the sweep and quarantine target the right siblings.
One caveat on the SIGSEGV claim
With the 100-byte residue I used, native usearch returned a clean Err rather than SIGSEGV, so I did not trigger the native crash locally (it is byte-pattern dependent, and your CI runners hit it). The exit-1 poison loop and the recovery are clearly demonstrated. The 512-byte floor cannot catch large-but-corrupt garbage, so a residual native-crash window remains on the serve path, which the constant's docs already acknowledge and the respawn cap bounds. Worth keeping the native view/load audit on the list as the PR notes.
Cross-PR conflict (please coordinate with #22)
This branch and #22 both modify src/main.rs and both add the identical blob_to_f32 as_chunks change to src/model.rs. A trial merge of #22 onto this branch conflicts in src/main.rs (two adjacent new #[cfg(test)] modules inserted before mod query_memo_tests). It is a trivial keep-both resolve, but #22's description currently says it "touches different code", which is not accurate. Whoever merges second will need to resolve it.
Verdict
Solid, and the recovery behavior is exactly right. No blocking issues.
Abhishek5517
left a comment
Abhishek5517
left a comment
There was a problem hiding this comment.
Approving. Verified the corrupt-index recovery before/after on the real --reindex load path: shipped v1.4.0 fails exit 1 and leaves the poison file, this branch quarantines and rebuilds cleanly. Full suite green (64). See my detailed comment above. Non-blocking: please coordinate the src/main.rs conflict with #22.
…ow-up) A reindex killed without cleanup leaves state that previously poisoned every later run: 1. A partial/corrupt .hnsw.usearch made every subsequent reindex fail at load and exit 1, and the daemon respawned one per tick, forever. The reindex write path now quarantines an unloadable index as .corrupt (keeping only the latest copy) and rebuilds from embed.db; stale .usearch.tmp files from a save killed between write and rename are swept. Write-path only: the serving path never deletes a file a concurrent reindex may be about to replace. 2. The daemon-mode KNN path retried a failed index reload on every call, forever, and re-mapped the file per query while a reindex kept publishing it; both are native-index construction loops. Reload attempts are now throttled to one per 5 seconds (reload_due), and a served (mmap) index keeps answering from the previous mapping when a re-view fails instead of degrading to an empty index; the previous free-before-load behavior is kept for RAM-copy handles, where the 2x transient is the real concern. 3. Pending rows whose text columns failed to decode were silently dropped by filter_map(r.ok()) and re-selected by NOT EXISTS in every later chunk; a chunk of only-bad rows ended the run 'successfully' with pending work the daemon respawned forever. Decoding is now NULL-tolerant (pending_row_to_text) so every selected row is embedded and inserted, residual failures are counted and logged, and an all-undecodable chunk aborts loudly. Tests: reload throttle, quarantine round-trip, keep-old-mapping on failed re-view (unix), and NULL/mistyped pending-row decode.
The keep-old-on-failed-reload test exposed on CI that usearch's native view()/load() parse the file header without validation and SIGSEGV on truncated bytes (observed on Linux and macOS runners). An error return was the assumed failure mode; a native crash bypasses both the quarantine (write path) and keep-old (serve path) handling entirely. Add a Rust-side plausibility gate before any native parsing: every legitimately published index holds at least one f32-384 vector (1536 bytes of data alone) and is written via save-to-tmp + rename, so a file under 512 bytes is always truncation residue from a killed process. try_load rejects it with a clean Err (the reindex caller quarantines), try_serve likewise, and knn's throttled reload keeps the current index and retries after backoff, now checked BEFORE freeing anything so even the load() branch no longer trades a live index for junk. The gate cannot catch large garbage; that residual native-crash risk is bounded by the host's respawn cap and noted in the constant's docs. The keep-old test now exercises the gate deterministically on every platform (no native parsing of garbage), and a new test pins try_load's clean rejection. The reload body moves to reload_from_disk for the early-return shape; behavior is otherwise unchanged.
Hand-testing a locally built binary showed the entry overstated defect 1: the host always spawns the sidecar with --parallel, and that path never loads the existing index (it rebuilds and renames at end of run), so a corrupt index already self-healed there. The load-and-fail behaviour is real only on a direct --reindex without --parallel. Also promotes the truncation-residue gate to its own entry: a native SIGSEGV on partially-written index bytes is the most serious of the four and was missing from the list entirely.
6d5ebad to
4821df2
Compare
|
Rebased onto main now that #22 has merged, resolving the conflict flagged in review on Travsr-com/travsr#754. Both PRs inserted a test module at the same point in Verified locally on Windows after the rebase: rustfmt clean, clippy |
2 participants
Sidecar-side follow-up to Travsr-com/travsr#735 (host-side fix: Travsr-com/travsr#754). A reindex killed without cleanup leaves residue — a large un-truncated
embed.db-wal, a possibly partial.hnsw.usearch, and orphaned lock metadata — that the sidecar handled badly. A live occurrence was observed on macOS as a daemon-mode sidecar at a 24 GB memory footprint with a 440 GB virtual size.Changes
1. Truncated index bytes could SIGSEGV the process. usearch's native
view()/load()parse the file header without validating it, so a partially-written.hnsw.usearchcould crash the sidecar outright instead of returning an error. This is not theoretical: it reproduced on this PR's own Linux and macOS CI runners once a test began feeding garbage to a served index. Every published index holds at least one f32-384 vector and is written save-to-tmp + rename, so a file below a 512-byte floor is always truncation residue; it is now rejected in Rust before any native parsing, on the serve path, the load path, and the KNN reload path. The floor cannot catch large garbage, which is called out in the constant's docs.2. KNN reload throttle + keep-old-on-failure (daemon mode). A failed reload was retried on every KNN call forever, and a rapidly republished file was re-mapped per query; both construct a fresh native index per query. Reload attempts are now throttled to one per 5 seconds (
reload_due, pure and unit-tested), and a served (mmap) index keeps answering from its previous mapping when a reload is rejected instead of degrading to an empty index. The #736 C2 free-before-load behaviour is preserved for RAM-copy handles, where the 2x transient is the real concern.3. Corrupt-index quarantine on the load-based reindex path. A direct
--reindexwithout--parallelcallstry_loadon the existing index and previously failed and exited 1 on every attempt, with nothing ever cleaning the bad file. That path now quarantines it as.corrupt(keeping only the latest) and rebuilds from embed.db; stale.usearch.tmpfiles are swept. Scope note: the--parallelpath the daemon always uses never loads the existing index — it rebuilds and renames at end of run — so it already self-healed. Both behaviours were verified by hand against a locally built binary.4. NULL-tolerant pending-row decode. Rows whose text columns failed to decode were silently dropped by
filter_map(|r| r.ok())and re-selected byNOT EXISTSin every later chunk; a chunk of only-bad rows ended the run "successfully" with pending work the daemon respawned forever.pending_row_to_text(shared by both reindex paths) now tolerates NULL/mistyped columns so every selected row is embedded and inserted, residual failures are counted and logged, and an all-undecodable chunk aborts loudly.Tests
New: reload-throttle decision table; quarantine round-trip (including replacing a prior quarantined copy);
try_loadrejecting truncation residue cleanly; a served index continuing to answer after a rejected reload (all platforms — the plausibility gate fires before the view/load split); NULL and mistyped pending-row decode against in-memory SQLite. Full suite 64 passed / 0 failed on Windows in release mode (per this repo's platform notes); clippy-D warningsand rustfmt clean; CI green on Linux, macOS and Windows.Also included:
blob_to_f32migrated fromchunks_exact(4)toas_chunks::<4>(). CI runners moved to Rust 1.98, whose newchunks_exact_to_as_chunkslint (denied via-D warnings) fails on that pre-existing line;mainfails the same way today. Semantics are identical andas_chunksstabilised in 1.88, below this repo's 1.91 floor.Manual verification
Against a locally built binary on Windows: killed a reindex host mid-run to produce the real residue (
embed.lock.infonaming a dead PID, un-truncated WAL, orphaned sidecar), then confirmed the next reindex recovers the stale metadata, completes all 1320 pending nodes, and releases the lock cleanly. Corrupt-index behaviour confirmed separately on both reindex paths.Notes for review