Repository navigation
Count cmux app RSS in Task Manager totals - #3587
Merged
Merged
Conversation
The Task Manager rollup is expected to match the RSS that macOS reports for the cmux application process tree. This regression fixture starts a real parent process with child processes, annotates a synthetic top window whose children are already represented, and compares the window rollup to ps for the full recursive tree. Constraint: Local tests are intentionally not run in this workspace; CI owns test execution.\nConfidence: high\nScope-risk: narrow\nDirective: Keep this as a runtime process-tree test; do not replace it with source-shape assertions.\nTested: Not run locally by policy.\nNot-tested: CI failure observed for test-only commit.
The Task Manager and cmux top rollup previously started from pane, tag, and browser roots only. That made the main SwiftUI app process invisible even though macOS attributes its RSS to cmux, so a leak in view state or scrollback storage could dominate system memory while the Task Manager still reported only child process usage.\n\nThis makes the app process root a first-class input to the existing system.top rollup and lets the snapshot expand it through the same process-tree/set-union accounting used for terminal and agent roots. Constraint: Keep CPU behavior untouched for #3583.\nRejected: Add a Task Manager-only adjustment | would leave cmux top and UI using different accounting rules.\nConfidence: high\nScope-risk: narrow\nDirective: Keep global resource accounting in system.top; do not add UI-only memory math.\nTested: Not run locally by policy; regression test added in prior commit for CI.\nNot-tested: Force Quit grouping of WebKit GPU/Networking XPC helpers, which are not descendants of the cmux app process.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAttaches the running application's PID into top-level window payloads in two payload paths, updates window annotation to derive and use per-window ChangesAttach application PID and revise top-window rollup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Merged origin/main so the PR runs against the latest CI and workflow state before handling review and check feedback. The merge only brings upstream workflow/test files and leaves the Task Manager memory accounting changes untouched. Constraint: iterate-pr requires syncing the PR branch with its base before handling CI feedback Confidence: high Scope-risk: narrow Tested: Not run locally per repository policy Not-tested: CI pending
Contributor
Greptile SummaryThis PR fixes a long-standing omission where the cmux application process PID was excluded from Task Manager and
Confidence Score: 5/5Safe to merge. The rollup logic change is well-scoped, the key-window attribution is correct, and both call sites in TerminalController attach the app PID exactly once before annotation. The app PID is attached to the key window before the snapshot is consumed, and the rootPIDs scope parameter in summaryPayload prevents the app process tree from absorbing other windows' pane children. Both call sites sequence the attach correctly. Previously flagged issues are resolved. The regression tests exercise a real forked process tree and compare against ps, which is a strong end-to-end signal. No actor isolation regressions, no blocking primitives in production code, and no new mutable singletons or side channels. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant TC as TerminalController
participant TCTS as v2AttachTopApplicationProcess
participant TCTA as v2AnnotateTopWindows
participant Snap as CmuxTopProcessSnapshot
TC->>TCTS: windowNodes (built from listMainWindowSummaries)
TCTS->>TCTS: getpid() to appProcessID
TCTS->>TCTS: find key window (or fallback to index 0)
TCTS-->>TC: "windowNodes[keyIdx][app_process_pids]=[appPID], others=[]"
TC->>Snap: capture(includeProcessDetails:) [Task.detached]
Snap-->>TC: processSnapshot
TC->>TCTA: "v2AnnotateTopWindows(&windowNodes, processSnapshot)"
loop each window
TCTA->>TCTA: "appProcessPIDs = window[app_process_pids]"
TCTA->>TCTA: "windowPIDs = appProcessPIDs union workspace PIDs"
TCTA->>Snap: topLevelPIDs(for: appProcessPIDs)
Snap-->>TCTA: app top-level PIDs to windowTopLevelPIDs
TCTA->>Snap: summaryPayload(for: windowPIDs, rootPIDs: appProcessPIDs)
Snap-->>TCTA: resources (RSS scoped, no cross-window child bleed)
end
TCTA-->>TC: allPIDs (includes appPID via key window)
TC->>Snap: summaryPayload(for: allPIDs) to totals (includes app RSS)
Reviews (9): Last reviewed commit: "Clarify main-thread image assertions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The app process must be counted once for global Task Manager totals, but assigning it to array index 0 made per-window resources shift whenever window ordering changed. Attach the app PID to the key window when one is present, then fall back to the first window only when no key window is marked. Constraint: Do not double-count the app RSS across windows Rejected: Keep first-window attribution | review feedback showed it makes per-window totals arbitrary in multi-window sessions Confidence: high Scope-risk: narrow Tested: Added unit coverage for key-window attribution and first-window fallback Not-tested: Local tests not run per repository policy
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The PR branch needed the latest main before handling review feedback and rerunning CI. The merge applied cleanly; the only follow-up was updating the bonsplit submodule checkout to the merged tree's recorded pointer. Constraint: iterate-pr workflow requires merging the base branch before CI and review-feedback iteration Confidence: high Scope-risk: moderate Tested: ./scripts/reload.sh --tag issue-3584-task-manager-memory-undercount reported success before review-fix edits Not-tested: Local test suite per repository policy
Per-window Task Manager resources should attribute the cmux app process once without letting that app root recursively claim terminal descendants owned by other windows. The window annotation now starts from direct app PIDs and still unions each workspace/pane/surface root for global totals. Constraint: Preserve global system.top totals while keeping per-window resources additive enough for multi-window inspection Rejected: Keep expanding the app PID root | it inflates the key window with processes already attributed to other windows Confidence: high Scope-risk: narrow Directive: Do not expand app_process_pids at the window level unless per-window attribution is redesigned Tested: git diff --check -- Sources/TerminalControllerTopSupport.swift cmuxTests/CmuxTopSnapshotScopeTests.swift Not-tested: Local XCTest run per repository policy; tagged reload was blocked behind an unrelated xcodebuild lock when committing
![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 `@cmuxTests/CmuxTopSnapshotScopeTests.swift`:
- Around line 251-258: The terminate() method currently kills childPIDs and
terminates process but doesn't remove the per-run temp directory; update the
fixture to store the temp directory URL (the cmux-top-<UUID> dir) as a property
on the same struct that defines terminate(), then add a safe cleanup step in
terminate() that calls try? FileManager.default.removeItem(at: tempDirectoryURL)
(or the stored URL) after killing processes and terminating the process so the
temporary cmux-top-<UUID> directory is deleted on both success and failure paths
without throwing.
- Around line 233-249: The start() function can throw after spawning the python3
Process and creating the temp directory, leaking the process and temp dir if
waitForChildPIDs throws; modify start() to ensure cleanup on any throw by
wrapping the waitForChildPIDs call in a do/catch (or using defer) that calls
process.terminate() and removes the temp directory before rethrowing, and update
SpawnedProcessTree and its terminate() to accept and store the workingDirectory
(or URL) so the normal return path also removes the temp dir; reference start(),
waitForChildPIDs, SpawnedProcessTree, and terminate() when making these changes.
🪄 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: d2e128bc-6bac-4af0-9772-c34ffe4df0c4
📒 Files selected for processing (3)
Sources/TerminalController.swiftSources/TerminalControllerTopSupport.swiftcmuxTests/CmuxTopSnapshotScopeTests.swift
The Task Manager RSS regression fixture creates a temporary Python process tree and per-run directory. Review feedback pointed out that setup failures and successful runs could leave those resources behind, so the fixture now owns the directory and cleans it on both normal termination and setup error paths. Constraint: Keep the change confined to test scaffolding; production process attribution is unchanged Rejected: Ignore as low-priority test cleanup | CI skips can otherwise leak subprocesses and temp files across runs Confidence: high Scope-risk: narrow Tested: git diff --check -- cmuxTests/CmuxTopSnapshotScopeTests.swift Not-tested: Local XCTest run per repository policy
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 `@cmuxTests/CmuxTopSnapshotScopeTests.swift`:
- Around line 248-250: The test is racing with child processes still performing
touch(...) memory writes after waitForChildPIDs returns; update the
SpawnedProcessTree/start() path to wait until the fixture signals memory-touch
completion or poll the process tree RSS until it stabilizes before returning:
modify waitForChildPIDs (or the caller that constructs
SpawnedProcessTree(process:..., childPIDs:..., directory:...)) to either read a
new readiness file written by the fixture when both children finish touching
memory or loop sampling the aggregated RSS for the parent+children (using the
same PID list from waitForChildPIDs) until successive samples differ by less
than the tolerance, then return. Ensure the change is applied to the other
similar call sites noted (around lines 285-297 and 312-325).
🪄 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: f32f00b5-ddad-4caa-bee7-0b628d540825
📒 Files selected for processing (1)
cmuxTests/CmuxTopSnapshotScopeTests.swift
…er-memory-undercount
The Task Manager memory regression fixture was publishing child PIDs before those children had finished faulting their resident pages. Make the fixture own a readiness file that is written only after each child allocation is touched, and keep startup cleanup in the same owner so skipped setup cannot leak temporary files or subprocesses. Constraint: Repository policy forbids local test runs; CI remains the validation surface for XCTest. Rejected: Poll recursive RSS from Swift | couples fixture readiness to the same ps sampling path used by the assertion and preserves timing sensitivity. Confidence: high Scope-risk: narrow Directive: Keep fixture readiness tied to a child-emitted completion signal, not an arbitrary sleep or RSS threshold. Tested: git diff --check Not-tested: Local XCTest execution, per repository policy.
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 `@cmuxTests/CmuxTopSnapshotScopeTests.swift`:
- Around line 379-383: The test deadlocks because process.waitUntilExit() is
called before draining the pipe; move the drain so you call
pipe.fileHandleForReading.readDataToEndOfFile() immediately after process.run()
to read the child's stdout into a Data buffer, then call
process.waitUntilExit(), and finally convert that Data to String; update the
block containing process.run(), process.waitUntilExit(), and
pipe.fileHandleForReading.readDataToEndOfFile() accordingly.
🪄 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: c774794f-5ba1-4125-9996-c5b3b7f04086
📒 Files selected for processing (1)
cmuxTests/CmuxTopSnapshotScopeTests.swift
The regression fixture compares captured RSS with ps output. Reading stdout before waiting lets ps make progress even when process lists exceed the pipe buffer, and the termination status now becomes an explicit skip instead of silent empty output. Constraint: Repository policy forbids local test runs; CI remains the validation surface for XCTest. Rejected: Keep wait-before-drain ordering | can deadlock if ps fills stdout before exit. Confidence: high Scope-risk: narrow Directive: When spawning Process with a Pipe, drain stdout/stderr concurrently or before waitUntilExit when output can be non-trivial. Tested: git diff --check Not-tested: Local XCTest execution, per repository policy.
The dock tile plugin could receive distributed icon-change notifications on whichever thread posted them, then read AppKit appearance state, update bundle icons, and replace the dock tile image view from that thread. That matches the observed CoreAnimation transaction committing an NSImageView layer during worker-thread teardown. Route dock tile setup and updates through the main queue before any AppKit, NSWorkspace, LaunchServices, or NSDockTile work. Add DEBUG-only main-thread tripwires around nearby NSImageView image paths so any future off-main caller fails at the violating site instead of later in CoreAnimation cleanup. Constraint: Issue #3590 has a crash signature but no deterministic repro. Rejected: Move every NSImage creation to main | many existing callers are already AppKit delegate/SwiftUI entrypoints, and doing so would add churn without evidence. Rejected: Refresh the Swift file-length budget | the tripwire calls can fit without increasing the tracked large-file line count. Confidence: medium Scope-risk: narrow Directive: Keep NSImageView image/layer mutations on the main thread; do not relax the dock tile main-queue boundary without a crash-log-backed reason. Tested: ./scripts/reload.sh --tag fix-3590-main-thread-image-views Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check Not-tested: Crash repro, because issue #3590 has no known reproduction path
…er-memory-undercount
The crash fix already moved dock tile ownership to the main queue. Cursor correctly pointed out that updateDockTile still wrapped its private body in a second performOnMain call even though every caller now reaches it from setDockTileOnMain, a main-queued distributed notification, or a main-dispatched appearance observer. Remove the redundant closure boundary and keep the DEBUG main-queue assertion at updateDockTile so future off-main callers still fail close to the violation. Constraint: AppKit, NSWorkspace, LaunchServices, and NSDockTile mutations must stay main-thread-only. Rejected: Remove the public setDockTile main hop | NSDockTilePlugIn entrypoints can still arrive outside our controlled callbacks. Confidence: high Scope-risk: narrow Directive: Keep updateDockTile callers main-thread-bound; do not reintroduce nested dispatch unless a new off-main entrypoint is identified. Tested: git diff --check Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Local XCTest, per repo policy to rely on CI for test execution.
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 383c584. Configure here.
Cursor flagged same-line assert statements that combined DEBUG-only main-thread checks with functional image code. Splitting those statements keeps the AppKit crash tripwires visible without hiding guard and assignment behavior behind semicolons. Constraint: AppKit image mutation paths need explicit main-thread tripwires after the NSImageView crash report Rejected: Refresh the Swift file-length budget | reviewer cleanup can stay within the existing budget Confidence: high Scope-risk: narrow Directive: Keep assertion-only checks visually separate from functional image updates Tested: rg same-line assertion scan; git diff --check; swift_file_length_budget Not-tested: local XCTest per repository policy
This was referenced May 13, 2026
This branch was successfully 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
2 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.
Summary
Fixes #3584.
Task Manager and
cmux topwere rolling up memory from pane-owned/tag-owned/browser-owned process roots, but they did not include the cmux application process itself. That made the main SwiftUI app PID invisible even though macOS Force Quit and the OS memory pressure view attribute that RSS to cmux. The sampler was already using RSS (proc_pidinfo(PROC_PIDTASKINFO).pti_resident_size), so this was an ownership/root selection bug rather than the wrong memory metric.The rollup previously walked these roots recursively:
It did not walk the
GhosttyTabs.app/cmux DEV application PID, which is where large view-tree, scrollback, attributed-string, image, and Mach VM allocations can live. I added the app PID as a first-class root in the sharedsystem.toppayload so both Task Manager andcmux topcount it through the same rollup path.Regression appears to date back to the original top/Task Manager implementation in
971372531 Add top snapshots and Task Manager window (#3290), which modeled totals around UI-owned child process roots.1e74b4f Fix task manager process ownership for agent terminalsbroadened terminal ownership via CMUX surface roots but still left out the app process root.Testing
CmuxTopSnapshotScopeTests.testWindowRollupMatchesPSForApplicationProcessTreespawns a real parent/child process tree, compares the window rollup RSS againstpsrecursive RSS, and asserts the app/root PID is included.Notes
This PR intentionally does not touch CPU accounting (#3583), add new columns, or redesign Task Manager UI. WebKit GPU/Networking XPC helpers may still require a separate OS-grouping decision if Force Quit includes helpers that are not descendants of the app process tree; this fix addresses the confirmed missing main app PID/root.
Note
Medium Risk
Changes how
cmux top/Task Manager aggregate process resources by adding a new root PID (app_process_pids) and updating rollup logic; mistakes could misattribute memory or regress reporting. UI changes are mostly main-thread enforcement but could surface new assertion failures in debug builds if call sites are off-main.Overview
Task Manager /
cmux topwindow rollups now include the cmux application process itself by attaching anapp_process_pidsroot (targeting the key window, falling back to the first) and folding those PIDs intotop_level_pidsand resource summaries viasummaryPayload(..., rootPIDs:).Adds regression tests that spawn a real parent/child process tree to validate RSS rollups against
ps, ensure app-process attribution stays isolated to the key window, and verify key-window/fallback attachment behavior.Separately tightens AppKit threading: Dock tile updates are routed through a single main-thread path (observer queue moved to
.main, removed nestedDispatchQueue.main.asyncinNSDockTilehelpers) and DEBUG-only main-queue assertions were added around several image/view update entry points.Reviewed by Cursor Bugbot for commit 65736d6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Include the cmux app process in Task Manager and
cmux toptotals so RSS matches macOS and the app PID is visible. Also route Dock tile and image updates through the main thread to prevent off‑main AppKit mutations.app_process_pidsas a first-class root and include it intop_level_pids; scope rollups withrootPIDsso app RSS is counted once and doesn’t absorb other windows.app_process_pids.ps, key-window attribution/isolation, readiness signals after touching allocations, pipe-drain ordering to avoidpsstalls, and cleanup of spawned processes and temp dirs.Written for commit 65736d6. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests