Skip to content

Stabilize remote tmux app-host test teardown - #9297

Closed
austinywang wants to merge 2 commits into
mainfrom
fix-remote-tmux-test-lifecycle
Closed

austinywang wants to merge 2 commits into
mainfrom
fix-remote-tmux-test-lifecycle

Conversation

@austinywang

@austinywang austinywang commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • close fixture-owned windows through non-interactive NSWindow.close() so test teardown never enters cmux’s final-window quit path
  • explicitly tear down every workspace panel before closing each fixture window
  • suppress closed-window history and retire the temporary recoverable route so terminal surfaces cannot accumulate across the serialized suite
  • leave all production remote-tmux close, detach, placement, and focus-neutral behavior under test unchanged

Failure evidence

Validation

  • final focused suite https://github.com/manaflow-ai/cmux/actions/runs/30624091140 on 756cc69b57: 8 tests in 1 suite passed in one app-host launch (4.234s), including the prior crash boundary; TEST SUCCEEDED
  • no review threads or actionable CodeRabbit findings
  • cmux-policy-check --mode branch --base origin/main
  • git diff --check
  • ./scripts/lint-pbxproj-test-wiring.sh (642 files)
  • local build intentionally skipped because the shared machine has only ~3 GiB free; CI compiled and ran the app-host suite

The same commit is included in the disposable full-CI integration run for #9266; that shard is still in progress.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Harness.closeWindow now tears down panels, suppresses closed-window history, directly closes the matching window, removes its recoverable route, and processes the main run loop even when no matching window exists.

Changes

Window close lifecycle

Layer / File(s) Summary
Window teardown and closure
cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift
Harness.closeWindow now performs panel teardown, suppresses closed-window history, closes the matching window directly, removes its recoverable route, and always runs the main loop.

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

Possibly related issues

  • manaflow-ai/cmux-dev-artifacts#7210: This change updates the same test harness and addresses window close and app restart instability.

Possibly related PRs

  • manaflow-ai/cmux#9110: Both changes cover remote tmux mirror close and detach teardown, including recoverable-route and closed-history behavior.
  • manaflow-ai/cmux#9108: Both changes update close behavior in the remote tmux mirror test suite.
  • manaflow-ai/cmux#9020: The changes are related to remote-tmux panel retirement and window closure lifecycle handling.

Suggested reviewers: lawrencecchen, azooz2003-bit

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 Swift Actor Isolation ✅ Passed The diff changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; the suite and Harness are explicitly @MainActor, so no production actor-isolation mistake is introduced.
Cmux Swift Blocking Runtime ✅ Passed The only changed file is test-only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; the existing 50 ms RunLoop delay and Task.sleep remain test scaffolding, and no production blocking primitive...
Cmux Browser Automation Off-Main ✅ Passed The only changed file is a MainActor test fixture; it changes window cleanup from performClose(nil) to window.close() and adds no browser socket automation or WebKit wait routing.
Cmux Expensive Synchronous Load ✅ Passed The PR changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; it adds no production agent-history load, and cleanup uses the existing cached/off-main close snapshot path.
Cmux Cache Substitution Correctness ✅ Passed The only changed path is cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; it changes fixture teardown, not production persistence, history, undo, or snapshot code.
Cmux No Hacky Sleeps ✅ Passed The patch changes only Swift test fixture code; it adds no TypeScript, JavaScript, shell, or non-Swift build/runtime delay covered by this rule.
Cmux Algorithmic Complexity ✅ Passed The only changed file is test-only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; its linear fixture cleanup is explicitly exempt, and no production complexity changed.
Cmux Swift Concurrency ✅ Passed The diff adds synchronous panel/window cleanup and AppKit NSWindow.close() in a @MainActor test fixture; it adds no Dispatch, Combine, completion-handler, or fire-and-forget Task pattern.
Cmux Swift @Concurrent ✅ Passed The diff adds no async or @concurrent declarations. Changed closeWindow is synchronous and @MainActor, while teardownAllPanels and route helpers are synchronous actor-bound calls.
Cmux Swift Package Boundaries ✅ Passed The diff changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift. It updates test-fixture cleanup and adds no production feature logic requiring a SwiftPM boundary.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; it changes no Package.swift, Package.resolved, Xcode project, .gitignore, workflow, or dependency files.
Cmux Swift Logging ✅ Passed The only changed file is a Swift test fixture; the diff adds teardown, NSWindow.close(), route cleanup, and RunLoop handling, with no logging APIs or diagnostic output.
Cmux User-Facing Error Privacy ✅ Passed The only changed file is a test fixture; it changes window cleanup APIs and adds no production user-facing errors, alerts, output, recovery copy, or exposed sensitive details.
Cmux Full Internationalization ✅ Passed The PR changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; added fixture cleanup calls and lifecycle handling introduce no user-facing text or localization entries.
Cmux Swiftui State Layout ✅ Passed The only changed file is an AppKit-based test fixture. The diff adds cleanup calls and NSWindow.close(); it introduces no SwiftUI view, state, geometry, lazy-row, or render-time mutation pattern.
Cmux Architecture Rethink ✅ Passed The only changed file is a test fixture. It uses NSWindow.close() and existing teardown/history/route owners; no new timing repair, state, observer, lock, or duplicate UI lifecycle path was added.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The diff changes only a test-only Harness fixture in cmuxTests; it adds no user-visible auxiliary window or identifier assignment. Test-only fixtures are explicitly allowed by the rule.
Cmux Source Artifacts ✅ Passed The only changed path is the hand-written Swift test cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; no artifact-like paths or generated outputs enter the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The PR changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift; no Swift file under a production Sources path is modified, so this rule is not applicable.
Cmux No Ambient Global State ✅ Passed The commit changes only cmuxTests/RemoteTmuxMirrorCloseDetachTests.swift and adds statements inside existing Harness.closeWindow; it adds no ambient global declarations or singleton state.
Title check ✅ Passed The title clearly identifies the main change: stabilizing remote tmux app-host test teardown.
Description check ✅ Passed The description explains the change, failure evidence, validation results, and testing limitations in sufficient detail.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-remote-tmux-test-lifecycle

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

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

@cursor

cursor Bot commented Jul 31, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants