Skip to content

base red: pty_reclaim takes poisoned locks instead of unwrapping (9e23760ae80; unwrap 1148 -> 1150) - #28853

Merged
lawrencecchen merged 3 commits into
feat-cmux-nextfrom
base-red/pty-reclaim-unwraps
Oct 31, 2026
Merged

lawrencecchen merged 3 commits into
feat-cmux-nextfrom
base-red/pty-reclaim-unwraps

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Base red on feat-cmux-next: the "Crash safety" step of cmux-next checks reports crash-ratchet: rust cmux-tui-core: unwrap 1148 -> 1150.

9e23760 ("the owner parks idle unwatched shells before the machine runs out of PTYs") was a direct push by lawrencecchen, with no PR. It added two non-test .lock().unwrap() calls in mux/pty_reclaim.rs reclaim_candidates. It is a 335-line fix for PTY exhaustion on a real machine, so a revert is not small.

This PR uses the crate's pattern, .lock().unwrap_or_else(PoisonError::into_inner), as the other mux readers of workspace_registry and state do (detached_terminals.rs, command_history.rs). The behavior changes only if a lock holder panicked; then the reaper reads the state instead of panicking. No waiver is recorded and the baseline does not change.

History: the first head also fixed 7 unwraps in apps/deploy.rs from 045f9e0 (1150 -> 1157). #28847 fixed those on the tip in the meantime. A merge of origin/feat-cmux-next takes the tip's deploy.rs, so this PR now changes only pty_reclaim.rs.

Testing

Check: scripts/cmux-next/check-crash-safety.sh "$PWD/Packages/macOS/CmuxNext" (the step's command). For the exports I used crash_ratchet.py --repo DIR --exported-tree.

  • 9e23760ae80^: ok. 9e23760ae80: unwrap 1148 -> 1150 (the first red sha).
  • Tip dff3196: unwrap 1148 -> 1150 (red).
  • This head (merged with dff3196): check-crash-safety: ok, crash-ratchet: ok, exit 0.
  • rustfmt --edition 2024 with cmux-tui/rustfmt.toml: clean.
  • python3 scripts/cmux-next/prepush-gate.py: no new violations.

Observed / Not run

Observed: I ran the checks above on a Linux VM and saw the results above. Fleet compile-check of the first head 39614fe, job c36db17abdc290f6f3efefaa: passed (CmuxNext, app, cmux-tui). The compile of this head comes in a comment below.
Not run: cargo test -p cmux-tui-core.

Changelog

none

Proof

Crash-safety change in Rust; no UI.

…ping (9e23760)

9e23760 (owner parks idle unwatched shells) added two
`.lock().unwrap()` in mux/pty_reclaim.rs reclaim_candidates, raising the
crash ratchet's rust cmux-tui-core unwrap row 1148 -> 1150. Use the crate's
pattern, `.unwrap_or_else(PoisonError::into_inner)`, as the other mux
readers of workspace_registry and state do. No behavior change unless a
holder panicked; then the reaper reads the state instead of panicking.
…ing (045f9e0)

045f9e0 (feat(apps): install = deploy + pin) added seven
`.lock().unwrap()` in the non-test code of apps/deploy.rs, raising the
crash ratchet's rust cmux-tui-core unwrap row by 7. Use the pattern the
same file already uses for instances (line 352) and apps/cancel.rs uses:
`.unwrap_or_else(PoisonError::into_inner)`. No behavior change unless a
holder panicked.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Oct 31, 2026
@cursor

cursor Bot commented Oct 31, 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.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent): SHIP at 39614fe

Correctness first. The review was read-only, in a fresh worktree at the head. I did not compile it: the fleet compile is a separate step.

  • Compile (by reading): pty_reclaim.rs gets PoisonError through use super::*; from the top-level use std::sync::{..., PoisonError, ...} in mux.rs (line 384). At least nine sibling mux files (creating_input.rs, construct.rs, lifecycle.rs, journal_maintenance.rs, ...) already use that same path. deploy.rs changes use std::sync::Mutex; to use std::sync::{Mutex, PoisonError};. That import is not a duplicate, because line 352 spells out std::sync::PoisonError::into_inner in full. The #[cfg(test)] mod tests { use super::*; } module is not affected.
  • Is poison recovery safe here: In pty_reclaim.rs, both sites only read (kept_terminals(), and a scan of content_ids and terminal_catalog). The crate already recovers poisoned state and workspace_registry locks in about 60 other places. In deploy.rs, the inner sites read the manifest or mirror.revision, or append a log line. apps/cancel.rs and apps/subscriptions.rs already recover this lock. The instances sites each do one BTreeMap get, insert or remove, so a panicking holder cannot leave a half-applied multi-step update. Deployments::load-side code at line 352 already recovers this same lock. Before the change, a poisoned lock made each of these paths panic again. No behavior changes when no lock is poisoned. The guard lifetimes are the same as before: the temporary guard in deploy_remove's save(...) call is dropped at the end of the statement, as it was before.
  • Crash ratchet: scripts/cmux-next/check-crash-safety.sh "$PWD/Packages/macOS/CmuxNext" at the head gives check-crash-safety: ok and crash-ratchet: ok. An export of base 6df0dca gives crash-ratchet: rust cmux-tui-core: unwrap 1148 -> 1157 (red, exit 1). The head merged with the current origin/feat-cmux-next 92f52ef (git merge-tree, no conflicts) gives crash-ratchet: ok. The PR changes nothing under scripts/, so the baseline and allowlist are unchanged. It adds no crash-allow.
  • Format: rustfmt --edition 2024 --check from cmux-tui/ on both files is clean.
  • REVIEW.md: No item applies. The PR adds no tests, strings, sleeps or new state. The ratchet itself is the regression check for a base-red fix of this type, and the PR description has the red/green results.
  • Risk tier: risk:normal is correct. These files are cmux-tui runtime logic. RELEASE_WORDS matches deploy only under scripts/ or .github/workflows/, so apps/deploy.rs is not in a high-tier path.

Not run: cargo build and test. The fleet compile result is still necessary.

lawrencecchen referenced this pull request Oct 31, 2026
…ne runs out of PTYs

macOS gives the machine 511 PTYs (kern.tty.ptmx_max). Every terminal host holds
one for life, so a session with hundreds of agent workspaces, each with a shell
tab nobody opened, used them all. On Lawrence's Mac (2026-10-30) the owner held
480 of 511, and 431 of those were shells idle at a prompt for a median 148 h with
no client. Every new workspace then failed with 'PTY capacity exhausted (os
error 6)'.

When fewer than 64 PTYs are free, the terminal reaper thread now parks shells
(park-tabs, tab-park-v1) until 128 are free. The tab stays and shows
'relaunch: {cwd}', and the app starts it again when it is shown. A shell is
parked only when all of these are true: it has sat at its prompt (OSC 133 A)
and printed nothing for 30 min or more, no client is attached to any of its views, no program reports an OSC 7501 status
in it, and it is not kept. The oldest prompt goes first. A launch that fails
for lack of PTYs wakes the pass at once. Nothing happens while PTYs are
plentiful.

Live, on the same rules through park-tabs: 511 -> 351 PTYs in use.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lawrencecchen referenced this pull request Oct 31, 2026
…ud VM

Manifest v2 gains deploy {server: {kind: container, image, port, health},
tenancy, targets} and implements[cmux.pane/1].remote = instance; billing
{model} is free only (Lawrence 2026-10-30). The validator refuses a remote
pane without deploy and a local target for a third-party app; the schema
refuses shell characters in image and health.

cmux.apps.install takes deploy {target}: self-hosted:<ssh host> (the
customer's own machine over SSH or Tailscale SSH, no cmux Cloud call) or
cloud-vm:<machine> (cmux cloud exec). One POSIX runner for both: docker or
podman run bound to the tailnet address (else 127.0.0.1, never every
interface), wait for the health path, print the URL. The install commits and
answers with the instance deploying; a worker records running + URL, or rolls
the install back (container removed) with the reason in the app log. Remove
removes the container. Instances persist in <state>/apps-instances.json and
show in apps-list and installed.list.
@blacksmith-sh

blacksmith-sh Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Found 9 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmppijibbeq/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmpvmwqwsi2/summary-missing.json (live):/summary-missing.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs
convention violations found (ERROR lines above)./
convention violations found (ERROR lines above).
View Logs
test_a_picked_signing_label_has_a_rerun_alternative (main.RunsOnFallbackTests.test_
a_picked_signing_label_has_a_rerun_alternative)/
test_a_picked_signing_label_has_a_rerun_alternative (main.RunsOnFallbackTests.test_
a_picked_signing_label_has_a_rerun_alternative)
View Logs
test_no_runs_on_chain_ends_on_the_reserved_side_label (main.RunsOnFallbackTests.tes
t_no_runs_on_chain_ends_on_the_reserved_side_label)/
test_no_runs_on_chain_ends_on_the_reserved_side_label (main.RunsOnFallbackTests.tes
t_no_runs_on_chain_ends_on_the_reserved_side_label)
View Logs

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

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fleet compile on head 39614fe: cmux-ci compile-check --package CmuxNext --app --recipe-version 2, job c36db17abdc290f6f3efefaa: CmuxNext passed (214 s), app passed (64 s), cmux-tui passed (38 s).

@lawrencecchen lawrencecchen added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Oct 31, 2026
…y-reclaim-unwraps

# Conflicts:
#	cmux-tui/crates/cmux-tui-core/src/apps/deploy.rs
@lawrencecchen lawrencecchen changed the title base red: cmux-tui-core takes poisoned locks instead of unwrapping (9e23760ae80, 045f9e02742; unwrap 1148 -> 1157) base red: pty_reclaim takes poisoned locks instead of unwrapping (9e23760ae80; unwrap 1148 -> 1150) Oct 31, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent): SHIP at 61fa3cb

Checked on a fresh detached worktree at the head (origin/feat-cmux-next = dff3196, the merge's second parent, so no newer tip to re-merge).

  • git diff dff319646ae 61fa3cb1a448 --stat: only cmux-tui/crates/cmux-tui-core/src/mux/pty_reclaim.rs (+7/-2). Nothing from the tip is dropped or reverted; apps/deploy.rs equals the tip. Crash baseline json and crash-allowlist unchanged; no crash-allow added.
  • Correctness: PoisonError comes in through use super::*; (mux.rs line 384 imports it). Both sites in reclaim_candidates only read: kept_terminals() is a SELECT, and the state guard only walks resource_indexes.content_ids and terminal_catalog. Recovery from poison is safe and matches detached_terminals.rs and command_history.rs on the same locks.
  • scripts/cmux-next/check-crash-safety.sh "$PWD/Packages/macOS/CmuxNext" at the head: check-crash-safety: ok, crash-ratchet: ok. Export of dff3196 with crash_ratchet.py --exported-tree: unwrap 1148 -> 1150, exit 1 (red reproduced).
  • rustfmt --edition 2024 --check on pty_reclaim.rs: clean.
  • risk:normal fits (cmux-tui logic, no high-tier path). The body describes the head accurately. Nit only: creating_input.rs uses the same pattern on a different mutex (creating_inputs.terminals), not on workspace_registry/state.

Not run here: cargo build/test (fleet compile runs separately).

@github-actions

github-actions Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 61fa3cb: 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=pr28853 -f ref=base-red/pty-reclaim-unwraps -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); base red: the same failure on the base dff3196; does not block

Shipped in:

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fleet compile on head 61fa3cb: cmux-ci compile-check --package CmuxNext --app --recipe-version 2, job 7f9e717ade8ce19d4f329009: CmuxNext passed (269 s), app passed (91 s), cmux-tui passed (39 s); no error: in the log.

@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 Oct 31, 2026
@lawrencecchen
lawrencecchen merged commit d685211 into feat-cmux-next Oct 31, 2026
102 of 115 checks passed
@lawrencecchen
lawrencecchen deleted the base-red/pty-reclaim-unwraps branch October 31, 2026 03:56

This branch was previously deployed

3 inactive deployments
artifacts — 61fa3cb1 Deployed Oct 31, 2026 by lawrencecchen via publish tree-addressed cmux-tui #87218
sentry-dsyms — 61fa3cb1 Deployed Oct 31, 2026 by lawrencecchen via upload cmux-tui dSYMs to Sentry #87218
ci-routing-agent — 61fa3cb1 Deployed Oct 31, 2026 by lawrencecchen via cmux-tui tree preflight #87218
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