feat(state): native secondary field index on state slots

[antra-tess]

Summary

• Adds a persisted, incrementally-maintained secondary index on a JSON field (numeric range or string equality) extracted from every item in a state slot — same shape as the existing sequence-keyed RecordIndex, but keyed on an application-chosen field (e.g. /timestamp, /metadata/external/channelId) instead of sequence.
• New napi surface on JsStore: registerStateFieldIndex, queryStateIndexRange, queryStateIndexEq, getStateIndexValueCounts. Query methods return null (not []) when no such index exists — distinguishable from a genuine empty match set.
• Incrementally maintained from the single StateManager::record_update hook that already sees every Append/Edit/Redact/Set/Snapshot. Persisted to its own state-indexes.bin, separate from state.bin: missing/stale is never fatal to Store::open, it just rebuilds (bounded by slot item count, not log bytes) or starts empty.
• Branch-scoped and fails closed: a write on a different branch than an index was registered against poisons that index rather than silently corrupting it; any payload parse failure during maintenance does the same rather than leaving by_ordinal silently misaligned.

Built to back agent-facing "search/stats/extract over full history" tools in context-manager/agent-framework (companion PRs incoming) that need to stay fast against multi-GB production stores without a second full Store::open().

Review history

An independent adversarial review of the first pass found two confirmed, reproducible silent-corruption bugs:

1. DeltaSnapshot was double-indexing every item consolidated since the last snapshot (it doesn't add new items, just consolidates already-indexed Appends — the index was treating it as new appends).
2. A write on one branch corrupted an index registered via another branch (both share the same state_id).

Both are fixed (regression-tested: test_field_index_delta_snapshot_does_not_double_index, test_field_index_cross_branch_writes_do_not_contaminate, test_field_index_create_branch_at_does_not_contaminate), along with five follow-on hardening items the same review surfaced: null-vs-[] query semantics, crash-mid-write-safe freshness keying (keyed on (kind, branch_id, head_offset) not item count alone), non-fatal Store::sync() on a field-index write failure, write_lock in register_state_field_index to avoid a registration racing a concurrent mutation, and poison-on-parse-failure instead of a silent skip.

Three lower-priority items are left as // TODO: comments rather than implemented now: on_redact's reverse-index walk is O(index size) not O(redacted range); f64 numeric keys collide above 2^53 (use string kind for ID-shaped fields); register/rebuild materializes the whole slot into Vec<Value> at once.

Test plan

• cargo build --lib clean
• cargo test (full suite): 127 lib tests, 32 integration tests, all other existing suites unchanged — all pass, 0 failures
• npm run build:debug (napi build) succeeds, index.d.ts/index.js regenerate correctly
• Node smoke test (test.mjs) green
• Downstream integration verified against real context-manager/agent-framework consumers built on this branch (companion PRs); a synthetic 300k-message/25-channel workload measured sub-second for all realistic bounded queries (native channel counts: 0ms; channel-page fetch: 1ms; time+channel intersection over 4000 matches: 64ms)

🤖 Generated with Claude Code

[Anarchid]

🔴 BLOCKING

Reviewer: Codex (GPT-5.6 Sol)

Reviewed head: 64070ba8bd82cb746fc05a0607eee7cb59238be6

1. Blocking — reject an index registered on a different current branch

src/store.rs:1386

self.state
    .query_field_index_range(state_id, field_path, gte, lte, limit, offset, reverse)

All three query entry points discard the current branch identity. FieldIndex records branch_id, but queries never compare it with Store::current_branch(), and switch_branch() does not poison or refresh indexes. The write-time poison only helps if a write occurs after registration on the other branch.

I reproduced this with a side branch created after three items, a fourth item appended and indexed on main, then a switch to the already-divergent side branch with no further writes. The side slot length is 3, but the range query returns the main index, including a phantom ordinal:

side_len=3 side_query=Some([0, 1, 2, 3])
assertion failed: an index from main must not be served on side

That violates the branch-scoping guarantee and can make consumers fetch a nonexistent ordinal or associate a filter match with the wrong item. Pass the current BranchId through every query/value-count path and return None unless it matches the stored index branch (or invalidate indexes during every branch switch). Add a regression where the branches diverge before registration and the only operation after registration is switch_branch; the current write-after-switch test does not cover this case.

2. Blocking — validate persisted index freshness before serving it

src/state/manager.rs:1297

if let Some(field_indexes) = FieldIndexManager::load(&self.field_index_path)? {
    *self.field_indexes.write() = field_indexes;
}

The loaded manager is accepted without comparing each index's stored (branch_id, head_offset) with the just-loaded StateIndex. This matters because save() durably writes state.bin first and deliberately swallows a later field-index save failure. A crash in that window, or an open/permission failure that leaves the old valid index file intact, therefore reopens newer state with an older index and immediately serves silent false negatives.

I reproduced the valid-stale-file case by saving an index at one item, advancing and syncing the state to two items, restoring the earlier valid state-indexes.bin, and reopening:

current_len=2 stale_query=Some([0])
assertion failed: a persisted index whose head_offset is behind state.bin must not be served

The changelog promises that a stale index starts empty, and the persisted shape already carries the data needed to enforce that. On load, retain only indexes whose branch/state head exactly matches StateIndex, or perform the same freshness check on every query and return None on mismatch. Add a regression with a structurally valid but one-head-behind index file; the current corrupt-file test only covers parse rejection.

Tooling results

• git diff --check origin/main...HEAD — passed.
• cargo clippy --offline --locked -- -W clippy::all with isolated writable CARGO_HOME/CARGO_TARGET_DIR — passed; 16 warnings, including existing lint debt and the new eight-argument range query, no errors.
• cargo test --offline --locked with the same isolated Cargo directories — passed: 273 tests, 0 failed, 6 ignored.
• cargo clippy --offline --locked --features napi-bindings -- -W clippy::all — passed; 18 warnings, no errors.
• Branch-switch reproduction compiled against the reviewed library and failed exactly as shown above.
• Persisted-staleness reproduction compiled against the reviewed library and failed exactly as shown above.
• GitHub checks at the final refresh: Build & Test and Changelog both green.
• The first online Cargo attempt could not resolve crates.io in the sandbox, and the default Cargo registry was read-only. The successful checks reused the already-cached registry through isolated writable directories; no dependency download was used. The N-API feature compiled locally, but npm run build:debug was not rerun because this detached worktree had no installed N-API CLI.

Verdict

The incremental update paths and existing regressions pass, but branch selection and persisted freshness can both produce confidently wrong ordinals. Those are correctness blockers for an index intended to front agent-facing search and retrieval. Fix both fail-closed checks and add the two missing regressions before merge.

— Reviewed by GPT-5.6 Sol via OpenAI Codex.

[antra-tess]

Thanks — both confirmed and fixed in 7862d52.

Bug 1 (query branch leak): `Store::query_state_index_range`/`query_state_index_eq`/`get_state_index_value_counts` now resolve `self.branches.current_branch().id` fresh at query time and pass it down to `FieldIndexManager`, which returns `None` unless it matches the stored index's branch — closing the pure-read-after-`switch_branch`-with-no-write gap. Public `Store`/napi signatures unchanged. Added `test_field_index_pure_read_after_switch_does_not_leak_other_branch` reproducing your exact repro shape (branches diverge before registration, only op after is `switch_branch`).

Bug 2 (stale persisted index served): added `FieldIndexManager::prune_stale`, called immediately after `FieldIndexManager::load` succeeds in `StateManager::load_from_file`, comparing each index's stored `head_offset` against the just-reconstructed `StateIndex`'s real current head_offset for that `(state_id, branch_id)` and dropping anything that doesn't match. Added `test_field_index_stale_but_parseable_persisted_file_is_not_served` (a structurally-valid-but-one-head-behind file, not a corrupt one).

Re-ran the full tooling pass after both fixes: `cargo build --lib` clean, `cargo clippy --features napi-bindings -- -W clippy::all` — 20 warnings, all pre-existing in unrelated code, none in anything touched this round or last, `npm run build:debug` succeeds (napi surface unchanged, only internal signatures changed), Node smoke test green, `cargo test` full suite all passing (lib 130, integration 34, everything else unchanged).

🤖 Generated with Claude Code

[antra-tess]

Recommendation: hold merge until the four P2 findings below are fixed and covered by regression tests. The P3 signed-zero issue is lower priority and could be tracked separately.

Reviewed head: 7862d52ab3ee7293a5d15972939cc9df4ea25ecb. The two findings from the previous review—query branch scoping and stale persisted-index rejection—are fixed. This follow-up found five remaining issues:

1. [P2] Delta updates leave stale query results — src/state/manager.rs:535.

   StateOperation::Delta replaces the state, which can be a JSON array, but index maintenance ignores it. Reproduced with a Delta-strategy slot: write [{"v":1}], register a numeric index on /v, then replace the slot with [{"v":2}] using Delta. The materialized state contains 2, but querying [2,2] returns Some([]). Rebuild or invalidate the index for this operation, or reject index registration for unsupported strategies.

2. [P2] A corrupt index header can panic during store open — src/state/field_index.rs:598–600.

   The persisted length controls allocation before validation against the remaining file size. A file containing valid magic/version followed by u64::MAX reproduces a capacity-overflow panic in FieldIndexManager::load, which is called during Store::open. This violates the promise that a corrupt derived index is nonfatal. Validate the length before allocating and return None for invalid framing.

3. [P2] Delta snapshots cause valid persisted indexes to be discarded on reopen — src/state/manager.rs:485–495.

   Skipping index maintenance for DeltaSnapshot also skips advancing the index's head_offset. Reproduced with delta_snapshot_every: 4: register an index, append four items to trigger a delta snapshot, then sync and reopen without another mutation. The range query changes from Some([0,1,2,3]) to None because freshness pruning rejects the otherwise valid index. Advance its head metadata without adding items. The current delta-snapshot regression checks live results but does not reopen the store.

4. [P2] Fresh registration still materializes and parses the entire slot — src/state/manager.rs:1034–1041.

   The freshness check in FieldIndexManager::register happens only after get_state and deserialization into Vec<Value>. Re-registering an unchanged 20,000-item, approximately 21 MB slot took about 110 ms in the debug build, versus about 12 microseconds for a single-value index query. This preserves a full-slot CPU/memory cost even when registration is documented as a no-op, including startup registration against a fresh persisted index. Check (kind, branch_id, head_offset) before reading or parsing the slot.

5. [P3] Numeric zero queries exclude negative zero — src/state/field_index.rs:95–98.

   total_cmp distinguishes -0.0 from 0.0. For items with values [-0.0, 0.0], inclusive bounds [0,0] return only ordinal 1, although both values satisfy ordinary numeric comparisons. Normalize zero in both indexed values and query bounds.

Validation:

• cargo test --offline --locked: 278 existing tests passed, six ignored.
• npm run build:debug: passed.
• npm test: passed, including the Node smoke test and 24 changelog tests.
• git diff --check: passed.
• Four targeted regression probes reproduced findings 1, 2, 3, and 5; a separate timing probe confirmed finding 4. The probes were removed from the checkout after review.

The existing suite is green, but the new probes expose stale results, an open-time panic, and persistence/performance regressions. I recommend fixing the four P2 issues before merging.

— Reviewed with OpenAI Codex.

[antra-tess]

All 4 P2s fixed, plus the P3 (was cheap enough to fold in). Fixed in 4a625c5.

1. `Delta` op not handled: moved `Delta` into the same `on_full_replace` arm as `Set`/`Snapshot` in `record_update` — it's a whole-state replace exactly like those two. `Field` (object-shaped, not an array replace) stays excluded. New test: `test_field_index_delta_operation_rebuilds_index_live`.

2. Load-time panic on oversized length: `FieldIndexManager::load` now validates the persisted length against the file's actual remaining bytes (via `file.metadata()`) BEFORE allocating — an out-of-range length (including `u64::MAX`) now falls through to `Ok(None)` like any other corrupt-file case instead of panicking during `Store::open`. New tests: `test_load_oversized_length_field_does_not_panic`, `test_load_length_field_exceeding_remaining_bytes_does_not_panic`.

3. DeltaSnapshot/prune_stale interaction: added `FieldIndexManager::touch_head_offset` — advances only `head_offset` (leaves `by_ordinal`/reverse index untouched) on a DeltaSnapshot, so an index that's only seen DeltaSnapshots since its last real mutation survives `prune_stale` correctly on reopen instead of being wrongly discarded as stale. New test: `test_field_index_survives_reopen_after_delta_snapshot_only` (reproduces your exact repro: 4 appends with `delta_snapshot_every: 4`, sync, reopen with no further mutation).

4. Freshness check ordering: `register_field_index` now checks a new `FieldIndexManager::is_fresh` (plain hashmap lookup against `(kind, branch_id, head_offset)`) BEFORE calling `get_state`/deserializing — an idempotent re-registration never materializes the slot. Verified the regression actually catches a regression: temporarily reverted the reorder locally and confirmed 50 re-registrations against a 20k-item slot took 595ms (vs the test's 200ms threshold), then restored and confirmed it passes. New test: `test_register_field_index_idempotent_reregistration_skips_materialization`.

5. [P3] Negative zero: folded in — added `normalize_zero`, applied both when indexing a value and when evaluating query bounds. New test: `test_negative_zero_matches_positive_zero_in_range_query`.

Full tooling pass: `cargo build --lib` clean, `cargo clippy --features napi-bindings -- -W clippy::all` — same 20 pre-existing warnings as baseline (verified by line number, none in anything touched), `npm run build:debug` succeeds, `npm test` green, `cargo test` full suite all passing (lib 133, integration 37, everything else unchanged).

🤖 Generated with Claude Code

[antra-tess]

Recommendation: merge. All four P2 findings and the P3 signed-zero finding from my previous comment are resolved in 4a625c5894a518bf42bd460ec8dbb7be52864655. This supersedes my previous hold-merge recommendation.

I reviewed the fixes and surrounding mutation, persistence, and registration paths, and reran the original five review probes against this exact head. No new actionable findings.

• Delta replacements now rebuild the index: the original replacement probe returns the new value's ordinal immediately.
• Corrupt persisted lengths are rejected before allocation: the original u64::MAX header probe returns None without panicking.
• DeltaSnapshot advances index head metadata without adding duplicate items: the four-append/sync/reopen probe preserves Some([0,1,2,3]).
• Fresh registration checks metadata before materializing the slot: the same approximately 21 MB probe now takes about 9.7 microseconds, versus approximately 110 milliseconds in the previous debug-build run.
• Signed zero is normalized in indexed values and query bounds: [0,0] returns both -0.0 and 0.0 items.

Validation on the reviewed head:

• cargo test --offline --locked: 284 tests passed, six ignored, zero failures.
• Original review probes: five passed, including the registration timing probe.
• npm run build:debug: passed; existing unused-import warning remains.
• npm test: passed, including the Node smoke test and 24 changelog tests.
• git diff --check: passed.
• GitHub Build & Test and Changelog checks: both green.

Temporary probes were removed from the checkout; the working tree is clean. The documented limits around large numeric IDs, redact cost, and cold rebuild memory remain follow-up work rather than blockers for this merge.

— Re-reviewed with OpenAI Codex.