test: kill hosted test shells before freeing their terminals

[teamleaderleo]

Several app-host tests freed a live terminal with a bare releaseSurfaceForTesting() or teardownSurface(). When the free starts while /usr/bin/login is still in its SIGHUP-ignoring startup, it waits out Ghostty's 12 s SIGHUP grace. The release helper waits synchronously on the main thread, and teardownSurface() waits while holding one of TerminalSurfaceRuntimeTeardownCoordinator's two close slots. The stall then lands on whichever test runs next. On main it hits a different test in each run, for example hiddenEntryNeverPublishes() or testSearchOverlayMountsAndUnmountsWithSearchState, at about 12.1 s each.

These call sites now go through releaseHostedSurfaceForTesting() or a new teardownHostedSurfaceForTesting(), which SIGKILL the terminal's processes before freeing. Both are just the plain call when no runtime exists. Covered call sites include TerminalPortalGeometryFixture.close(), GhosttySurfaceOverlayTests.tearDown, TerminalOffscreenStartupTests, and 17 other test files. CloudRestoreReplayFixture keeps teardownSurface() because its manual-IO surface has no shell. Test-only change; product teardown and the 12 s grace for real sessions are untouched.

Testing

Focused runs of the same three suites on the owned Mac mini lane (glaeda-std-xcode-26.6):

Suite	Before, main 6431ac2 (run)	After, 982c16d (run, attempt 2)
TerminalWindowPortalCommittedGeometryTests (11 tests)	13.63 s, one test at 12.11 s	1.99 s, slowest test 0.37 s
GhosttySurfaceOverlayTests (22 tests)	13.70 s, one test at 12.08 s	1.92 s
TerminalOffscreenStartupTests (29 tests)	0.69 s	1.01 s
All 51 selected tests	14.39 s	2.93 s

All 51 tests passed in both runs. The after run's first attempt never started a test: XCTest hung before connecting to testmanagerd on that host, and a rerun of the failed job passed. These numbers cover only these suites. I did not measure a full app-host shard.

🤖 Generated with Claude Code

---

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

---

Summary by cubic

Fixes intermittent ~12 s stalls in the app-host test suites by killing each hosted terminal's shell before freeing its terminal.

Test teardown exits now go through releaseHostedSurfaceForTesting() or the new teardownHostedSurfaceForTesting(), which SIGKILL the terminal's processes before the native free. Previously, with /usr/bin/login still in its SIGHUP-ignoring startup, the free waited out Ghostty's 12 s SIGHUP grace and stalled whichever test ran next. Both helpers fall back to the plain call when no runtime exists. CloudRestoreReplayFixture and ScrollbackTestTerminal keep bare teardownSurface() because their manual-mirror surfaces have no shell and would spin the 1 s kill deadline doing nothing. Product teardown is unchanged.

^{Written for commit 7f82ed9. Summary will update on new commits.}

Summary by CodeRabbit

• Tests
  • Standardized cleanup of hosted terminal surfaces across test fixtures, including orderly shutdown of associated shell processes. This improves consistency for checks covering terminal startup, input, rendering, notifications, and lifecycle behavior. Test expectations are unchanged, and there are no changes to end-user functionality.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e5ae4759-c74f-4bbb-9c1c-c56881bcfc7a

📥 Commits

Reviewing files that changed from the base of the PR and between 982c16d and 7f82ed9.

📒 Files selected for processing (1)

• cmuxTests/ScrollbackTestTerminal.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 2e87b9fa-3e70-4923-8a2a-097ffd4ad6b8

📥 Commits

Reviewing files that changed from the base of the PR and between cfdde0b and 982c16d.

📒 Files selected for processing (20)

• cmuxTests/AgentRelayTTYOwnershipTests.swift
• cmuxTests/AppDelegateShortcutRoutingTests.swift
• cmuxTests/CanvasPaneContentMountTests.swift
• cmuxTests/DockPortalReconcileTests.swift
• cmuxTests/DockRuntimeParityTests.swift
• cmuxTests/GhosttyTerminalStartupEnvironmentTests.swift
• cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift
• cmuxTests/NotificationRowSnapshotBoundaryTests.swift
• cmuxTests/NotificationScrollRestoreLifecycleTests.swift
• cmuxTests/PlainPastePTYFixture.swift
• cmuxTests/ScrollbackTestTerminal.swift
• cmuxTests/SocketTerminalBindingRegressionTests.swift
• cmuxTests/TerminalAgentPanelInitialTitleTests.swift
• cmuxTests/TerminalAndGhosttyTests.swift
• cmuxTests/TerminalControllerSocketSecurityTests.swift
• cmuxTests/TerminalPortalGeometryFixture.swift
• cmuxTests/TerminalSearchOverlayMouseReleaseTests.swift
• cmuxTests/TerminalSurfaceTestTeardown.swift
• cmuxTests/TerminalUploadFailureNotificationTests.swift
• cmuxTests/TextBoxEscapePassthroughTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Tests and fixtures now use hosted-surface testing helpers for release and teardown. A new teardown helper kills hosted shell processes before calling teardownSurface(). Test expectations and production behavior do not change.

Changes

Hosted Surface Test Cleanup

Layer / File(s)	Summary
Hosted-surface teardown helper cmuxTests/TerminalSurfaceTestTeardown.swift	Adds teardownHostedSurfaceForTesting(). It kills hosted shell processes before calling teardownSurface().
Hosted-surface cleanup call sites cmuxTests/*	Tests and fixtures replace surface-only release or teardown calls with hosted-surface testing helpers. Test expectations remain unchanged.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 982c1

This PR changes test cleanup rather than product teardown, and the selected tests are reported to run faster. Whether a residual 12-second delay is possible remains unestablished, so no concrete merge-blocking regression is identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 982c1

The change is confined to test cleanup and does not expose a new production entrypoint. The remaining risk is limited to the test host: cleanup may target a process that no longer belongs to the terminal being closed.

Retained concerns

• Low · security · inferred: Expanded test cleanup uses a cached TTY to SIGKILL device-associated processes without revalidating their ownership or identity. An unrelated or newly reused process could be affected if the snapshot no longer represents the hosted surface.

Security review details

Security Blast Radius

• inferred — The new invocation path is confined to test-hosted terminal cleanup; the plausible collateral scope is other processes accessible to that test host, not production sessions or an external service.

Security Findings and Attack Paths

• inferred — If a TTY or enumerated PID ceases to identify only this hosted surface’s processes before signalling, the device-wide kill loop could terminate an unrelated process. The evidence does not establish that this occurs or an externally reachable attack path.

Trust Boundaries and Controls

• observed — The lookup requires a live runtime surface, and the kill loop excludes PID 1, the test host PID, and the host process group. These controls do not establish ownership of every other process returned for the TTY.

Resilience and Maintainability Implications

• observed — If process lookup cannot obtain a TTY, the kill helper returns and the wrapper still invokes teardown; the intended fast close is therefore conditional on successful lookup.

Hardening Proposals

• proposed — Constrain termination to verified processes belonging to the hosted runtime, or validate process identity immediately before signalling, if the test environment can provide that identity.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 57 functions across 20 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (24 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main test-only change: killing hosted test shells before freeing terminals.
Description check	✅ Passed	The description explains the problem, the helper behavior, affected scope, test results, performance impact, and known test limitation. The omitted checklist and demo video are non-critical because th…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input	✅ Passed	PASS — the authoritative PR diff changes only cmuxTests cleanup call sites and adds TerminalSurface test teardown helpers. It does not change Cloud terminal creation, cmux-tui transport, manual re…
Cmux Swift Actor Isolation	✅ Passed	PASS: The pull request changes only files under cmuxTests/; no production Swift files changed. The new teardownHostedSurfaceForTesting() helper is test-only, explicitly @MainActor, and only dele…
Cmux Swift Blocking Runtime	✅ Passed	PASS: The PR changes only cmuxTests/*.swift; no production or runtime Swift file changes are present. The new killShellProcessesForTesting() helper uses polling and usleep, but it is explicitly …
Cmux Browser Automation Off-Main	✅ Passed	PASS: The authoritative diff changes only cmuxTests teardown helpers and call sites. The added teardownHostedSurfaceForTesting() only kills shell processes before calling teardownSurface(), and …
Cmux Expensive Synchronous Load	✅ Passed	PASS: The authoritative diff changes only files under cmuxTests/. It replaces test cleanup calls and adds teardownHostedSurfaceForTesting(), which kills test shell processes before calling `teardo…
Cmux Cache Substitution Correctness	✅ Passed	PASS: The authoritative diff changes only 20 files under cmuxTests/; no production Swift, TypeScript, or JavaScript files changed. The edits replace test cleanup calls and add `teardownHostedSurface…
Cmux No Hacky Sleeps	✅ Passed	PASS: The authoritative diff changes only 20 Swift files under cmuxTests; it contains no TypeScript, JavaScript, shell, or build/runtime-script changes. The added helper is deterministic test teardo…
Cmux Algorithmic Complexity	✅ Passed	PASS. The review-scoped diff changes only cmuxTests/ files. It adds a test-only teardownHostedSurfaceForTesting() wrapper and replaces test cleanup calls; no production Swift, TypeScript, JavaScri…
Cmux Swift Concurrency	✅ Passed	The diff does not introduce a flagged legacy async pattern. It adds only a synchronous @MainActor test helper that calls the existing killShellProcessesForTesting() and teardownSurface(), plus t…
Cmux Swift @Concurrent	✅ Passed	PASS. The diff adds only the synchronous @MainActor teardownHostedSurfaceForTesting() helper and replaces test cleanup calls. It introduces no nonisolated async function, no @concurrent annota…
Cmux Swift Package Boundaries	✅ Passed	PASS: The reviewed diff changes only files under cmuxTests/. It adds and uses test-only teardown helpers, with no production Sources/ or SwiftPM package changes. The rule explicitly allows test co…
Cmux Swiftpm Lockfiles	✅ Passed	The authoritative PR diff changes only 20 files under cmuxTests/, all Swift test code. It contains no Package.swift, Package.resolved, .gitignore, workflow, or cmux.xcodeproj changes. Theref…
Cmux Swift Logging	✅ Passed	PASS: The pull request changes only cmuxTests/*.swift test and fixture code. The diff adds process-kill and teardown helpers plus replaces test cleanup calls; it adds no print, debugPrint, `dump…
Cmux User-Facing Error Privacy	✅ Passed	PASS: The authoritative diff changes only 20 files under cmuxTests/. It adds a test-only teardown helper and replaces test cleanup calls. It introduces no production code, user-facing errors, alerts…
Cmux Full Internationalization	✅ Passed	The authoritative diff changes only 20 files under cmuxTests/. The changes add a test-only teardown helper and replace test cleanup calls. No production Swift files, string catalogs, Info.plist file…
Cmux Swiftui State Layout	✅ Passed	PASS: The PR changes only 20 Swift test files. The diff adds TerminalSurface test cleanup helpers and replaces surface cleanup calls. It adds no SwiftUI views, ObservableObject/@Published state,…
Cmux Architecture Rethink	✅ Passed	PASS. The PR changes only test cleanup call sites and adds teardownHostedSurfaceForTesting(). The new helper reuses the existing killShellProcessesForTesting() and then calls the existing `teardow…
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	PASS. The diff changes only files under cmuxTests/. It adds test teardown helpers and replaces test cleanup calls. It does not add or materially change NSWindow, NSPanel, NSWindowController, S…
Cmux Source Artifacts	✅ Passed	PASS: The authoritative diff changes only 20 existing cmuxTests/*.swift source files. Changes add or update test helpers and test cleanup calls. No artifact directories, generated logs, screenshots,…
Cmux No Test Or Debug Seam In Production Source	✅ Passed	PASS: The reviewed diff changes 20 files, all under cmuxTests/, and changes no Swift file under a production Sources/ path. The added ...ForTesting helper and call-site updates are test-target s…

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

CI failure attribution

CI passes on 7f82ed9ebc (run 36317866969 attempt 3).

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's.

[github-actions]

Merge receipt for 7f82ed9ebc: every check was green at merge (15 verified; 15 skipped by policy). Full suite runs on main after merge.