Skip to content

Fix ghost terminal surface rebind after close - #808

Merged
lawrencecchen merged 3 commits into
mainfrom
feat-ghost-surface-debug-logging
Mar 3, 2026
Merged

lawrencecchen merged 3 commits into
mainfrom
feat-ghost-surface-debug-logging

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Mar 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes the ghost terminal behavior where a Ctrl-D closed terminal can briefly reappear in a split (half-present/orphaned surface state).

Root Cause

A stale portal bind path could reattach a terminal surface after close had already started, so visibility/active flags could flip back before deinit completed.

Changes

  • Add lifecycle generation/state guards on TerminalSurface portal lifecycle transitions.
  • Seal lifecycle on close so stale binds cannot reattach closed surfaces.
  • Add DEBUG instrumentation across split/close/portal paths to capture lifecycle transitions and blocked binds.
  • Add debug.portal.stats CLI debug command and portal stats plumbing.
  • Add regression test tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py that catches close->visible rebind races during split churn.

Validation

  • CMUX_SOCKET=/tmp/cmux-debug-ghost-surface-orphan-regression.sock python3 tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py
  • Result: PASS: no close->visible rebind races during split-down + ctrl-d churn (iters=16, closes=16)

Summary by cubic

Prevents “ghost” terminals from briefly reappearing after Ctrl-D by guarding portal binds with surface lifecycle/generation and sealing the surface on close/deinit. Adds portal integrity stats and a regression test that fails on any close→visible rebind or portal orphan/stale state.

  • Bug Fixes

    • Added lifecycle (.live/.closing/.closed) and generation to TerminalSurface; binds are rejected when not live or on surface/generation mismatch.
    • On panel close, begin close lifecycle, hide/detach hosted views, and seal on deinit; portal registry detaches prior mappings when a guarded bind is rejected.
    • GhosttyTerminalView passes expected surface ID/generation to guarded binds; blocked reasons and counts are logged for diagnosis.
  • New Features

    • Added debug.portal.stats with per-window and total counts (entries, mapped_hosted, mapped terminals, orphans, visible orphans, stale, blocked binds/reasons) to verify portal integrity.
    • Added regression test tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py that churns split-down + Ctrl-D and asserts zero orphans/stale plus no close→visible rebinds.

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

Summary by CodeRabbit

  • New Features

    • Added a diagnostic v2 command to report portal and portal-binding statistics.
  • Bug Fixes

    • Strengthened portal binding lifecycle and guard checks to prevent invalid reattachments and ghost-terminal visibility during splits, closes, and reparenting.
  • Chores

    • Added extensive DEBUG-only logging around split, panel, and portal lifecycle events for improved observability.
  • Tests

    • Added a regression test ensuring closed terminal surfaces are fully cleaned up and do not reappear.

@vercel

vercel Bot commented Mar 3, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Mar 3, 2026 10:56pm

@coderabbitai

coderabbitai Bot commented Mar 3, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds debug instrumentation and guarded portal-binding lifecycle checks across terminal surface/view, portal registry, panel/tab/workspace split flows, and exposes portal stats via v2; also adds a regression test verifying no portal-orphan rebinds after Ctrl‑D closes.

Changes

Cohort / File(s) Summary
Portal registry & portal logic
Sources/TerminalWindowPortal.swift, Sources/.../WindowTerminalPortalRegistry.swift, Sources/.../TerminalWindowPortalRegistry.swift
Add guarded bind APIs (optional expectedSurfaceId/expectedGeneration), block/reason tracking, blockedBind counters, debugPortalStats aggregation, per-portal DebugStats and hosted-subview introspection, and detailed DEBUG logging for blocked binds and stats.
Surface & view lifecycle
Sources/GhosttyTerminalView.swift, Sources/GhosttySurfaceScrollView.swift, Sources/GhosttyNSView.swift, Sources/TerminalSurface.swift
Introduce portal lifecycle state (live/closing/closed), generation counters, canAcceptPortalBinding guards, begin/mark-close lifecycle methods, propagation of expectedSurfaceId/expectedGeneration through views, and deinit instrumentation with DEBUG logs.
Panels / Tab / Workspace instrumentation
Sources/Panels/TerminalPanel.swift, Sources/TabManager.swift, Sources/Workspace.swift, Sources/AppDelegate.swift
Add conditional DEBUG logging around split creation, panel creation ids, panel/tab close paths, and invoke surface.beginPortalCloseLifecycle before terminal panel cleanup; add SplitDirection.debugLabel helper used in logging.
TerminalController (v2 API)
Sources/TerminalController.swift
Expose a DEBUG-guarded v2 method debug.portal.stats that returns TerminalWindowPortalRegistry.debugPortalStats() for remote inspection.
Tests
tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py
New regression test that repeatedly splits/Closes (Ctrl‑D) bottom-right terminal panes and asserts via logs + v2 debug.portal.stats that no orphaned / visible-orphan / stale portal entries occur.

Sequence Diagram(s)

sequenceDiagram
participant TabMgr as TabManager
participant Panel as TerminalPanel / Window
participant PortalReg as TerminalWindowPortalRegistry
participant Surface as TerminalSurface / GhosttyView
participant Test as Test harness / Client

TabMgr->>Panel: request create split (direction)
Panel->>PortalReg: ensure surface/host wiring for new panel
Note over PortalReg,Surface: Binding attempt may include expectedSurfaceId & expectedGeneration
PortalReg->>Surface: bind(hostedView, anchorView, ..., expectedSurfaceId, expectedGeneration)
alt expected matches and surface live
    PortalReg-->>Surface: bind success (map entry)
    Surface-->>PortalReg: debugStats / generation confirmed
else mismatch or closing/closed
    PortalReg--xSurface: bind blocked (increment blockedBindCount, log reason)
    PortalReg->>PortalReg: cleanup stale mapping if needed
end
Test->>PortalReg: query debug.portal.stats()
PortalReg-->>Test: aggregated portal + blocked-bind stats
Note right of Surface: close lifecycle: beginPortalCloseLifecycle -> markPortalLifecycleClosed -> generation++
PortalReg->>Surface: further bind attempts with old generation will be blocked
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

"A hop, a tap, a debugty cheer —
I guard the portal, keep it clear.
Generation ticks, then close with grace,
No ghostly terminal takes my place.
— 🐇"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.80% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly summarizes the main fix: preventing a ghost terminal surface from rebinding after it has been closed, which is the core issue addressed across the entire changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-ghost-surface-debug-logging

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

@greptile-apps

greptile-apps Bot commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Fixes the ghost terminal bug where Ctrl-D closed terminals could briefly reappear in splits due to stale portal bind paths reattaching surfaces during close.

Key Changes:

  • Adds lifecycle state machine (live/closing/closed) with generation tracking to TerminalSurface
  • Portal binding now validates expectedSurfaceId and expectedGeneration before attaching, blocking stale binds
  • TerminalPanel.close() calls beginPortalCloseLifecycle() to seal the surface before detachment
  • Comprehensive DEBUG instrumentation across split/close/portal paths for diagnostics
  • New debug.portal.stats CLI command with orphan/stale subview detection
  • Regression test validates no close→visible rebind races during split churn

Validation:
The included regression test specifically targets the race condition and passes: no surfaces transition back to visible after close has started.

Confidence Score: 5/5

  • This PR is safe to merge with high confidence
  • Score reflects a well-architected fix with proper state machine design, generation-based staleness detection, comprehensive test coverage, and extensive debug instrumentation. The changes are focused, non-invasive to existing code paths, and directly address the root cause of the ghost terminal race condition.
  • No files require special attention

Important Files Changed

Filename Overview
Sources/GhosttyTerminalView.swift Adds lifecycle state machine (live/closing/closed) with generation tracking to prevent stale portal binds
Sources/TerminalWindowPortal.swift Implements portal binding guards with expectedSurfaceId/generation validation and debug stats tracking
Sources/Panels/TerminalPanel.swift Initiates close lifecycle transition when panel closes, properly sequenced before detachment
tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py Comprehensive regression test validating no close->visible rebind races during split churn

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[TerminalSurface Created] -->|generation=1| B[State: live]
    B -->|Panel.close called| C[beginPortalCloseLifecycle]
    C -->|generation=2| D[State: closing]
    D -->|Surface deinit| E[markPortalLifecycleClosed]
    E -->|generation=3| F[State: closed]
    
    G[Portal Bind Request] --> H{canAcceptPortalBinding?}
    H -->|state != live| I[REJECT - Block stale bind]
    H -->|expectedSurfaceId mismatch| I
    H -->|expectedGeneration mismatch| I
    H -->|All checks pass| J[ACCEPT - Bind surface to window portal]
    
    D -.->|Blocks any new binds| I
    F -.->|Blocks any new binds| I
    
    style B fill:#90EE90
    style D fill:#FFD700
    style F fill:#FF6B6B
    style I fill:#FF6B6B
    style J fill:#90EE90
Loading

Last reviewed commit: 56b5df0

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
Sources/TabManager.swift (1)

1869-1876: Deduplicate split direction label mapping.

The same SplitDirection → string switch is repeated in three places (Lines 1869-1876, 1901-1908, 2038-2044). Extracting a small helper/computed property (e.g., direction.debugLabel) would reduce drift risk in future log updates.

Also applies to: 1901-1908, 2038-2044

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/TabManager.swift` around lines 1869 - 1876, The repeated
switch-to-string mapping for SplitDirection should be extracted into a single
computed property on the SplitDirection type (e.g., add var debugLabel: String {
switch self { case .left: return "left" ... } }) and replace the three inline
switches that assign directionLabel with a call to direction.debugLabel; update
all usages where a debug label is needed (the places that currently switch on
direction) to use this new property to avoid duplication and drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 131-142: The _portal_integrity_error function currently coerces
missing totals keys to 0 which can false-pass; instead, first assert that totals
contains the required keys ("orphan_terminal_subview_count",
"visible_orphan_terminal_subview_count", "stale_entry_count") and if any are
missing return a non-nil error stating which keys are absent, otherwise parse
the integer values and perform the existing orphan/stale checks (do not silently
default missing keys to 0); update the logic in _portal_integrity_error to
detect malformed stats payloads and fail closed by returning an error when
required counters are not present.
- Around line 181-214: The rebind detector incorrectly suppresses visible events
while deinit is in-progress because the RE_VISIBLE_ON handling checks "sid in
close_pending and sid not in deinit_started"; remove the deinit_started
exclusion so any visible event for a sid in close_pending is treated as a
violation (the RE_DEINIT_END handler already discards sids from close_pending on
completion). Update the RE_VISIBLE_ON branch in _find_close_rebind_violations to
append the line when sid is in close_pending (remove the "and sid not in
deinit_started" condition), keeping the existing RE_DEINIT_BEGIN/RE_DEINIT_END
logic and sets (deinit_started, close_pending) intact.

---

Nitpick comments:
In `@Sources/TabManager.swift`:
- Around line 1869-1876: The repeated switch-to-string mapping for
SplitDirection should be extracted into a single computed property on the
SplitDirection type (e.g., add var debugLabel: String { switch self { case
.left: return "left" ... } }) and replace the three inline switches that assign
directionLabel with a call to direction.debugLabel; update all usages where a
debug label is needed (the places that currently switch on direction) to use
this new property to avoid duplication and drift.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a086ebc and 56b5df0.

📒 Files selected for processing (8)
  • Sources/AppDelegate.swift
  • Sources/GhosttyTerminalView.swift
  • Sources/Panels/TerminalPanel.swift
  • Sources/TabManager.swift
  • Sources/TerminalController.swift
  • Sources/TerminalWindowPortal.swift
  • Sources/Workspace.swift
  • tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py

Comment thread tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py
Comment thread tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 8 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py">

<violation number="1" location="tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py:211">
P1: The regression detector stops flagging close→visible rebinds after `deinit.begin`, which can miss visibility rebinds that occur before `deinit.end` (still before deinit completion).</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py Outdated
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in commit 7a0fb42:

  • tightened debug.portal.stats validation to fail closed on missing/non-integer totals counters
  • fixed close->visible detector to flag any visible transition while close is pending (including during deinit)
  • deduped SplitDirection debug label mapping via SplitDirection.debugLabel

@cubic-dev-ai please re-review.
@coderabbitai review

@cubic-dev-ai

cubic-dev-ai Bot commented Mar 3, 2026

Copy link
Copy Markdown

Addressed the review feedback in commit 7a0fb42:

  • tightened debug.portal.stats validation to fail closed on missing/non-integer totals counters
  • fixed close->visible detector to flag any visible transition while close is pending (including during deinit)
  • deduped SplitDirection debug label mapping via SplitDirection.debugLabel

...

@lawrencecchen I have started the AI code review. It will take a few minutes to complete.

@coderabbitai

coderabbitai Bot commented Mar 3, 2026

Copy link
Copy Markdown

@lawrencecchen, thanks for the update! I'll kick off a fresh review of the changes including commit 7a0fb42.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 124-125: The RPC call in _portal_stats currently uses
c._call("debug.portal.stats") with the default 20s timeout and should be bounded
by the integrity loop deadline; update _portal_stats to accept a timeout
argument (e.g., timeout_s) and call c._call("debug.portal.stats",
timeout_s=timeout_s) (or similar param name used by cmux._call), then modify
callers such as _wait_for_portal_integrity(timeout_s=...) to compute the
remaining deadline and pass that remaining timeout into _portal_stats so each
RPC is never allowed to block longer than the integrity loop budget.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56b5df0 and 7a0fb42.

📒 Files selected for processing (2)
  • Sources/TabManager.swift
  • tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • Sources/TabManager.swift

Comment thread tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py (1)

124-125: ⚠️ Potential issue | 🟠 Major

Bound debug.portal.stats RPC timeout to the integrity-loop deadline.

Line 125 still uses c._call("debug.portal.stats") with the default 20s timeout (see tests_v2/cmux.py), so _wait_for_portal_integrity(timeout_s=...) can overrun its budget and hang/flap under RPC latency.

💡 Suggested fix
-def _portal_stats(c: cmux) -> dict:
-    stats = c._call("debug.portal.stats") or {}
+def _portal_stats(c: cmux, *, timeout_s: float) -> dict:
+    stats = c._call("debug.portal.stats", timeout_s=timeout_s) or {}
     if not isinstance(stats, dict):
         raise cmuxError(f"debug.portal.stats returned non-dict payload: {stats!r}")
     return stats
@@
 def _wait_for_portal_integrity(c: cmux, *, timeout_s: float, context: str) -> None:
     deadline = time.time() + timeout_s
     last = None
     error = None
     while time.time() < deadline:
-        last = _portal_stats(c)
+        remaining = deadline - time.time()
+        if remaining <= 0:
+            break
+        last = _portal_stats(c, timeout_s=min(remaining, 0.5))
         error = _portal_integrity_error(last)
         if error is None:
             return
         time.sleep(POLL_S)

Also applies to: 165-171

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py` around lines 124
- 125, _portal_stats currently calls c._call("debug.portal.stats") with the
default RPC timeout which can exceed the overall _wait_for_portal_integrity
deadline; change _portal_stats to accept a timeout/deadline parameter (e.g.,
timeout_s or remaining_s) and pass that through to c._call as the RPC timeout,
and update callers (including _wait_for_portal_integrity and the other
occurrences around lines 165-171) to compute and forward the remaining time so
the RPC is bounded by the integrity-loop deadline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 124-125: _portal_stats currently calls
c._call("debug.portal.stats") with the default RPC timeout which can exceed the
overall _wait_for_portal_integrity deadline; change _portal_stats to accept a
timeout/deadline parameter (e.g., timeout_s or remaining_s) and pass that
through to c._call as the RPC timeout, and update callers (including
_wait_for_portal_integrity and the other occurrences around lines 165-171) to
compute and forward the remaining time so the RPC is bounded by the
integrity-loop deadline.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 56b5df0 and 7a0fb42.

📒 Files selected for processing (2)
  • Sources/TabManager.swift
  • tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 8 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Sources/TerminalController.swift">

<violation number="1" location="Sources/TerminalController.swift:8534">
P2: Avoid `DispatchQueue.main.sync` here; it can deadlock if the debug command is invoked on the main thread and it bypasses the existing `v2MainSync` guard. Use `v2MainSync` (or a `Thread.isMainThread` guard) to keep it safe and consistent.</violation>
</file>

<file name="tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py">

<violation number="1" location="tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py:125">
P2: The portal integrity poll is using an unbounded RPC timeout relative to the loop deadline. A slow `debug.portal.stats` call can block far longer than the integrity budget and make this test hang or become flaky. Pass an explicit short timeout for this RPC.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread Sources/TerminalController.swift Outdated
Comment thread tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py Outdated
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Addressed follow-up review items in commit e536b86:

  • v2DebugPortalStats now uses v2MainSync instead of direct DispatchQueue.main.sync
  • portal stats test RPC now uses bounded per-call timeout tied to remaining integrity-loop deadline

@cubic-dev-ai please re-review.
@coderabbitai review

@coderabbitai

coderabbitai Bot commented Mar 3, 2026

Copy link
Copy Markdown

@lawrencecchen, thanks for the follow-up! The two changes in commit e536b86 look like solid improvements:

  • Routing v2DebugPortalStats through v2MainSync is the right call for consistency with the rest of the v2 dispatch layer.
  • Bounding the per-call timeout to the remaining integrity-loop deadline avoids runaway waits.

I'll kick off a fresh incremental review now covering those changes.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cubic-dev-ai

cubic-dev-ai Bot commented Mar 3, 2026

Copy link
Copy Markdown

Addressed follow-up review items in commit e536b86:

  • v2DebugPortalStats now uses v2MainSync instead of direct DispatchQueue.main.sync
  • portal stats test RPC now uses bounded per-call timeout tied to remaining integrity-loop deadline

@cubic-dev-ai please re-review.
...

@lawrencecchen I have started the AI code review. It will take a few minutes to complete.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py (1)

247-279: Guarantee workspace cleanup with finally to avoid test state leakage.

At Line 278, c.close_workspace(workspace_id) only runs on the success path. If any earlier step raises, the workspace can remain open and affect subsequent runs.

♻️ Suggested refactor
     with cmux(SOCKET_PATH) as c:
         c.activate_app()
         workspace_id = c.new_workspace()
-        c.select_workspace(workspace_id)
-        time.sleep(0.2)
-
-        c.new_split("right")
-        layout = _wait_for_panes(c, 2, timeout_s=PANE_TIMEOUT_S, context="initial right split")
-        _wait_for_portal_integrity(c, timeout_s=INTEGRITY_TIMEOUT_S, context="after initial right split")
-
-        for iteration in range(1, ITERATIONS + 1):
-            right_panel_id = _rightmost_panel(layout)
-            c.focus_surface_by_panel(right_panel_id)
-            c.new_split("down")
-            layout = _wait_for_panes(
-                c,
-                3,
-                timeout_s=PANE_TIMEOUT_S,
-                context=f"iter={iteration} after split down",
-            )
-
-            bottom_right_panel_id = _bottom_right_panel(layout)
-            layout = _close_bottom_right_via_ctrl_d(
-                c,
-                bottom_right_panel_id=bottom_right_panel_id,
-                context=f"iter={iteration}",
-            )
-            _wait_for_portal_integrity(c, timeout_s=INTEGRITY_TIMEOUT_S, context=f"iter={iteration} integrity")
-            if POST_CLOSE_SETTLE_S > 0:
-                time.sleep(POST_CLOSE_SETTLE_S)
-
-        c.close_workspace(workspace_id)
+        try:
+            c.select_workspace(workspace_id)
+            time.sleep(0.2)
+
+            c.new_split("right")
+            layout = _wait_for_panes(c, 2, timeout_s=PANE_TIMEOUT_S, context="initial right split")
+            _wait_for_portal_integrity(c, timeout_s=INTEGRITY_TIMEOUT_S, context="after initial right split")
+
+            for iteration in range(1, ITERATIONS + 1):
+                right_panel_id = _rightmost_panel(layout)
+                c.focus_surface_by_panel(right_panel_id)
+                c.new_split("down")
+                layout = _wait_for_panes(
+                    c,
+                    3,
+                    timeout_s=PANE_TIMEOUT_S,
+                    context=f"iter={iteration} after split down",
+                )
+
+                bottom_right_panel_id = _bottom_right_panel(layout)
+                layout = _close_bottom_right_via_ctrl_d(
+                    c,
+                    bottom_right_panel_id=bottom_right_panel_id,
+                    context=f"iter={iteration}",
+                )
+                _wait_for_portal_integrity(c, timeout_s=INTEGRITY_TIMEOUT_S, context=f"iter={iteration} integrity")
+                if POST_CLOSE_SETTLE_S > 0:
+                    time.sleep(POST_CLOSE_SETTLE_S)
+        finally:
+            c.close_workspace(workspace_id)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py` around lines 247
- 279, Wrap the test body that uses cmux and the created workspace in a
try/finally so workspace cleanup always runs: after calling c.new_workspace()
(workspace_id) and c.select_workspace(workspace_id) execute the split/iteration
logic inside try and call c.close_workspace(workspace_id) in the finally block
(guarding that workspace_id is set) to ensure the workspace is closed even on
exceptions; reference cmux, c.new_workspace, c.select_workspace, and
c.close_workspace to locate where to add the try/finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 145-149: The test currently coerces
totals["orphan_terminal_subview_count"],
totals["visible_orphan_terminal_subview_count"], and totals["stale_entry_count"]
via int(...), which masks malformed values; change this to strict integer
validation instead of coercion: read the raw values from totals (the keys
referenced above), assert their types are exact integers (e.g., type(value) is
int or equivalently isinstance(value, int) and not isinstance(value, bool)), and
raise/propagate a TypeError when a value is not an exact int so the test fails
closed on non-integer counters.

---

Nitpick comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 247-279: Wrap the test body that uses cmux and the created
workspace in a try/finally so workspace cleanup always runs: after calling
c.new_workspace() (workspace_id) and c.select_workspace(workspace_id) execute
the split/iteration logic inside try and call c.close_workspace(workspace_id) in
the finally block (guarding that workspace_id is set) to ensure the workspace is
closed even on exceptions; reference cmux, c.new_workspace, c.select_workspace,
and c.close_workspace to locate where to add the try/finally.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7a0fb42 and e536b86.

📒 Files selected for processing (2)
  • Sources/TerminalController.swift
  • tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • Sources/TerminalController.swift

Comment on lines +145 to +149
try:
orphan = int(totals["orphan_terminal_subview_count"])
visible_orphan = int(totals["visible_orphan_terminal_subview_count"])
stale = int(totals["stale_entry_count"])
except (TypeError, ValueError):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fail closed on counter types; int(...) coercion can mask malformed portal stats.

At Line 146–148, using int(...) accepts/truncates non-integer values (0.7, "0", False), which can false-pass integrity checks that are supposed to reject non-integer counters.

💡 Suggested hardening
-    try:
-        orphan = int(totals["orphan_terminal_subview_count"])
-        visible_orphan = int(totals["visible_orphan_terminal_subview_count"])
-        stale = int(totals["stale_entry_count"])
-    except (TypeError, ValueError):
+    orphan_raw = totals["orphan_terminal_subview_count"]
+    visible_orphan_raw = totals["visible_orphan_terminal_subview_count"]
+    stale_raw = totals["stale_entry_count"]
+    if type(orphan_raw) is not int or type(visible_orphan_raw) is not int or type(stale_raw) is not int:
         return (
             "portal totals contains non-integer counters "
-            f"(orphan={totals.get('orphan_terminal_subview_count')!r}, "
-            f"visible_orphan={totals.get('visible_orphan_terminal_subview_count')!r}, "
-            f"stale={totals.get('stale_entry_count')!r})"
+            f"(orphan={orphan_raw!r}, visible_orphan={visible_orphan_raw!r}, stale={stale_raw!r})"
         )
+    orphan = orphan_raw
+    visible_orphan = visible_orphan_raw
+    stale = stale_raw
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py` around lines 145
- 149, The test currently coerces totals["orphan_terminal_subview_count"],
totals["visible_orphan_terminal_subview_count"], and totals["stale_entry_count"]
via int(...), which masks malformed values; change this to strict integer
validation instead of coercion: read the raw values from totals (the keys
referenced above), assert their types are exact integers (e.g., type(value) is
int or equivalently isinstance(value, int) and not isinstance(value, bool)), and
raise/propagate a TypeError when a value is not an exact int so the test fails
closed on non-integer counters.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 8 files

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py (1)

145-149: ⚠️ Potential issue | 🟠 Major

Use strict integer type checks for portal counters.

Line 145–149 still coerces with int(...), which accepts values like "0", False, and 0.7; that can false-pass malformed debug.portal.stats payloads instead of failing closed.

💡 Proposed hardening
-    try:
-        orphan = int(totals["orphan_terminal_subview_count"])
-        visible_orphan = int(totals["visible_orphan_terminal_subview_count"])
-        stale = int(totals["stale_entry_count"])
-    except (TypeError, ValueError):
+    orphan_raw = totals["orphan_terminal_subview_count"]
+    visible_orphan_raw = totals["visible_orphan_terminal_subview_count"]
+    stale_raw = totals["stale_entry_count"]
+    if (
+        type(orphan_raw) is not int
+        or type(visible_orphan_raw) is not int
+        or type(stale_raw) is not int
+    ):
         return (
             "portal totals contains non-integer counters "
-            f"(orphan={totals.get('orphan_terminal_subview_count')!r}, "
-            f"visible_orphan={totals.get('visible_orphan_terminal_subview_count')!r}, "
-            f"stale={totals.get('stale_entry_count')!r})"
+            f"(orphan={orphan_raw!r}, "
+            f"visible_orphan={visible_orphan_raw!r}, "
+            f"stale={stale_raw!r})"
         )
+    orphan = orphan_raw
+    visible_orphan = visible_orphan_raw
+    stale = stale_raw
#!/bin/bash
set -euo pipefail

# 1) Confirm current implementation still uses int(...) coercion.
sed -n '131,160p' tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py

# 2) Verify Python coercion behavior that can mask malformed counters.
python - <<'PY'
samples = ["0", False, 0.7, 0]
for v in samples:
    try:
        print(f"value={v!r:<6} type={type(v).__name__:<7} int(v)={int(v)} strict_int={type(v) is int}")
    except Exception as e:
        print(f"value={v!r:<6} type={type(v).__name__:<7} int(v)=<error {type(e).__name__}> strict_int={type(v) is int}")
PY
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py` around lines 145
- 149, The test currently coerces portal counters with int(...) which accepts
strings, bools and floats; instead, fetch each value from totals
(totals["orphan_terminal_subview_count"],
totals["visible_orphan_terminal_subview_count"], totals["stale_entry_count"])
and perform a strict type check (require type(value) is int — not isinstance to
exclude bool) before assigning to orphan, visible_orphan, and stale; if the
check fails, raise or assert (e.g., ValueError/AssertionError) so malformed
debug.portal.stats payloads fail the test rather than silently coerced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py`:
- Around line 145-149: The test currently coerces portal counters with int(...)
which accepts strings, bools and floats; instead, fetch each value from totals
(totals["orphan_terminal_subview_count"],
totals["visible_orphan_terminal_subview_count"], totals["stale_entry_count"])
and perform a strict type check (require type(value) is int — not isinstance to
exclude bool) before assigning to orphan, visible_orphan, and stale; if the
check fails, raise or assert (e.g., ValueError/AssertionError) so malformed
debug.portal.stats payloads fail the test rather than silently coerced.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7a0fb42 and e536b86.

📒 Files selected for processing (2)
  • Sources/TerminalController.swift
  • tests_v2/test_split_cmd_shift_d_ctrl_d_no_portal_orphans.py

bn-l pushed a commit to bn-l/cmux that referenced this pull request Apr 3, 2026
* Fix ghost terminal lifecycle rebind race

* Address review feedback on portal regression checks

* Address follow-up review feedback

This branch was successfully deployed

1 active deployment
Preview — e536b861 Deployed Mar 3, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant