fix(history): make the durable set answerable, not guessed (#286, #296) - #300
Conversation
Two places answered a different question than the one asked, and both turned a real failure into a startup that looks fine. The no-op-deletion skip asked "is this pair in memory?" when the question is "could it be in the durable set?". An entry evicted for capacity is gone from the maps while the last checkpoint still holds it, so a ForwardDelete on it wrote no tombstone, posted no scrub, and came back on the next launch (#286). `UserHistory` now tracks the keys that eviction — or a deletion whose durability was never established — left behind, and the skip consults both. Tracking keys rather than a single "something was evicted" bit is what keeps the fast path: a bit is permanently set once a heavy user reaches max_unigrams, which would put a 5.9ms p50 flush plus a full checkpoint on every ForwardDelete of a never-learned candidate (measured via profile_apply_records_lock_hold). Each key carries the epoch it was raised at; the covering pass retires a key only if that epoch has not moved since the saved snapshot was cloned, because a set alone cannot tell a key re-raised mid-write from one raised before it, and retiring that key would reopen the bug. The failed v1->v2 migration commit reported a clean startup (#296). It set none of the report's states, so is_clean() answered true, every Swift branch fell through, and the WAL was frozen for the rest of the session. The report now derives two facts instead of relying on each branch to remember: migration_failed from the migration's own outcome, and appends_frozen once from wal.is_frozen(). Deriving matters — the Err branch only freezes when a legacy WAL was consumed, so a v1 checkpoint beside a fresh WAL failed the commit without freezing anything, and an is_frozen()-based report would have missed that half entirely. It also now schedules the heal: a compaction writes exactly the checkpoint whose write failed. Both fields reach the log line, which otherwise reads "checkpoint=Loaded wal=Clean" — indistinguishable from a healthy start. HistoryWal::frozen becomes an Arc<AtomicBool> so the state can be read off the mutex without a hand-synced mirror; the follow-up needs that for the UI thread, and a mirror with an update-point list is the shape that caused #296 in the first place. Checkpoint format unchanged: to_data/from_data enumerate their fields, so the new state cannot reach the file (pinned by a test). Refs #286, #296
…ht seam /simplify pass over the previous commit. Four review angles converged on the same set; behaviour is unchanged and every regression pin still fails under mutation. - Fold the deletion-side raise into `apply_batch`. `seq: None` already means "no frame reached the WAL" and already gates `applied_seq`; the residue is the same witness, so raising it there deletes the engine-side accumulator, a per-tombstone clone, and both comments that existed only to justify hand-ordering the raise against the apply. - Collapse the deletion check into one predicate, `deletion_has_durable_target`. Two public halves the caller ORs together is the shape of the bug being fixed — answering only the in-memory half is exactly #286. - Nest the residue maps like `UserHistory`'s own, so probing borrows instead of building owned tuple keys. The neighbouring `contains_entries` documents avoiding precisely that allocation inside the wal mutex; the first version contradicted it on the same code path. - One definition of the bigram key walk (`bigram_keys`), now shared by record / remove / contains / residue. It had grown to five hand-rolled copies of `pair[0].1 / pair[1].0 / pair[1].1`. - Revert `HistoryWal::frozen` to a plain `bool`. The `Arc<AtomicBool>` had no consumer — it was staged for the follow-up, and an Arc bound to one `HistoryWal` instance is its own drift risk. `set_frozen` stays: the single write path is what the change was actually worth. The follow-up lands the sharing mechanism with the reader that needs it. - `reset_durable_residue` was a provable no-op (`*h = empty` already installs a default), `durable_residue()` existed only to feed its own covering call, and `evict` had been widened to `pub` with no caller outside the module. All three gone; `DurableResidue` is private again. - Derive the residue cap from `max_unigrams + max_bigrams` instead of restating 10_000, and correct its doc: the residue is populated in normal at-capacity operation, not only when the disk is failing. - `evict_map` moves the owned keys it already has instead of cloning them. - Surface `migrationFailed` / `appendsFrozen` in all three Swift log arms. Both can co-occur with a migration or a quarantine, and those arms returned first — the fix was one branch away from the bug it fixes. - `compaction_recommended` reads `report.appends_frozen` rather than re-deriving `wal.is_frozen()` 38 lines after recording it.
Four axes on the diff; no CRIT, and the IMP findings clustered on two
things the earlier passes had left inconsistent.
Structure — `cover_durable_residue` now runs under the wal mutex, folded
into the lock the truncation takes three lines later. It was the only
`inner` mutation outside that mutex, which quietly falsified the
invariant the deletion-skip check cites for being race-free ("every
history mutation runs under the wal mutex") and the design memo's promise
that swapping the RwLock for an ArcSwap stays local. The retain costs a
few hundred µs at capacity, once per compaction — cheaper than an
exception in that invariant. Measured after the move (M4, 20k entries, 2
contending readers): Committed p50 467µs / p99 1.26ms, Tombstone p50
5.18ms / max 8.29ms — at or below the pre-change baseline.
Reachability — the residue cap is a constant again. Deriving it from
`max_unigrams + max_bigrams` looked tidier but is wrong: those are
user-settable while the compaction that clears the residue fires on a
fixed frame threshold, so a small-capacity configuration would saturate
during ordinary typing on a healthy disk, reinstating the per-delete
flush the per-key design exists to avoid. Sized so one compaction
interval cannot reach it, saturation stays a symptom of checkpoints
failing. It also now warns when it latches; every other degraded
transition in the module does.
Accuracy — the log line said the failed migration "retries next launch",
but this same change added `migration_failed` to
`compaction_recommended`, so a startup compaction retries it in-session.
Corrected in all three places that carried the wording. `set_frozen`'s
doc claimed to prevent unreported freezes; it is a plain assignment and
enforces nothing — what closes that class is deriving `appends_frozen`
from `is_frozen()` once, after every branch. The residue's "exactly two
routes" note now says which case it deliberately excludes and why.
Also: drop `Debug` from `DurableResidue` (it holds raw readings and
surfaces, and neither `UserHistory` nor `HistoryEntry` derives it, so
user input cannot reach a `{:?}`); route `record_segments_at` through
`bigram_keys` — it writes the keys the residue matches against and was
the one site still spelling them by hand; stop allocating `prev_surface`
in `remove_entries`; refresh the AGENTS.md settled notes, which still
named a method `/simplify` removed and rebutted only one of the two folds
#296 proposed; SPEC now describes the saturation fallback and cites the
design memo by name rather than a section number SPEC does not have.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
15 findings across 10 finder angles plus a gap sweep. The sweep caught a regression the earlier commits introduced, and three mutation probes showed the suite was not guarding what its tests claimed. The regression: adding `migration_failed` to `compaction_recommended` routed the v1->v2 conversion through `run_compact_impl`, which writes a v2 checkpoint straight over the v1 file with none of the migration commit's preconditions -- no `.v1.bak`, no `Migrated` state. On the path where the commit failed because the directory was unwritable, the backup copy failed for the same reason, so the scheduled compaction would destroy the only copy of the v1 bytes the moment the disk recovered. The retry is worth keeping, so it is now gated on the rescue copy existing rather than reverted; `backup_v1_checkpoint` reports whether one is in place (freshly written, or left by an earlier attempt). Both halves are pinned. Three tests were passing under mutation and now fail: - `evict()`'s bigram residue raise had no coverage at all -- transposing the key, or deleting the loop body, left 96/96 green. The one test that reached that code seeded zero bigrams. A wrong tuple there reopens #286 on the bigram axis. - the two-test merge in the previous commit dropped the probe that pinned the residue as keyed by (reading, surface); `.is_some()` in place of `.is_some_and(|inner| inner.contains_key(surface))` passed. - `test_evict_enforces_both_caps` cited a `||` short-circuit hazard the types cannot express (`evict_map` returns a Vec), so it could not fail for its stated reason. It now asserts what actually matters. Correctness: - Saturation keyed off raise count, not set size, so a re-raise of an already-tracked key at the cap threw away all 50,000 keys for a raise that needed no capacity. It now saturates after the insert, on size. Reachable with no timing window: one segment list containing the same pair twice. - The deletion no-op check reads pre-batch memory while `apply_batch` runs once after the loop, so `[Committed(X), Deletion(X)]` in one batch found X nowhere, skipped the tombstone, and then learned X. Not reachable from real input today (verified by tracing every Deletion producer and 8000 proptest cases), but nothing guarded or documented it. The check now also consults earlier records in the same batch. - The migration commit writes a checkpoint from `history` itself, so it settles the residue the post-replay `evict()` raised; nothing was retiring those keys. Efficiency: - The residue's bigram map nests one level deeper than `UserHistory`'s. Its buckets are filled by eviction (globally lowest-scoring entries, so they pile up under common particles), not by successor distribution, so the inherited linear scan was bounded only by MAX_RESIDUE -- measured 60us per ForwardDelete at 49k keys in one bucket, on the key thread inside the wal mutex. The probe is now an O(1) borrowed lookup. - `cover` walked the residue twice, the second time only to recount; `retain_newer` now returns its own count and `count_keys` is gone. Docs that were actively wrong: - `epoch` claimed to be monotonic; `clear` installs a fresh residue and resets it to 0. What keeps `cover` from comparing across a clear is `compact_gate`, which is now named where the comparison happens. - the `cover` cost comment said "a few hundred us at capacity"; measured ~550us at 10k and ~3ms at the 50k cap. - SPEC and the Swift comment claimed both new report flags leave `checkpoint_state`/`wal_state` healthy. True for `migration_failed`; `appends_frozen` is the opposite in four of its five paths. - `record_segments_at` -- the only writer of bigram keys -- was the one site still hand-rolling the walk the previous commit claimed it shared. Also de-flaked `test_deleting_an_evicted_entry_still_persists`, which asserted a WAL entry count the scrub compaction it had just spawned was racing to erase. Latency unchanged (M4, release, idle): Committed p50 519us / p99 1.10ms, Tombstone p50 5.05ms / max 8.21ms.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043f2df6e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex R3, 4 findings. Three were on the rescue gate the previous commit added, and they share a root: the guard was on `compaction_recommended` — a scheduling hint — when what has to be protected is the checkpoint *write*. Gating one scheduler left the others. - P1: `appends_frozen` is its own term in `compaction_recommended`, so a migration that consumed a legacy WAL freezes the WAL on failure and re-enables the very recommendation the gate had just suppressed. - P1: the gate covered startup scheduling only. With a v1 checkpoint beside a fresh v2 WAL the WAL stays appendable, so the next deletion's scrub — or the entry-count threshold, or an append-failure heal — reaches `run_compact_impl` anyway and overwrites the unrescued file. - P2: `.v1.bak` existing is not proof of a rescue. `fs::copy` truncates its destination first, so a copy that dies partway leaves a short file, and a backup written from an older v1 state is not a rescue for these bytes. So the guard moves to `run_compact_impl`, immediately before the save, where all four routes converge. It retries the rescue first: if the disk has recovered enough to copy, it has recovered enough to write, and the state clears itself; otherwise the write is refused and `CompactOutcome:: Failed` keeps the scrub posted for a later attempt. `compaction_recommended` goes back to the plain `migration_failed` term, since scheduling is no longer the thing that has to be safe. `ensure_v1_rescued` replaces the best-effort copy: it writes atomically (so there is no partial destination) and accepts an existing backup only on a byte-for-byte match, and it is now callable after startup because the retry belongs at the write. It short-circuits when the path no longer holds v1 bytes, so it cannot copy a v2 checkpoint over the rescue slot. Tests: the engine-level pin drives a compaction with the v1 bytes unrescued and the backup path made unwritable, and asserts the checkpoint is byte-for-byte untouched and the flag still set — then that clearing the obstruction lets the same call migrate and preserve the v1 bytes. The lex-core pin covers truncated, empty, and stale `.v1.bak` contents. The two migration tests now assert `v1_unrescued` rather than the scheduling hint they used to. Also (P2): the shipped log line said the retry waits for the next launch, which stopped being true once the retry moved to the write; it no longer claims a time. 634 tests, accuracy 108/108 + 7/7, swift 144, MSRV 1.88, audit clean.
|
All four addressed in 02a67d6. The three rescue-gate findings shared a root — the guard was on |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02a67d67b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex R4 raised three more P1s in recovery.rs, all on the rescue gate. That is the third consecutive round on one mechanism, so the root check runs before another patch — and it says the mechanism should not exist. The LXUD v2 design settled `.v1.bak` as best-effort (decision #13, §7 step 3): a manual rescue hatch, explicitly *not* a correctness dependency, with migration proceeding whether or not the copy lands. Two commits ago I made it a precondition for writes. Every round since has found another site that must now join a protocol the design said should not exist: - R3: four schedulers reach the compaction write; a scheduling-level gate covered one. - R4: the migration commit itself still overwrites v1 with only a best-effort backup, so the guard protected one writer and not the original; `gc_v1_backup` deletes the very backup the flag attests to; and `write_atomic`'s `.v1.bak.tmp` holds raw history bytes that the privacy wipe does not know to remove. Each is real. Fixing them one at a time means adding a fourth, fifth and sixth participant. Fixing the root means deleting the protocol. Re-deriving the original finding: its actual content was that `compaction_recommended |= migration_failed` schedules a compaction that overwrites the v1 file. The fix for that is to not schedule it. I instead kept the in-session retry and built a mechanism to make it safe — but the retry was never required. #296 asks for a failed migration to be *reported*, and it is: `migration_failed`, `is_clean() == false`, and a log line that names it. Nothing in that issue asks for it to be healed before the next launch, and a compaction cannot heal it anyway, because a compaction is not the migration. So: `ensure_v1_rescued`, `v1_unrescued`, the engine-side write guard and the byte-matching backup validation are all gone, `backup_v1_checkpoint` is best-effort `fs::copy` again, and `migration_failed` no longer drives `compaction_recommended`. The legacy-WAL variant still heals through `appends_frozen`, which is a real degradation a compaction genuinely fixes — that term predates this PR and is untouched. Net effect on the original finding: the v1 checkpoint is no longer overwritten by a generic compaction on a failed migration, which is what it asked for. The three R4 findings dissolve with the mechanism. Tests now pin that a failed migration schedules nothing, on both variants and regardless of whether a backup happens to be present, so a future change cannot quietly reintroduce compaction-as-migration. 632 tests, accuracy 108/108 + 7/7, swift 144, MSRV 1.88, audit clean.
|
All three addressed in aa7619b — by removing the mechanism rather than extending it. Three consecutive rounds landed on the v1-rescue gate, so I ran a root check instead of patching again. The LXUD v2 design settled Re-deriving the finding that started this: its content was that So |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa7619bb87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… know Codex R5 (P2). The line said a failed migration is retried next launch, but that is only true for one of the two variants. When a legacy WAL was consumed the path freezes the WAL, and the compaction `appends_frozen` schedules to thaw it writes the v2 checkpoint as a side effect — so the conversion completes in-session and there is nothing left to retry. The timing is therefore not a property of `migration_failed` at all, which is also why encoding it in Swift was the wrong shape: the line now states the fact (`migration=failed(v1 kept)`) and the per-variant explanation lives in the engine, where the branch condition actually is. Same correction applied to the `migration_failed` field doc, SPEC and the AGENTS.md settled note, all of which had inherited the same over-generalization from the previous commit.
|
Fixed in the head commit. You are right that the timing is variant-dependent — and the sharper point is that it is not a property of |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
PR1 of a two-PR slice over the four history-persistence issues (#295 / #296 / #286 / #288). They are one class — a requested deletion, or a durability failure, reaches neither disk nor the user — with two distinct roots:
A channel built on a lying truth is worse than no channel, so this PR does (1) and PR2 does (2).
#286 — deleting an evicted entry was a silent no-op
The no-op-deletion skip asked "is this pair in memory?" when the question is "could it be in the durable set?". An entry evicted for capacity is gone from the maps while the last checkpoint still holds it, so a ForwardDelete on it wrote no tombstone, posted no scrub, and the conversion came back on the next launch.
UserHistorynow tracks the keys that eviction — or a deletion whose frame never reached the WAL — left behind, and the check is a single predicate,deletion_has_durable_target. Two design points, both pinned by tests that fail under mutation:max_unigrams, which would put a flush plus a full checkpoint on every ForwardDelete of a never-learned candidate — the common case the skip exists for.#296 — a failed migration commit reported a clean startup
The
Errbranch set none of the report's states, sois_clean()answered true, all three Swift branches fell through, and the WAL was frozen for the rest of the session.The report now derives two facts instead of asking each branch to remember one:
migration_failedfrom the migration's own outcome. This matters: theErrbranch only freezes when a legacy WAL was consumed, so a v1 checkpoint beside a fresh WAL failed the commit without freezing anything — anis_frozen()-based report misses that half entirely, and the existing fixture would have passed while it shipped broken.appends_frozenonce fromwal.is_frozen(), after every branch has run.migration_failedalso joinscompaction_recommended, so the write is retried in-session rather than only on the next launch — a compaction writes exactly the checkpoint whose write failed. Both fields reach the log line, which otherwise readscheckpoint=Loaded wal=Clean, indistinguishable from a healthy start.Not in scope
#295 / #288 (PR2), #297 (tracing), #299 (benches — its pre-existing failure is why
--all-targetsis absent from the clippy gate here).Verification
Conversion accuracy — unchanged, as expected: no cost, weight, reranker, dictionary-source or conversion-path change. Run because CI's
accuracyjob fires on thecorefilter.mise run accuracymise run accuracy-historyHot-path latency (
profile_apply_records_lock_hold, M4, 20k entries against a cap of 10k so every measured commit evicts, 2 contending readers):At or below baseline on every percentile; the differences are run-to-run variance. The cost this PR does add is real but narrow: at capacity, deleting an entry that eviction dropped now pays a flush and a background checkpoint. That is the correct price for a deletion that would otherwise not happen, and it is why the residue tracks keys — an unlearned candidate keeps the fast path whether or not the history is full.
Tests — 629 Rust + 144 Swift, all green. New coverage: the reopen-after-delete symptom of #286, both halves of #296 (with and without a legacy WAL), the re-raise-during-checkpoint case the epoch stamps exist for, saturation, eviction selection order, and a pin that the checkpoint body is byte-identical. Each of the three load-bearing fixes was mutation-checked — reverting it makes a named test fail.
Format unchanged —
to_data/from_dataenumerate their fields, so the new state cannot reach the file. No migration.Gates — fmt, clippy, test, accuracy ×2,
mise run audit,mise run test-swift, MSRV 1.88.0,/simplify,/lexime-review(4 axes; Axis 1 skipped — no dictionary/cost/POS surface). All findings fixed, none deferred.🤖 Generated with Claude Code