Repository navigation
Fix browser and Codex helper lifecycle leaks - #4191
Closed
austinywang wants to merge 31 commits into
Closed
austinywang wants to merge 31 commits into
austinywang wants to merge 31 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughCentralizes WKWebView release teardown across panels and popups, introduces CmuxWebView and ReactGrab safe teardown hooks, ensures panels are torn down on app termination, and adds owner-PID propagation and an owner-exit watcher so the Codex monitor exits promptly; regression tests and reload cleanup were added for the monitor lifecycle. ChangesResource Lifecycle and Helper Process Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning, 2 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Line 18693: The current code silently converts malformed --owner-pid values to
nil (ownerPID) which later treats it as "alive"; change the parsing so that you
first check optionValue(commandArgs, name: "--owner-pid") for presence, then
attempt Int.init and validate the result is a positive integer; if the option is
present but parsing fails or value <= 0, print an error message and exit
non‑zero (fail fast) instead of treating it as nil. Update the ownerPID
initialization logic (the use of optionValue/ownerPID) to perform this explicit
presence+validation check and call the existing exit/error path when invalid
input is detected.
In `@Sources/AppDelegate.swift`:
- Line 1559: The code currently builds managers using the debug-filtered
allMainWindowTabManagersForDebug(), which can omit valid TabManager instances;
replace that call with the non-debug set of all registered tab managers (use the
project’s canonical "all registered" accessor instead of
allMainWindowTabManagersForDebug()), still append the optional tabManager via
[tabManager].compactMap{ $0 }, and then iterate those managers to call
teardownAllPanels() on each so no panels are skipped during termination.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34b78e2a-9165-46e4-965f-39388fd2ef4e
📒 Files selected for processing (9)
CLI/cmux.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView.swiftSources/Panels/ReactGrab.swiftSources/TerminalController.swiftscripts/reload.shtests/test_codex_feed_hooks.py
Contributor
Greptile SummaryThis PR fixes two categories of resource leaks: orphaned
Confidence Score: 5/5Safe to merge; teardown paths are idempotent and guarded, Codex monitor changes are well-tested, and the previously-flagged issues are addressed. Both the WebKit teardown and Codex monitor changes are carefully structured: idempotency flags prevent double-teardown, the DispatchSourceProcess watcher replaces the polling-only kill(0) check, and the prompt-submit lease/retire ordering is now correct. The one remaining note (no early title guard in the AppDelegate close-hook) is a minor hot-path optimisation, not a correctness issue. The regression test suite directly covers the failure modes this PR was introduced to fix. Sources/AppDelegate.swift — the NSWindow.close swizzle path and orderOutStaleBrowserWebInspectorWindowIfNeeded are the highest-surface-area additions and worth a quick read-through during QA. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[prompt-submit / codex hook] --> B{client.isRelayBacked?}
B -- Yes --> C[skip system.identify\nskip owner-PID]
B -- No --> D[codexMonitorOwnerPID\nsystem.identify RPC]
D -- nil returned --> E[log owner-unavailable\nskip monitor start]
D -- valid PID --> F[createCodexMonitorLease]
C --> F
F -- failure --> G[log lease-unavailable\nskip monitor start]
F -- leasePath --> H[retireCodexMonitorLeases\npreserving new lease]
H --> I[startCodexTranscriptMonitor\n--owner-pid only for local]
I --> J{monitor loop}
J -- ownerExitWatcher.hasExited --> K[exit: owner dead]
J -- lease retired --> L[exit: replaced by newer monitor]
J -- deadline --> M[exit: 4-hour max]
J -- wait --> N[waitForCodexTranscriptChange\nDispatchSourceFileSystemObject\nDispatchSourceProcess waker]
N --> J
Reviews (15): Last reviewed commit: "fix: keep popup inspector ownership duri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanel.swift (1)
3094-3100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset loading state when swapping out the current
WKWebView.These two replacement paths tear down the active web view without clearing the panel’s pending loading state. If the old view is replaced mid-navigation and the new view does not immediately emit a fresh loading cycle,
isLoadingcan stay stucktrueand the oldloadingEndWorkItemcan outlive the retired view.Suggested fix
webViewObservers.removeAll() webViewCancellables.removeAll() faviconTask?.cancel() faviconTask = nil faviconRefreshGeneration &+= 1 + loadingEndWorkItem?.cancel() + loadingEndWorkItem = nil + loadingGeneration &+= 1 + loadingStartedAt = nil + isLoading = false tearDownCurrentWebViewForRelease(previousWebView, reason: "profileSwitch")webViewObservers.removeAll() webViewCancellables.removeAll() faviconTask?.cancel() faviconTask = nil faviconRefreshGeneration &+= 1 + loadingEndWorkItem?.cancel() + loadingEndWorkItem = nil + loadingGeneration &+= 1 + loadingStartedAt = nil + isLoading = false tearDownCurrentWebViewForRelease(oldWebView, reason: reason)Also applies to: 3447-3452
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserPanel.swift` around lines 3094 - 3100, When replacing the current WKWebView (the code path that calls webViewObservers.removeAll(), webViewCancellables.removeAll(), faviconTask?.cancel(), faviconTask = nil, faviconRefreshGeneration &+= 1, and tearDownCurrentWebViewForRelease(previousWebView, reason: "profileSwitch")), also reset the panel’s loading state to avoid isLoading getting stuck: explicitly set isLoading = false and cancel/nil any pending loadingEndWorkItem (and clear any pending navigation-related state) so the old view’s loading callbacks cannot keep the panel marked as loading after the view is torn down; apply the same change to the other replacement site around lines 3447-3452.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3094-3100: When replacing the current WKWebView (the code path
that calls webViewObservers.removeAll(), webViewCancellables.removeAll(),
faviconTask?.cancel(), faviconTask = nil, faviconRefreshGeneration &+= 1, and
tearDownCurrentWebViewForRelease(previousWebView, reason: "profileSwitch")),
also reset the panel’s loading state to avoid isLoading getting stuck:
explicitly set isLoading = false and cancel/nil any pending loadingEndWorkItem
(and clear any pending navigation-related state) so the old view’s loading
callbacks cannot keep the panel marked as loading after the view is torn down;
apply the same change to the other replacement site around lines 3447-3452.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 819de641-396c-4b5b-9e65-e598aac5117b
📒 Files selected for processing (5)
CLI/cmux.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swifttests/test_codex_feed_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
20998-21015:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not spawn the monitor when owner PID discovery fails.
ownerPIDis optional here, so asystem.identifytimeout or malformed/missingapp_pidstill falls through tostartCodexTranscriptMonitor(...)without--owner-pid. That recreates the orphan-helper path this PR is trying to close on the failure path. Treat missing owner PID as a failed monitor launch and breadcrumb it instead of starting an unowned helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 20998 - 21015, The code currently calls startCodexTranscriptMonitor even when codexMonitorOwnerPID(client: client) returns nil; change the control flow so that if ownerPID is nil you do not spawn the monitor: emit a telemetry.breadcrumb (e.g., "codex-hook.monitor.owner-unavailable" or reuse the existing breadcrumb pattern) with has_owner_pid: false and any relevant context, and return/skip calling startCodexTranscriptMonitor; keep codexMonitorOwnerPID and startCodexTranscriptMonitor as the unique symbols to locate the change and ensure no --owner-pid-less monitor is started on owner PID discovery failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 18772-18780: Update the CLI error thrown when parsing --owner-pid
(in the block using optionValue(commandArgs, name: "--owner-pid") and assigning
ownerPID) to include the command name "cmux hooks codex monitor" and a concrete
recovery step; replace the current generic CLIError message with one that names
the failing command and suggests the user provide a positive integer (e.g.,
"cmux hooks codex monitor: invalid --owner-pid; please provide a positive
integer PID and retry") so the CLIError thrown by this parsing branch contains
both context and an explicit next action.
In `@tests/test_codex_feed_hooks.py`:
- Line 530: The test creates a subprocess with a hardcoded "/bin/sleep" binary
when assigning owner via subprocess.Popen([...]); replace the hardcoded path
with a portable lookup using shutil.which("sleep") (and fall back or assert if
None) so the subprocess uses the system sleep binary found at runtime; update
the owner = subprocess.Popen(...) call to use the resolved path variable instead
of "/bin/sleep" and mirror the same approach used at the earlier occurrence
around line 483.
- Line 483: The test uses a hardcoded "/bin/sleep" when launching the subprocess
(owner = subprocess.Popen(["/bin/sleep", "0.1"])); replace that hardcoded path
with a portable lookup using shutil.which("sleep") and handle the case where
which returns None (fail the test or skip). Update the subprocess.Popen call to
use the resolved path variable and add a clear error/skip if
shutil.which("sleep") returns None so the test remains portable across systems.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 20998-21015: The code currently calls startCodexTranscriptMonitor
even when codexMonitorOwnerPID(client: client) returns nil; change the control
flow so that if ownerPID is nil you do not spawn the monitor: emit a
telemetry.breadcrumb (e.g., "codex-hook.monitor.owner-unavailable" or reuse the
existing breadcrumb pattern) with has_owner_pid: false and any relevant context,
and return/skip calling startCodexTranscriptMonitor; keep codexMonitorOwnerPID
and startCodexTranscriptMonitor as the unique symbols to locate the change and
ensure no --owner-pid-less monitor is started on owner PID discovery failure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 394becfb-6899-4b8e-8256-63ff5a37a829
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_codex_feed_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 18772-18784: The code currently parses --owner-pid (ownerPIDRaw ->
parsedOwnerPID) and throws CLIError before registering the lease-file cleanup
defer, leaving the lease file behind on parse errors; move or add the lease
cleanup registration (the defer that removes the lease file) so it runs
unconditionally before parsing/validating ownerPID (i.e., ensure the defer that
cleans the lease file is installed immediately after the lease file is
created/recognized and before any guards that can throw), and apply the same
change to the analogous block referenced around ownerPID usage (the other
occurrence noted) so both error paths will always trigger the cleanup.
- Around line 20993-21023: The current code calls retireCodexMonitorLeases(...)
before codexMonitorOwnerPID(client:), causing the existing monitor to be torn
down if owner lookup times out; change the flow so you first call
codexMonitorOwnerPID(client:) and only if it returns a non-nil ownerPID then
call retireCodexMonitorLeases(sessionId:..., turnId: nil, env: ...) (or
otherwise delay retirement) and proceed to createCodexMonitorLease(...) and
startCodexTranscriptMonitor(...); update the block around codexMonitorOwnerPID,
retireCodexMonitorLeases, createCodexMonitorLease, and
startCodexTranscriptMonitor to ensure retirement happens after a successful
owner lookup.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 252472c5-c422-45ce-86ce-56ee1f1876d2
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_codex_feed_hooks.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3823-3830: Move the observer/cancellable cleanup to occur before
calling tearDownCurrentWebViewForRelease in close(): clear
webViewObservers.removeAll(), webViewCancellables.removeAll() (and cancel
loadingEndWorkItem if needed) prior to calling
tearDownCurrentWebViewForRelease(webView, reason: "panelClose") so any
KVO/Combine callbacks enqueued by stopLoading()/loadHTMLString("") cannot run
and repopulate state; keep the existing
navigationDelegate/uiDelegate/webViewDidRequestClose niling and
loadingEndWorkItem cancellation after teardown as appropriate.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e69ab04-bd66-4cc5-acdc-108439d1e15b
📒 Files selected for processing (3)
CLI/cmux.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanel.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:14045">
P2: The new global live-window check is too broad and can prevent stale Web Inspector windows from being ordered out whenever any panel uses detached devtools.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6679257. Configure here.
lawrencecchen
added
the
stale-revisit
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4188
Summary
Verification
ps: 103com.apple.WebKit.WebContenthelpers totaling about 10.5 GB RSS, 99 orphaned toppid=1; 9cmux hooks codex monitorhelpers totaling about 1.1 GB RSS, 5 orphaned--owner-pidCMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4188-memory-helper-leaks --launch.Note
Medium Risk
Touches app termination, NSWindow swizzling, and CLI monitor startup rules; mistakes could close the wrong inspector window or stop monitors early, but changes are scoped to teardown and helper lifecycle rather than auth or data paths.
Overview
Addresses orphaned WebKit helpers and lingering
cmux hooks codex monitorprocesses by tightening browser teardown and making Codex transcript monitors owner-aware.Browser / WebKit: Adds a shared, idempotent
WKWebViewrelease path (stop load, clear ReactGrab and paste handlers/user scripts, nil delegates,about:blank, remove from hierarchy) on panel close, profile/context resets, popup close/deinit, and app quit viateardownAllWorkspacePanelsBeforeAppTeardown.CmuxWebViewgainscmuxPrepareForReleaseTeardown()for closure/handler cleanup.Web Inspector: Stale detached “Web Inspector” windows are
orderOut(with ownership checks on panel/popup) instead of forcibly closed;NSWindow.close/ close-button handling routes throughorderOutStaleBrowserWebInspectorWindowIfNeededso live inspectors tied to a panel or popup are kept.Codex monitors:
system.identifyreturnsapp_pid; local sockets pass--owner-pidand exit when that process dies (DispatchSourceProcess+ faster 10s owner polls). Relay-backed sockets skip owner PID /system.identifyfor monitor start. Monitors are not started without a valid owner PID (local) or a successful lease;scripts/reload.shpkills lingering monitor helpers on tagged reload. Regression tests cover owner exit, relay behavior, lease failure, and invalid--owner-pid.Reviewed by Cursor Bugbot for commit f3fa5c1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes browser and Codex helper lifecycle leaks to reduce stray WebKit and monitor processes. Web views and popups now tear down idempotently, popup inspector ownership is preserved during close, stale Web Inspector windows are ordered out only when not owned, and Codex monitors are owner‑aware and exit fast.
WKWebViewteardown on profile/context reset, webview swap, popup close, panel close/deinit, and app quit. Stops loads, clears ReactGrab and paste‑as‑plain‑text handlers/user scripts, nils delegates, loads about:blank, removes the view;CmuxWebViewaddscmuxPrepareForReleaseTeardown. SwizzledNSWindow.closeandsendEventorderOut (not close) stale “Web Inspector” windows and hide them from the Windows menu; live detached inspectors owned by a panel or popup are preserved (ownership kept through popup close).system.identifynow returnsapp_pid. Local sockets require a valid owner PID and pass--owner-pid; relay sockets skip owner PID andsystem.identify. Monitors watch the owner with aDispatchSourceProcess, poll every 10s, wake filesystem waits, and exit promptly on owner death; start is skipped if owner PID is missing/invalid or lease write fails.scripts/reload.shreaps lingeringcmux hooks codex monitor. Tests cover relay owner‑PID skipping, invalid owner input, lease failure, and owner‑exit behavior.Written for commit f3fa5c1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores