fix: plug NotificationCenter observer accumulation and per-surface browser state leaks#2151
Conversation
…nresolvable Previously, v2SurfaceSendText, v2SurfaceSendKey, v2SurfaceClearHistory, and v2SurfaceReadText would silently fall back to ws.focusedPanelId when a caller supplied a surface_id that could not be resolved (e.g. a stale ref or an ordinal whose mapping had not yet been registered). This caused two distinct bugs: - manaflow-ai#2042: Commands like `cmux send --surface surface:9999` would succeed (exit 0) and deliver input to the focused pane instead of returning an error, making automation that targets specific surfaces unreliable. - manaflow-ai#2045: When the fallback landed on a browser panel, the subsequent ws.terminalPanel(for:) check failed and returned "Surface is not a terminal", making valid terminal surfaces appear broken when addressed by ref. The fix adds an explicit check: if params["surface_id"] is present but v2UUID() returns nil (resolution failure), we immediately return a not_found error instead of falling back to the focused pane. When surface_id is absent, the existing focused-pane fallback is preserved for backward compatibility. Fixes manaflow-ai#2042, Fixes manaflow-ai#2045 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…owser state leaks Leak 1 (GhosttyApp): Add deinit to GhosttyApp that removes all entries in appObservers via NotificationCenter.default.removeObserver, then clears the array. Although GhosttyApp.shared is a process-lifetime singleton today, the deinit ensures observers are balanced if the object is ever deallocated and prevents the observers from holding strong references beyond the app's intent. Leak 3 (TerminalController per-surface dictionaries): Add cleanupBrowserSurface(surfaceId:) to TerminalController that removes the per-surface entries from v2BrowserFrameSelectorBySurface, v2BrowserInitScriptsBySurface, v2BrowserInitStylesBySurface, v2BrowserDialogQueueBySurface, v2BrowserDownloadEventsBySurface, and v2BrowserUnsupportedNetworkRequestsBySurface. Call this from Workspace.splitTabBar(_:didCloseTab:fromPane:) alongside the existing per-surface cleanup so all six dictionaries are pruned when a surface is destroyed. Partial fix for manaflow-ai#2078 (Leak 2 and Leak 4 addressed separately). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@anthhub is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request adds lifecycle cleanup and resource management to prevent stale state after component teardown. A Changes
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)
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. Comment |
Greptile SummaryThis PR fixes two of four identified memory leaks:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Workspace
participant BonsplitController
participant TerminalController
participant NotificationCenter
Note over User,NotificationCenter: Surface close (tab path — FIXED)
User->>Workspace: Close tab
Workspace->>BonsplitController: splitTabBar(_:didCloseTab:fromPane:)
Workspace->>Workspace: Remove panelDirectories, surfaceTTYNames, etc.
Workspace->>TerminalController: cleanupBrowserSurface(surfaceId:) ✅
TerminalController->>TerminalController: Remove all 6 v2Browser*BySurface entries
Note over User,NotificationCenter: Surface close (pane path — STILL LEAKS)
User->>Workspace: Close pane
Workspace->>BonsplitController: splitTabBar(_:didClosePane:)
loop for each panelId in closedPanelIds
Workspace->>Workspace: Remove panelDirectories, surfaceTTYNames, etc.
Note right of Workspace: ❌ cleanupBrowserSurface NOT called
end
Note over User,NotificationCenter: GhosttyApp deinit (Leak 1 — FIXED)
TerminalController-->>GhosttyApp: (process exit / dealloc)
GhosttyApp->>NotificationCenter: removeObserver (didBecomeActive) ✅
GhosttyApp->>NotificationCenter: removeObserver (didResignActive) ✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/TerminalController.swift (1)
5293-5302: Extract sharedsurface_idresolution into one helper to prevent drift.The same parse/validate/fallback block is repeated four times. A single helper will keep behavior and error text consistent.
♻️ Refactor sketch
+private func v2ResolveExplicitOrFocusedSurfaceId( + params: [String: Any], + workspace: Workspace +) -> Result<UUID, V2CallResult> { + if params["surface_id"] != nil { + guard let surfaceId = v2UUID(params, "surface_id") else { + return .failure(.err( + code: "not_found", + message: "Surface not found for the given surface_id", + data: nil + )) + } + return .success(surfaceId) + } + guard let focused = workspace.focusedPanelId else { + return .failure(.err(code: "not_found", message: "No focused surface", data: nil)) + } + return .success(focused) +}Also applies to: 5353-5362, 5396-5405, 5456-5465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5293 - 5302, Extract the repeated parse/validate/fallback logic for surface_id into a single helper (e.g., a private method on TerminalController like resolveSurfaceId(params: [String: Any], ws: WebSocket) -> Result<UUID, ErrorResult> or optional UUID with an error-out pattern) and replace the four duplicated blocks that set surfaceId (currently using v2UUID(params, "surface_id"), ws.focusedPanelId, and producing result = .err(code: "not_found", message: "Surface not found for the given surface_id", data: nil)) with calls to that helper; the helper should: check params["surface_id"], call v2UUID(params, "surface_id") when present and return the same not_found error when v2UUID yields nil, or return ws.focusedPanelId when no param was provided, preserving the exact error text and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 222-229: The cleanupBrowserSurface function currently removes many
per-surface caches but omits v2BrowserElementRefs; update
cleanupBrowserSurface(surfaceId: UUID) to also remove the entry from
v2BrowserElementRefs by calling v2BrowserElementRefs.removeValue(forKey:
surfaceId) so stale element references are pruned when a surface is cleaned up.
In `@Sources/Workspace.swift`:
- Line 10200: The pane-close flow currently bypasses the didCloseTab path and
therefore doesn't call
TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId), leaving
browser-surface resources leaked; update the pane-close handling (the method
that closes panes—look for the pane-close handler / function that removes
panels) to invoke TerminalController.shared.cleanupBrowserSurface(surfaceId:
panelId) for any panelId/ surfaceId being removed (mirror the call made in
didCloseTab), ensuring cleanup runs both on tab-close and pane-close code paths.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 5293-5302: Extract the repeated parse/validate/fallback logic for
surface_id into a single helper (e.g., a private method on TerminalController
like resolveSurfaceId(params: [String: Any], ws: WebSocket) -> Result<UUID,
ErrorResult> or optional UUID with an error-out pattern) and replace the four
duplicated blocks that set surfaceId (currently using v2UUID(params,
"surface_id"), ws.focusedPanelId, and producing result = .err(code: "not_found",
message: "Surface not found for the given surface_id", data: nil)) with calls to
that helper; the helper should: check params["surface_id"], call v2UUID(params,
"surface_id") when present and return the same not_found error when v2UUID
yields nil, or return ws.focusedPanelId when no param was provided, preserving
the exact error text and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 067b8546-8cd2-437a-80f8-7a55e990005a
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/Workspace.swift
| func cleanupBrowserSurface(surfaceId: UUID) { | ||
| v2BrowserFrameSelectorBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserInitScriptsBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) | ||
| } |
There was a problem hiding this comment.
Prune stale browser element refs during surface cleanup.
Line 222 clears most per-surface browser caches, but v2BrowserElementRefs is left behind. That can accumulate stale entries across repeated browser surface churn.
🧹 Proposed fix
`@MainActor`
func cleanupBrowserSurface(surfaceId: UUID) {
v2BrowserFrameSelectorBySurface.removeValue(forKey: surfaceId)
v2BrowserInitScriptsBySurface.removeValue(forKey: surfaceId)
v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId)
v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId)
v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId)
v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId)
+ v2BrowserElementRefs = v2BrowserElementRefs.filter { $0.value.surfaceId != surfaceId }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func cleanupBrowserSurface(surfaceId: UUID) { | |
| v2BrowserFrameSelectorBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserInitScriptsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) | |
| } | |
| func cleanupBrowserSurface(surfaceId: UUID) { | |
| v2BrowserFrameSelectorBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserInitScriptsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserElementRefs = v2BrowserElementRefs.filter { $0.value.surfaceId != surfaceId } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 222 - 229, The
cleanupBrowserSurface function currently removes many per-surface caches but
omits v2BrowserElementRefs; update cleanupBrowserSurface(surfaceId: UUID) to
also remove the entry from v2BrowserElementRefs by calling
v2BrowserElementRefs.removeValue(forKey: surfaceId) so stale element references
are pruned when a surface is cleaned up.
| } | ||
| clearRemoteConfigurationIfWorkspaceBecameLocal() | ||
| AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId) | ||
| TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId) |
There was a problem hiding this comment.
Apply browser-surface cache cleanup in pane-close flow as well
Line 10200 fixes tab-close cleanup, but pane-close cleanup can bypass didCloseTab and currently does not call cleanupBrowserSurface(surfaceId:). That leaves a remaining leak path for browser panels closed via pane close.
💡 Proposed fix
func splitTabBar(_ controller: BonsplitController, didClosePane paneId: PaneID) {
@@
if !closedPanelIds.isEmpty {
for panelId in closedPanelIds {
+ TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId)
panels[panelId]?.close()
panels.removeValue(forKey: panelId)
untrackRemoteTerminalSurface(panelId)
pendingRemoteTerminalChildExitSurfaceIds.remove(panelId)
panelDirectories.removeValue(forKey: panelId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` at line 10200, The pane-close flow currently
bypasses the didCloseTab path and therefore doesn't call
TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId), leaving
browser-surface resources leaked; update the pane-close handling (the method
that closes panes—look for the pane-close handler / function that removes
panels) to invoke TerminalController.shared.cleanupBrowserSurface(surfaceId:
panelId) for any panelId/ surfaceId being removed (mirror the call made in
didCloseTab), ensuring cleanup runs both on tab-close and pane-close code paths.
|
Closing this PR — the Leak 1 fix (deinit on a static singleton) is ineffective since GhosttyApp is |
![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 3 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:228">
P2: cleanupBrowserSurface clears per-surface dictionaries but leaves v2BrowserElementRefs entries tied to the surfaceId, so element refs allocated for a closed browser surface are never released and can grow unbounded across create/close cycles.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:10200">
P1: Apply `cleanupBrowserSurface(surfaceId:)` in the pane-close flow as well. Surfaces closed through `didClosePane` can currently skip browser cache teardown and retain per-surface state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| clearRemoteConfigurationIfWorkspaceBecameLocal() | ||
| AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId) | ||
| TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId) |
There was a problem hiding this comment.
P1: Apply cleanupBrowserSurface(surfaceId:) in the pane-close flow as well. Surfaces closed through didClosePane can currently skip browser cache teardown and retain per-surface state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 10200:
<comment>Apply `cleanupBrowserSurface(surfaceId:)` in the pane-close flow as well. Surfaces closed through `didClosePane` can currently skip browser cache teardown and retain per-surface state.</comment>
<file context>
@@ -10197,6 +10197,7 @@ extension Workspace: BonsplitDelegate {
}
clearRemoteConfigurationIfWorkspaceBecameLocal()
AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId)
+ TerminalController.shared.cleanupBrowserSurface(surfaceId: panelId)
// Keep the workspace invariant for normal close paths.
</file context>
| v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId) | ||
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) |
There was a problem hiding this comment.
P2: cleanupBrowserSurface clears per-surface dictionaries but leaves v2BrowserElementRefs entries tied to the surfaceId, so element refs allocated for a closed browser surface are never released and can grow unbounded across create/close cycles.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalController.swift, line 228:
<comment>cleanupBrowserSurface clears per-surface dictionaries but leaves v2BrowserElementRefs entries tied to the surfaceId, so element refs allocated for a closed browser surface are never released and can grow unbounded across create/close cycles.</comment>
<file context>
@@ -216,6 +216,18 @@ class TerminalController {
+ v2BrowserInitStylesBySurface.removeValue(forKey: surfaceId)
+ v2BrowserDialogQueueBySurface.removeValue(forKey: surfaceId)
+ v2BrowserDownloadEventsBySurface.removeValue(forKey: surfaceId)
+ v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId)
+ }
+
</file context>
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserUnsupportedNetworkRequestsBySurface.removeValue(forKey: surfaceId) | |
| v2BrowserElementRefs = v2BrowserElementRefs.filter { $0.value.surfaceId != surfaceId } |
1 participant
Summary
Partial fix for #2078 — addresses Leak 1 and Leak 3 only. Leak 2 (AppDelegate observer imbalance) and Leak 4 (BrowserProfileStore eviction) are left for a follow-up PR.
Leak 1 — GhosttyApp NotificationCenter observers never removed
GhosttyAppregistered twoNSApplicationactivation observers viaNotificationCenter.default.addObserver(forName:object:queue:)duringinitializeGhostty(), storing them inappObservers. The class had nodeinit, so these block-based observers were never unregistered and the token objects were never released.Fix: Add
deinittoGhosttyAppthat iteratesappObservers, callsNotificationCenter.default.removeObserveron each token, then clears the array. WhileGhosttyApp.sharedis a process-lifetime singleton, the deinit ensures observers are always balanced if the object is ever deallocated and makes the lifecycle intent explicit.Leak 3 — TerminalController per-surface browser dictionaries never cleared
Six
[UUID: …]dictionaries inTerminalControlleraccumulate state keyed by browser-surface ID:v2BrowserFrameSelectorBySurfacev2BrowserInitScriptsBySurfacev2BrowserInitStylesBySurfacev2BrowserDialogQueueBySurfacev2BrowserDownloadEventsBySurfacev2BrowserUnsupportedNetworkRequestsBySurfaceEntries were written when surfaces were created but never removed when surfaces were destroyed, causing unbounded growth over sessions with many workspace/surface creation cycles.
Fix: Add
cleanupBrowserSurface(surfaceId:)toTerminalControllerthat callsremoveValue(forKey:)on all six dictionaries. Invoke it fromWorkspace.splitTabBar(_:didCloseTab:fromPane:)— the canonical surface-destruction callback — alongside the existing per-surface cleanup block (e.g.panelDirectories,surfaceTTYNames, etc.).Test plan
_NSObserverTarget)🤖 Generated with Claude Code
Summary by cubic
Fixes two memory leaks by unregistering
NotificationCenterobservers and clearing per-surface browser state on tab close. Also makessurface_idhandling strict by returningnot_foundwhen provided but unresolvable to prevent misrouted commands.Bug Fixes
NotificationCenterobservers indeinitto stop observer/token accumulation. (Leak 1)surface_idis present but cannot be resolved, returnnot_foundinstead of falling back to the focused pane; nosurface_idstill falls back. Fixes send/send-key/read-screen silently fall back to focused pane when given a non-existent surface handle #2042, send/read-screen/send-key --surface surface:N errors 'not a terminal' for valid terminals #2045.Migration
surface_idmust ensure it’s valid; stale or unknown IDs now returnnot_found.surface_idinstead of relying on implicit fallback.Written for commit 53832d5. Summary will update on new commits.
Summary by CodeRabbit