Fix orphaned child processes when closing workspace tabs#889
Conversation
|
@novarii is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds explicit teardown APIs and calls to deterministically release terminal surface resources: a new TerminalSurface.teardownSurface(), TerminalPanel.close() invoking it, Workspace.teardownAllPanels() to bulk-close panels, and TabManager.closeWorkspace() updated to call teardown before removing a workspace. Changes
Sequence DiagramsequenceDiagram
actor User
participant TabMgr as TabManager
participant Workspace
participant TermPanel as TerminalPanel
participant Surface as TerminalSurface
User->>TabMgr: closeWorkspace(workspace)
activate TabMgr
TabMgr->>Workspace: teardownAllPanels()
activate Workspace
loop each panel
Workspace->>TermPanel: close()
activate TermPanel
TermPanel->>Surface: teardownSurface()
activate Surface
Surface->>Surface: mark portal lifecycle closed
Surface->>Surface: clear & release callback context
Surface->>Surface: null runtime surface
Surface->>Surface: free surface on main actor
deactivate Surface
TermPanel->>Workspace: unregisterSubscriptions() / unregisterFromPortScanner()
deactivate TermPanel
end
Workspace->>Workspace: clear panels & surface mappings
deactivate Workspace
TabMgr->>TabMgr: remove workspace by index
deactivate TabMgr
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
2155-2163: Clear remaining panel-derived caches during workspace teardown.Line 2161–Line 2163 only clears
panelsandsurfaceIdToPanelId. In the exact delayed-retention case this PR targets, per-panel metadata can remain on the retainedWorkspacelonger than needed. Consider reusing existing metadata cleanup so teardown is deterministic and consistent with normal close paths.Proposed diff
func teardownAllPanels() { for (panelId, panel) in panels { panel.close() panelSubscriptions.removeValue(forKey: panelId) PortScanner.shared.unregisterPanel(workspaceId: id, panelId: panelId) } panels.removeAll() surfaceIdToPanelId.removeAll() + pruneSurfaceMetadata(validSurfaceIds: []) + restoredTerminalScrollbackByPanelId.removeAll(keepingCapacity: false) + terminalInheritanceFontPointsByPanelId.removeAll(keepingCapacity: false) + lastTerminalConfigInheritancePanelId = nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 2155 - 2163, The teardownAllPanels() loop currently closes panels, removes panelSubscriptions entries, and unregisters ports but does not clear per-panel metadata caches (so metadata can linger on retained Workspace); update teardownAllPanels() to invoke the same metadata cleanup used by the normal panel close path for each panel (e.g., call the existing per-panel cleanup function used by closePanel(panelId:) or the panel's metadataClear method) before or after panel.close(), and ensure you also remove any per-panel entries from panelSubscriptions and surfaceIdToPanelId so all panel-derived caches are deterministically cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 2155-2163: The teardownAllPanels() loop currently closes panels,
removes panelSubscriptions entries, and unregisters ports but does not clear
per-panel metadata caches (so metadata can linger on retained Workspace); update
teardownAllPanels() to invoke the same metadata cleanup used by the normal panel
close path for each panel (e.g., call the existing per-panel cleanup function
used by closePanel(panelId:) or the panel's metadataClear method) before or
after panel.close(), and ensure you also remove any per-panel entries from
panelSubscriptions and surfaceIdToPanelId so all panel-derived caches are
deterministically cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c00e696f-fae6-49f0-8557-bbd713f70414
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/TabManager.swiftSources/Workspace.swift
Greptile SummaryThis PR fixes orphaned child processes (zsh, claude, etc.) when closing workspace tabs by introducing eager, deterministic Ghostty surface teardown rather than relying on ARC deallocation. The fix is well-scoped: Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabManager
participant Workspace
participant TerminalPanel
participant TerminalSurface
participant Ghostty
User->>TabManager: closeWorkspace(workspace)
TabManager->>TabManager: guard tabs.count > 1
TabManager->>TabManager: guard let index (ownership check)
TabManager->>TabManager: clearNotifications / unwireBrowserTracking
TabManager->>Workspace: teardownAllPanels()
loop For each panel
Workspace->>Workspace: panelSubscriptions.removeValue(panelId)
Workspace->>Workspace: PortScanner.unregisterPanel(...)
Workspace->>TerminalPanel: panel.close()
TerminalPanel->>TerminalSurface: beginPortalCloseLifecycle("panel.close")
TerminalPanel->>TerminalSurface: teardownSurface()
TerminalSurface->>TerminalSurface: markPortalLifecycleClosed("teardown")
TerminalSurface->>TerminalSurface: surface = nil
TerminalSurface-->>TerminalSurface: Task { @MainActor } scheduled
end
Workspace->>Workspace: panels.removeAll()
Workspace->>Workspace: surfaceIdToPanelId.removeAll()
Workspace->>Workspace: pruneSurfaceMetadata(validSurfaceIds: [])
TabManager->>TabManager: tabs.remove(at: index)
TabManager->>TabManager: update selectedTabId if needed
Note over TerminalSurface,Ghostty: Next main-actor drain
TerminalSurface->>Ghostty: ghostty_surface_free(surfaceToFree)
Ghostty-->>User: SIGHUP → child process exits
Last reviewed commit: 90fffd4 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 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/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:2161">
P2: `teardownAllPanels()` performs partial cleanup and leaves multiple panel-indexed state maps populated, which can retain stale workspace state when deallocation is delayed.</violation>
</file>
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:1001">
P2: Only tear down panels after confirming the workspace exists in this manager; otherwise a stale/non-owned workspace can be terminated unexpectedly.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
@lawrencecchen I have started the AI code review. It will take a few minutes to complete. |
🧩 Analysis chain🏁 Script executed: Length of output: 262396
SummaryThe approach is sound: rather than relying on ARC deallocation to send SIGHUP via Issues Found🔴
|
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
1 issue found across 5 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="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:4026">
P2: The assertion on `manualUnreadPanelIds` is trivially true because no panel is ever marked unread in the test setup. Add `workspace.markPanelUnread(initialPanelId)` (and/or for `splitPanel.id`) before calling `teardownAllPanels()` to actually verify that teardown clears unread state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Pushed follow-up fixes for latest thread feedback:
|
@lawrencecchen I have started the AI code review. It will take a few minutes to complete. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
When closing a workspace tab via the sidebar X button, child processes (login → zsh → claude) survived as orphans because TabManager.closeWorkspace() only removed the workspace from the tabs array without explicitly freeing Ghostty surfaces. It relied on ARC to cascade deallocation, but SwiftUI views and Combine publishers held references, delaying or preventing ghostty_surface_free() (which sends SIGHUP) from ever running. This adds explicit teardown on the workspace close path: - TerminalSurface.teardownSurface(): idempotent method to free the Ghostty runtime surface eagerly, matching the existing deinit logic - TerminalPanel.close() now calls teardownSurface() to ensure SIGHUP is sent - Workspace.teardownAllPanels() iterates all panels and closes them - TabManager.closeWorkspace() calls teardownAllPanels() before removing the workspace from the tabs array
lawrencecchen
force-pushed
the
fix/orphaned-processes-on-workspace-close
branch
from
46e32ba to
90fffd4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
4154-4167: Optional: assert tab titles remain unchanged to lock in ownership no-op semantics.Line [4163] already proves order stability via IDs; adding a title snapshot would fully guard the “order + titles unchanged” expectation.
♻️ Suggested test hardening
let initialTabIds = manager.tabs.map(\.id) + let initialTabTitles = manager.tabs.map(\.title) let initialSelectedTabId = manager.selectedTabId @@ XCTAssertEqual(manager.tabs.map(\.id), initialTabIds) + XCTAssertEqual(manager.tabs.map(\.title), initialTabTitles) XCTAssertEqual(manager.selectedTabId, initialSelectedTabId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 4154 - 4167, Add an assertion to also verify that tab titles remain unchanged after calling manager.closeWorkspace(externalWorkspace): capture a snapshot of manager.tabs' titles (e.g., manager.tabs.map(\.title)) into a variable before the call and assert it equals manager.tabs.map(\.title) after the call; similarly, assert that externalWorkspace.panelTitles (already captured as externalPanelTitlesBefore) matches after the call to lock in the "no-op ownership" semantics—update near the existing initialTabIds/initialSelectedTabId captures and following XCTAssertEqual checks that reference manager.tabs, manager.selectedTabId, and externalWorkspace.panelTitles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 4154-4167: Add an assertion to also verify that tab titles remain
unchanged after calling manager.closeWorkspace(externalWorkspace): capture a
snapshot of manager.tabs' titles (e.g., manager.tabs.map(\.title)) into a
variable before the call and assert it equals manager.tabs.map(\.title) after
the call; similarly, assert that externalWorkspace.panelTitles (already captured
as externalPanelTitlesBefore) matches after the call to lock in the "no-op
ownership" semantics—update near the existing initialTabIds/initialSelectedTabId
captures and following XCTAssertEqual checks that reference manager.tabs,
manager.selectedTabId, and externalWorkspace.panelTitles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89810db3-4bc6-4477-af97-72489c578159
📒 Files selected for processing (5)
Sources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/GhosttyTerminalView.swift
- Sources/Panels/TerminalPanel.swift
- Sources/TabManager.swift
|
@greptile-apps review\n@cubic-dev-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| func teardownAllPanels() { | ||
| let panelEntries = Array(panels) | ||
| for (panelId, panel) in panelEntries { | ||
| panelSubscriptions.removeValue(forKey: panelId) | ||
| PortScanner.shared.unregisterPanel(workspaceId: id, panelId: panelId) | ||
| panel.close() | ||
| } | ||
|
|
||
| panels.removeAll(keepingCapacity: false) | ||
| surfaceIdToPanelId.removeAll(keepingCapacity: false) | ||
| panelSubscriptions.removeAll(keepingCapacity: false) | ||
| pruneSurfaceMetadata(validSurfaceIds: []) | ||
| restoredTerminalScrollbackByPanelId.removeAll(keepingCapacity: false) | ||
| terminalInheritanceFontPointsByPanelId.removeAll(keepingCapacity: false) | ||
| lastTerminalConfigInheritancePanelId = nil | ||
| lastTerminalConfigInheritanceFontPoints = nil | ||
| } |
There was a problem hiding this comment.
bonsplitController tabs are not torn down during workspace teardown
teardownAllPanels() clears panels and surfaceIdToPanelId, but the bonsplitController still retains its full tab and pane tree. After teardown, the bonsplit controller will have tabs whose surfaceIdToPanelId lookups all return nil (since the map is cleared). If any deferred bonsplit delegate callback fires after teardown (e.g., from a prior closeTab call that was already in flight), the workspace's delegate will find empty lookups and silently return — so this is unlikely to crash. However, any code that continues to observe bonsplitController.tabs(inPane:) on the torn-down workspace will see stale tab entries.
Consider iterating over all bonsplit pane/tab IDs and calling bonsplitController.reset() (or equivalent) after the panel loop, or at minimum documenting that bonsplit cleanup is intentionally deferred to dealloc. This mirrors the fact that closePanel(_:force:) drives bonsplit closure, but teardownAllPanels() bypasses that path.
| XCTFail("Expected focused panel in new workspace") | ||
| return | ||
| } | ||
|
|
||
| workspace.setPanelCustomTitle(panelId: initialPanelId, title: "Initial custom title") | ||
| workspace.setPanelPinned(panelId: initialPanelId, pinned: true) |
There was a problem hiding this comment.
Pre-condition assertion relies on async title population
The test asserts XCTAssertFalse(workspace.panelTitles.isEmpty) before teardown, expecting that creating a workspace and splitting a panel will populate panelTitles synchronously. This holds today because panelTitles[terminalPanel.id] = terminalPanel.displayTitle is set inline during panel creation (Workspace.swift line ~1155). However, if displayTitle ever returns an empty string on creation and the real title arrives asynchronously (e.g., from a shell prompt update), this precondition assertion would silently pass with an empty dict — meaning the post-teardown XCTAssertTrue(workspace.panelTitles.isEmpty) would also trivially pass without actually validating teardown behavior.
Consider using an explicit XCTAssertGreaterThan(workspace.panelTitles.count, 0, "panelTitles should be populated before testing teardown") with a message to make the dependency visible, or directly seed workspace.panelTitles before the precondition check.
2 participants
Summary
TabManager.closeWorkspace()relied on ARC deallocation to free Ghostty surfaces, but retained references delayed/preventeddeinitTerminalSurface.teardownSurface()to eagerly free the Ghostty surface (sends SIGHUP), called fromTerminalPanel.close()Workspace.teardownAllPanels()which closes all panels and cleans up subscriptions/port scanner registrationsTabManager.closeWorkspace()now callsteardownAllPanels()before removing the workspace from the tabs arrayTest plan
ps -eo pid,comm | grep -E "zsh|claude"and verify no orphaned processes remainSummary by cubic
Prevents orphaned shell/Claude processes when closing workspace tabs by eagerly freeing Ghostty surfaces and tearing down all panels. SIGHUP is now delivered deterministically, and closing a non-owned workspace is ignored.
Written for commit 90fffd4. Summary will update on new commits.
Summary by CodeRabbit