Repository navigation
Harden memory diagnostics for long-session surface leaks - #6267
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a ChangesMemory Diagnostics Infrastructure
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over AppDelegate,SentrySDK: Startup
AppDelegate->>sentryStartMemoryContextRefresh: applicationDidFinishLaunching
sentryStartMemoryContextRefresh->>sentryRefreshMemoryContext: reason: "startup"
end
sentryRefreshMemoryContext->>CmuxTopProcessSnapshot: captureCached()
CmuxTopProcessSnapshot-->>sentryRefreshMemoryContext: PID, memory, threads
sentryRefreshMemoryContext->>TerminalSurfaceRegistry: diagnosticSnapshot()
TerminalSurfaceRegistry-->>sentryRefreshMemoryContext: TerminalSurfaceRegistryDiagnosticSnapshot
sentryRefreshMemoryContext->>SentrySDK: configureScope(cmux.memory)
rect rgba(144, 238, 144, 0.5)
Note over sentryStartMemoryContextRefresh,SentrySDK: Periodic (every 300s)
loop
sentryRefreshMemoryContext->>SentrySDK: configureScope(cmux.memory)
end
end
rect rgba(255, 182, 193, 0.5)
Note over AppDelegate,sentryStopMemoryContextRefresh: Teardown
AppDelegate->>sentryStopMemoryContextRefresh: applicationWillTerminate
sentryStopMemoryContextRefresh->>sentryStopMemoryContextRefresh: cancel task
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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 |
![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
🤖 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 `@scripts/capture-memory.sh`:
- Around line 21-27: The script's option parsing does not validate that required
option values are provided before executing shift 2. If --pid or --out flags are
passed without a following value (or with another flag starting with -- as the
next argument), the shift 2 command will fail with an unhelpful shell error
rather than triggering the intended usage error message. For both the --pid and
--out cases, add a validation check after capturing the second argument (using
the pid="${2:-}" and out_dir="${2:-}" pattern) to verify the captured value is
not empty and does not start with a dash. If the value is invalid or missing,
call your usage error handling function before attempting shift 2. Only execute
shift 2 after confirming a valid value was provided.
🪄 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: bf56e26f-34ec-4b74-ac7b-4846ad027e2c
📒 Files selected for processing (6)
Packages/CmuxTerminalEngine/Sources/CmuxTerminalEngine/SurfaceRegistry/TerminalSurfaceRegistry.swiftPackages/CmuxTerminalEngine/Sources/CmuxTerminalEngine/SurfaceRegistry/TerminalSurfaceRegistryDiagnosticSnapshot.swiftPackages/CmuxTerminalEngine/Tests/CmuxTerminalEngineTests/TerminalSurfaceRegistryTests.swiftSources/AppDelegate.swiftSources/SentryHelper.swiftscripts/capture-memory.sh
Greptile SummaryThis PR adds three low-risk diagnostic hardening pieces: a
Confidence Score: 5/5Read-only diagnostic addition guarded by a telemetry feature flag; the only mutation is a Sentry scope update on the main actor. All changes are non-invasive: the registry method is a bounded snapshot behind the existing lock, the Sentry refresh is rate-limited and runs off the main actor, and the shell script is developer-only tooling. No production state machine, persistence, or UI layout was modified. No files require special attention; the only nit is a missing Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as AppDelegate
participant SH as SentryHelper (MainActor)
participant DT as Detached Task (utility)
participant TSR as TerminalSurfaceRegistry
participant Sentry as SentrySDK
App->>SH: sentryStartMemoryContextRefresh()
SH->>SH: sentryScheduleMemoryContextRefresh(minimumInterval: 0)
SH->>DT: "Task.detached { sentryRefreshMemoryContext() }"
DT->>DT: CmuxTopProcessSnapshot.captureCached()
DT->>TSR: diagnosticSnapshot()
TSR->>TSR: "lock.lock() → count surfaces & placements → lock.unlock()"
TSR-->>DT: TerminalSurfaceRegistryDiagnosticSnapshot
DT->>Sentry: "MainActor.run { configureScope(cmux.memory) }"
Note over SH: On breadcrumb / capture event (rate-limited 300s)
SH->>DT: "Task.detached { sentryRefreshMemoryContext() }"
App->>SH: sentryStopMemoryContextRefresh()
SH->>DT: task.cancel()
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as AppDelegate
participant SH as SentryHelper (MainActor)
participant DT as Detached Task (utility)
participant TSR as TerminalSurfaceRegistry
participant Sentry as SentrySDK
App->>SH: sentryStartMemoryContextRefresh()
SH->>SH: sentryScheduleMemoryContextRefresh(minimumInterval: 0)
SH->>DT: "Task.detached { sentryRefreshMemoryContext() }"
DT->>DT: CmuxTopProcessSnapshot.captureCached()
DT->>TSR: diagnosticSnapshot()
TSR->>TSR: "lock.lock() → count surfaces & placements → lock.unlock()"
TSR-->>DT: TerminalSurfaceRegistryDiagnosticSnapshot
DT->>Sentry: "MainActor.run { configureScope(cmux.memory) }"
Note over SH: On breadcrumb / capture event (rate-limited 300s)
SH->>DT: "Task.detached { sentryRefreshMemoryContext() }"
App->>SH: sentryStopMemoryContextRefresh()
SH->>DT: task.cancel()
Reviews (2): Last reviewed commit: "fix: make memory telemetry refresh event..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sentryMemoryContextTask = Task { @MainActor [weak self] in | ||
| while !Task.isCancelled { | ||
| // Intended periodic telemetry refresh; cancellation is wired to app termination. | ||
| do { | ||
| try await Task.sleep(nanoseconds: 300 * 1_000_000_000) | ||
| } catch { | ||
| break | ||
| } | ||
| guard self != nil else { break } | ||
| sentryRefreshMemoryContext(reason: "periodic") | ||
| } | ||
| } |
There was a problem hiding this comment.
Task.sleep polling loop in production code
The cmux-swift-blocking-runtime rule explicitly flags Task.sleep in shipped app code and requires "a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition" instead. The rule does not allow Task.sleep in production code even for periodic work — "polling loops in shipped app/runtime code" are listed as failures by default.
The concrete replacement is a Timer.publish(every: 300, on: .main, in: .common) Combine sequence (or AsyncTimerSequence if Swift Clocks are already in the dependency graph), which drives sentryRefreshMemoryContext(reason: "periodic") without a sleeping poll loop. Cancellation can still be wired to applicationWillTerminate via the same sentryMemoryContextTask?.cancel() pattern.
Rule Used: Flag new blocking or timing-based synchronization ... (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.
The sleep-loop concern was addressed in b263e49 as part of the Sentry helper move: periodic refresh now uses a RunLoop Timer and schedules detached refresh work, with cancellation wired through sentryStopMemoryContextRefresh().
— Claude Code
| let workspaceSurfaceCount = objects.filter { $0.focusPlacement == .workspace }.count | ||
| let rightSidebarDockSurfaceCount = objects.filter { $0.focusPlacement == .rightSidebarDock }.count |
There was a problem hiding this comment.
workspaceSurfaceCount and rightSidebarDockSurfaceCount are derived by calling $0.focusPlacement on each object after lock.unlock(). Every other placement query in this file (isRightSidebarDockSurface) reads from the locked surfaceFocusPlacements dictionary. If any TerminalSurfacing implementation exposes focusPlacement as a mutable stored property on a class, this is an unsynchronized read that could race with register() or unregister() on a background thread.
Capturing surfaceFocusPlacements under the lock (alongside objects) and using placements[$0.id] for the filter would keep the snapshot fully within the lock boundary and consistent with the rest of the registry's access pattern.
There was a problem hiding this comment.
Fixed in b263e49 by deriving placement counts from the registry's locked surfaceFocusPlacements table in a single pass, instead of reading focusPlacement after unlocking.
— Claude Code
Resolve .github/swift-file-length-budget.tsv conflict: keep AppSection.swift at its actual 933 lines (single entry, no duplicate) and TerminalPanel.swift at 879. Bump AppDelegate.swift ceiling 17610->17612 to reconcile pre-existing main drift (#6267 grew the file +2 without updating the budget). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Closes #6266
Summary
This PR does not claim a speculative renderer/WebKit/native-heap fix. I first checked the version context from #6266:
publishV2JSON retention fix is already on this branch/current main:CmuxSocketEventMapper.publishwraps the JSON parse/publish path inautoreleasepool, andCmuxEventBusTests.testPublishV2ReadTextResponseDoesNotAccumulateOnLongLivedThreadcovers the prior large-response leak.publishV2, but I could not get an accelerated current-main cloud repro because the cloud Mac lease timed out waiting forcmux-loader-27648982604to appear in local Tailscale/MagicDNS.Given that, this PR ships the low-risk hardening requested in the issue:
scripts/capture-memory.shcapturesps,footprint, andvmmap -summaryby default, with explicit--heavyforleaksandmalloc_historyonly on a fresh profiled instance.cmux.memorycontext containing app footprint/RSS/thread count and live terminal surface/runtime-surface counts. Sampling runs off the main actor and refreshes at startup plus activity-driven Sentry breadcrumb/capture points with a 5-minute throttle, with no timer or sleep loop.TerminalSurfaceRegistryexposes a bounded diagnostic snapshot, with a regression invariant proving unregistered/deallocated surfaces and freed runtime pointers drop out of the counts.Evidence
Local live production cmux 0.64.16 (96), pid 72227, was sampled read-only with
footprintandvmmap -summary:MALLOC_SMALL483 MB,IOSurface431 MB, graphics unmapped 264 MB,IOAccelerator100 MB.Sentry lookup was attempted with the provided workflow, but this shell is not authenticated (
sentry auth statusreports not authenticated) and no Sentry token/org/project environment variables are present, so the pid 708 event could not be fetched from here.Cloud Mac accelerated repro was attempted through the required skill. GitHub Actions run https://github.com/manaflow-ai/cmux-loader/actions/runs/27648982604 reached the VM hold step, but local discovery timed out waiting for
cmux-loader-27648982604;macfleet devicesalso could not be used because no local token is stored. The held runner was canceled.Testing
aae49b5d7 test: add terminal surface registry diagnostics invariantfails becauseTerminalSurfaceRegistry.diagnosticSnapshot()does not exist.swift test --package-path Packages/CmuxTerminalEngine --filter TerminalSurfaceRegistryTests/diagnosticSnapshotDropsUnregisteredSurfacesAndRuntimePointersbash -n scripts/capture-memory.shandscripts/capture-memory.sh --pid $$ --out /tmp/cmux-capture-memory-smoke-event-refresh-$$--pidand--outvalues../tests/test_ci_swift_file_length_budget.sh,python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv,./tests/test_ci_pbxproj_test_wiring.sh,./scripts/check-pbxproj.sh./Users/austinwang/manaflow/cmuxterm-hq/skills/autoreview/scripts/cmux-policy-check --mode branch --base origin/main.No local app build,
reload.sh, barexcodebuild, or app launch was run.Demo Video
No demo video is available. The cloud Mac attempt did not become reachable through local Tailscale/MagicDNS, and this PR does not launch a local app build by instruction.
Checklist
footprint/vmmap -summaryevidence from the already-running production app.--heavy.Summary by CodeRabbit