fix: clean up per-surface state on tab close to prevent unbounded memory growth#2192
fix: clean up per-surface state on tab close to prevent unbounded memory growth#2192anthhub wants to merge 5 commits intomanaflow-ai:mainfrom
Conversation
|
@anthhub is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant NotificationStore
participant TerminalController
Workspace->>NotificationStore: clearNotifications(panelId)
NotificationStore-->>Workspace: cleared
alt close is not detaching
Workspace->>TerminalController: cleanupSurfaceState(surfaceId)
TerminalController-->>TerminalController: remove per-surface maps & refs
TerminalController-->>Workspace: cleanupComplete
else detaching/move
Workspace-->>TerminalController: (skip cleanup for detach/move)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
5784-5785:⚠️ Potential issue | 🟠 MajorGate browser-state cleanup to true destruction paths and cover pane-close closures
cleanupBrowserSurfaceStateis unconditional at Line 10301, but detached tabs (isDetachingat Line 10223) are transferred, not destroyed (see transfer path at Line 10244). That can drop browser command state during moves.
Also, pane-close flow explicitly bypasses per-tab callbacks (Line 5784), anddidClosePanecurrently does not invoke browser-surface cleanup, so leak coverage is incomplete.💡 Proposed fix
@@ - AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId) - TerminalController.shared.cleanupBrowserSurfaceState(surfaceId: panelId) + AppDelegate.shared?.notificationStore?.clearNotifications(forTabId: id, surfaceId: panelId) + if !isDetaching { + TerminalController.shared.cleanupBrowserSurfaceState(surfaceId: panelId) + } @@ func splitTabBar(_ controller: BonsplitController, didClosePane paneId: PaneID) { @@ if !closedPanelIds.isEmpty { for panelId in closedPanelIds { panels[panelId]?.close() + TerminalController.shared.cleanupBrowserSurfaceState(surfaceId: panelId) panels.removeValue(forKey: panelId) untrackRemoteTerminalSurface(panelId)Also applies to: 10223-10301, 10423-10466
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5784 - 5785, cleanupBrowserSurfaceState is being invoked unconditionally which drops browser command state during tab transfers (when isDetaching is true) and pane-close flow bypasses per-tab didClose callbacks; modify the logic so cleanupBrowserSurfaceState is only called on true destruction paths (guard against isDetaching/transfer paths in the code around cleanupBrowserSurfaceState), ensure didClosePane triggers browser-surface cleanup for genuinely closed tabs, and wire the pendingPaneClosePanelIds map into didClosePane to emit per-tab didClose callbacks before removing state so pane-close closures are covered; update references: cleanupBrowserSurfaceState, isDetaching, didClosePane, and pendingPaneClosePanelIds.
🤖 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 201-211: cleanupBrowserSurfaceState(surfaceId:) currently removes
many per-surface stores but misses v2BrowserElementRefs, leaving stale element
references; update the function (cleanupBrowserSurfaceState) to also remove the
entry for the given surfaceId from v2BrowserElementRefs (i.e., call
removeValue(forKey: surfaceId) on v2BrowserElementRefs) so all per-surface state
is purged when a surface is destroyed.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 5784-5785: cleanupBrowserSurfaceState is being invoked
unconditionally which drops browser command state during tab transfers (when
isDetaching is true) and pane-close flow bypasses per-tab didClose callbacks;
modify the logic so cleanupBrowserSurfaceState is only called on true
destruction paths (guard against isDetaching/transfer paths in the code around
cleanupBrowserSurfaceState), ensure didClosePane triggers browser-surface
cleanup for genuinely closed tabs, and wire the pendingPaneClosePanelIds map
into didClosePane to emit per-tab didClose callbacks before removing state so
pane-close closures are covered; update references: cleanupBrowserSurfaceState,
isDetaching, didClosePane, and pendingPaneClosePanelIds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a889ccc5-0c42-4124-ba71-7b1d36384b91
📒 Files selected for processing (2)
Sources/TerminalController.swiftSources/Workspace.swift
Greptile SummaryThis PR fixes a memory leak (issue #2078 Leak 3) by adding Confidence Score: 3/5Not safe to merge as-is; the missing The core memory-leak fix is correct and well-structured, but calling Sources/Workspace.swift — the unconditional call to Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Bonsplit
participant Workspace
participant TerminalController
User->>Bonsplit: Close tab (normal close)
Bonsplit->>Workspace: splitTabBar(_:didCloseTab:fromPane:)
Note over Workspace: isDetaching = false
Workspace->>Workspace: panels.removeValue(panelId)
Workspace->>Workspace: clearNotifications(forTabId:surfaceId:)
Workspace->>TerminalController: cleanupBrowserSurfaceState(surfaceId:) ✅
TerminalController->>TerminalController: removeValue x6 dictionaries
User->>Bonsplit: Detach tab (drag to new window)
Bonsplit->>Workspace: splitTabBar(_:didCloseTab:fromPane:)
Note over Workspace: isDetaching = true
Workspace->>Workspace: pendingDetachedSurfaces[tabId] = DetachedSurfaceTransfer
Workspace->>Workspace: panels.removeValue(panelId) [workspace-local, OK]
Workspace->>Workspace: clearNotifications(forTabId:surfaceId:)
Workspace->>TerminalController: cleanupBrowserSurfaceState(surfaceId:) ⚠️ BUG
Note over TerminalController: Init scripts, styles, frame selector wiped from singleton
Bonsplit->>Workspace: attachDetachedSurface (new workspace)
Note over Workspace: Browser state gone — init scripts won't replay
Reviews (1): Last reviewed commit: "fix: clean up per-surface browser dictio..." | Re-trigger Greptile |
![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 2 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:204">
P2: cleanupBrowserSurfaceState doesn’t clear v2BrowserElementRefs entries for the closed surface, so element refs allocated by snapshot/find APIs remain indefinitely and can still grow unbounded across tab closes.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:10301">
P2: Browser per-surface runtime state is cleaned during detach/move close path, causing transferred browser panels to lose state before reattach.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
anthhub
changed the title
There was a problem hiding this comment.
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 `@Sources/Workspace.swift`:
- Around line 10301-10303: The current cleanup call
TerminalController.shared.cleanupSurfaceState(surfaceId: panelId) is only
invoked in the didCloseTab path when !isDetaching, leaving per-surface
dictionaries unchanged for pane-close and workspace teardown flows; update the
pane-close and workspace-teardown code paths (and any panel deinit/teardown
handlers) to call TerminalController.shared.cleanupSurfaceState(surfaceId:
panelId) unconditionally when a panel is destroyed (or ensure the existing
didCloseTab branch runs for those flows), so per-surface state is removed
regardless of isDetaching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Gentle ping on this one — it fixes a memory leak where per-surface state grows unbounded as tabs are closed. Pretty small change if anyone has a few minutes to look. |
|
Hi @austinywang @lawrencecchen 👋 Gentle ping on this one — it fixes unbounded memory growth from per-surface state not being cleaned up on tab close. Would appreciate a review when you get a chance! |
|
Quick triage request on this PR. This one still seems valuable to me because it targets the per-surface cleanup / memory-growth issue, but it is now conflict-stale. If maintainers still want this direction, I can rebase it and refresh the branch; if not, I am happy to close it rather than leave it hanging. A short yes/no on whether this is still worth reviving would be enough. |
Six per-surface dictionaries in TerminalController (v2BrowserFrameSelectorBySurface, v2BrowserInitScriptsBySurface, v2BrowserInitStylesBySurface, v2BrowserDialogQueueBySurface, v2BrowserDownloadEventsBySurface, v2BrowserUnsupportedNetworkRequestsBySurface) are populated when browser panels interact with socket commands but never cleaned up when the surface is destroyed. Over long sessions with many workspaces created and closed, these dictionaries grow without bound. Add TerminalController.cleanupBrowserSurfaceState(surfaceId:) and call it from Workspace.splitTabBar(_:didCloseTab:fromPane:) alongside the existing panel dictionary cleanup. Partially addresses manaflow-ai#2078 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…UID maps 扩展 cleanupBrowserSurfaceState → cleanupSurfaceState，追加两类额外清理： 1. v2BrowserElementRefs：过滤掉 surfaceId 匹配的条目。surface 关闭后这些 refs 永远无法被解析（entry.surfaceId == surfaceId guard 必然失败）， 继续保留只会造成无界增长。 2. v2RefByUUID[.surface] / v2UUIDByRef[.surface]：surface 创建时写入双向 映射，但关闭时从未清理；v2RefreshKnownRefs() 只添加不删除。先查出 ref string 再双向移除，不影响其他 kind（.workspace/.pane/.window）。 同步将方法改名为 cleanupSurfaceState，因为清理范围已不限于 browser 状态， 并更新调用方 Workspace.splitTabBar(_:didCloseTab:fromPane:)。 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Surface state must not be cleaned up when a tab is being detached (dragged to a new window), as the surface continues to live in the new window. Only clean up on genuine tab close. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and pane-close paths
anthhub
force-pushed
the
fix/per-surface-dict-leak
branch
from
1685946 to
6e0b0bf
Compare
|
Rebased this onto current main and resolved the Workspace.swift conflicts. I kept the later pending-input / restored-agent cleanup from main and preserved the original !isDetaching guard. I couldn’t do a full local build in this checkout because GhosttyKit.xcframework isn’t present here, but the branch should be back in reviewable shape. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/TerminalController.swift (1)
204-211: Doc comment understates the caller set.The comment names only
Workspace.splitTabBar(_:didCloseTab:fromPane:), but per the PR summary this method is also invoked fromteardownAllPanels,splitTabBar(_:didClosePane:), and theclosePanelflow (seeSources/Workspace.swift:12213). Worth broadening to avoid future readers assuming a single call site.📝 Proposed doc tweak
/// Remove all per-surface state for a destroyed surface. - /// Called from Workspace.splitTabBar(_:didCloseTab:fromPane:) to prevent unbounded dictionary growth. + /// Called from Workspace teardown/close flows (e.g. closePanel, splitTabBar(_:didCloseTab:fromPane:) + /// when not detaching, splitTabBar(_:didClosePane:), teardownAllPanels) to prevent unbounded + /// dictionary growth. Must not be called for detach/move closes — the surface lives on elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 204 - 211, The doc comment for cleanupSurfaceState(surfaceId:) currently only lists Workspace.splitTabBar(_:didCloseTab:fromPane:) as a caller but misses other call sites; update the comment to enumerate all known callers (e.g., teardownAllPanels, splitTabBar(_:didClosePane:), the closePanel flow referenced in Workspace.swift) or state that it’s called from multiple teardown/close flows so readers don’t assume a single call site, keeping the existing bullet list of cleaned-up state intact and mentioning why removal is necessary for each caller category.
🤖 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/TerminalController.swift`:
- Around line 204-211: The doc comment for cleanupSurfaceState(surfaceId:)
currently only lists Workspace.splitTabBar(_:didCloseTab:fromPane:) as a caller
but misses other call sites; update the comment to enumerate all known callers
(e.g., teardownAllPanels, splitTabBar(_:didClosePane:), the closePanel flow
referenced in Workspace.swift) or state that it’s called from multiple
teardown/close flows so readers don’t assume a single call site, keeping the
existing bullet list of cleaned-up state intact and mentioning why removal is
necessary for each caller category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a6bfe89-3bd0-4be9-a89f-5d620f110c88
📒 Files selected for processing (2)
Sources/TerminalController.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/Workspace.swift
1 participant
Problem
TerminalControlleraccumulates per-surface state that is never cleaned up when a surface is destroyed:v2BrowserFrameSelectorBySurface,v2BrowserInitScriptsBySurface,v2BrowserInitStylesBySurface,v2BrowserDialogQueueBySurface,v2BrowserDownloadEventsBySurface,v2BrowserUnsupportedNetworkRequestsBySurface) — written during browser/socket interactions, never removedv2BrowserElementRefs— element references keyed by surface ID, never pruned after surface close (stale refs can never be resolved anyway per the guard at L6899)v2RefByUUID[.surface]/v2UUIDByRef[.surface]— handle mappings written on creation, only added-to byv2RefreshKnownRefs()but never removedOver long sessions with many workspaces created and closed, these dictionaries grow without bound.
Fix
Sources/TerminalController.swift: AddcleanupSurfaceState(surfaceId:)that clears all 8 categories of per-surface data in one call.Sources/Workspace.swift: CallcleanupSurfaceStatefromsplitTabBar(_:didCloseTab:fromPane:)alongside the existing panel dictionary cleanup.Note on Leak 1 from #2078
The reported observer accumulation in
GhosttyAppdoes not exist —appObservers.appendruns only insideinitializeGhostty(), which is called once fromprivate init(). SinceGhosttyAppisstatic let shared, the observer count is always exactly 2.Test plan
Partially addresses #2078
Summary by CodeRabbit