Repository navigation
acpmux: a tags or rules change logs its line before its metadata (R-DURABILITY S3b) - #35565
Merged
Merged
Conversation
…nd ack after durable (R-DURABILITY S1) A new crate, no consumer yet. It is stage 1 of the R-DURABILITY rewrite (handoff/rewrites/R-DURABILITY/DESIGN.md): every acknowledged change of the cmux-tui daemon and of acpmux goes into one journal of this format, and a reply goes out only once its record is synced. - Journal::append copies one record into memory (no I/O, no wait) and returns its LSN. One writer thread takes every pending record as one batch, writes it with one write, and syncs it once when it holds an acked record (group commit). Durable listeners then release the replies that wait for those LSNs. Stream records (terminal output, streamed tokens) are written in the next batch and synced with the next acked batch. - Sync levels: F_BARRIERFSYNC per acked batch on macOS (fdatasync on Linux), and F_FULLFSYNC at most 1 s after the first barrier sync, so a power loss takes back at most 1 s of acked batches, always a suffix. - Recovery is the one start path (clean exit, crash, kill -9, OS crash): it syncs the last segment, checks every record's CRC and LSN, and hands the records after the caller's compaction point to its replay. A segment is only ever appended to: a torn tail stays in its file and the journal goes on in a new segment whose header names the last good LSN. Damage that is not a torn tail moves the later segments to quarantine/ (never deleted), and no LSN is used twice. - A failed write is cut back and written again in the same segment; a failed sync is never retried on that segment (its pages may be clean and lost): the records after the durable LSN go into a new segment. - Storage is the file system seam. The `sim` feature's SimStorage keeps synced and unsynced bytes apart and crashes at any storage operation, then keeps what a process crash, an OS crash or a power loss keeps. Tests: unit tests of the format, group commit, ack after durable, listeners, sync levels, idle (no storage operation), backlog, seal, lock, rotation, failed writes and syncs at every operation, torn tails, quarantine and LSN reuse; the crash simulation (200 seeds per crash kind by default, CMUX_JOURNAL_CRASH_SEEDS for the acceptance run); and a round trip with a torn tail on the real file system. OWNERS.md: a row for the crate (R-DURABILITY rewrite owner).
…clippy) Fleet job 1a494d9372ec2bb7d71e8e1e at 26e4e71 refused two lines: E0689 (is_multiple_of on an untyped range in a test) and dead_code (Active::seq in the writer: the next segment's number is next_seq). The acpmux notify::Watcher import error in the same run is a base red.
… segment while nothing is owed - macOS sync_file fell back to fsync on ANY fcntl error. An EIO from F_FULLFSYNC or F_BARRIERFSYNC then became a success of a weaker sync, and the writer counted the records durable. Now only "not supported" (ENOTSUP, EOPNOTSUPP, EINVAL, ENOTTY: a network or FAT volume) falls back; any other error reaches the writer, which starts a new segment. - After a rotation the writer made the next segment at its next pass even with nothing to write (the shutdown pass did it too). A segment is now made only when records or a full sync are owed. - Test-only and lint-safety rewrites: a slice parameter in place of &Vec, a type alias for the crash model, a range check, one split match scrutinee.
Fleet job 7f6bfd5afb225cd1cee9d0f1 at 36f09c8: 24 of 25 crate tests pass (the three 200-seed crash simulations included). The one red, a_compaction_point_past_the_end_never_reuses_an_lsn (left [1], right [1, 10]), was a test fault: it appended LSN 10 in the restarted storage and then read back the first one. The clippy type_complexity hit of the same run is the crash model type, fixed in f55f8c5.
fcntl(F_BARRIERFSYNC / F_FULLFSYNC) and fsync can return EINTR. The writer counted that as a failed sync and moved to a new segment. Now the call is made again, as std's sync_data and sync_all do (cvt_r).
…1 review regressions (red) The S1 review (FIX at 89293fb) found that the crash simulation could not find its F1 and F4: it never crashed inside recovery, a failed operation had no partial effect, and a failed sync never lost a page. This commit changes only the simulation and the tests; the next commit fixes the journal. The simulated machine (sim.rs) now has: - a page cache over a disk, per 4 KiB page; a sync writes the dirty pages; a sync that fails writes some of them and drops the rest as clean, so a later sync reports success without them (Linux write-back error behavior); - a write that fails writes a random prefix first; - one ordered log of names (journaling file system): a folder sync makes durable every change up to the last one in that folder; a rename between folders is one change; an unsynced folder can be lost; - a process crash keeps the machine whole, so a later OS crash still loses what it would have lost; - an OS crash writes each dirty page whole, not at all, or a prefix of its sectors; a power loss also cuts back to a common point after the last full sync; - counters of open files and of injected failures. The crash runs (crash_sim_tests.rs) now run several epochs per seed: appends, a crash at a random operation (sometimes after a failed write or sync), and an open that sometimes crashes inside recovery and is tried again. After each open the oracle checks every record byte for byte, that every record an earlier open replayed is still there, that the epoch's records are a prefix of its appends, and that every acked record (fully durable after a power loss; every written one when only processes crashed) survives. Four runs: process, OS, power loss, and mixed. Regression tests for the review findings, red on the code before the fix: - F1 a_crash_inside_the_quarantine_step_never_reuses_a_quarantined_lsn - F2 a_damaged_header_in_the_last_segment_never_reuses_its_lsns - F3 a_panic_on_the_writer_thread_closes_the_journal - F4 a_restart_after_a_failed_sync_keeps_every_later_ack - F6 recovery_holds_at_most_two_files_open
…eral segments (red) The reviewer's F2 scenario has two segments: S1 holds 1..10, S2 (the last) holds 11..20, and S2's header is damaged. The test now rotates segments (300-byte segments, 20 acked records) and damages the last one's header, so the report must say lost_through 20 and the next append must take 21. On the code before the fix the moved segment's LSNs read as 0, so the next append reuses one.
…S crash) (red) On the code before the S1 fix, the crash-sim step was green (fleet 184129d8 at 263da86): 6 of 7 runs passed, and the mixed run failed only seed 140 in the full run and passed when it ran alone. The random faults reached the order the review found (F4) in 1 of 800 runs. So these tests were no red/green proof. The order: the sync of a write that spans pages fails and the disk gets none of them (Linux keeps them clean in the cache, so a later sync reports success); the process is killed in the writer's backoff; the next open reads the failed segment from the cache, replays its records and goes on; an OS crash or a power loss then takes those pages, and records an open already replayed are gone. - sim.rs: `fail_next_sync(pages, kind)` fails the next sync that has at least that many dirty pages, and the disk gets none of them. `crash_after_failure(n)` makes the n-th operation after the next injected failure the crash point. - crash_sim_tests.rs: a new run forces the order on every seed (a_kill_after_a_failed_sync_then_os_crashes_keep_every_acked_record): epoch 0 aims the fault, the kill is a process crash at the writer's retry, the open after it does not crash, epoch 1 ends in an OS crash, and a third open checks that the records the second open replayed are all there. In the random runs, one epoch in four aims the fault, with the crash 1 to 3 operations after the failed sync and a crash of any kind after it. - A sim test checks the aimed fault itself (the_sim_fails_the_aimed_sync_and_crashes_in_the_backoff). Expected on this commit (old journal code): the forced run fails every seed; the mixed run fails most runs. The fix commit follows.
…y, LSN floor, writer end) Review FIX at 89293fb (PR #33919 comment 6610468202). The regression tests are the two commits before this one. - Sync markers (format.rs): an entry with flags 1 and no payload, whose LSN field says "every record of this segment before me is synced". The writer appends one after each successful sync that covered records (after the durable listeners, so no reply waits for it), and syncs it fully at a seal and at a rotation. - F4, recovery (step 4 and 6): the last segment is trusted only up to its last marker. Its records after the marker may sit in pages a failed sync dropped (a later sync on another descriptor reports success without them), so recovery writes them again into a new segment: header, records and a marker, fully synced under `<name>.seg.new`, renamed into place, folder synced. The journal always goes on in a segment this open made, never in one an earlier process wrote. A restart in the writer's backoff after a failed sync is then safe. - F1, quarantine: every LSN a quarantined segment holds counts in the LSN floor at every open, and a quarantine that no new segment followed (a crash in the middle of an earlier recovery) is reported as damage again. The highest LSN of every moved segment is read before the first rename. - F2, highest LSN: a segment whose header is damaged is read from its first record (each record's checksum covers its LSN), then the bytes after the first bad entry are scanned for records and markers that still check out. The same scan runs after a cut in the last segment: a marker there is media damage (reported), records there are a torn write (`torn_lsns`); their LSNs are never used again. - F3, writer end: a guard closes the journal when the writer thread ends, also by a panic: appends fail with Closed, waits end with WriterEnded, the listeners are dropped (so a channel a held reply waits on closes), and stats.failing says why. The crate does not abort the process; the daemon owns that policy. - F6: recovery closes each segment after its header is read and opens one again only to read it. (Retirement of compacted segments is an S2 API, before the first consumer.) - F5: recovery syncs the parent folder after it makes the journal folder. - F7: format.rs and lib.rs now say what the code does: a failed write is cut back and retried in the same segment; the cut removes only bytes never synced and never acknowledged; nothing shortens a segment past a synced record. Docs: journal.rs says when open's replayed records are durable (on Ok) and what a stopped writer does.
… (review G5) sim.rs had 924 lines and 63 functions, over the 60-function limit for a new Rust file (the god-file check warned "not in baseline: split it"). The machine model (each file's page cache and disk, the ordered log of names, and what an OS crash or a power loss leaves) moves to sim/machine.rs (297 lines, 19 functions); the process view and the Storage implementation stay in sim.rs (639 lines, 30 functions). No behavior change: the items move as they are, made visible to the sim module.
…ash sim (red) Re-review FIX at 3498975 (PR #33919 comment 6620113993). Tests and sim only; the fix follows. - sim: the pages a failed sync dropped stay in the cache until an eviction; `evict_dropped_pages_after_read(true)` makes each read evict the dropped pages it read, so a second read of them sees the disk's bytes. - G1 recovery_copies_the_records_it_checked_when_the_cache_drops_them: a sync fails and drops record 2's pages, a kill in the backoff, the next open reads record 2 from the cache (replayed) and the pages are evicted after that read; then an OS crash. Record 2 and the record 3 acked after the restart must survive. On this code recovery's second, unchecked read stages zeros under a vouching marker, and the last open keeps only record 1 and reports damage. - G2: the F1 regression asserts that every open after the damage reports the loss (lost_through 40). On this code a crash at the open's last step (the folder sync after the new segment's rename) loses the report for good. - G3 a_journal_in_a_symlinked_folder_opens_and_round_trips (real file system): a journal in a symlinked state folder, and a journal folder that is a symlink. On this code the parent folder's sync opens with O_NOFOLLOW (ELOOP). - G4 an_open_after_a_finished_quarantine_reads_no_quarantined_segment: on this code every open reads every quarantined segment in full.
Re-review FIX at 3498975 (PR #33919 comment 6620113993); the regression tests are the commit before this one (red: fleet de35f6d0). - G1: recovery writes again exactly the bytes its checked read returned. The last segment's entries after its last marker are kept from the checked read (header and payload) and staged from memory; the second, unchecked read of the file is gone. A page that a failed sync dropped can be evicted between two reads, and the second read then returned the disk's zeros, which the copy kept under a vouching marker. - G2: the damage an open finds is written to `DAMAGE` (whole, fully synced, renamed into place, folder synced) before the new segment that hides it from later opens. Every open reports the recorded damage first, until the daemon calls the new `Journal::clear_damage`, which moves the record into `quarantine/` (kept, never deleted). - G3: a folder sync follows a symlink in its path (no O_NOFOLLOW): a state folder, or the journal folder itself, may be a symlink. The sync is for the folder that holds the entries. - G4: the quarantined segments are read for their LSNs only while no live segment is numbered above them (an unfinished quarantine); once one is, its first LSN already carries their floor. - G6: `checked_div` in the scan of the bytes after a cut (the crate's one tracked arithmetic hit). - G5 (the sim split) is the commit before the regressions.
Third review FIX at cb893c6 (PR #33919 comment 6621681018). Tests only. - G7 A a_damaged_header_after_a_quarantine_never_reuses_a_quarantined_lsn: after a finished quarantine, one byte of the empty new segment's header changes. On this code the next open skips the quarantine scan (the listing counts the damaged segment) and the next append reuses a quarantined LSN. - G7 B a_damaged_newest_segment_after_a_quarantine_reports_its_records: the newest segment holds acked 41..43, and its header and its first record are damaged. On this code the moved segment is scanned with a base far below 41, so the loss is not reported through 43 and the next append is not 44. - N1: the F1 regression also asserts that an open reports each loss once (no reported LSN range lies inside another). On this code a crash after the quarantine's renames makes the next open report the same loss twice.
…y.rs Third review FIX at cb893c6 (PR #33919 comment 6621681018); the regression tests are the commit before this one. - G7: the quarantine scan is skipped only when a segment whose header checked out is numbered above every quarantined segment (the folder listing also counted a newest segment with a damaged header, which carries no floor). The scans start from the running floor: `quarantined_floor` and `quarantine` pass the highest LSN counted so far as the base of the next file's scan, so a quarantined file with no readable header and no readable first record is scanned for the LSNs right above what is counted. - The "unfinished quarantine" report is made only when the quarantine holds LSNs nothing else counted (an old quarantine behind a segment that a crash tore at its creation holds none, and a crash is not damage). Its reason now also names a damaged newest header. - N1: a loss whose LSN range lies inside a recorded one is the same loss found again after a crash: it is neither recorded nor reported twice. - N2: recovery.rs's docs name `DAMAGE`; two doc lines over 100 columns are wrapped (lib.rs, sim.rs). - recovery.rs was at 987 of its 1000 lines: the damage record moves to recovery/damage.rs, the entry reader and the LSN scans to recovery/scan.rs (moved as they are).
…e (N1) The red run of the N1 regression (fleet 27d011ca, op 64 of the F1 test) shows the case the previous commit did not cover: a crash after the quarantine's renames and before DAMAGE is written; the next open finds both the damaged synced record of the last segment (7..14) and the unfinished quarantine (7..40), and reported both. Now every loss inside another (recorded or found by this open) is dropped, the larger range kept; DAMAGE is rewritten only when its set of losses changed.
Fourth review FIX at 215c8f0 (PR #33919 comment 6622740919). Tests only. - G8 a_recovery_copy_above_a_quarantine_never_reuses_a_quarantined_lsn (the reviewer's three steps): records 2..5 unsynced and a kill -9; open #1 copies them into C1 and goes on there with 6..10; C1's header is damaged, open #2 quarantines C1, copies 2..5 again into C2 and makes the empty E (first LSN 11); E's header is damaged. On this code open #3 sees C2 numbered above the quarantine, skips its scan and gives 6, an LSN the quarantine holds. - N3: G7 B and the F1 test assert that every file in quarantine/ is named by a reported loss. On this code the absorbed loss's names are dropped (G7 B). - N4: the F1 test asserts that the loss through 40 names the segment where the journal ends (00000000000000000002.seg). On this code a crash after the moves into quarantine/ and before DAMAGE turns it into the vaguer "unfinished quarantine" report.
… the moves (G8, N3, N4) Fourth review FIX at 215c8f0 (PR #33919 comment 6622740919); the regression tests are the commit before this one. - G8 (the G7 class): the skip of the quarantine scan trusted a live segment numbered above the quarantine to carry its floor, which a recovery copy does not (its records start at vouched + 1). Now each moved segment is named `<segment>.<n>.<floor>`, with the highest LSN counted when it moved (that file's and every one before it, scanned from the open's floor), and every open takes the quarantine's floor from the names alone: no quarantined file is read (G4 stays), and no header can hide the floor. `quarantined_floor` and the `checked_max_seq` skip are gone. The "unfinished quarantine" report is made when the names' floor is above everything else counted. - N4: `quarantine()` is split into `plan_quarantine` (measure and name every file, no change on disk) and `move_quarantined`; the loss, with the planned names, goes to DAMAGE before the moves. A crash after the moves keeps the precise report (the damaged segment and reason). - N3: when one loss absorbs another, the absorbed loss's files and reason go to the kept one, so DAMAGE names every quarantined file. - Docs: recovery step 5 and lib.rs name the quarantine file format.
…essary_sort_by) Fleet d9f22bed at b589044: the 33 journal tests pass, and clippy -D warnings fails at recovery.rs:478 (`consider using sort_by_key`).
…S2's projector) R-DURABILITY S2 (acpmux event logs through the journal) projects each chat's log line into its ndjson file once the journal has WRITTEN the batch that holds it, so readers see stream lines at once. The listener runs on the writer thread after the write and before the sync, once per batch that moved the written LSN: a batch written again into a new segment after a failed sync is not heard twice. When the writer stops, these listeners are dropped too. Test: heard while the batch's sync still runs, once. Port of 145faa6 (branch rewrite/r-durability-s2-acpmux-logs) onto the S1 review fixes.
…paction point covers) R-DURABILITY S2: a daemon's projector compacts the journal into its own files (acpmux: the chat ndjson logs, then a synced checkpoint). Without a delete the segment count only grows (review finding F6). `retire_through(compacted)` deletes the segments that hold no record after the caller's compaction point: a segment goes when the next segment's "previous last LSN" is at or before it, so the newest segment never goes. The point is capped at the durable LSN. The deletes and a folder sync run on the caller's thread (the projector, never a request path). A delete that fails leaves its segment live for a later call. Why a crash during a retirement is safe: the caller reopens from a point at or after `compacted`, and recovery reads nothing from a segment the point covers (it skips it on its next segment's header), so any subset of the deletes may reach the disk. A point older than the retired segments is reported as Damage::Missing. Quarantined segments are never deleted. - Storage::remove_file (FsStorage: std::fs::remove_file; SimStorage: one more change in the ordered log of names, durable at the folder sync). - The live segment list: recovery's, then each segment the writer makes. - JournalStats::segments_retired. - Tests: only covered segments go, and a second call deletes none; an OS crash after it keeps every record after the point and an older point is reported; a crash at every operation of a retirement (process, OS crash, power loss) keeps every record after the point and never reuses an LSN; the newest segment stays; a point in the middle of what this writer wrote. This adds a delete of journal data, so its PR is risk:high.
store.rs (2139 lines, 120 fns) and config.rs (1909 lines, 97 fns) are over
the god-file limit, and the journal store (R-DURABILITY S2) must not grow
them. MemoryStore moves to store/memory.rs and StoreMode, StoreConfig and its
defaults to config/store_config.rs; both stay exported where they were
(crate::store::MemoryStore, crate::config::{StoreConfig, StoreMode}). No
behavior change: the code moves as it is.
R-DURABILITY S2 (DESIGN.md 3.3b). Today no event-log append is synced: a line
a client was told about is on the disk only after the next flush (shutdown),
so an OS crash or a power loss takes back every line since (E12). This commit
adds only the switch, a seam and the tests; the journal store follows.
- `store.journal` (StoreConfig::journal, default false).
- `Store::wait_durable(timeout)`: tests and tools wait until every appended
record is durable. The default returns at once (the Local and Memory stores
never sync an append), which is today's behavior.
- `store::open_with_journal` (tests): the store with the journal on a given
`cmux_journal::Storage`. Here it ignores the journal and opens LocalStore.
- acpmux depends on cmux-journal (dev: with the `sim` feature).
- store/journal_store_tests.rs, red on this commit:
- a_line_the_store_says_is_durable_survives_an_os_crash (lines 2..20 are
lost: nothing synced them);
- a_deleted_chat_stays_deleted_after_a_crash and
a_line_the_log_refused_is_never_written_later pass here, and must keep
passing with the journal store (no replay may bring them back);
- a_kill_keeps_every_line_once passes here and must keep passing (a replay
after a kill must write no line twice).
…DURABILITY S2)
DESIGN.md 3.3b. With `store.journal` on, each event-log append is one record
in `<home>/journal/` (cmux-journal: group commit, one sync per batch; a power
loss takes back at most the last second, always a suffix), then the same line
goes to the chat's ndjson as before. Readers, segments and file formats do not
change. Default off.
- store/journal_store.rs (JournalStore over LocalStore):
- append: under one lock, the journal record (a memory copy, no I/O), then
the log line; a line the log refuses gets a `cancel` record. Agent stream
lines (`in` session/update, claude.*, codex.*) are Class::Stream, every
other line Class::Ack.
- checkpoint (`<home>/journal-projected.json`): every 30 s while the journal
grew, past 64 MiB, at flush (shutdown) and before a retention commit. It
syncs each log written since (LocalStore::sync_log, with no lock held
while the disk works) and the sessions folder, writes the LSN (temp file,
sync, rename, folder sync, every error checked), then retires the journal
before it. A log close that failed its sync freezes the checkpoint until a
restart (LocalStore::sync_failed).
- recovery: the records after the checkpoint; for each chat, the log is cut
right after its last readable line below the first journaled seq (later
segments removed, the boundary one shortened, their kind-index copies
dropped), and the journaled lines are written again in order. `forget`
deletes a chat again; `cancel` keeps a refused line out.
- delete: `forget` record, folder removal, then a bounded wait until the
record is durable.
- seal: the logs refuse writes and the journal syncs fully.
- LocalStore: `append` is serialize + `append_line`; `sync_log`; a failed sync
at a log's close is noted (`sync_failed`).
- store::open opens the JournalStore when `store.journal` is on.
Not yet (S2c): replies and events do not wait for durability; the daemon does
not stop when the journal closes; the writer thread keeps the process's
Utility I/O policy.
…its delete (red) - a_chat_made_again_after_its_delete_keeps_its_files_through_a_replay: a chat is deleted (its forget record), made again under the same id (a transfer back: session.json and new lines), then a kill -9. On this code the replay deletes the folder again and its session.json is gone. - a_line_after_the_seal_is_refused_and_never_written_later: coverage of the seal (it passes here; the fix also closes a race the test cannot force).
…ps a chat made again - seal takes the appends' lock: an append lands in both the journal and the logs, or in neither. Before, an append that raced the seal could leave a journal record whose line the sealed logs refused (its cancel then failed on the closed journal), and the next start would write that line. - append and delete check the logs' seal first, under the same lock, and do not touch the journal after it (no error per append at shutdown). - replay deletes a forgotten chat again only when no line follows its forget record: otherwise the chat was made again under the same id, and its folder holds that chat's own files.
Fifth review FIX at 9c47594 (PR #33919 comment 6623794646). Tests only. - a_skip_hidden_behind_a_recovery_copy_is_never_given_out_again (the reviewer's route): record 1 synced, 2..10 Stream and 11 Ack synced as one run with marker 11, media damage in record 5. Open #1 reports (4, 11], copies 2..4 into segment 2 and makes the empty segment 3 from 12. Segment 3's header is damaged. On this code open #2 reads segment 1 only through the copy's previous last LSN, never scans the damage again, and gives 5. - torn_lsns_hidden_behind_a_recovery_copy_are_never_given_out_again: the same for torn records 4..5 (a tear, then a whole record that no marker vouches for) behind the copy of 2..3: on this code the next append gets 4.
Fifth review FIX at 9c47594 (PR #33919 comment 6623794646); the regression tests are the commit before this one. When an open gives out LSNs above everything its kept and new segments say (damaged synced records or torn records in the last segment, which a copy then hides from every later open), the only record of that floor was the header of the empty segment it makes; a damaged header lost it, and DAMAGE is cleared by `clear_damage` (torn records are not damage at all). Now such an open makes the empty file `FLOOR.<lsn>` and syncs its folder before any new segment, and every open takes the highest floor from the folder listing (no file read), as it does for the quarantine names. The caller's compaction point, the quarantine's names and this open's moves say their floors already: no file is made for them.
Both sides added tests after the same test in cmux_journal_tests.rs; both are kept (the G9 tests, then the S2 retirement section).
Review FIX at ab0e953 (PR #34901 comment 6624439824). Each test fails on this commit and passes after the fix that follows it: - 1a lines_written_with_the_switch_off_survive_turning_it_on_again - 1b a_kill_while_the_journal_syncs_keeps_the_lines_the_log_got - 1c a_line_flush_session_made_durable_survives_losing_its_record - 2 a_folder_put_back_after_a_delete_is_not_deleted_by_the_replay - 3 a_kill_in_a_delete_leaves_the_chat_whole_or_gone - 4 a_delete_does_not_wait_for_the_journal_sync - 5 a_failed_log_sync_freezes_the_checkpoint, a_checkpoint_that_keeps_failing_asks_the_daemon_to_stop - 6 one_chat_that_cannot_be_repaired_does_not_stop_the_open - 7 the_journal_storage_refuses_a_lost_home - the repair keeps a log's lines after a hole, also through a crash in it: an_os_crash_hole_is_repaired_and_the_lines_after_it_kept, a_crash_inside_a_repair_loses_no_line - nits: an_unreadable_checkpoint_fails_the_open, the_open_waits_for_the_previous_daemon_to_let_go_of_the_journal, a_crashed_checkpoint_temporary_is_removed_at_open, a_crash_right_after_a_checkpoint_reports_no_damage - the catch-up with the switch off: switching_off_after_a_crash_keeps_every_acknowledged_line - cmux-journal: check_append_says_what_append_would_do So they compile here, this commit adds the seams they use, with no behavior: `Store::stop_reason` (default None), `Journal::check_append` (always Ok), `FencedStorage` (never asks its fence), the open report and a test stop point after a repair's cut.
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
…sion.json another journal wrote reaches it A folder moved in from another home (build adoption), checkpointed and replayed, and a journal made again from nothing: the other journal's journalLsn must not hide a change made here. The first test is the reviewer's (fleet 4d0d98bc and 071ad7c4 at e944a7c).
…d; a file another journal wrote follows the stamp rule session.json now carries journalId (16 hex digits, a string) beside journalLsn. The checkpoint's "held" check (F2) and the replay's rule compare an LSN only for a file with this journal's id: a file from another journal (a chat folder build adoption moved in from another home, or a home whose journal was made again) counts as one this journal did not write, so the stamp rule decides, as for a run with the switch off. cmux-journal: Journal::journal_id (the id already in every segment header; kept across opens and retirements), with a unit test. Red at 7c8cb95: fleet c29b659d (Linux) and 47570494 (macOS) running; the reviewer's red at e944a7c: 4d0d98bc (macOS) and 071ad7c4 (Linux).
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 4, 2026 20:26 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
…o the S2 follow-ups Conflict: journal_meta_tests.rs, where both sides added tests at the end (this branch's review tests of #35562; S3a's N1 tests). Both kept.
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 4, 2026 20:55 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
artifacts
November 4, 2026 21:09 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
sentry-dsyms
November 4, 2026 21:09 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
artifacts
November 4, 2026 21:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Review (r-durability-fu-reviewer): SHIP at 7fc15b7 Re-review after my SHIP at 71d7e09 (comment 6638442340). That SHIP counted only for that head. This head adds only merges down the stack (the S2 follow-ups fixes, the S3a fixes with N1, feat 7427100). What I checked
Not in this PRMy delta review of #35562 at dbd1e79 (FIX, finding D1, P2) is in the base under this PR, not in its own change: the next open consumes a repair file even when the chat's folder is not there. When #35562's fix is merged down here, the new head needs a new receipt. Evidence and what I did not run
|
Base automatically changed from
rewrite/r-durability-s2-followups
to
feat-cmux-next
November 4, 2026 23:01
github-actions
Bot
added
risk:high
S2fu (#35562) landed on feat as one squash commit (0b9e166), so its store files no longer match the S2fu commits this branch had merged. Conflicts in 4 store files (journal_store.rs, journal_store/meta.rs, journal_store/repair.rs, journal_store_tests.rs), all resolved to feat's copy: this branch never changed a store file of its own. Check: the result differs from feat 07c879a in exactly this PR's three hub files (hub/meta_line_order_tests.rs, hub/mod.rs, hub/tags.rs; 129 lines added, 4 removed), and in no file under store/ or cmux-journal/.
lawrencecchen
temporarily deployed
to
ci-routing-agent
November 4, 2026 23:35 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
|
github-actions
Bot
added
risk:normal
lawrencecchen
temporarily deployed
to
sentry-dsyms
November 4, 2026 23:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
artifacts
November 4, 2026 23:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
lawrencecchen
temporarily deployed
to
artifacts
November 4, 2026 23:50 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
|
Review (r-durability-fu-reviewer): SHIP at 6a5a6d1 Delta review after my SHIP at 7fc15b7 (comment 6642506217). The PR's base is now What I checked
Evidence and what I did not run
|
lawrencecchen
added
the
ready-to-land
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stage S3b of the R-DURABILITY rewrite (design:
handoff/rewrites/R-DURABILITY/DESIGN.md3.3e, "C2 ... revised 2026-11-04"). Stacked on the S2 follow-ups PR, which is stacked on #35483 (S3a).E6: a chat metadata change that also logs a line (
tags,renamed,rules,policy,mode) must never be, after a crash, in the chat's metadata without its line in the log. With the event journal (S2, S3a), records survive a crash as a prefix in append order. So the change is safe when its line reaches the store before any metadata image that holds it.renamedandpolicyalready did this;tagsandrulessaved their metadata first. (modemostly did; see the review notes below.) This PR moves their line first. Their reply still waits for the save (and, with the journal on, for its record: S2c).Why not one combined record (the design's first plan): an event-log line goes to the log file before its record (S2's log-first append), and the repair keeps a line only the log holds. So after a kill the log can hold the line while no record of it is durable, and one record with both the line and the image would not make them atomic in the log. It would also race
MetaWriter's coalesced copies.What changes
hub/tags.rsset_tags_answeredandhub/mod.rsset_rules_answered: the line, thensave_meta_answered.Known gaps
MetaWritercopy that another thread takes between the change and its line, appended before the line, then a crash in the next group commit (DESIGN.md 3.3e).71d7e096cba(SHIP, comment 6638442340; P3, follow-ups owned by R-DURABILITY, written into DESIGN.md 3.3e):write_mode_statesaves before themodeline when cmux's mode mark clears; a chat's first append after a start (catch_up_locked) can save an image that already holds the new tags before thetagsline; with the journal on, a watcher can see thetagsevent one group commit before its metadata record is durable (the gate holds the event only to theAckLSN read when the line is queued); other sites save, then log (seen.rs,work_below.rs,turns.rsmodel_request,tap.rs,watch.rs;transfer_out.rson purpose).Testing
acpmuxhub::tags::meta_line_order_tests(a recording store checks the order of the store calls):a_tag_change_logs_its_line_before_its_metadata,a_rules_change_logs_its_line_before_its_metadata: red at7a4f02a56f3(the tests before the fix), owned Mac aws-m4pro-7, fleet 88309e91: tags["save:tagged", "line:tags"]; rules["line:permission_chat_allowance", "save:ruled", "line:rules"].a_rename_logs_its_line_before_its_metadata: a guard, green before and after.Merged down since the SHIP at
71d7e096cba(no change to this PR's own files): the S2 follow-ups fixes (the idle checkpointer race5279af4bbf7, the repair file3b7007c175d), the S3a fixes (0d246364d90, N1fc83b326fba) and feat7427100d5d0(with #35845). Head7fc15b776ef: fleet 1a6d2f28 (owned Mac aws-m4pro-7),journal_350 of 350; fleet ea9d2c52 (Linux, hq run 41907479976),hub::1139 of 1139 (acpmux 1125, cmux-remote 1, cmux-tui 13). fmt at66e1df5ec5e: fleet 9232d441, green. The 50 runs ofa_failed_log_sync_freezes_the_checkpointat71d7e096cbahad 1 failure, the idle checkpointer race (#35562 has the analysis); 0 of 50 at the fix.Green at
71d7e096cba: fleet 25d8970a (owned Mac aws-m4pro-3), the 3 order tests pass; fleet 0b984386 (owned Mac aws-m4pro-5),hub::1083 of 1084. The one failure,hub::shutdown_tests::a_cut_turn_of_agent_messages_puts_none_of_them_back, timed out once in the full run (shutdown_tests.rs:237,Elapsed) and passed on its rerun; it uses no tags or rules.Observed / Not run
policyandmodein this test (their paths need a running agent); they write the line first by reading.Merged down after #35562 landed (head
6a5a6d138c9)bf2f48d677amerges feat07c879aab80, which has #35562 as one squash (0b9e1668a7b). It conflicted in 4 store files (journal_store.rs,journal_store/meta.rs,journal_store/repair.rs,journal_store_tests.rs). All 4 are resolved to feat's copy, because this PR never changed a store file of its own.6a5a6d138c9is a clean merge-main of featecfe8d94e9d. Against feat, the head differs only in this PR's 3 hub files (hub/meta_line_order_tests.rs,hub/mod.rs,hub/tags.rs; 129 lines added, 4 removed).Green at
6a5a6d138c9:journal_: 355 of 355 (fleet 172c6c27, aws-m4pro-7).meta_line_order: 3 of 3 (fleet 8a6b3a10, aws-m4pro-8).all hub::with fmt and clippy-D warnings: 1139 of 1139 (fleet da5b97a1, Blacksmith; hq run 41935007261).all store::with fmt and clippy-D warnings: 356 of 356 (fleet 51e7e557, Blacksmith; hq run 41935006187).Changelog
none
Proof
No capture: a daemon change with no UI.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Makes tags and rules changes log their event line before saving their metadata, closing a durability gap: after a crash, a change could previously survive in the chat's metadata without its line in the log. With the event journal, records survive as a prefix in append order, so logging first guarantees the line is durable before any metadata image holds the change.
renamed,policy, andmodealready did this; only tags and rules saved first, and their replies still wait for the save.Adds
meta_line_order_tests.rs, a recording store that asserts the store call order for tags and rules changes plus a rename guard.Written for commit 71d7e09. Summary will update on new commits.