taskmanager: honest memory labels, untruncated grouping, agent row folding - #7624
austinywang wants to merge 6 commits into
Conversation
…lding Three accounting fixes from the #7596 audit: - The headline "Memory" tile summed per-process phys_footprint across cmux and every descendant, double-counting the dyld shared cache, shared frameworks, and shared IOSurfaces (22.4 GB shown vs 28 GB whole-machine RSS). Relabel it "Summed Footprint" with an explanatory tooltip (en/ja) and matching cmux top CLI help; the value itself is unchanged and App Footprint remains the accurate single-process number. - Program rows and child-memory groups keyed on proc_name, which is p_comm truncated to 16 chars, so all WebKit helpers collapsed into a row named "com.apple.WebKi". Group names now canonicalize through the untruncated proc_pidpath basename when the comm looks truncated, fixing the whole truncation class without WebKit-specific hardcoding. - Coding-agent CLI processes with version-string binaries (e.g. Claude's ~/.local/share/claude/versions/2.1.204) were folded into the "Claude Code" coding-agents row but ALSO surfaced as a raw "2.1.204" program row. Program rows now exclude agent-classified PIDs, and child-memory groups fold agent processes under the agent display name, via one shared classifier. Part of #7596 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds canonical process-name and coding-agent lookup logic, applies it to memory diagnostics and summary payloads, renames the Task Manager metric to Summed Footprint, and updates tests, localization, and project wiring. ChangesSummed footprint labeling and canonical process naming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CmuxTopProcessSnapshot
participant CmuxTopProcessNaming
participant CmuxTopMemoryDiagnostics
participant CmuxTaskManagerView
CmuxTopProcessSnapshot->>CmuxTopProcessNaming: codingAgentDefinitionsByPID(for: pids)
CmuxTopProcessNaming-->>CmuxTopProcessSnapshot: definitionsByPID map
CmuxTopProcessSnapshot->>CmuxTopProcessNaming: cmuxTopCanonicalProcessName(name, path)
CmuxTopProcessNaming-->>CmuxTopProcessSnapshot: canonical title
CmuxTopMemoryDiagnostics->>CmuxTopProcessNaming: codingAgentDefinitionsByPID(for: pids)
CmuxTopProcessNaming-->>CmuxTopMemoryDiagnostics: agentDefinitions map
CmuxTopMemoryDiagnostics->>CmuxTopMemoryDiagnostics: derive groupName and group id
CmuxTaskManagerView->>CmuxTopProcessSnapshot: read total.memoryBytes
CmuxTaskManagerView-->>CmuxTaskManagerView: render "Summed Footprint" with localized help
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors)
✅ Passed checks (20 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 |
Greptile SummaryThis PR updates Task Manager memory labeling and process grouping. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "taskmanager: document why the agent memo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "taskManager.summary.summedFootprint": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "ar": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "الذاكرة" | ||
| } | ||
| }, | ||
| "bs": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Memorija" | ||
| } | ||
| }, | ||
| "da": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Hukommelse" | ||
| } | ||
| }, | ||
| "de": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Speicher" | ||
| } | ||
| }, | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Memory" | ||
| } | ||
| }, | ||
| "es": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Memoria" | ||
| } | ||
| }, | ||
| "fr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Mémoire" | ||
| } | ||
| }, | ||
| "it": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Memoria" | ||
| "value": "Summed Footprint" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "メモリ" | ||
| } | ||
| }, | ||
| "ko": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "메모리" | ||
| } | ||
| }, | ||
| "nb": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Minne" | ||
| } | ||
| }, | ||
| "pl": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Pamięć" | ||
| } | ||
| }, | ||
| "pt-BR": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Memória" | ||
| } | ||
| }, | ||
| "ru": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Память" | ||
| } | ||
| }, | ||
| "th": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "หน่วยความจำ" | ||
| } | ||
| }, | ||
| "tr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Bellek" | ||
| } | ||
| }, | ||
| "uk": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Пам’ять" | ||
| "value": "合計フットプリント" | ||
| } | ||
| }, | ||
| "zh-Hans": { | ||
| } | ||
| } |
There was a problem hiding this comment.
The new taskManager.summary.summedFootprint key only defines en and ja, while this catalog still has other supported locales and nearby memory summary keys include those translations. On a French, German, or other supported non-English/non-Japanese system, this changed Task Manager tile falls back to English next to translated summary labels.
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Intentional: cmux's supported localization set is currently English and Japanese (see CLAUDE.md localization policy), and the new key matches the sibling summary key taskManager.summary.cpu, which is also en+ja only. The broader locale list on the old taskManager.summary.memory key was legacy coverage; machine-copying it to a renamed label with different semantics would not count as localization under the repo policy.
— Claude Code
| "taskManager.summary.summedFootprint.help": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "内存" | ||
| "value": "Sum of each process's physical footprint. Shared memory (frameworks, dyld cache, IOSurfaces) is counted once per process, so this overstates real usage." | ||
| } | ||
| }, | ||
| "zh-Hant": { | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "記憶體" | ||
| "value": "各プロセスの物理フットプリントの合計です。共有メモリ（フレームワーク、dyldキャッシュ、IOSurface）はプロセスごとに重複して計上されるため、実際の使用量より大きく表示されます。" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new help text has the same locale gap as the label: only en and ja values are present in a catalog that still supports the other app locales. Users in those locales get an English tooltip for this production UI text instead of localized help.
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Same as the label key: en+ja is the repo's supported localization set (CLAUDE.md policy), matching sibling taskManager.summary.cpu. Additional locales are out of scope for this PR.
— Claude Code
| ?? cmuxTopCanonicalProcessName(name: process.name, path: process.path) | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let groupName = displayName.isEmpty ? "pid-\(pid)" : displayName | ||
| let key = groupName |
There was a problem hiding this comment.
This changed the child-memory diagnostic key from the old case-insensitive process name to the case-preserved display name. If two child processes report the same executable with different casing, such as node and Node, they now become separate groups; because groups are sorted and limited by RSS, the real top child process group can be understated or omitted.
| let key = groupName | |
| let key = groupName.lowercased() |
There was a problem hiding this comment.
Fixed in 84f78fa: the grouping key is lowercased again so casing variants fold into one group, while id/name keep the first-seen display casing. Added a node/Node regression assertion to memoryDiagnosticsFoldAgentsAndCanonicalizeChildGroups.
— Claude Code
| @@ -0,0 +1,30 @@ | |||
| import Foundation | |||
|
|
|||
| nonisolated func cmuxTopCanonicalProcessName(name: String, path: String?) -> String { | |||
There was a problem hiding this comment.
This production helper is added as an internal top-level function even though it is only used by the cmux top snapshot/grouping code. That creates new ambient module API for process-name behavior; keeping it private/fileprivate or moving it onto the owning snapshot/accounting type preserves the intended scoped ownership.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Keeping the free function: it is a pure, stateless name transform in its own domain file (CmuxTopProcessNaming.swift) with the cmuxTop prefix, matching existing top-level cmuxTop* declarations (cmuxTopMemoryDiagnosticDefaultGroupLimit, cmuxTopNegativeScopeTTLNanoseconds). It is consumed by two sibling files in the same accounting domain plus tests, so private/fileprivate is not possible, and attaching it as a static member to CmuxTopProcessSnapshot would just create a static-as-namespace surface on a type whose state it does not use.
— Claude Code
Program totals, coding-agent totals, and memory diagnostics each ran an independent live agent-classification pass (KERN_PROCARGS2 sysctl per candidate PID). Under process churn between payload sections the same snapshot could exclude an agent from program_totals while omitting it from coding_agents, and the argument reads ran up to 3x per refresh. Memoize the per-PID definition (including nil results, via updateValue) behind an NSLock on the snapshot so every payload section shares one classification and the sysctl read runs at most once per PID. Adds a regression test: classify while the agent process is alive, reap it, and assert the same snapshot keeps the classification in both program and coding-agent sections. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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/CmuxTopSnapshot.swift (1)
356-379: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCoding-agent definitions are recomputed independently across sibling summary methods.
codingAgentDefinitionsByPID(for: pids)is called separately here and incodingAgentSummaryPayload, and a third time inCmuxTopMemoryDiagnostics.memoryDiagnosticGroups. If a caller builds program rows, coding-agent rows, and memory-diagnostic groups from the same pid set in one refresh (likely, given they all derive from the app's descendant pids), this repeats the same per-pid classification (includingprocessArgumentsIfNeeded's sysctl-based reads for candidates) up to 3x per refresh cycle.Consider computing this map once per refresh and threading it through as an optional parameter (defaulting to a fresh computation for callers that only need one view), so the expensive classification isn't triple-computed on scalable process counts.
As per path instructions: "Apply
.github/review-bot-rules/algorithmic-complexity.mdduring review. For production code over scalable user data, flag ... repeated sort/filter/map work in hot paths."Also applies to: 432-450
🤖 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/CmuxTopSnapshot.swift` around lines 356 - 379, The same coding-agent classification map is being recomputed in multiple summary paths, causing repeated expensive per-pid work on each refresh. Update CmuxTopSnapshot.programSummaryPayload, codingAgentSummaryPayload, and CmuxTopMemoryDiagnostics.memoryDiagnosticGroups to accept and reuse a shared codingAgentDefinitionsByPID result (with an optional parameter that defaults to recomputing when called standalone), so the map is built once per pid set and threaded through the sibling summary methods.Source: Path instructions
🤖 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/CmuxTopProcessNaming.swift`:
- Around line 3-10: cmuxTopCanonicalProcessName is being used as cross-file API,
so it should not remain a file-scope free function. Move this logic onto
CmuxTopProcessSnapshot as a static method named canonicalProcessName(name:path:)
and update the callers in CmuxTopMemoryDiagnostics and CmuxTopSnapshot to use
Self.canonicalProcessName(name:path:). Keep the existing behavior unchanged
while making the symbol an owned API instead of an ambient top-level function.
---
Outside diff comments:
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 356-379: The same coding-agent classification map is being
recomputed in multiple summary paths, causing repeated expensive per-pid work on
each refresh. Update CmuxTopSnapshot.programSummaryPayload,
codingAgentSummaryPayload, and CmuxTopMemoryDiagnostics.memoryDiagnosticGroups
to accept and reuse a shared codingAgentDefinitionsByPID result (with an
optional parameter that defaults to recomputing when called standalone), so the
map is built once per pid set and threaded through the sibling summary methods.
🪄 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: c34901c8-7730-4193-bb78-a5a33b9ebc3f
📒 Files selected for processing (8)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxTopMemoryDiagnostics.swiftSources/CmuxTopProcessNaming.swiftSources/CmuxTopSnapshot.swiftSources/TaskManagerView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxTopAccountingGroupingTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Restore the lowercased grouping key from main so casing variants of the same executable fold into one child-memory group (Greptile finding); group id/name keep the first-seen display casing. Extends the grouping test with a node/Node case-variant assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 84f78fa. Configure here.
Review round 2 fixes: - The Task Manager payload parser treated a present-but-empty program_totals array as a missing legacy field and recomputed program aggregates client-side from process rows, which recreated the very coding-agent program rows the backend now intentionally filters out (e.g. two Claude processes and no other duplicated program). Fall back to client aggregation only when the field is absent, and cover both cases with a parser regression test. - Wrap the per-snapshot agent-classification cache in CmuxTopCodingAgentDefinitionMemo with a private lock and private dictionary so no module code can touch the mutable state without the lock, preserving the snapshot's @unchecked Sendable safety argument. - Update testUnavailableMemorySourcesAreExposedInAggregatePayloads for the agents-excluded program-rows semantics: a non-agent worker pair keeps the program-aggregate unavailable-memory coverage and a codex pair keeps the coding-agent coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keep the group id equal to the lowercased grouping key (main's stable semantics) so folded case-variant groups don't flip ids across samples and churn childMemoryAggregate row identity; only the display name keeps first-seen casing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment-only. Names the constraint (synchronous nonisolated payload builders in the socket pipeline) that makes actor isolation a larger out-of-scope refactor; conscious exception to the actor-first policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Part of #7596 (memory audit, slice 3 — Task Manager accounting).
Three fixes:
cmux toptotals help) is a naive sum of per-processphys_footprintacross cmux + all descendants; shared pages (dyld cache, frameworks, shared IOSurfaces) are counted once per process, which is how the audit session showed 22.4 GB while whole-machine RSS across ALL processes was 28 GB. The tile is now "Summed Footprint" with a tooltip explaining the double-counting (localized en/ja), and thecmux tophelp text says the same. The value is deliberately unchanged — "App Footprint" (single-PID phys_footprint) remains the accurate number, per the issue's "fix or honestly label" guidance.proc_name(p_comm, 16-char max), so the ~223 WebKit helpers grouped as "com.apple.WebKi". NewcmuxTopCanonicalProcessNameprefers the untruncatedproc_pidpathbasename when the comm looks truncated (15/16 chars and the basename extends it, case-insensitive) — no WebKit-specific hardcoding, fixes every truncated helper name. Applied to both program rows and child-memory diagnostic groups.~/.local/share/claude/versions/2.1.204), so it appeared BOTH in the "Claude Code" coding-agents row and as a raw "2.1.204" program row. Program rows now exclude agent-classified PIDs, and child-memory groups fold agent processes under the agent's display name — one sharedcodingAgentDefinitionsByPIDclassifier mirrors the existing gating exactly (argv reads still gated byshouldReadArguments).Per-pane WebContent PIDs were already attributed via
webview-root-pid; attributing the shared WebKit GPU/Networking helpers to individual panes needs per-pane WebKit SPI that doesn't exist today — deferred, noted on the issue.Tests
New
CmuxTopAccountingGroupingTests(wired into cmuxTests in pbxproj;lint-pbxproj-test-wiring.shpasses): canonical-name truncation matrix, agent-PID exclusion from program rows (agent still present incoding_agents), canonical program-row display names, child-group agent folding + truncated-helper renaming, and a guard that totals math (clampedAddsum) is unchanged.python3 scripts/swift_file_length_budget.pypasses with no TSV changes — at-cap files stayed at cap (CmuxTopSnapshot.swift 664, CLI/cmux.swift exactly 35,600 via 1:1 replacement); new logic lives in new/untracked files.Localization audit
Added
taskManager.summary.summedFootprint+.helpwith en and ja values (matching the locale coverage of the siblingtaskManager.summary.cpu); removed the now-unreferencedtaskManager.summary.memorykey (verified zero references repo-wide);taskManager.column.memoryuntouched. Thecmux topCLI help block was already a non-localized bare string; the 1:1 replacement preserves that pre-existing pattern.Part of #7596
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Changes are limited to Task Manager / top labeling, grouping, and snapshot parsing; memory totals math is explicitly unchanged and covered by tests.
Overview
Task Manager and
cmux topnow call the headline memory total Summed Footprint (with a tooltip / help text that shared pages are counted per process, so the number can overstate real usage). The underlying sum is unchanged.Process accounting grouping is tightened:
cmuxTopCanonicalProcessNameexpands truncatedproc_namevalues using the executable path basename; program totals and child-memory diagnostic groups use that name and case-insensitive keys (stable lowercased group ids). Coding-agent PIDs are classified once per snapshot via a memoized helper and are excluded from program rows while appearing under agent display names in diagnostics andcoding_agents.The Task Manager client treats a present but empty
program_totalspayload as intentional (no Program Totals section) instead of rebuilding aggregates from hierarchy rows. NewCmuxTopAccountingGroupingTestscover these behaviors.Reviewed by Cursor Bugbot for commit 6c42590. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Clarifies Task Manager memory accounting and improves grouping by relabeling the memory tile, fixing truncated names, folding coding-agent processes under agent rows, and stabilizing memory-diagnostic group IDs. Also honors backend-provided empty
program_totals. Part of #7596 Task Manager accounting.cmux tophelp. Value unchanged.proc_nameis truncated; applied to program rows and memory diagnostics.program_totalsin the payload; legacy fallback only when the field is absent so filtered-out agent rows aren’t recomputed client-side.Written for commit 6c42590. Summary will update on new commits.
Summary by CodeRabbit