PR #29311 - manaflow-ai/cmux

Summary

kill -9 of acpmux could lose an acknowledged change to a chat's metadata. _acpmux/draft_set (the schema calls it "the one durable shared draft") came back older after a restart. Fuzz row g found it: seed 26369, job 74fc575c39c118270101ec7a.

Cause: Hub::save_meta copied the chat's metadata, released the lock, and wrote the copy to session.json. Nothing put two saves of one chat in order (LocalStore::writable takes only a shared lock). A save that copied the metadata before a change could write after that change's own save. So session.json kept the older value, and the request had already answered ok. If the daemon died before the next save, the change was gone. The draft lives only in session.json, and so do the other fields below.

Fix: one writer per chat, which commits versioned changes in order (hub/meta_save.rs, MetaWriter::commit):

Each change is made in memory first and then takes the chat's next version (requested += 1).
The writer is whichever caller finds no writer at work. It reads target = requested, copies the metadata (the copy holds every change up to the target), writes it (write_atomic: on disk, renamed, folder synced), sets durable = target, and goes on while a newer version waits. No other caller writes, so a copy older than a committed change is never written after it. A burst of changes shares one write (group commit).
A request that reports a change (save_meta_answered) answers only when durable >= its version. A commit that fails fails the requests it covered with store.save_failed, and the next change commits again. A change no request answers for takes its version and returns.
No I/O under the writer's mutex, no wall-time compare, and no retry. A writer that panics hands the role back.
session.json and the event journal keep their format, so existing chats load as before.
The creation save and the fork save go through the same writer.
Review of this PR (comment 6543584040), fixed red first:
- The shutdown flush waits for each chat's commit (write_meta) before the store is sealed, and logs an error. Before, a save running elsewhere let seal() come first, and the chat's last state was lost.
- A failed commit fails its requests, and the writer goes on with a newer version that waits. A change nobody waits for may have come during the failed commit.
- transfer_commit answers committed only once movedTo is on disk.
- session/set_model (modelRequest, models.currentModelId) and _acpmux/set_tools (tools) answer after their commit.
- markUnread on an unread chat and an unchanged bookmark_set wait for every earlier change of the chat (MetaWriter::barrier).

The request paths that answer through the writer. meta_save_order covers the first 9; every test in meta_save_kill_point.rs that walks the fields (the main kill test and the held-answer test) covers all 11:

Field	Request
`composerDraft`	`_acpmux/draft_set`
`name`	`_acpmux/rename`
`modes.currentModeId`	`session/set_mode`
`unread`	`_acpmux/markUnread`
`bookmarks`	`_acpmux/bookmark_set`
`tags`	`_acpmux/tag`, `_acpmux/archive`
`turnCapMs`	`_acpmux/set_turn_cap`
`permissionPolicy`	`_acpmux/set_policy`
`permissionRules`	`_acpmux/set_rules`
`modelRequest`	`session/set_model`
`tools`	`_acpmux/set_tools`

Test seam (debug builds only), ACPMUX_TEST_SAVE_META_HOLD_DIR (hub/meta_save.rs): hold-<id> holds a chat's next commit between its copy and its write. kill-<id> makes the daemon SIGKILL itself at that point.

Testing

Focused command: cmux-ci run --class light --script scripts/ci/cmux-tui-rust-check.sh --ref <sha> --arg test --arg meta_save. It runs 11 tests: the 4 kill/order tests and the writer's 7 unit tests.

tests/meta_save_order.rs: for each field, hold an older save, change the field, let the held save go, SIGKILL the test's own daemon, restart, and read the field.
tests/meta_save_kill_point.rs:
- every field: hold an older save, send change C and wait for its answer, let the held save go, then send change D with the kill point armed. The daemon dies in D's commit before D's write. After the restart C must be there.
- an answer that comes while an older commit is held: kill at once; the field must survive (this catches set_model and set_tools).
- a no-op (markUnread on an unread chat, an unchanged bookmark) answered while the change it repeats is held: kill at once; the state must survive.
Unit tests (hub/meta_save_tests.rs): order and answer after a racing commit; a panicking writer hands the role back; a failed commit fails its request and the next change commits; a change nobody waits for is written by the writer at work; a change during a failed commit is still committed; the shutdown flush saves a change made while another save runs, before the seal; a move's commit answers only once movedTo is on disk.

Reds (each fails on its own check, in the full run and alone):

e8be2c924ba (feat 5ada6804dc8 + first tests), job 261977bdf11437b273d8b391: all 9 acknowledged fields lost in both kill/order tests.
fuzz-red/acpmux-g2-review-red @ 40ec4a5c07d (the previous head 45b70582d0c + the review tests), job 012a3e9b3bf789f14b54c036: the 5 review tests fail. Flush: unread false on disk after flush and seal. Failed commit: the newer copy never written. Move: committed answered before movedTo on disk. Held answers: set_model came back m1 and the tool came back on. No-op: unread false after kill.

Green at 54df0c0d099 (this head), 5 of 5, each "11 tests run: 11 passed": 273fb13e853b8f5cf76729c5, b75259e721730b1a26d5f93c, 8232a07825a20854551b0d08, 29a8f3151f481e8af5f37814, fbfcaf1103c43a454d72b16e.

Clippy at this head, job a87c69969397a849d24f422b: green.

Fuzz row g on feat + the g1 fix (#29175) + this head + the row g harness (#29103), branch fuzz-red/acpmux-g2y-on-g1fix-a7fb56dc, job 6be7bbf48a0dced76a584b65: 4 of 4 tests, 12 seeds, 144 kills, 0 violations. Request latency p99 81-97 ms, max 178 ms.

Full cmux-tui-rust-check.sh test at 4d5886f3a70 (this head minus a test-name fix in meta_save_kill_point.rs), job ebfd44940f093759e71c380b: 13,545 passed. The reds:

meta_save_kill_point ... a_no_op_...: a setup fault in the test at that commit (two cases' holders took one name, the second rename was refused). Fixed in 54df0c0d099; green 5 of 5 above.
7 cmux-tui tests (cli_audit a_server_that_speaks_another_protocol_... and 6 setup tests) are red at the feat base 5ada6804dc8 too (job d35ce63b82d4f2b8ef1f472b): base findings.
startup_load a_restart_over_long_chats_...: red 3 of 3 at the base and 3 of 3 at 45b70582d0c (STATUS 1611). Base finding. ready_ms 5-6 ms at both (budget 100 ms).
peer_restart close_1012_...: red 3 of 3 at the base and 3 of 3 at 45b70582d0c. Base finding.
agent_host orphan_exit::a_daemon_connecting_while_the_harness_ends_is_told_it_exited: a base finding. Feat base 8320cc3f with no PR commits failed it at :173 twice on macOS (job 7c7ca0e14381718ab737c2d1, aws-m4pro-8); Linux 3 of 3 pass (97353d67, 8d141f02, 1f259c64). At 5ada6804dc8 it failed 1 of 3 (f01b5f6d843d). It drives only the agent host; this PR does not touch that code. Owner PL-acpmux, in followups.md.
Flaky: cmux-tui-core local_resize_bounds_the_drain_to_a_queued_snapshot (passed alone).

draft_set latency (scratch bench, 500 calls on one chat, Blacksmith Linux; before = feat + test, run 41195755296; after = 4d5886f3a70, job 1a98f1ca7eb87b80adde07de):

	before p50 / p99 / max	after p50 / p99 / max
alone	0.24 / 0.32 / 0.35 ms	0.29 / 0.39 / 0.46 ms
while 4 clients rename the same chat	0.47 / 0.68 / 0.79 ms	0.66 / 0.89 / 3.23 ms

A change to a chat now waits until a commit that holds it is on disk. Changes to different chats do not wait for each other.

Observed / Not run

Observed: the jobs above and their result lines.
Not run: no local build (repo rule). macOS fleet not run for this head; Linux route only. The bench ran on Blacksmith Linux, where an fsync is cheap. On a Mac disk, a change that comes while its chat's commit runs waits up to one more fsync.

Follow-up (not in this PR): the writer serves later changes until none waits, so its own request can wait while changes keep coming within one fsync each. set_bookmark holds the chat's move lock meanwhile. It is not a deadlock (the write needs only the meta lock and store locks), but under load it has no bound. A bound: the writer answers its own request after the first commit that covers it, and hands the loop to the next waiter.

Performance

draft_set latency, 500 calls on one chat, Blacksmith Linux (scratch bench; before = feat + test, run 41195755296; after = job 1a98f1ca7eb87b80adde07de):

	before p50 / p99	after p50 / p99
alone	0.24 / 0.32 ms	0.29 / 0.39 ms
while 4 clients rename the same chat	0.47 / 0.68 ms	0.66 / 0.89 ms

The p99 rise (0.32 to 0.39 ms alone, 0.68 to 0.89 ms with 4 renamers) is the cost of answering only after the change's commit is durable. The perf lead accepts it: it is inside the budget.

Changelog

Fixed: A change to a chat (its draft, name, mode, unread mark, bookmarks, tags, turn cap, policy or rules) is no longer lost when acpmux stops suddenly right after it answered.

Proof

No capture: daemon fix, no UI.