Skip to content

fix(cmux CLI): a list that lacks the window's workspace reads the window again with the barrier (#32201 follow-up) - #32327

Merged
cmux-bot[bot] merged 2 commits into
feat-cmux-nextfrom
perf/cli-list-stale-window
Nov 2, 2026
Merged

cmux-bot[bot] merged 2 commits into
feat-cmux-nextfrom
perf/cli-list-stale-window

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #32201 (CLI-1: workspace list reads the app window without the app's read barrier). #32201 merged at its first reviewed head (8185ff0), before this fix for that review's Medium finding (#32201 (comment)) reached it.

The gap: cmux workspace close <the workspace the window shows>; cmux workspace list. The app's last published snapshot, which #32201 reads with after: 0, can still show the closed workspace. The overlay then marks every row unfocused until the app publishes again.

Fix: when a workspace list of the app's session lacks the workspace the app's window shows, the snapshot is older than the daemon's answer, so the overlay reads the window once more with after: "sync". Every other list keeps the single fast read. app_workspaces takes the barrier; the with_read_barrier doc names a caller-chosen after.

Owner: the cmux CLI (cmux-tui/crates/cmux-tui).

Changelog

none

Testing

Regression test: a_list_without_the_window_s_workspace_reads_the_window_again_with_the_barrier (cmux-tui, cli/app_focus/tests.rs): a fake app answers after: 0 with the pre-close window and after: "sync" with the post-close window; the list must read twice and take focused from the second read.

Observed / Not run

Observed: the red and green steps above. Not run: an app round with a close then a list under load.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes the workspace close <shown> then workspace list case where the overlay marked every row unfocused because the app's last published snapshot still showed the closed workspace. When a list of the app's session lacks the workspace the window shows, the overlay now reads the window once more with after: "sync"; every other list keeps the single fast read, and the read barrier's doc now notes the caller-chosen after.

Written for commit f283afe. Summary will update on new commits.

View guided diff Turn on auto-fix

…dow again with the barrier (#32201 review)

The review of #32201 found one daemon write that can leave the app's
last published snapshot behind: `workspace close <shown>` and then
`workspace list`. The fast snapshot still shows the closed workspace, so
the overlay set every row unfocused. When a list of the app's session
lacks the window's workspace, the overlay now reads the window once more
with `after: "sync"`; every other list keeps the fast read. The read
barrier's doc names the caller-chosen `after`.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@cursor

cursor Bot commented Nov 2, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot 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.

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

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

Review f283afe: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr32327 -f ref=perf/cli-list-stale-window -f platform=macos -f backend_mode=local

Shipped in:

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (subagent, correctness first): SHIP at f283afe

Scope: git diff origin/feat-cmux-next...f283afe98f3 (cli/app.rs, cli/app_focus.rs, cli/app_focus/tests.rs). I read the code and used git only. I did not compile or run tests.

The change applies cleanly on the merged #32201 code

  • git range-diff fe8272889f3~1..82312c5f64a a4becc242f3~1..f283afe98f3: both commits are = (patch-identical). The added and removed lines of the two pairs are the same, line for line. No hunk is lost and no hunk is duplicated.
  • The reviewed parent (fe8272889f3~1) and this PR's merge base (8787d4cc9a8) differ in these three files by one line only: app.rs:856 .or_else(|| super::vm::created_line(...)), which another PR added. It is far from the hunk at app.rs:1052. app_focus.rs and tests.rs at the merge base are byte-identical to the reviewed parent. So the merged perf(cmux CLI): workspace list reads the app window without the app's read barrier (CLI-1) #32201 code is the code the fix was written against.
  • feat moved 48 commits past the merge base. In cli/ it changed vm.rs, vm/tests.rs, a coverage test and the same app.rs:856 line (created_line(..., global)). None touches app_focus*. git merge-tree against the feat tip is clean.
  • app_workspaces changed its signature (after: Value). On the feat tip its only caller is app_focus.rs:255, which this PR replaces. agents.rs:595 calls overlay_focused only. No other caller breaks.

The earlier review's points still hold

The code is the same, so the analysis in #32201 (comment) applies unchanged:

  • The fix: after: 0 first. If window_left_the_list (a list, or {value: [..]}, that names a known workspace and lacks app.shown), read again with after: "sync" and overlay from that read. workspace get (one object) and rename/move (value is an object) never trigger it. An empty list and an empty known never trigger it.
  • The second read fails safe: None skips the overlay and keeps the daemon's focused, the base behaviour on failure.
  • Worst case equals the base path (fast read + one "sync" read with its retry). The common case is one fast read (busy_app test still sees one request, now asserted as after == 0).
  • The new test is behavioral and not vacuous. It needs two reads, the second with "sync", and focused from the second snapshot. It failed on the test-only commit for the right reason (one read only).

Residuals that still apply (Low, do not block, follow-ups):

  1. Low (performance): while the window shows a workspace of another session (remote), each home list misses shown and has known rows, so it pays the second "sync" read. Result is correct; only the speedup is lost in that case. Refinement: compare the session of app.shown with the listed rows.
  2. Low (stale result, narrow): single-record replies (workspace get/rename/move right after a close of the shown workspace) get no second read, so focused can be false until the app publishes.

New

  1. Low (REVIEW.md "Docs and comments out of date"): the doc of workspace_list_reads_the_window_without_waiting_for_the_apps_read_barrier (cli/app_focus/tests.rs:212-216) still says the window is state "which no daemon write of the caller changes ... and never waits for the main actor". The test right below it and the new comment at app_focus.rs:267-276 say a close does change it. This was earlier finding 4; the PR body is now correct (it describes the close case), but the test doc was not updated. A one-line doc fix; does not block.

No other findings. Label risk:normal is correct (CLI read path). The body has the red and green steps, the clippy note (red only in mux/tests/read_paths.rs, base red from #32177) and ### Observed / Not run. At review time the PR's checks were pending, with none red.

@cmux-bot
cmux-bot Bot merged commit e3b4662 into feat-cmux-next Nov 2, 2026
185 of 197 checks passed
@cmux-bot
cmux-bot Bot deleted the perf/cli-list-stale-window branch November 2, 2026 13:35
@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026

This branch was previously deployed

1 inactive deployment
ci-routing-agent — f283afe9 Deployed Nov 2, 2026 by lawrencecchen via cmux-tui tree preflight #101157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant