[teamleaderleo]

Follow-up correctness fixes for the closed-summary cache:

• keep list and full serialized rows on independent generations, so building one form cannot make the other form appear fresh;
• capture the generation before constructing a row;
• remove cached bytes when a session is purged, deleted, or reopened;
• invalidate time-expiring tags when their deadline passes;
• keep summary construction out of write_snapshot, avoiding nested metadata and permission locks in the writer;
• cover list-then-full reads, appends after close, shown/attached state, tag expiry, concurrent writers, cold loading, and purge eviction against an uncached walk.

Leo asked: “you (or other guys) should audit my computer and see what is taking up all the CPU and optimize against that.” The prior cache benchmark on a profiled Mac running the NEXT nightly measured the same 7,828-record fixture (7,295 closed) at 227.378→150.376 ms for watch/list, 207.694→149.910 ms for sessions?usage, and 235.739→181.206 ms for peer summaries. These follow-ups preserve those bytes and invalidation behavior.

metric	before	after	delta	harness
watch/list full build CPU	227.378 ms	150.376 ms	-33.9%	measure_7828_full_idle_closed_summary_cache
sessions?usage full build CPU	207.694 ms	149.910 ms	-27.8%	measure_7828_full_idle_closed_summary_cache
peer sync summaries CPU	235.739 ms	181.206 ms	-23.1%	measure_7828_full_idle_closed_summary_cache

Validation runs through cmux-ci for the acpmux test suite and clippy.

agent-identity footer --repo manaflow-ai/cmux
Agent session: agent session

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary by cubic

Fixes correctness bugs in the closed-summary cache so cached rows never serve stale data, and reorders the purge event so watchers can't resolve a just-purged session.

• Tracks list and full serialized rows on independent generations and tag-expiry deadlines, so building one form can't make the other look fresh; empty rows are never served.
• Builds rows lazily on cache miss; write_snapshot only evicts entries, avoiding nested metadata and permission locks in the writer.
• Evicts entries when tags expire or sessions are purged, deleted, or reopened.
• Emits the purge event only after the session is unregistered from live lookup and its cache entry is removed.
• Adds tests covering list-then-full reads, appends after close, shown/attached state, tag expiry, concurrent writers, cold loading, and purge eviction.

^{Written for commit 1ae48f9. Summary will update on new commits.}

[teamleaderleo]

Review of manaflow-ai/cmux #31635 at e2428a5

What I checked: I diffed merge-base 6e3e5d4...e2428a5 (5 files, +104/-18) and read the touched code in mod.rs (cached_closed_summary, the new ClosedSummaryCacheEntry, remove_cached_closed_summary), meta_save.rs write_snapshot, turns.rs (the purge path, about lines 2125-2205), transfer.rs, own_host.rs resolve and the list_guard.rs tests. Nothing was built or run.

The three blocking findings of the #31529 review

1. Per-form generations: fixed. Each form now has its own list_generation/full_generation and expiry, and a miss stamps only the form it rebuilt. A list build can no longer mark old full bytes fresh. The test now runs a list build, then the full form, then append-after-close and attach/detach.
2. Generation read after build in write_snapshot: fixed, because write_snapshot no longer fills the cache at all. cached_closed_summary still loads the generation before it builds the row, which is correct.
3. New lock acquisition in write_snapshot: fixed. It now only removes the entry, which takes closed_summary_cache alone. Base already took that lock in its non-Closed branch. No summary build runs on the writer path, so the deadlock with hosts.rs set_status while holding session.permissions is gone. closed_summary_cache is never held while another lock is taken: the insert serializes under it, but serializing is pure.

Old follow-ups:

• Purge eviction: done in the turns.rs purge, the transfer.rs failed-import delete and the purged early return in write_snapshot.
• Size bound: still unbounded, but deleted chats are now evicted, so the leak is much smaller.
• Time-expiring tags: now handled by the per-form next_expiry.

The red: remote_group_cli.rs::interrupted_grouped_ephemeral_is_purged_after_successful_open

Judgement: unrelated to this PR. It is a race in the test that already exists on base.

• Hub::resolve (own_host.rs) reads only the sessions map, through resolve_here/resolve_exact. It never reads closed_summary_cache, so nothing in the cache can make a purged session resolvable.
• In the purge path (turns.rs), the "purged" event is sent before self.sessions.remove(&session.id) (about line 2200). Two steps sit between them: a synchronous store.delete and an awaited spawn_blocking(file_uploads::remove_session). The test breaks out on the purged event and calls local.resolve(&purged) right away, so it can run while the purge task is still inside that spawn_blocking. On a loaded host, resolve then still finds the session and the assertion fails.
• The only change this PR makes in that path is one uncontended mutex remove (remove_cached_closed_summary), placed right after purged.store(true) and before the event. It changes neither the order of the event and the map removal nor the awaits between them. The write_snapshot change only affects the cache.
• Fix (separate PR, test or purge ordering): have the test wait until resolve fails, or send purged after the session is unregistered.

Other checks

• Stale paths:
  • rename, tags, archive, usage metadata, mark-seen and concurrent writers: covered by commit_meta's bump plus the test.
  • shown/attached and append-after-close: covered by events.changed/send bumps plus per-form generations.
  • children counts: a child's status and commit bump the global generation.
  • reopen: the cache is served only while status() == Closed, and write_snapshot evicts the entry on a non-Closed commit.
  • cold load: the cache starts empty.
  • I found no new stale path.
• No new production unwrap/expect. The unwrap()s added in list_guard.rs are test-only. to_string(...).unwrap_or_default() with the !bytes.is_empty() guard is fine.

Follow-ups

• [follow-up] A late reinsert after purge. A cached_closed_summary that is running at the same moment as a purge holds its own Arc<Session> and can insert after the purge's eviction. The result is mostly a leaked entry. It could only be served if a replacement session with the same id stays Closed and no event follows. The purged event's bump makes that unlikely. Fix: skip the insert when session.purged is set, or when the session is not Arc::ptr_eq to the one in sessions.
• [follow-up] Inputs that change with no event (a config reload of usage_limits changing closed rows' usageWarnings) still need an events.changed() on reload. This is carried over from acpmux: cache serialized closed summaries #31529.
• [follow-up] The cache is still unbounded for closed chats that are never deleted (two serialized rows each, the full one with the tool inventory). Consider caching only the list form, or bounding the map.
• [follow-up] The purge test calls remove_cached_closed_summary directly instead of going through a real purge (stop(purge=true)), so it does not cover the turns.rs eviction.
• [follow-up] Unrelated race in the remote_group_cli purge test, described above.

Follow-ups: skip the cache insert for purged/replaced sessions; invalidate on config reload; bound the map size; the purge test should exercise a real purge; fix the race in the remote_group_cli purged-event test separately.

Review (claude opus-5.5 medium subagent): SHIP at e2428a5

[teamleaderleo]

Review (subagent claude-opus-5-5 medium): SHIP at 316bc68

Checked the whole diff and the last commit. In kill(.., purge=true) the purged event now goes out after store.delete, the sessions map removal and remove_live_session_index, with no await in between. Events::send bumps changes/summary_generation before it broadcasts, so a watcher that gets the event can't resolve the session or reuse a stale list snapshot. Both error paths are consistent now. A failed store.delete returns before both the removal and the event, where it used to send the event and then fail. The stale-incarnation early return sends neither, the same as before. The closed-summary cache keeps a generation and a tag-expiry deadline for each form and reads the generation before it builds a row, so a concurrent bump only causes a miss. It is dropped on purge, on a purged or reopened write_snapshot, and on a failed import's delete. I don't see a path that serves stale closed data.

Follow-ups:

• [follow-up] Between the sessions removal and the purged send, a fixed-ID trigger creator on another worker thread could publish a same-id session and get its created event out before this stale purged. Peers and TUIs that key on session_id would then drop the replacement until the next list. The window is microseconds. One fix is to send under the sessions lock, or to tag the event with the incarnation.
• [follow-up] cached_closed_summary running at the same time as a purge can re-insert a cache entry after remove_cached_closed_summary. That leaks a small orphan row, which is never read because the session can no longer be resolved. A purged check before the insert would close it.

[teamleaderleo]

Delta review of manaflow-ai/cmux #31635: e2428a5 -> 316bc68

Scope: git diff e2428a59..316bc681, one file, turns.rs, +19/-17. The "purged" HubEvent send moves out from before store.delete to after sessions.remove and remove_live_session_index. The code it sends is unchanged. Read via git only; nothing built or run.

(1) Event comes after the session stops resolving, on every path

• There is exactly one emitter of a local "purged" record: Hub::kill with purge=true in turns.rs. A grep for "purged".into() and kind: "purged" in src finds only that one. bg_purge, the ephemeral purge after a successful open (the test's path, through kill(purge)) and every other purge go through it. The transfer.rs failed-import delete only evicts the cache and emits no purged record. peers.rs announce_remote(.., "purged") is the mirror of a remote row and is outside this diff.
• New order: store.delete, then file_uploads cleanup, then the guarded sessions.remove (ptr_eq), then remove_live_session_index, then send. Hub::resolve reads the sessions map (own_host.rs), and the store row is gone too, so nothing can lazy-load it back. A subscriber that wakes on the event now always gets resolve(..).is_err(). The test's race is closed by construction, not just made less likely.
• On the replacement early return (not ptr_eq), the event is still not sent. That is unchanged.

(2) Consumers still receive it

• self.events is the hub-wide list_guard::Events broadcast (a broadcast::Sender plus generation counters). It is not a per-session channel, so removing the session first neither drops the event nor leaves it with nowhere to go. send still bumps summary_generation, and it now does so after the cache eviction, which slightly narrows the late-reinsert window from the earlier review.
• server/mod.rs deliver: the gate is conn.watch_all or conn.sub_opts(id), both per-connection and not taken from the hub map. The purged branch builds its notification from ev.session_id alone, with no resolve. So watch streams, the TUI (tui/events.rs), the webview and Swift clients, and peer links (peer.rs then purge_tombstones) all still get MUX_SESSION_CHANGED kind=purged. A subscriber with an event stream still gets MUX_EVENT.
• tasks.rs watch_orphans: it still runs a pass on purged. Cards go to the giver, and orphan_tasks_of already ran before close, so a purged worker's cards are unaffected. A purged giver is now skipped in that pass, which is correct.
• [follow-up] server/chats.rs push_activity runs hub.resolve(id) for purged (activity.rs changes_activity lists it). Before this change that resolve usually succeeded, by the same race the test hit, and sent a _acpmux/chat_changed carrying the dying session's attention/preview/parent. Now it never sends one on purged. chat_windows::activity_changed still wakes before the resolve, and the earlier status=closed event already sent closed activity. So windows recompute and only the incremental chat-index push loses an update that was itself stale. The correct push would be nulls for the chat key. That needs the key captured before removal (for example in the purged record's msg) and is not a regression worth blocking on.

(3) Error paths

• If store.delete fails, ? now returns before the event. The session stays in the map and resolvable (with purged=true and its cache entry evicted, as before), and no purged notice goes out. Before this change the notice went out and the session still resolved, so clients dropped a row that still existed. The new state is the more consistent of the two. It is still an imperfect half-purged state, but that predates this PR.
• The session_tmp and file_uploads cleanups ignore their errors (let _), so the event is still sent when they fail.

(4) Locks

• The sessions guard is dropped before remove_live_session_index and before the send. The send is synchronous, with no await after unregistering ("No awaits follow unregistering" still holds). No lock is now held across an await.
• The purge_seam test hook still sits after store.delete. stop_fence_tests asserts the session is registered at the seam, which is still true, and it checks no events.

(5) Why the failure got likelier, and whether this fixes it

• Before, the window was the time between the send and sessions.remove: the synchronous store.delete, #[cfg(test)] seam take, and an awaited spawn_blocking(file_uploads::remove_session). The PR's only addition before the send was remove_cached_closed_summary, an uncontended mutex remove that runs before the window opens and does not lengthen it. One failure in three against none in three is weak evidence. Host load or blocking-pool contention explains it as well as the PR does. Either way, the fix removes the window entirely rather than shrinking it, so the cause does not matter for correctness.

Follow-ups:

• [follow-up] push_activity no longer emits chat_changed on purged, because resolve now fails. Emit null activity for the chat key, carrying the key in the purged record, so the chat index clears attention/preview/parent at once.
• [follow-up] A store.delete failure leaves a half-purged session (purged=true, still registered, no notice). This predates the PR. Consider unregistering anyway, or clearing purged.
• Carried over from the previous review: skip the cache insert for a purged or replaced session; invalidate on config reload; bound the cache; make the purge test exercise a real purge.

Review (claude opus-5.5 medium subagent): SHIP at 316bc68

[teamleaderleo]

Delta review of #31635 at 8d25e0b (merge of feat-cmux-next 27f0109 into 316bc68, which had SHIP: #31635 (comment)).

• The PR's own diff against its new base (git diff 27f01095 8d25e0b3) touches the same 5 files as before (hub/list_guard.rs, meta_save.rs, mod.rs, transfer.rs, turns.rs).
• Line-by-line, its added and removed lines are identical to the reviewed diff (6e3e5d4c..316bc681), except one removed line. Base rustfmt-wrapped the old cache.entry(id).or_insert_with(...) line in hub/mod.rs, and the PR still replaces it with the per-form generation entry. So the conflict was a reformat of a line the PR removes, and the resolution keeps the reviewed code unchanged: independent list/full generations, write_snapshot only evicting, purged sent after unregistering.
• Nothing from the base merge lands inside the PR's hunks. The other files in the merge (peer_forward.rs, processes.rs and so on) are base changes already on feat-cmux-next.

Land on the checks at this head: the acpmux suite, clippy and the purge test rerun at 8d25e0b. Compare any red by name against base 27f0109.

Follow-ups: unchanged from the previous review.

Review (claude opus-5.5 medium): SHIP at 8d25e0b

[blacksmith-sh]

Found 2 test failures on Blacksmith runners:

Failures

Test	View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job _names_its_warm_sources)/ test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job _names_its_warm_sources)	View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl ights_validate)/ test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl ights_validate)	View Logs

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[teamleaderleo]

Review (subagent claude-opus-5-5 medium): SHIP at 8d25e0b

Delta from the SHIP at 316bc68. git show --remerge-diff shows the only hand resolution is one hunk in hub/mod.rs. It keeps the PR's ClosedSummaryCacheEntry init with separate list and full generations. Base's side of that hunk was only a rustfmt reflow of the old tuple init (autofix d3e2960), so no base code was dropped. The other base acpmux changes merged cleanly: borrowed session pages (#30706), process pages (#30726), peer forward/upload waits (#30771) and the detach reply (#31629). They are read-only or peer-side and don't change a local session's meta or status, so they add no path that needs a bump. summary_generation still bumps on every events.send/changed() and every commit_meta, and the purge, reopen and transfer paths still evict the entry.

Follow-ups: none in the code. CI note, not a code finding: acpmux tests, clippy/panic ratchet, protocol IR and api-compat at this head fail on cargo --locked being unable to update cmux-tui/Cargo.lock. The base checkout (27f0109) fails the same way, so this is the base red that #31717 (b732874) fixed after this merge's base. No Rust test or lint ran at this head. Bring #31717 in (update-branch) and land on green acpmux tests and clippy.

[teamleaderleo]

Delta review of #31635 at 1ae48f9: a clean merge of feat-cmux-next b732874 (includes #31717, the Cargo.lock fix) into 8d25e0b, which had SHIP (#31635 (comment)).

The PR's own diff against its new base (git diff b732874b 1ae48f98) is identical line for line to its diff at 8d25e0b (git diff 27f01095 8d25e0b3). It has the same 5 files in hub/ and the same +123/-36. Nothing from the base merge lands in the PR's hunks.

Land on the checks at this head (the acpmux suite, clippy and the purge test), with any red compared by name against base b732874.

Follow-ups: unchanged.

Review (claude opus-5.5 medium): SHIP at 1ae48f9

[teamleaderleo]

Review (subagent claude-opus-5-5 medium, carried): SHIP at 1ae48f9

This is a clean base merge of 8d25e0b (SHIP at #31635 (comment)) with feat-cmux-next b732874, which brings in the #31717 Cargo.lock fix. The head tree 46ef725c is byte-identical to git merge-tree --write-tree of the two parents, so no conflict was resolved by hand. It lands once acpmux tests and clippy are green at this head.
Follow-ups: none new.

[github-actions]

cmux status _{(one comment, edited in place; each line is updated by the job that owns it)}

Shipped in:

• cmux NEXT nightly 1.0.0-nightly.4138631602601 (86faa171ca)
• cmux NEXT nightly 1.0.0-nightly.4138723445501 (2429d60a67)
• cmux NEXT nightly 1.0.0-nightly.4138963169701 (ad01dee1d0)
• cmux NEXT nightly 1.0.0-nightly.4139084486201 (b5060b494d)
• cmux NEXT nightly 1.0.0-nightly.4139171596001 (fb7e28b51d)