fix(cmux-tui-core): a regression test for the poisoned arm of the creating-input check; it loads the routes before its poison check (#34174 review follow-ups) by lawrencecchen · Pull Request #35127 · manaflow-ai/cmux

lawrencecchen · 2026-11-04T08:22:53Z

Summary

Follow-ups from the last review of #34174 (R-DAEMON-STATE S1.4, merged as eaa00d9; review comment "Review (claude-reviewer-rds-s14): SHIP at 5aa3ec4", non-blocking notes 1-4). Behind the input lane switch CMUX_TUI_RDS_INPUT (default off on feat-cmux-next).

A regression test for the poisoned arm (note 3). 3b2e29f made the creating-input live check take the old read on a poisoned session state, with no test. New test mux::tests::signaled_mutex::a_poisoned_session_state_answers_the_creating_input_checks_from_the_state: with the lane on, a create's commit puts its terminal in the state and then panics. The cut-short guard publishes no routes, so knows_terminal_input_target must answer from the state.
Load order (note 1). live_surface_for_terminal now loads the published routes before its poison check, as Mux::surface does (poison never clears, so routes it uses come from a change that finished before any panic). Its old read still recovers the poison. The reviewer found no wrong answer with the old order; this makes the two readers follow one rule, and the comment says why the old read differs.
Comments (notes 2 and 4). The creating-input lock is a leaf only while the state is not poisoned: on a poisoned state its holders take the session lock under it, so no code may take it while it holds the session lock (doc of live_surface_for_terminal, and DESIGN S1.4). begin_creating_input: on both arms the live read ends before the insert, so a commit can make the id live in between; that entry is then written to the live terminal in order.

Testing

Focused command: ./scripts/verify-cmux-tui-hosted.sh --filter a_poisoned_session_state_answers_the_creating_input_checks_from_the_state (Linux test (linux)).

The fix this test covers (3b2e29f) is already on feat, so the test commit e942baa alone passes. To show it catches the defect, test/rds-s14-poison-red-never-merge (248abb0) removes the poison check: run https://github.com/manaflow-ai/cmux/actions/runs/41797164005 FAILED ("a poisoned state answered from the published routes, which lack the terminal its state holds").
Green at 71f21cd, run https://github.com/manaflow-ai/cmux/actions/runs/41797172027: passed.
--filter creating_input at 71f21cd, run https://github.com/manaflow-ai/cmux/actions/runs/41797179957: 13 passed, 0 failed (the process_support::creating_input integration tests, the new test, and rds_input_lane::a_key_waits_for_no_creating_input_lock).

Compile: fleet job 97d92dbabbb6c17adeb0633a at 71f21cd: CmuxNext, app and cmux-tui passed.

Observed / Not run

Observed: the hosted runs above and the fleet compile.
Not run: macOS execution of the focused tests; a test for the load-order change (no answer differs between the two orders in this function, as the fix(cmux-tui-core): a key skips the creating-input lock while nothing is creating; the lock becomes a leaf (R-DAEMON-STATE S1 step 4) #34174 review showed).

Changelog

none

Proof

No capture: daemon-only change behind a default-off switch, no UI.

…put checks from the state, not the published routes (#34174 review follow-up) The poison check of 3b2e29f had no test. Lane on: a create's commit puts its terminal in the state and panics; the cut-short guard publishes no routes, so knows_terminal_input_target must answer from the state.

…fore its poison check, as Mux::surface does; the comments state the leaf rule and the insert race (#34174 review follow-ups) - live_surface_for_terminal loads the published routes before the poison check (poison never clears), so the routes it answers with come from a change that finished before any panic; its old read still recovers the poison. - The creating-input lock is a leaf only while the state is not poisoned: on a poisoned state its holders take the session lock under it, so no code may take it under the session lock (doc and DESIGN S1.4). - begin_creating_input: on both arms the live read ends before the insert; an entry made for a terminal that just went live is written to it in order, by the creation's finish or by the next input on another path. Test: mux::tests::signaled_mutex::a_poisoned_session_state_answers_the_creating_input_checks_from_the_state.

cursor · 2026-11-04T08:22:59Z

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 · 2026-11-04T08:32:16Z

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

Review 4e5eb5f: 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=pr35127 -f ref=fix/rds-s14-followups -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on 3a5484f to compare; do not merge until green; see lane-rules

Shipped in:

cmux NEXT nightly 1.0.0-nightly.4182499138501 (e0aa712b3b)
cmux NEXT nightly 1.0.0-nightly.4184087390301 (08052d331e)
cmux NEXT nightly 1.0.0-nightly.4184224384601 (a3abfc4160)
cmux NEXT nightly 1.0.0-nightly.4184409870901 (78ee0e108e)

blacksmith-sh · 2026-11-04T08:36:35Z

Found 2 test failures on Blacksmith runners:

Failures

Test	View Logs
`nightly cmux-tui resolve/nightly cmux-tui resolve`	View Logs
`the short nightly decide gate must use the pooled Linux route with a Blacksmith fallbac` `k/` `the short nightly decide gate must use the pooled Linux route with a Blacksmith fallbac` `k`	View Logs

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

lawrencecchen · 2026-11-04T08:40:41Z

Review (claude-reviewer-rds-s14): FIX at 71f21cd
reviewer-session: 28d328fb-296f-470d-8b3b-6a14b496a6b4
author-session: ae25c6d154f94c01a
R9: pass | layer-ownership.md row "Terminals: PTY, process, output, cwd, title, exit, grid size" (Rust, session host). The PR adds no writer. It reorders one read in live_surface_for_terminal, changes comments and adds a test. The route tables and the catalog host index keep their only writers, State::insert_catalog_terminal and remove_catalog_terminal, under the session state guard. The creating-input map keeps its only writers, CreatingInputs::insert and remove, under its own lock.
perf claim: none

One defect must be fixed before the merge. The product change and the test are correct. I traced both in the head checkout. I did not build or run anything locally.

Defect

The new test fails clippy with -D warnings: unused_qualifications at cmux-tui/crates/cmux-tui-core/src/mux/tests/signaled_mutex.rs:137, :138, :141 and :143.
- use super::*; (:3) already brings these into scope: crate::terminal_host_runtime::TerminalHostIdentity (:137), crate::terminal_host::TerminalId (:138), crate::Surface (:141) and crate::SurfaceOptions (:143). cargo clippy -p cmux-tui-core --all-targets -- -D warnings stops with 4 "unnecessary qualification" errors.
- This red comes from the PR. It fails these jobs:
  - clippy (changed cmux-tui packages): run 41799062615, job 126470077693;
  - lint (linux) in run 41797172027 (job 126464553933), run 41797179957 (job 126464747615) and run 41797164005 (job 126464554854).
- After the merge it would make feat's lint red, and a base lint red skips the tests after it in the same job (ci(web): lint and budget gates no longer skip the webview and sidecar tests #22197).
- Fix: remove the crate::...:: prefixes at those four places. Keep crate::terminal_host::HostIncarnation at :139: it is not in scope, and clippy does not flag it. fix(cmux-tui-core): link peers and host proxy authors are published; S1.1 review follow-ups (R-DAEMON-STATE S1 step 2) #34171 had the same lint in the same file, at :120 (fixed in 10f1669).
- The fleet compile cannot catch this, because it runs cargo check on lib and bin targets, not on tests.

Answers to the hand-off questions

Does the test prove what it claims? Yes.

Before the panic, the lane read answers false, because the routes and the state both lack the terminal.
In the closure, surfaces.insert copies the map that the published snapshot shares. Then the panic cuts the change short. The cut-short guard publishes nothing (signaled_mutex.rs:425), and its MutexGuard poisons the state.
So the last knows_terminal_input_target must take the old read. It finds the placeholder by its host identity, which is a plain field (surface/metadata.rs:98).
On 248abb0 (poison check removed), the lane reads the routes, answers false and fails at :156 with the stated message (run 41797164005, job 126464554986). The panic at :153 in that log is the caught one.
These limits are not defects:
- The test calls only knows_terminal_input_target. begin_creating_input and finish_creating_input use the same live_surface_for_terminal, so this is enough.
- The red arm can catch the regression only if no other thread takes the poisoned lock between the panic and the check, because a guard that recovers the poison publishes the half-made tables. A test mux has an in-memory registry, so it starts no journal maintenance, and it has no terminal threads. The red run did fail.

Can the new load order give a different answer? It cannot give an answer that the old path cannot give.

If the state is not poisoned during the call, both orders read the same routes. If it was poisoned before the call, both orders take the old read.
The two orders can differ only if another thread poisons the state during the call:
- The new order answers from routes loaded before any poisoning (a finished change), or from the old read.
- The old order can also answer from half-made tables, if a guard that recovered the poison published them after the check.
- In each case, the answer is the state at some instant in the call, before or after the change that panicked. The old path also reads that state, because it recovers the poison.
The arc-swap guard drops at the end of the if block, before the old read takes the session lock.

Can the drop of the poisoned test mux hang or panic? No.

Mux::drop calls finalize_terminal_journal, which takes the state with unwrap_or_else(PoisonError::into_inner) (mux/lifecycle.rs:184), as its comment at :180 says.
The placeholder's shutdown_for_daemon returns at PtyRuntime::ExitedHosted. finish_terminal_reader finds no reaper thread and no reader thread, and the capture gate closes at once. None of those locks is poisoned, because the panic held only the session state.
The output store has no directory, because the in-memory registry has no database path. So output_store.shutdown returns at once (output_store.rs:1004).
Evidence: each green run drops the mux at the end of a passing test. The red run drops it while the failed assertion unwinds, and it does not abort ("finished in 0.02s").
Not from this PR: a persistent mux can still panic there. output_store.shutdown calls final_pass, which calls Mux::terminal_resource_surface (mux.rs:949) for each dirty output log, and that function uses self.state.lock().unwrap(). This contradicts "cleanup must not panic again" (lifecycle.rs:180-183). If it runs while the panic that poisoned the state unwinds, the process aborts. A follow-up should fix this.

Are the comments exact? Almost. Three small wording points (non-blocking):

creating_input.rs:386 says "a create whose commit panicked has its terminal in the state and none in the routes". That is true only until a guard that recovers the poison drops. Such a guard publishes the half-made tables, because the state changed and the guard was not cut short. The test's own last read is such a guard. Suggested text: "and, until a guard that recovers the poison publishes, none in the routes". The StateGuard drop comment ("the half-made tables are never published", signaled_mutex.rs:420-423) has the same limit, but that code is from S1.2.
creating_input.rs:378 says "The creating-input lock is a leaf only while the state is not poisoned". Off the lane, it is never a leaf. Suggested text: "On the lane, the creating-input lock is a leaf only while ...". The rule after it ("no code may take the creating-input lock while it holds the session lock") holds today: no caller does, and this PR adds none.
creating_input.rs:135 says "the entry is then written to the live terminal in order". The bytes can also go back to their senders, as for any queue: on a cancelled creation (failure "cancelled"), at the expiry and on a write failure. Suggested text: "written to the live terminal in order (or given back, as any queue)".

DESIGN.md S1.4 now has the qualifier. The S1 target summary at DESIGN.md:195 does not: it still says "The creating lock becomes a leaf: its holders read the published routes, never Mux::state".

Evidence at the head

test (linux):
- green run 41797172027 (job 126464554129): the new test is ok;
- filter run 41797179957 (job 126464747719): 13 passed, 0 failed. These include the five process_support::creating_input tests, the new test and rds_input_lane::a_key_waits_for_no_creating_input_lock.
lint (linux) in all three runs shows the 4 errors above. Its other reds are base findings, and none names a file of this PR:
- type_complexity at crates/cmux-tui/tests/link_push_bench.rs:59 (feat Cloud copy: compressed fs.write bytes (fs-write-deflate-v1) and fewer round trips per file #34538);
- the panic ratchet RED on cmux-host string_slice 3 > 0 at crates/cmux-host/src/github_cli_identity.rs (feat Cloud gh shim: an indented hosts.yml counts as the person's own gh login #35046, Cloud gh shim: answer gh auth status and gh api user from cmux's GitHub connection #33581).
Fleet compile job 97d92dbabbb6c17adeb0633a at 71f21cd: CmuxNext ok (301 s), app ok (92 s), cmux-tui cargo check ok (49 s), exit=0.
macOS lint and tests was still queued when I posted.
The PR description still has the placeholder "Compile: COMPILE_LINE".

… mux scope (clippy unused_qualifications); comment wording (#35127 review 1) - mux/tests/signaled_mutex.rs: TerminalHostIdentity, TerminalId, Surface and SurfaceOptions are in scope through use super::*; the crate:: prefixes failed clippy -D warnings (clippy (changed cmux-tui packages), lint (linux)). - creating_input.rs: a queue can also be given back; the leaf rule is stated for the lane; the routes lack a panicked create's terminal only until a later change that recovers the poison publishes them.

lawrencecchen · 2026-11-04T08:58:57Z

Review (claude-reviewer-rds-s14): SHIP at 4e5eb5f
reviewer-session: 98f6aac3-7aed-4548-8817-bf501dbecf3c
author-session: ae25c6d154f94c01a
R9: pass | layer-ownership.md row "Terminals: PTY, process, output, cwd, title, exit, grid size" (Rust, session host). The PR adds no writer. It reorders one read in live_surface_for_terminal, changes comments and adds a test. The route tables and the catalog host index keep their only writers, State::insert_catalog_terminal and remove_catalog_terminal, under the session state guard. The creating-input map keeps its only writers, CreatingInputs::insert and remove, under its own lock.
perf claim: none

No defect blocks the merge. 4e5eb5f fixes the defect from my FIX at 71f21cd. It changes only the test and comments. I did not build or run anything locally.

The defect is fixed

mux/tests/signaled_mutex.rs:137, :138, :141 and :143 now name TerminalHostIdentity, TerminalId, Surface and SurfaceOptions with no prefix.
All four come into scope through use super::*; from the imports at mux.rs:438, :440 and :443. Those imports have no cfg gate, so the names resolve on Linux and on macOS.
crate::terminal_host::HostIncarnation (:139) keeps its path, because it is not in scope.
clippy (changed cmux-tui packages) at this head (run 41801794729, job 126479092478) checked cmux-tui-core with all targets and shows no error in this PR's files. Its only error is type_complexity at crates/cmux-tui/tests/link_push_bench.rs:59:12 (feat Cloud copy: compressed fs.write bytes (fs-write-deflate-v1) and fewer round trips per file #34538). That is a base finding: the feat-cmux-next run 41801255441 (d9507dc) fails the same job on the same line.
lint (linux) in run 41801819083 (job 126480556620) has only base findings: the same link_push_bench.rs:59 error, and the panic ratchet RED on cmux-host string_slice 3 > 0 at crates/cmux-host/src/github_cli_identity.rs (feat Cloud gh shim: an indented hosts.yml counts as the person's own gh login #35046, Cloud gh shim: answer gh auth status and gh api user from cmux's GitHub connection #33581).

The rest of the commit

In creating_input.rs, only comments changed. So the product code is the code I reviewed at 71f21cd, and my answers about the test, the load order and the drop still hold. Fleet compile job 97d92dbabbb6c17adeb0633a covers that code.
The new comments are exact:
- creating_input.rs:137: "or given back, as any queue";
- :378: "On the lane, the creating-input lock is a leaf only while ...";
- :386-387: "until a later change that recovers the poison publishes them".
Optional nit at :386-387: the half-made tables are published by any later holder that recovers the poison, a read included (the test's own last knows_terminal_input_target is one). "a later holder" would be more exact than "a later change". This nit does not need a new review.
Corrections to my FIX comment:
- DESIGN.md already had the poison qualifier at lines 196-198 when I reviewed. I read only line 195, so my point 4 was wrong.
- The PR body now names compile job 97d92dbabbb6c17adeb0633a. That edit came after my comment.

Evidence at the head

test (linux) in run 41801819083 (job 126480556428, filter creating_input): 13 passed, 0 failed. These include the five process_support::creating_input tests, a_poisoned_session_state_answers_the_creating_input_checks_from_the_state and rds_input_lane::a_key_waits_for_no_creating_input_lock.
Fleet compile job 97d92dbabbb6c17adeb0633a at 71f21cd: CmuxNext, app and cmux-tui ok, exit=0. 4e5eb5f changes no product code.
macOS lint and tests was still queued when I posted.

lawrencecchen · 2026-11-04T08:59:29Z

At 4e5eb5f: receipt #35127 (comment) (SHIP at this head). Focused run https://github.com/manaflow-ai/cmux/actions/runs/41801819083 at this head (--filter creating_input): test (linux) 13 passed, 0 failed. Fleet compile job 97d92dbabbb6c17adeb0633a passed at 71f21cd; the head changes only the test's imports and comments. The clippy red is link_push_bench.rs:59 (base, also red on feat run 41801255441). ready-to-land added.

…d state, so a session with a poisoned state still shuts down (#35127 review note) Mux::drop -> finalize_terminal_journal -> the output store's last write (snapshot::final_pass, and its memory budget) looks each dirty terminal up here. The plain unwrap() panicked inside the drop on a poisoned state lock, so the rest of the shutdown (the journal writer's stop, the browser runtime's shutdown) never ran. finalize_terminal_journal itself already recovers the poison; this lookup now does the same. Test: mux::tests::agent_roster_poison::a_persistent_session_with_a_poisoned_state_still_shuts_down (red at 7ef8ee6: panicked at output_store/memory_budget.rs:143).

…(terminal_resource_surface recovers the poisoned guard) (#35279) * test(cmux-tui-core): a persistent session with a poisoned state still shuts down (red) Mux::drop -> finalize_terminal_journal -> output_store.shutdown -> snapshot::final_pass looks each dirty terminal up with Mux::terminal_resource_surface, whose plain unwrap() panics on a poisoned state lock; the rest of the drop then never runs (#35127 review note). * fix(cmux-tui-core): Mux::terminal_resource_surface recovers a poisoned state, so a session with a poisoned state still shuts down (#35127 review note) Mux::drop -> finalize_terminal_journal -> the output store's last write (snapshot::final_pass, and its memory budget) looks each dirty terminal up here. The plain unwrap() panicked inside the drop on a poisoned state lock, so the rest of the shutdown (the journal writer's stop, the browser runtime's shutdown) never ran. finalize_terminal_journal itself already recovers the poison; this lookup now does the same. Test: mux::tests::agent_roster_poison::a_persistent_session_with_a_poisoned_state_still_shuts_down (red at 7ef8ee6: panicked at output_store/memory_budget.rs:143). * test(cmux-tui-core): a terminal launch credential is refused on a poisoned session state (#35279 review, red) With terminal_resource_surface recovering a poisoned state, check_launch_credential (an authorization decision) verifies a terminal from a catalog a panicked change may have left half made; before #35279 it panicked. The other auth checks fail closed on a poisoned lock (origin_gate peer_or_local). * fix(cmux-tui-core): a terminal launch credential fails closed on a poisoned session state; the drop test drops the last handle itself (#35279 review) - Should-do 1: check_launch_credential refuses a terminal credential (credential_closed) while the session state is poisoned: the daemon cannot show the terminal is still open, and the catalog lookup now recovers the poison instead of panicking. The same rule as the other auth checks (origin_gate peer_or_local). credential_closed is an existing wire word, so the resource catalog does not change. - Should-do 2: the output store's writer holds a mux handle during its pass, so drop(mux) could run the drop on the writer thread, outside catch_unwind. The test now takes the last handle (Arc::try_unwrap, at most 5 s) and drops the Mux inside catch_unwind. Test: a_terminal_credential_is_refused_on_a_poisoned_state (red at 70f614b). * test(cmux-tui-core): the poison tests name Instant, Duration and CredentialCheck through the module scope (clippy unused_qualifications)

lawrencecchen added 2 commits November 4, 2026 08:06

lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 4, 2026

lawrencecchen temporarily deployed to artifacts November 4, 2026 08:31 — with GitHub Actions Inactive

lawrencecchen temporarily deployed to ci-routing-agent November 4, 2026 08:44 — with GitHub Actions Inactive

lawrencecchen temporarily deployed to sentry-dsyms November 4, 2026 08:58 — with GitHub Actions Inactive

lawrencecchen temporarily deployed to artifacts November 4, 2026 08:58 — with GitHub Actions Inactive

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 4, 2026

lawrencecchen temporarily deployed to artifacts November 4, 2026 09:00 — with GitHub Actions Inactive

Merge branch 'feat-cmux-next' into fix/rds-s14-followups

77ce030

cmux-bot Bot temporarily deployed to ci-routing-agent November 4, 2026 09:23 Inactive

cmux-bot Bot added stack-blocker and removed stack-blocker labels Nov 4, 2026

lawrencecchen merged commit 5de31bb into feat-cmux-next Nov 4, 2026
207 of 218 checks passed

lawrencecchen deleted the fix/rds-s14-followups branch November 4, 2026 09:39

Navigation Menu

Repository navigation

fix(cmux-tui-core): a regression test for the poisoned arm of the creating-input check; it loads the routes before its poison check (#34174 review follow-ups) - #35127

fix(cmux-tui-core): a regression test for the poisoned arm of the creating-input check; it loads the routes before its poison check (#34174 review follow-ups)#35127
lawrencecchen merged 4 commits into
feat-cmux-nextfrom
fix/rds-s14-followups

lawrencecchen commented Nov 4, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Nov 4, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026 •

edited

Loading

Uh oh!

blacksmith-sh Bot commented Nov 4, 2026 •

edited

Loading

Uh oh!

lawrencecchen commented Nov 4, 2026

Uh oh!

lawrencecchen commented Nov 4, 2026

Uh oh!

lawrencecchen commented Nov 4, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

lawrencecchen commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Observed / Not run

Changelog

Proof

Uh oh!

cursor Bot commented Nov 4, 2026

Bugbot needs on-demand usage enabled

Uh oh!

github-actions Bot commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blacksmith-sh Bot commented Nov 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failures

Uh oh!

lawrencecchen commented Nov 4, 2026

Defect

Answers to the hand-off questions

Evidence at the head

Uh oh!

lawrencecchen commented Nov 4, 2026

The defect is fixed

The rest of the commit

Evidence at the head

Uh oh!

lawrencecchen commented Nov 4, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

lawrencecchen commented Nov 4, 2026 •

edited

Loading

github-actions Bot commented Nov 4, 2026 •

edited

Loading

blacksmith-sh Bot commented Nov 4, 2026 •

edited

Loading