fix(history): carry an unpersisted deletion across the restart that undoes it (#312) - #320
Conversation
…undoes it The runtime durability ledger is an atomic on `LexUserHistory`, so it starts at zero on every open. `DeletionNotPersisted`'s `Io` half is the one condition with no startup heal — no frame reached the WAL, the fallback checkpoint failed too, so the old checkpoint still holds the entry and wins on the next start. The report died with the process, which meant the channel went silent at precisely the moment the loss materialised: the entry came back, and `open_report()` read `checkpoint: Loaded, wal: Clean` (#312, the remainder of #295). A sidecar `<checkpoint>.deletion-pending` is the ledger's on-disk projection. The checkpoint header's reserved bytes cannot serve: the raise condition *is* that the checkpoint write failed, so a header flag rides the one channel that just broke, and setting it in place would mean rewriting a CRC-protected header outside tmp+rename — trading a torn checkpoint, and the whole history with it, for the ability to report one deletion. The marker carries a witness, not a bit. `Io` records `Lost` and always reports; `SyncFailed` records the tombstone's seq, and the next start suppresses the report when the loaded state reached it, because replay applied that deletion after all. Only `NotFound` means clean — a read error, a short file, a bad magic all resolve to `Lost`, so corruption can only push toward reporting and no CRC is needed. It holds no input strings: which entry was deleted is the text the deletion was meant to erase. Writes merge rather than replace. `Lost` absorbs and unflushed witnesses keep the higher seq, because a `SyncFailed` append does not freeze the WAL: a later record in the same session can still fail with `Io`, and a plain overwrite would hand the next start a witness it can suppress — losing exactly the report this exists for. Retraction pairs with the ledger cover, in the same call and under the same wal guard. Between a successful CAS and a separate unlink sits a window in which a fresh raise writes a marker the unlink then destroys; that window is not something a deterministic test can pin, so the guard witness makes covering without the mutex fail to compile instead. Surfaced as its own `OpenReport` field and a latching `EngineInitFailure.historyDeletionLost`. Not `durability_issues()`, which reports what holds now and retracts when a checkpoint covers it — nothing retracts this, the deletion is already lost. Not `data_loss_suspected()`, whose wording is "past learning was lost": this is the inverse, data that survived. And not `compaction_recommended` — a compaction here would checkpoint the resurrected entry and cover the ledger, i.e. report success. Consumption is an explicit `ack_open_report()` rather than the open, so the only durable trace outlives the gap between the report being built and something acting on it. The failing disks this exists for are where a process is least likely to survive that gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SPEC.md §保存 still ended on "the report is gone on the restart where the `Io` deletion resurrects (#312 — the remaining gap in #295)", which is now false, and the family listing named every sibling except the new one. Four AGENTS.md entries would otherwise contradict the implementation they are supposed to be the review lens for — the failure mode #317 hit, where a settled note kept suppressing findings about a mechanism the same PR had replaced. - (c) says the deletion raise does not branch on the error variant. The marker does branch, and the entry now says why that is not a contradiction: inside a session neither half is durable, but across a restart `SyncFailed`'s frame is replayable, so reporting it would be an alarm about data that is gone. - (e) forbids merging the durability issues into `initFailures`, which is exactly the shape of `historyDeletionLost`. The rule is restated as the one actually doing the work — retraction, not provenance — so it now settles both directions instead of reading as a blanket ban. - The ledger-placement entry justified itself with "lex-core cannot see the `SyncFailed` raise", and lex-core now owns the marker format. The blocker was never the crate; it is that `apply_batch`'s witness must not widen. - (iii) is closed, with the sidecar's settled shape recorded — including the class that stays open by construction: the marker shares the checkpoint's directory, so a failure of that whole directory takes it too. Swift tests pin the pair that a failing disk shows together: the past loss and the live one must not render as the same sentence twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AGENTS (a) settled the sibling decision on this path with numbers (F_FULLFSYNC 5.9ms p50 / 11.7ms max), and this one arrived with an adjective. Measured: the marker costs 10.1ms p50 against 12.3ms for the fallback checkpoint the same call makes next, so it roughly doubles a failure-only path rather than adding a new order of magnitude. The two cheaper shapes are written down with why neither is taken, since both are the obvious reviewer questions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
/simplify's reuse, simplification and altitude passes independently landed on the same spot, and the sharpest version is that the doc comment was wrong. `checkpoint_tmp_path` claimed that deriving the name in lex-core "keeps the injection from silently becoming a no-op if the naming changes" — but it re-spelled `suffixed(path, ".tmp")` as a second expression, so it defended against the test drifting and not against `write_atomic` drifting, which is the direction that matters. Had the writer moved, the injector would have planted a directory nobody visits and four #312 tests would have passed vacuously. `persist::tmp_path` is now the definition `write_atomic` itself calls, and the four sites that had grown their own copy forward to it. Also local: one `note_breach` for the batch accumulation the two append arms spelled out by hand, `map_or` for the same shape inside `merge_write`, one `stateDetail` for the log prefix Swift was building twice, and the two adjacent `if report.deletionLost` blocks merged. Not taken, deliberately: an injectable writer for checkpoint saves (the seam `WalIo` gives the WAL, which would retire both this helper and the older parent-as-a-file trick), deferred-error in `remove_recovery_artifacts` (a pre-existing fail-fast hole where a stubborn .v1.bak blocks the .corrupt-* sweep), and a family module owning the five path derivations. Each is a change to shared infrastructure that #312 does not need, and bundling them here would put unrelated risk in a persistence PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… evidence The `/code-review max` sweep found the lifecycle inverted in several places at once. Each retraction path unlinked the marker on its own authority, and two of them acted on evidence weaker than the claim they were retiring. **Startup no longer retracts at all.** A witness satisfied by replay was only satisfied out of the page cache — the flush that failed still never happened — so unlinking there was retract-then-persist, the inverse of the discipline the raise path follows. If the startup compaction then failed and power was lost, the frame vanished with no record left anywhere. The claim now passes to the runtime ledger, and the first durable checkpoint settles it. **An unsatisfied witness is promoted to `Lost`.** `adopt_empty` re-bases seq numbering to the checkpoint's applied_seq + 1, so after a WAL quarantine an unrelated later frame can reach an old witness and settle a report that is still owed. Having answered the question once, the answer stops depending on a comparison a reset can invalidate. **The marker is written in place.** tmp+rename protects against a torn file, but a torn marker decodes to `Lost` — the strongest claim — so there was nothing to protect. What it did add was a window: a crash between the tmp's flush and the rename left the stronger claim in a sibling that `read` ignores and the next `remove` deletes. One flush instead of two, on the key thread. **A claim whose write failed is kept in memory.** The merge ran against the file alone, so a failed write dropped the claim and let the next, weaker one start from an empty read — on the failing disk where writes fail, which is the only disk this path runs on. **The ack moved to where the row is rendered.** `bootstrap()` runs on every launch including the short-lived IMKit probe launches this controller already designs around, none of which show a menu; acking there consumed the report on the user's behalf and put #295's gap back one layer up. It now also asks the ledger before unlinking (the startup flag is frozen at open, so acting on it alone deletes a marker a later raise wrote) and takes the wal mutex with `try_lock`, since it runs on the main thread exactly when a compaction may be holding that mutex across file I/O. **A non-file at the marker path can be cleared.** Every retraction unlinks, and `remove_file` fails on a directory, so anything leaving one there latched a report telling the user to delete an entry — with no path in the system able to clear it, not even a full wipe. Also: `clear` retracts the latched Swift row (the engine wiped the marker while the menu kept asking the user to re-delete a nonexistent entry); the new row is in both localization catalogs, where an English system would otherwise have shown it in raw Japanese; the read is bounded to the record length; and the recovery-events log line no longer double-reports a lost deletion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `/code-review max` test-quality pass found five assertions that would survive the mutation they were written to catch, four proven by injecting it. - The quarantine-rotation test re-implemented `.corrupt-` matching in the test helper, so it could not observe the production predicate at all; widening `quarantined_files` left it green. It now calls the real predicate and drives a real rotation past its keep limit. - The "either order" merge test never produced its second order: an `Io` append freezes the WAL, and the frozen guard turns every later append into `Io` too, so both iterations were `Lost` twice. The reachable route runs through a compaction that thaws the file without settling the claim — which is what the test now builds, and what the doc comment now says instead of the two wrong reasons it gave before. - The witness was never joined to the seq the WAL actually assigned; recording `seq + 1` or `seq - 1` passed everything, one latching a false privacy alarm and the other silently suppressing a real loss. - Nothing asserted that memory-only learning writes no marker, so a raise widened to cover it would have latched a deletion warning for a deletion nobody requested. - `cover`'s outstanding-transition guard had no coverage at all: deleting it left the suite green, while a compaction covering memory-only learning would unlink an inherited marker. - The migration test used a `Lost` fixture, whose verdict depends on nothing the migration touches, so moving the read past the commit changed nothing. Renamed to what it can actually detect. Also new: the ack must not delete a marker a later raise wrote, a non-file at the marker path must still be clearable, a promoted claim must survive a re-based seq space, and EngineContainer's branch placement is now a pure function so the "a quarantine must not mask a lost deletion" claim is testable without asserting over Array.map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eletion Stage 5 of the gate found the seam its own fix pass left untested: recovery reports `deletion_pending_checkpoint`, the engine seeds its ledger from it, and nothing checked the join. Removing the seeding entirely left the suite green — the same shape as the five gaps the review pass had just closed. Built against real files. The `hist_with_io` fixture mocks every WAL write, so no test using it can produce a tombstone frame that a reopen replays, which is the structural reason the witness had no end-to-end coverage to begin with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The quoted latency came from the tmp+rename form the in-place write replaced. Re-measured on the same machine: 4.0ms p50 against 10.1ms, since dropping the second flush also drops the rename and the directory fsync. AGENTS settled the sibling decision on this path with numbers; a stale number is the same defect as no number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`/lexime-review` found the marker still being retracted by parties with no standing over the claim they were retiring — the same class the correctness pass fixed twice, in the two places it had not looked. **A cover no longer retracts an inherited report.** Two axes found this independently and one proved it with a throwaway test: a session that inherits an undelivered `deletion_lost`, then loses a deletion of its own, then heals, covers its own breach — and unlinked the shared file on the way past. The next launch reported nothing while the earlier session's entry sat in the checkpoint. A checkpoint written now persists the *resurrected* entry, so it settles nothing about the inherited claim; only delivery or a wipe does. **A witness the checkpoint already covers is retracted outright.** The post-replay `applied_seq` answers "the checkpoint covered it" and "replay reached it" with the same number, and only the first is durable. Conflating them showed a live durability warning for a deletion that was already persisted — the residue of a crash between a successful save() and its unlink. The checkpoint's own applied_seq, captured before replay, separates them. **A file longer than the record decodes as malformed.** Reading exactly 16 bytes made any longer file a well-formed *prefix*, i.e. a suppressible witness — the one malformed shape resolving toward silence, which falsified both the "malformed always reports" rule and the absent-CRC argument resting on it. **The session's sticky claim is one atomic, not a second lock.** Its "only under the wal mutex" rule was prose in a struct where every other such rule is compiler-checked. It turns out no lock was needed at all: seqs are monotonic within a session, so a later unflushed witness always dominates an earlier one and only `Lost` has to survive a failed write. Swift: the clear-retraction moved into a `defer`, since the engine unlinks the marker at the wipe's commit point and *then* runs the steps whose failure it throws — so the throwing path was exactly the one where the row was left standing and the session continued. And `stateDetail()` is built lazily again; hoisting it had quietly defeated the warmup saving its own comment claims. Docs: SPEC and AGENTS said three retraction paths with no exceptions; there are four, and one of them does not apply to an inherited claim. SPEC's runtime-row entry gained the third origin this PR added. DegradedStatus's latch rule now names its single exception. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fixture used a witness beyond the replayed applied_seq, so decoding it as a valid prefix still reported and the length check's mutation survived. Seq 1 is satisfiable by the fixture's own replay, which is what makes the assertion about the length check rather than about the witness. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e menu On-device verification killed the premise the acknowledgement rested on. IMKit calls `menu()` by itself: with the marker staged at the real path, an untouched relaunch consumed it four seconds in, before anyone looked. "The menu was built" is not evidence that anything was displayed — the same defect as acking at bootstrap, one layer further along, and the same one #295 describes: the channel goes quiet exactly when the loss materialises. The only signal this process can have that a report reached a person is that the person acted on it. The lost-deletion row is now the one enabled row in the status list, and clicking it is the acknowledgement: it retires the on-disk record and drops the latched row. Every other row stays disabled, because none of them is backed by anything a click could settle — they either re-derive from live state or die with the process. `DegradedStatus.rows` returns rows rather than titles so the menu can tell the two apart, and the property is asserted directly: exactly one row is acknowledgeable, and it is that one. Re-verified on-device: the record survives 25 seconds of an untouched launch, the click removes it, and the next launch reports nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@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: 773911fde1
ℹ️ 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".
**A zero witness is malformed, not a covered one.** A torn marker keeping the
header and the witness flag but zeroing the seq decoded as `Unflushed { seq: 0 }`.
WAL numbering starts at 1, so `outstanding(0)` is false against every
applied_seq: recovery read it as "the checkpoint already covers this", removed
the marker, and reported nothing. That is the one direction this format forbids
— the same hole as the over-long file, in the field rather than the length.
**An acknowledgement that retires nothing now says so.** `ack_open_report`
returns early when a compaction holds the wal mutex, and when this session has
an unpersisted deletion of its own; both leave the marker on disk. Swift
dropped the row regardless, so the user's acknowledgement silently did not
take, the warning returned on the next launch, and the row — the only
affordance for retrying — was gone. It now returns whether the record was
retired, and the row goes only with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d16e4b77f
ℹ️ 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".
The marker was evaluated against the checkpoint that had been *loaded*, and on the migration path this function goes on to write a different one. When replay applied a tombstone, the migration's own commit persisted a checkpoint that already contained the deletion — and the marker, asked before it, still answered against the v1 file that predates the deletion. The engine seeded a live durability warning for an already-persisted deletion and left the marker to an asynchronous compaction that may never run. Codex proposed settling when the migration save succeeds. Taken at the root instead: the question the predicate asks is whether the checkpoint durable **when this function returns** covers the witness, so it is asked once, at the end, against . The migration case then needs no special handling — it is simply a startup where the durable checkpoint is the one this function wrote. Same predicate as R1's zero-witness finding, which is why the loop's ≥2-round root-check fired. Written out: both are the settle predicate being fed a wrong input, and the two fixes establish its only two inputs at their sources — well-formedness at the decoder, durability here. No missing abstraction, and the mechanism is the design's own on-disk projection of the ledger, not a reconcile-after-the-fact. Continued rather than paused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa3af70563
ℹ️ 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".
…tatus row The loop's ≥2-round root-check fired at R2, so the cumulative fix-delta design re-gate ran now rather than at TERMINAL. It found the R1 and R2 fixes coherent but the layer that explains them contradicting itself and the code. **Two rules about one row, collapsed into one.** R1 made the acknowledgement report whether it retired the record, so the row survives an ack that could not complete. Ten lines away, `clearHistory` still dropped that same row unconditionally in a `defer`, justified by a comment arguing that retracting a session early beat the alternative — which is verbatim the compensation R1 had just rejected as insufficient. Both cannot be right about the same row. The engine now answers one question, `deletion_report_owed()`, and both call sites ask it: an acknowledgement the engine could not complete and a wipe that failed before its commit point both leave the report owed, because they are the same question. The ack's bool is gone with the second rule it existed to carry. **`@discardableResult` removed.** It let a future call site drop the row without checking — disabling the compiler check that makes the rule structural rather than conventional, on the one method whose result *is* the fix. **AGENTS and SPEC described a mechanism this PR no longer has.** Both said "startup never retracts" while `open_recovering` has always retracted against a covering checkpoint, and R2 widened that to the one the migration path writes itself. Worse, the do-not-raise list named "a retraction at startup" as settled — telling the external reviewer to skip the code R2 had just changed. That list must never suppress review of the mechanism it describes, so it now scopes the rejection to *replay-evidence* retraction and says the rest is in scope. The #317 pattern, caught by firing the re-gate mid-loop instead of at the end. Also: a test comment claimed "a migration checkpoint is never coverage", a rule its own new neighbour disproves — the claim decides, not the migration; and the module's fail-safe list now names the two shapes that would resolve toward suppression rather than only the ones that fail to parse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 810b5dab54
ℹ️ 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".
Four findings, two roots, and the first root is one I had been patching by symptom for three rounds. **The decoder accepts only what the writer emits.** Flags 0x03 passed the bit test and became a witness, so a malformed marker with a low seq read as covered and the report vanished. That is the fourth shape of one bug — after a wrong length, an unknown version, and a zero seq — each found separately, each a byte the writer never produces being read as if it meant something. Decoding is now a round trip against `encode`, which leaves no unchecked byte, and rejects three more shapes nobody had named: a non-zero reserved field, a witness flag with no seq, and a seq with no flag. The loop's root-check asked whether a canonical algorithm was missing; this time the honest answer was yes. **The reader refuses to open what the writer could not have created.** A FIFO at the marker path makes a read-only open wait for a writer that never comes, inside `LexUserHistory::open`, on the thread the IME starts up on — the input method would never become available. The file type is checked first, via `symlink_metadata` so a symlink to a FIFO is the same answer. **Removal reports whether the path is clear, and the acknowledgement settles only when it is.** `remove` returned `()`, so an unlink that failed on a path the engine does not control still dropped the row while the record stood. **And it no longer deletes what it did not create.** The `remove_dir_all` fallback added last round would walk an unbounded tree on the menu thread and destroy whatever a restore had put there. Only an empty directory — a placeholder — is cleared now. A full one stays, and the `false` return makes that visible: the unclearable-latch problem is solved by *saying* the acknowledgement did not take, not by silently deleting someone else's files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two of the new fixtures carried a witness beyond the fixture's applied_seq, so a decoder reverted to a flags bit-test would still have reported and the case proved nothing — the same trap the over-long case had. Seq 1 is satisfiable by the fixture's replay, which is what makes the assertion about the decoder. Also records the FIFO test's failure mode: without the file-type guard it does not fail, it hangs, because a startup that never completes is the defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb2a64d9fd
ℹ️ 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".
Three findings, and two of them are one root — the writer-side counterpart of the reader rule the last round established. **The writer owns the marker path.** `read` refused to open anything that is not a regular file, but `File::create` still followed a symlink: a link left there by a restore had the engine truncate and overwrite a file it has no business touching, or block on a link to a FIFO inside the synchronous deletion path. Anything that is not its own regular file is now unlinked first — the link, not the target. **And that closes the failed-promotion hole too.** Promotion to `Lost` is what stops a re-based sequence space from satisfying a stale witness, so a promotion that could not be written reopened it. A read-only *file* is still removable — that needs write permission on the parent, not on the file — so the writer replaces it instead of giving up. What is left is the directory-level failure this design already documents as unclosable, rather than a second class. **A wipe's leftover marker no longer warns about an empty history.** `clear` ignored `remove`'s result, so an unremovable marker outlived a wipe and warned on the next launch about a history that provably holds nothing. Settled by the rule `clear` already uses for the ledger: a `Lost` claim says an entry survived the deletion, and an empty loaded state means there is no such entry. That rule took two attempts to state correctly, both caught by existing tests. Keyed on the durable set it wrongly settled a replayed-but-unflushed deletion, where the checkpoint still holds the entry a power loss would bring back; applied to every claim it wrongly settled an `Unflushed` one, which is about durability rather than presence. It is scoped to `Lost`, against the state actually loaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
R22 の 2 件、いずれも real として修正しました (9dd3312)。どちらも doc のみで、どちらも私の書き残しです — 私自身の staleness が finding になるのは 5 ラウンド連続です。 なので今回は指摘された 2 行だけでなく、それぞれの規則の記述をすべて sweep しました。両方の sweep で、報告されていなかった箇所が 1 つずつ追加で見つかっています。 R21 が置き換えたバイト比較の規則: SPEC が「判定は flush したバイト列との一致で行う」と書き、AGENTS も括弧書きで同じことを言っていました。両方とも「どの段で失敗したかは writer が返す ( ack のタイミング: この再発は構造ではなく手順の問題です: 規則を変え、コードと目の前にある 1 つの説明を直し、その兄弟を置き去りにする。語ではなく概念で grep するのが見つけ方で、実際それが今回どちらの sweep でも未報告の箇所を掘り出しました。 Rust 709 / Swift 200 / clippy clean / accuracy 108/108 + 7/7 / CI 全 green。 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd3312a64
ℹ️ 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".
One finding, doc-only, naming four sites — and the sweep found a fifth. Sixth round running where my own staleness is the finding, and the shape has not changed: R17 corrected `DegradedStatus` when the acknowledgement landed and left every sibling saying "nothing retracts this one". The distinction those sentences were protecting is real and worth keeping — it is why the row latches instead of sitting on the polled channel — but it was stated one notch too strong. What no amount of *disk recovery* retracts is not the same as what nothing retracts: acknowledging the row retires it with the claim still true but delivered, and a wipe retires it by making the claim false. Both go through the owed-predicate rather than inferring from which action ran. Corrected together: `SPEC.md`, `AGENTS.md`, `EngineInitFailure.swift`, `OpenReport::deletion_lost`, and the test comment at the round-trip test.
|
R23 の 1 件、real として修正しました (2736c53)。指摘は 4 箇所を挙げていましたが、sweep で 5 箇所目が見つかりました。 私自身の staleness が finding になるのは 6 ラウンド連続で、形も変わっていません: R17 で確認 (ack) が入ったとき ただしその文が守ろうとしていた区別は本物で、残す価値があります — polled チャネルではなく latch する理由そのものです。ただ一段強すぎた: 「ディスクの回復では撤回されない」と「何によっても撤回されない」は別です。確認は主張を真のまま配信済みにして行を退役させ、全消去は主張を偽にして退役させる。どちらも「エンジンがまだ負っているか」の述語を通ります。 まとめて修正: Rust 709 / Swift 200 / clippy clean / accuracy 108/108 + 7/7 / CI 全 green。 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2736c53d8f
ℹ️ 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".
…ined v1 F2 (P2) is a real data-loss risk, and it breaks a rule written down in the same file. The `compaction_recommended` feeder R11 added can fire while `migration_failed` is true; `open` then spawns a compaction that writes a v2 snapshot over the retained v1 checkpoint with none of the commit's steps — no `.v1.bak`, no `Migrated`. `migration_failed`'s own doc states exactly this hazard and concludes "the next launch retries properly"; my feeder walked straight past it. Both marker feeders are now vetoed while the migration commit is failing. They are promptness only — ordinary commits reconcile the marker regardless — so the veto costs latency and nothing else, which is what makes it the right resolution rather than a trade. F1 and F3 are doc, and both are mine. `DegradedStatus`'s opening invariant still said nothing during the session retracts an init failure, three lines above the exception R23 added to the same comment. And a test rationale still argued the assertion holds because a compaction would wrongly reassure — the R11 reversal made that false: `inherited_owed` sits outside the session ledger, so a cover cannot settle it and the projection re-asserts `Lost` after every one. The assertion holds because the disk already says `Lost`. Mutation-checked: dropping the veto fails the new test.
|
R24 の 3 件、いずれも real として修正しました (ee6f8ec)。 F2 (P2) は実際のデータ損失リスクで、しかも同じファイルに書いてある規則を破っていました。 R11 で足した marker の feeder 2 つとも、migration commit が失敗している間は veto します。どちらも promptness 専用で、通常のコミットが marker を照合するので、veto の代償は latency だけです — これがトレードオフではなく正しい解決である理由です。 F1 と F3 は doc で、どちらも私のものです。 mutation 確認: veto を外すと新テストが落ちます。 Rust 710 / Swift 200 / clippy clean / accuracy 108/108 + 7/7 / Linux clean / CI 全 green。 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee6f8ecaf8
ℹ️ 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".
…ants one F1 (P2) shows R24's fix was the wrong shape, not merely incomplete. I gated the two marker feeders on `!migration_failed`, but a replayed `Unflushed` witness necessarily sets `replayed_deletion`, which schedules the same worker — so the hazard R24 was about was still reachable by the path R24's own scenario takes. "No compaction may run over a retained v1 checkpoint" is a property of *running a compaction at all*; it belongs on the recommendation, which is where it now sits. The legacy-WAL variant stays exempt because there the compaction **is** the intended heal. The test proved the point by failing to: it planted the witness without its WAL frame, so it exercised only the feeders the fix had gated and passed while the hazard stood. It now writes the tombstone frame, and asserts on `replayed_deletion` — the feeder that was missed. F3 (P2). `FlushedNotRenamed` was returned before `sync_parent_dir`, so the orphan's *name* was not durable even though its contents were: the tmp's directory entry had never been flushed. A power loss could drop it and leave only the suppressible canonical witness for later frames to satisfy, silencing an owed report. On this path the parent-dir fsync is load-bearing rather than the best-effort it is on the success path, and its failure downgrades the result to `NotDurable`. F2 (P3). SPEC still carried the "`deletion_lost` deliberately does not feed `compaction_recommended`" rule that R11 reversed, and now also records the veto. Mutation-checked: removing the veto fails the strengthened test. Removing the parent-dir sync is *undetectable* — no deterministic test distinguishes a synced directory entry without simulating power loss — and the code says so rather than implying coverage.
|
R25 の 3 件、いずれも real として修正しました (135eec7)。 F1 (P2) は R24 の修正が「不完全」ではなく形が間違っていたことを示しています。 marker の feeder 2 つを 「保持中の v1 checkpoint の上で compaction を走らせてはならない」はcompaction を走らせること自体の性質であって、個々の feeder の性質ではありません。判定そのものに veto を置きました。legacy WAL を消費した場合だけは例外です — そこでは compaction こそが意図された heal なので。 テストがそれを証明し損ねていた点もご指摘のとおりです: witness を WAL frame 抜きで植えていたため、修正が gate した feeder しか通らず、危険が残ったまま pass していました。tombstone frame を書き、見落とされていた feeder である F3 (P2) — F2 (P3) — SPEC が R11 で反転した「 mutation: veto を外すと強化したテストが落ちます。親 dir の sync を外すのは検出不能です — 電源断をシミュレートせずに同期済み dir entry を区別できる決定的テストは無い — のでコードにそう明記し、カバーされているかのように書くのは避けました。 Rust 710 / Swift 200 / clippy clean / accuracy 108/108 + 7/7 / Linux clean / CI 全 green。 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 135eec75a6
ℹ️ 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".
…where it is asked Third round on one hazard, and my guard was at the wrong level all three times: R24 gated the two marker feeders, R25 moved the veto to the startup recommendation, and both left the runtime schedulers open. Deleting a learned candidate after the directory recovers reaches the scrub branch, calls `spawn_compact()`, and `run_compact_impl` overwrites the retained v1 checkpoint with a v2 snapshot that skips the migration commit's `.v1.bak` and `Migrated` steps. Guarding schedulers is inherently incomplete — there are five (startup hint, scrub branch, threshold, `clear`'s heal, `FollowUp`) and nothing stops a sixth — so "no compaction may run while a v1 checkpoint awaits its migration commit" is now an invariant of the compaction itself, checked in `run_compact_impl`. That is the single site that performs the write, which is what makes it structure rather than convention (CLAUDE.md 正しさは構造で守る). The startup hint stays suppressed, but only to avoid spawning a worker that would refuse. `OpenReport::v1_checkpoint_retained` names the condition instead of leaving each site to recombine `migration_failed` with the legacy-WAL exemption — the exemption being that there the compaction *is* the intended heal. The test reaches the executor directly rather than through a scheduler: going through one would test that scheduler, which is the level this moved away from. Mutation-checked — removing the refusal fails it.
|
R26 の 1 件、real として修正しました (db2de24)。同じ危険で 3 ラウンド目で、3 回とも私の guard の階層が間違っていました。 R24 は marker の feeder 2 つを gate、R25 は veto を起動時の推奨に移動 — どちらも実行時のスケジューラを開けたままでした。ご指摘のとおり、ディレクトリが復旧したあとに学習済み候補を削除すると scrub 分岐が スケジューラ側で gate するのは原理的に取りこぼします — 現状 5 つ (起動時ヒント / scrub 分岐 / 閾値 /
テストはスケジューラ経由ではなくexecutor を直接叩きます: スケジューラ経由にすると、まさに今回離れた階層をテストすることになるので。mutation 確認済み — 拒否を外すと落ちます。 Rust 711 / Swift 200 / clippy clean / accuracy 108/108 + 7/7 / Linux clean / CI 全 green。 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db2de241e7
ℹ️ 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".
…he file, not about startup Both findings follow from one mismatch: `v1_checkpoint_retained` states "a v1 checkpoint is still the only copy" but was read straight off the immutable startup report. - A committed `clear` writes an empty v2 checkpoint over that very path, superseding the un-migrated copy on purpose. The veto stayed set, so the heal `clear` posts for its own failed physical steps was refused for the life of the process — a frozen WAL and a half-finished wipe with no retry, on a flow that deliberately does not restart. - While the veto holds the threshold never clears, so every commit spawned an OS thread from the key path that did nothing but refuse. The fact now lives in one runtime cell seeded from the report, cleared at the single other site that writes the checkpoint path (`clear`'s commit point). `run_compact_impl` refuses on it (load-bearing, R26's placement unchanged) and both schedulers skip creating a worker that could only reach that refusal — same cell, so they cannot disagree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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. Hooray! 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d39d7dbb03
ℹ️ 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".
… the success path too
`AtomicWriteFailure`'s own doc states the rule: the stage a write reached
"is not something a caller can infer … so the writer says which it was".
The success path broke it, collapsing "renamed, and the directory entry
is flushed" and "renamed, but the entry is not" into `Ok(())` plus a log
line.
For the checkpoint and the user dict that is the right trade — their
previous content is a complete older state, so a rollback costs recency.
For the deletion marker the previous content is a *weaker claim*: a
just-promoted `Lost` rolling back to a suppressible `Unflushed{seq}`, or
a first-ever write vanishing outright. The process meanwhile recorded
`Holds(Lost)`, so the projection skipped and appends passed the freeze —
and on the next start an unrelated frame above the restored witness
answers it "applied", retiring a report that is still owed.
`write_atomic_staged` now returns `Durable` / `NameUnconfirmed`;
`merge_write` maps the latter to `MarkerState::Unknown` and hands the
belief back instead of the caller synthesizing one from "it returned
Ok". Nothing else changes: `Unknown` satisfies no desired state, so
every commit re-projects, and `flushed != Holds(Lost)` keeps appends
frozen until the promotion is durably named. `FlushedNotRenamed` still
reports `Holds` — that branch is only reached with the parent-dir fsync
already confirmed, so `read` finds the orphan.
No new syscall: the dir fsync already ran, its result was discarded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
R30 の 1 件 (P2)、real として修正しました (3b4f03f)。 root は指摘より一段上でした:
検証の正直な状態: 変異を 3 つ入れて 1 つだけ捕捉されました。捕捉されたのは SPEC.md の「親 dir fsync は best-effort・log-only」は marker については誤りになったので、区別の根拠つきで書き換えました。AGENTS.md の ledger 項も同様です。 @codex review |
|
@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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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
Closes #312. Half-closes the remainder of #295 (see "Follow-ups").
The gap
durability_ledgeris an atomic onLexUserHistory, so it starts at zero onevery open.
DeletionNotPersisted'sIohalf is the one condition with nostartup heal — no frame reached the WAL, the §5.4 fallback checkpoint failed
too, so the old checkpoint still holds the entry and wins on the next start.
The report died with the process, which meant the channel went silent at
precisely the moment the loss materialised: the entry came back, and
open_report()readcheckpoint: Loaded, wal: Clean.The mechanism
A 16-byte sidecar,
<checkpoint>.deletion-pending, as the ledger's on-diskprojection.
failed checkpoint write, so a header flag rides the one channel that just
broke; and setting it in place would mean rewriting a CRC-protected header
outside tmp+rename — trading a torn checkpoint, and the whole history with
it, for the ability to report one deletion.
IorecordsLostand always reports;SyncFailedrecords the tombstone's seq, and the next start suppresses thereport when the loaded state reached it, because replay applied that deletion
after all.
NotFoundis clean. A read error, a short file, a bad magic, a filelonger than the record — all resolve to
Lost. Corruption can only pushtoward reporting, which is why there is no CRC.
tmp+rename protects nothing here while its gap loses a stronger claim to a
sibling
readignores and the nextremovedeletes.Lostabsorbs; unflushed witnesseskeep the higher seq. A
SyncFailedappend does not freeze the WAL, so alater
Ioin the same session would otherwise be downgraded to asuppressible witness.
of the page cache; the flush that failed still never happened. The claim
passes to the runtime ledger so the first durable checkpoint settles it. A
witness the checkpoint covers is a different case and is retracted outright.
Lost, because a WAL quarantinere-bases seq numbering and an unrelated later frame could otherwise satisfy
it.
between a CAS and a separate unlink is not expressible rather than merely
tested — but a cover does not retract an inherited report, since a
checkpoint written this session persists the resurrected entry.
Surfaced as its own
OpenReportfield and a latchingEngineInitFailure.historyDeletionLost— notdurability_issues(), whichreports what holds now and retracts when a checkpoint covers it, and nothing
retracts this.
What on-device verification found
The acknowledgement went through two wrong homes before the right one. Acking
at
bootstrap()consumed the report on IMKit probe launches. Moving it tomenu()looked correct — until the marker was staged at the real path and anuntouched relaunch consumed it four seconds in: IMKit calls
menu()withoutdisplaying anything.
The only evidence this process can have that a report reached a person is that
the person acted on it, so the lost-deletion row is now the one enabled row in
the status menu and clicking it is the acknowledgement.
Verified on the real IME: the row renders (localized), the record survives 25s
of an untouched launch, a click removes it, and the next launch is silent.
What stays open by construction
The marker lives in the checkpoint's directory, so a failure of that whole
directory (read-only volume, EACCES, parent removed) takes the marker with it.
ENOSPC and single-block EIO — where a 16-byte write succeeds and a multi-MB one
does not — are the classes this closes. Recorded in SPEC and AGENTS rather than
papered over.
Test plan
clippy -D warningsclean, msrv 1.88.0, audit clean.mise run accuracy108/108,mise run accuracy-history7/7 — bothunchanged; no dictionary, cost, weight, reranker or conversion-path change.
guard witness makes covering without the wal mutex unrepresentable).
Gate
/pre-pushall six stages./code-review maxreported 15 findings, all fixed;/lexime-reviewfound 2 IMP + 8 MIN, all fixed — including three settledAGENTS entries that this PR's own mechanism would otherwise have contradicted.
Follow-ups (not bundled)
Filed separately rather than widened into a persistence PR:
remove_recovery_artifactsreturns on its first error before reachingthe
.corrupt-*files, which do hold user text. A pre-existing weakness thisPR routes around rather than fixes. Also covers
write_atomicnot cleaning upits own tmp on failure.
WalIo),which is why this PR ships a
checkpoint_tmp_pathtest hook.#295 should be closed when this merges.
🤖 Generated with Claude Code