fix(acpmux): the metadata writer: a no-op retries a failed commit, a panicking write commits newer changes, the writer answers its own request first (g2 follow-ups)

Merged
LA
5d ago
+157-8

Summary

Follow-ups from the review of #29311 (the per-chat metadata writer, fuzz row g finding g2). Each fix has a red test first.

  1. A retried no-op does not keep a failed commit's error. Before: after a failed commit, a request that changed nothing (markUnread on an unread chat, an unchanged bookmark) failed again until some other change came, because MetaWriter::barrier waited for a version the failed commit covered. Now, when the last commit failed and nothing was committed since, the barrier takes a new version and commits it.
  2. A test for the second session/set_model path. meta_save_kill_point.rs gets a models.currentModelId case: a running harness that reports models and has no model option (FAKE_PICKER, FAKE_MODELS). This path was already fixed in #29311 (it answers after its commit), so the case is green before and after; it guards that path.
  3. A write that panics no longer leaves newer changes unwritten. Before: the writer's panic hand-back set failed and returned, so a change nobody waits for that came during that write stayed in memory only. Now a panic in a write counts as a failed commit: the writer goes on with the newer changes, then the panic reaches its own caller (resume_unwind).
  4. The writer answers its own request first. Before: the writer committed later changes until none waited, so its own request could wait for any number of commits while changes kept coming, and set_bookmark held the chat's move lock all that time. Now waiting versions are counted. Once the writer's own version is durable, it hands the next commit to a request that waits for one, and goes on itself only for changes nobody waits for. A request that waits for its commit (Wait::Durable) waits for at most the commit under way and its own. After its own commit, the writer can still commit background saves of the same chat (changes no request waits for) before it hands the role back.

No on-disk format change (session.json and the journal are unchanged).

Testing

Focused command: cmux-ci run --class light --script scripts/ci/cmux-tui-rust-check.sh --ref <sha> --arg test --arg meta_save (14 tests).

  • Red at b8cc073c2b5 (feat after #29311 + the tests), job 601c34bbbbd4dfedccb83db8. The 3 new unit tests fail on their own check, in the full run and alone:
    • a_no_op_after_a_failed_commit_commits_again: "the retried no-op commits again: disk full";
    • a_writer_that_panics_still_commits_a_newer_change: the newer copy is never written (left [], right [2]);
    • the_writer_answers_its_own_request_after_its_first_commit: the writer serves B first (left [("a",1),("a",2)]).
      The 11 earlier tests pass.
  • Green at 6a4af4cadca, 5 of 5: 99884f1c46dd571089bd2589, 4b89e6056ddc1b2e127ba673, 47e62123966532f2bec10552, c1c4d276297eda612a117107, a6e832a399295ecec3d12524 ("14 tests run: 14 passed" each).
  • Clippy at 6a4af4cadca, job 55a2d62de5e559ce4604ee13: green. (The first clippy run, 8b05a33d925d, stopped on a base red in hub/processes.rs that feat fixed in #30009; this branch merges feat after it.)
  • Fuzz row g on this head + the row g harness (#29103), branch fuzz-red/acpmux-g2fu-rowg-79131d49, job aee348b390992d045c6639e7: 4 of 4 tests, 12 seeds, 144 kills, 0 violations; request latency p99 94-112 ms, max 167 ms.
  • Whole suite at this head, job 846d2514d67bb08249e9290c: 14,040 passed, 5 failed. All 5 are base findings: each fails 3 of 3 on plain feat 55361f16bf0 and 3 of 3 at this head (STATUS 3019):
    • acpmux::send_attach_family send_message_carries_attachments_to_a_child
    • acpmux::subsession_trust_inherit a_full_access_chat_lends_its_folder_trust_to_sub_sessions_under_it
    • cmux-agent-pane-policy the_allowlist_is_what_the_page_sends
    • cmux-tui cloud_help help_lists_exactly_the_offered_cloud_verbs
    • cmux-tui remote_cli_docs cloud_help_lists_every_offered_cloud_action.

Performance

draft_set latency, 500 calls on one chat, Blacksmith Linux (scratch bench; before = #29311's head, job 1a98f1ca7eb87b80adde07de; after = job 565dbd10d4f5eb939a9cd23d):

before p50 / p99after, 3 runs: p50 / p99
alone0.29 / 0.39 ms0.22 / 0.88, 0.21 / 0.37, 0.25 / 0.34 ms
while 4 clients rename the same chat0.66 / 0.89 ms0.53 / 0.75, 0.54 / 0.70, 0.58 / 0.76 ms

After runs: jobs 565dbd10d4f5eb939a9cd23d, 99b9eb0198e41afef0a1e4a9, 84eb0723e2a470f680ec8937. The 3 runs as measured. With 4 renamers, p50 and p99 are lower than before in all 3 runs. Alone, the p99 was 0.88, 0.37 and 0.34 ms against 0.39 ms before; these 3 runs do not show why the first one was higher.

Observed / Not run

Observed: the jobs above and their result lines.
Not run: no local build (repo rule). macOS fleet not run; Linux route only.

Changelog

none

Proof

No capture: daemon change, no UI.