PR #35963 - manaflow-ai/cmux

Summary

Fixes #35905 (its sync failures freeze the store; one restart path is narrowed to #35915 item 5, below) for the acpmux event journal behind store.journal (default off; R-DURABILITY). store.journal (default off) stays off until this and the other data-loss gaps land.

The journal store's rule is that a failed sync of a file or folder under the store's home freezes the store, as a failed log sync does: a later sync of the same file or folder may report success for what the failed one dropped, so no checkpoint may go past it. The journal then keeps every line, and the daemon stops for a restart that writes them again. Three paths broke the rule, and each could let a checkpoint retire records whose data an OS crash then loses:

A retention rewrite's folder sync. commit_compaction syncs the chat's events folder after it renames the rewritten segments. A failure returned SyncFailed to the retention pass, which only logs a warning. A line appended to a rewritten segment whose rename was lost could then be retired by the next checkpoint.
A retention rewrite's record. events/compacted.json (written before the renames, so that a log that may hold nothing never passes for a purged chat's, and again after them) went through config::write_atomic, which drops its folder sync's error. So a rewrite whose record may be lost went on to its renames.
A kept-aside metadata image. When a chat's folder was replaced outside the store (a transfer's swap), the checkpoint keeps the image in journal-unapplied/. A failed sync there returned an error but froze nothing, so the next checkpoint wrote the file again, synced the folder again (a sync that may report success without the dropped entry) and retired the image's record.

What changes

JournalStore::commit_compaction runs the rewrite inside counted_sync (#35895): a SyncFailed anywhere in it freezes the store, and a checkpoint in flight waits for it.
compact.rs writes the record with a new write_durably: the same temporary file, sync and rename as write_atomic, but a failed sync of the file or of its folder is a SyncFailed error, and the rewrite stops before its renames.
write_through keeps an image aside inside counted_sync, and keep_aside_meta's file sync is a SyncFailed too.
Not changed: files that the open keeps aside (its repair and its replay). An error there fails the open, so no checkpoint runs past it.

Testing

New module store::journal_sync_tests (because journal_store_tests.rs is over its size limit, its split follows #35866). New test seam FAIL_DIR_SYNC_WHEN: the next sync of a folder for which a condition holds fails. So a test names a sync by the state it runs in, not by its rank, which differs between the code before and after the fix.

a_failed_sync_of_a_rewritten_log_folder_freezes_the_store: the events folder's sync after the renames fails (condition: no staged copy left). A line is then appended to the rewritten segment. A checkpoint must not go on, the store must ask the daemon to stop, and the restart must have every line.
a_failed_sync_of_a_rewrite_record_folder_freezes_the_store: the events folder's sync while the record is in place and the copies wait for their renames (only the record's own write makes that state). The rewrite must fail with the segment unchanged, and the store freezes.
a_failed_sync_of_a_kept_aside_image_freezes_the_store: the journal-unapplied/ sync of a kept image fails. The second checkpoint must not go on. With the kept file gone (the lost entry), the restart keeps the image aside again from the journal.

Red on feat (the tests alone, before the fix): all 3 fail, in the full run and alone, each for its reason: at 3fcdc7a7857 on macOS (fleet edd042fb) and Linux (fleet 71dfdf20); with test 3's second checkpoint, at e05c27508b6 on macOS (fleet 7fe6f4ba). Test 1: "a checkpoint went past a failed sync of a rewritten log's folder". Test 2: "the rewrite went on past a failed sync of its record's folder". Test 3: "a checkpoint went past a failed sync of the folder that keeps an image aside".

Green at the head: 6823956542f: macOS journal_ 362 of 362 (fleet c856f747); macOS compact 54 of 54 (fleet bc18bdd4); Linux all store:: 363 of 363 with fmt and clippy -D warnings (fleet 20cadbf5, Blacksmith; hq run 41940786873).

Observed / Not run

Observed: the fleet steps above, each log read.
Not run: a real disk's failed fsync (the tests use the sync seams); a failed sync of the record's or kept image's file (as opposed to its folder): it takes the same path into counted_sync, but no seam fails a plain file's sync; a dogfood daemon with the switch on.

Review of `6823956542f` (FIX): what changed

The review found one P2 (finding 1) and two P3s.

Finding 1 (P2). The freezes are correct. But the restart after a freeze must also make again the folder entries that the failed sync may have dropped, and for two paths it wrote in place:

Path 3, the kept image: fixed here. keep_aside_meta now writes through Inner::write_kept, as the repair's kept lines already are: a new file under a temporary name, synced, renamed into place, the folder synced. So a restart that keeps the image aside again makes a new folder entry, which the folder's next sync covers. write_kept's own file sync is now a SyncFailed, so the kept image's failed file sync still freezes the store.
- Red at bb040e72546: the reviewer's test, a_restart_after_a_failed_kept_image_sync_makes_its_entry_again (red on macOS fleet b165aaf9 on the reviewer's branch). My red run: macOS fleet c207df0e (aws-m4pro-8). The command:
```
cmux-ci run --class isolated --script scripts/ci/cmux-tui-rust-check.sh --ref bb040e72546dcc8e2069fe6caebfc5f63f38e6dd --os macos --arg test --arg a_restart_after_a_failed_kept_image
```
  The compile finished (Finished test profile ... in 3m 40s), then the test failed at its path-3 assertion (journal_sync_tests.rs:286), in the full run and alone:
```
the restart's first checkpoint moved from 1 to 2, past the image's record (2), while journal-unapplied/chat-a.2.session.json is still the entry (inode 105807663) whose folder sync failed: no step re-made it
```
Path 1, the rewrite's renames: narrowed, tracked in #35915 item 5. The restart's rewrite is the code that #35866 changes (on feat-cmux-next every restart rewrites; after #35866 only a freeze does), so this fix goes on top of #35866. The fix: at the open, for a chat whose compaction record says complete: false, copy each segment to a temporary file and rename it in place, then sync events/. store.journal stays off until it lands (the ON plan in DESIGN.md 6).
The review also found that the crash model (sync_model::folder_synced) treats a later successful folder sync as durable, the opposite of the store's rule for folders. That design question is in #35915 item 5.

P3. The comments of counted_sync and commit_compaction now say what they count: the retention commit's renames and the folder sync after them are counted; the rewritten copies' own syncs run in stage_compaction, before any rename, outside the count.

Commit note: 524e7e58178 holds both the path-3 fix (meta.rs, repair.rs) and the P3 comments (journal_store.rs, checkpoint.rs). The pre-commit hook refused the first of the two commits I meant to make, because of the other files' unstaged edits. I did not rewrite the pushed branch.

Green at the head 524e7e58178: macOS journal_ (fleet 9b4a58ac) 363 of 363; Linux all store:: with fmt and clippy (fleet 1ffa5339) 364 of 364 (Blacksmith; hq run 41956968432).

Risk

risk:high (journal durability), behind the default-off switch. The change makes three failure paths freeze the store; a sync that succeeds behaves as before. The rewrite's commit now holds a sync ticket, so a checkpoint in flight waits for it; the commit already runs on the retention pass's spawn_blocking thread after a checkpoint of its own.

Changelog

none

Proof

No capture: a daemon storage change behind a default-off setting, with no UI.