Repository navigation
Cap sidebar status/metadata dictionaries to bound the sidebar view-graph livelock (#5845) - #5855
Merged
Merged
Conversation
The sidebar status/metadata socket API lets agents and CI scripts insert entries under arbitrary caller-chosen keys. Under many long-running agent sessions these @published dictionaries grow without bound, leaking memory and making the per-tick removeDuplicates equality check and the display-order sort that feed the sidebar view graph progressively more expensive on the main thread (#5845). logEntries is already capped; statusEntries and metadataBlocks are not. This test fails without the cap (count grows to 3x the bound). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…5845) The sidebar status/metadata socket API upserts entries under arbitrary caller-chosen keys. logEntries was already capped, but statusEntries and metadataBlocks could grow without bound: under ~30 long-running agent sessions over hours they accumulate memory and make every per-tick removeDuplicates equality check and sidebarStatus/MetadataBlocksInDisplayOrder() sort that feeds the sidebar LazyVStack view graph progressively more expensive on the main thread — compounding the never-draining view-graph transactions seen in the .hang/.cpu_resource samples. Add a generous per-workspace cap (200) enforced from a didSet on each @published dictionary, evicting lowest-priority then oldest entries so retention matches the existing display-order sort. The cap is far above any realistic integration (the collapsed sidebar shows a handful of pills), so it only clamps pathological growth. Mirrors the logEntries cap. Refreshes the Swift file-length budget for the added lines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughMakes Workspace sidebar dictionaries writable and adds didSet trimming that enforces 200-entry caps; trimming keeps highest-priority and newest entries, evicts the rest, and evictions for status entries purge related agent PID runtime state. Tests validate bounding and PID-cleanup behavior. ChangesSidebar Memory Bounding
Sequence DiagramsequenceDiagram
participant Trimmer as trimSidebarStatusEntriesIfNeeded
participant Purger as purgeAgentRuntimeState
participant Clear as clearAgentPID
participant Ports as refreshTrackedAgentPorts
Trimmer->>Purger: evictedStatusKeys
Purger->>Clear: clearAgentPID(... clearStatus:false, refreshPorts:false) for matching PID keys
Clear-->>Purger: cleared?
Purger->>Ports: refreshTrackedAgentPorts() if any cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
Contributor
Greptile SummaryThis PR caps
Confidence Score: 5/5Safe to merge. The cap is enforced on every write path via didSet, reserved cmux application-state keys are pinned, coupled agent PID/lifecycle state is purged before eviction, and the set_status --pid multi-step race is handled by the just-inserted grace tier. Recursion in didSet terminates at depth two. The change is narrowly scoped to the two dictionary properties causing unbounded growth. The tiered eviction logic is well-reasoned and matches the existing display-order sort, reserved keys cannot be lost, and coupled runtime state stays correctly bounded. Nine regression tests cover all edge cases. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["statusEntries write via didSet"] --> B{"count > 200?"}
B -->|No| C["Return — no-op"]
B -->|Yes| D["Compute liveAgentStatusKeys and justInsertedKeys"]
D --> E["Sort: Reserved > Live > Fresh > Priority/Timestamp"]
E --> F["kept = top 200 keys"]
F --> G["evictedKeys = all keys minus kept"]
G --> H{"evictedKeys empty?"}
H -->|Yes| I["Return"]
H -->|No| J["purgeAgentRuntimeState: clearAgentPID + clearAgentLifecycle"]
J --> K["statusEntries = filter kept (re-enters didSet)"]
K --> L{"count > 200?"}
L -->|No| M["Return — depth 2 terminus"]
L -->|Yes| D
Reviews (12): Last reviewed commit: "Refresh Swift file-length budget after m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+14480
to
+14493
| private func trimSidebarStatusEntriesIfNeeded() { | ||
| guard statusEntries.count > Self.maxSidebarStatusEntries else { return } | ||
| let keptKeys = statusEntries.values | ||
| .sorted { lhs, rhs in | ||
| if lhs.priority != rhs.priority { return lhs.priority > rhs.priority } | ||
| if lhs.timestamp != rhs.timestamp { return lhs.timestamp > rhs.timestamp } | ||
| return lhs.key < rhs.key | ||
| } | ||
| .prefix(Self.maxSidebarStatusEntries) | ||
| .map(\.key) | ||
| let kept = Set(keptKeys) | ||
| // Reassigning re-enters didSet, but the next pass sees count <= cap and | ||
| // returns immediately, so recursion terminates at depth two. | ||
| statusEntries = statusEntries.filter { kept.contains($0.key) } |
Contributor
There was a problem hiding this comment.
sidebarStatusEntriesInDisplayOrder() first calls sidebarStatusEntriesVisibleForDisplay() — which hides "structured agent hook status" entries whose panel is no longer active — then sorts. trimSidebarStatusEntriesIfNeeded() skips that filter and sorts all entries, including invisible structured-agent entries. An invisible entry with a high-priority or recent timestamp will claim one of the 200 slots, potentially evicting a visible non-structured entry. The PR description says "entries that are actually shown always survive," but that guarantee only holds when every entry above the cap threshold is non-structured or already visible.
Contributor
Author
There was a problem hiding this comment.
Practical impact is negligible, so I left the ranking as priority/timestamp. The structured-agent-hook keys are a fixed 16-key allowlist (AgentHibernationLifecycleStatusKeys.allowedStatusKeys) — they cannot grow, so they can occupy at most ~16 of the 200 slots. The flood that actually triggers the cap is always non-structured (always-visible) keys, and the display path re-applies sidebarStatusEntriesVisibleForDisplay() to the survivors before showing the collapsed top entries, so what the user sees is unaffected. I softened the 'always survive' wording in the PR description to reflect this bound.
— Claude Code
Comment on lines
+14490
to
+14493
| let kept = Set(keptKeys) | ||
| // Reassigning re-enters didSet, but the next pass sees count <= cap and | ||
| // returns immediately, so recursion terminates at depth two. | ||
| statusEntries = statusEntries.filter { kept.contains($0.key) } |
Contributor
There was a problem hiding this comment.
objectWillChange / publisher notification on every overflow insert
When an insert pushes statusEntries above the cap, didSet reassigns the whole dictionary via statusEntries = statusEntries.filter {…}. Because statusEntries is @Published, this second assignment fires objectWillChange a second time in the same synchronous call. Any Combine subscriber that has already received the over-cap value (e.g. sidebarObservationPublisher → .removeDuplicates()) will receive a second, distinct emission for what is logically one write. On a workload with persistent overflow, this doubles the emission rate into the chains the PR is trying to quiet. The same issue exists in trimSidebarMetadataBlocksIfNeeded(). A cheaper fix is to mutate with removeValue(forKey:) per evicted key (one notification per eviction, coalesced), or to stage the removal outside didSet entirely.
Contributor
Author
There was a problem hiding this comment.
Keeping the single whole-dictionary reassignment. The second objectWillChange fires only on the overflow path (count > cap), i.e. exactly the pathological flood the cap exists to bound — never in normal operation where the dicts stay well under 200. The suggested per-key removeValue is not cheaper for the common case (a flood evicts one entry per insert → one extra notification either way) and is strictly worse for the bulk path (restore assigns the whole dict once → N notifications vs the reassign's 1), so I left the reassign as the lower-amplitude option. The doubled emission on the already-degenerate overflow path is bounded and far below the unbounded growth it replaces.
— Claude Code
Contributor
There was a problem hiding this comment.
Your analysis is correct on all three counts. The reassign and per-key paths produce the same notification count on the single-entry overflow path (1 insert notification + 1 trim notification either way), the reassign is strictly fewer notifications than per-key removal on the bulk-assign path, and the overflow path itself is the bounded pathological case the cap exists to contain.
I'll retract the concern — the current approach is the right choice here.
![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 `@Sources/Workspace.swift`:
- Around line 14482-14490: The current trimming logic builds keptKeys from
statusEntries.values and then uses those to filter the dictionary, which can
misalign if keys and values diverge; change the selection to iterate the
dictionary pairs so the ranking is done on (dictKey, value) tuples: for
statusEntries use statusEntries.map { (key, value) in ... } or
statusEntries.sorted(by:) over entries, compare value.priority and
value.timestamp, and return the dictKey for the top Self.maxSidebarStatusEntries
into keptKeys/kept; apply the same change to the other trim path that uses the
same pattern (the block that computes keptKeys/kept near lines 14500-14509) so
both trimming branches compare and keep by the actual dictionary key.
🪄 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: 4a367d0d-4cb0-4a6a-9da0-75933fae4843
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceSidebarObservationTests.swift
…5845) Autoreview caught that set_status --pid couples a status key to agent PID runtime state (agentPIDs / agentPIDPanelIdsByKey / agentPIDKeysByPanelId and the port-scan tags keyed off them). The new sidebar status cap removed the status entry but left that coupled state behind, so the same ever-distinct-key workload with PIDs could still grow those maps without bound — the exact hot path the cap is meant to contain. trimSidebarStatusEntriesIfNeeded now purges the agent runtime state for every evicted status key (via clearAgentPID, status removal handled by the cap filter) before the entries leave statusEntries, so dotted status keys still resolve. Adds a regression test for the set_status --pid eviction path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rt (#5845) Autoreview follow-up: set_status --pid inserts the status entry first, then records the PID. If the workspace is already at the status cap with higher-priority entries, a new low-priority status self-evicts on insert, so the earlier eviction-purge sees no PID yet and the PID is then recorded against an absent status key — leaving agentPIDs / port-scan tags growing under a flood of distinct low-priority keys. Route both set_status --pid call sites through recordAgentPIDForSurvivingStatusKey, which records the PID only if the status entry survived the cap. set_agent_pid (intentionally PID-without-status) keeps using recordAgentPID directly. Adds a regression test for the self-eviction path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeRabbit: derive the keep-set ranking from the dictionary's (key, value) pairs and keep by storage key in both trim paths, so the ranking key and the filter key can never diverge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#5845 review) Autoreview (codex): set_status --pid re-pings with unchanged display fields are no-ops, so a live agent status keeps its original insertion timestamp. A pure timestamp/priority cap could then evict that active status under a flood of newer distinct keys, hiding a live agent and purging its PID. Rank statuses with a coupled agent PID (statusKeysWithCoupledAgentRuntime) ahead of plain telemetry in the eviction sort, so live agent statuses survive without bumping the display timestamp (which would reintroduce per-ping row churn). The coupled-PID purge still fires only when live statuses themselves overflow. Updates the eviction regression test to the all-live scenario and adds a test that a PID-backed status with an old timestamp survives a newer-key flood. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-hang-lazystack # Conflicts: # .github/swift-file-length-budget.tsv
… review) Autoreview (codex): some active agent statuses are lifecycle-backed without a PID — e.g. FeedCoordinator records a needs-input badge via setAgentLifecycle and writes statusEntries[statusKey] with no agent PID. The PID-only protected set let a telemetry flood evict that pending needs-input status, hiding an agent decision while leaving lifecycle state behind. statusKeysWithCoupledAgentRuntime now also includes the keys of agentLifecycleStatesByPanelId, so lifecycle-backed statuses are retained too. Adds a regression test for a PID-less needs-input status surviving a flood. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview (codex): remote.error and remote.port_conflicts are app-owned status entries used as application state (hasProxyOnlyRemoteSidebarError reads remote.error to preserve connected state during proxy-only reconnects), not external telemetry. The cap ranked them like any set_status key, so a flood of newer/higher-priority keys could evict active SSH/port-conflict error state. Add reservedSidebarStatusKeys and rank them as the top retention tier (above live-agent and priority/timestamp), so cmux-owned state is never evicted. Adds a regression test covering the worst-case ranking inputs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview (codex): the cap protects lifecycle-backed status keys, but with more than 200 distinct lifecycle-backed keys the oldest are still evicted — and the purge only cleaned PID-coupled state, so agentLifecycleStatesByPanelId kept every evicted key. statusKeysWithCoupledAgentRuntime() then re-traverses that ever-growing set on each trim, reintroducing the memory/CPU growth class. purgeAgentRuntimeState now also clears lifecycle state (clearAgentLifecycle) for every evicted status key, idempotent with the PID path. Adds a regression test inserting 2x the cap of lifecycle-backed keys and asserting the lifecycle map stays bounded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n trim (#5845 review) Autoreview (codex): trimming synchronously from the statusEntries didSet makes multi-step status+runtime updates non-atomic. set_status --pid (and detached-agent adoption) insert the visible status first and record the coupled PID/lifecycle afterward, so when the workspace is at cap with higher-priority telemetry the new agent status self-evicted before it was marked live, and the follow-up PID was then dropped — breaking core agent sidebar/PID behavior under the flood. Rank keys added by the triggering write (diffed from didSet oldValue) above plain telemetry but below reserved/live, so a just-inserted status survives its own trim long enough for recordAgentPIDForSurvivingStatusKey to mark it live. It's a tier, not an absolute pin: a bulk insert above the cap still ranks within the tier by priority/timestamp, so the cap stays enforced. Applied to both status and metadata trims. Updates/adds regression tests for the survive-over-telemetry and self-evict-against-live-statuses cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-hang-lazystack # Conflicts: # .github/swift-file-length-budget.tsv
…review) Autoreview (codex): adoptDetachedAgentRuntimeState writes the transferred status into statusEntries and then records every transferred PID unconditionally. With the cap, an adopted status can self-evict when the destination is already full of higher-ranked live/reserved entries, so the unconditional record recreated an agentPIDs/ownership/port-scan record with no surviving status — the orphan class this patch bounds. Merge adopted statuses in one assignment (so they share the just-inserted grace tier in a single trim), then adopt the coupled PID/ownership only for status-backed keys whose status survived the cap. PID-only keys (set_agent_pid, no transferred status) are still adopted unconditionally. Adds regression tests for the evicted and surviving adoption cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…review) Autoreview (codex): several review-driven tests referenced fix-only symbols (recordAgentPIDForSurvivingStatusKey, Workspace.reservedSidebarStatusKeys), so a test-only commit against origin/main would fail to compile rather than fail an assertion, weakening the red/green proof. Rewrite those cases to assert the observable behavior through pre-existing APIs (statusEntries plus literals): the grace tier keeps a just-inserted status, a new non-live status self-evicts against a full set of live statuses, and reserved keys (literal remote.error/remote.port_conflicts) survive. All tests now compile against main and fail there for the right reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-hang-lazystack # Conflicts: # .github/swift-file-length-budget.tsv
This was referenced Jun 11, 2026
This was referenced Jun 12, 2026
Closed
azooz2003-bit
added a commit
that referenced
this pull request
Jun 13, 2026
…021 + 6026) (#6033) * Sidebar: remove whole-content rows-height measurement (fixes layout livelock) Replace the LazyVStack background GeometryReader -> SidebarWorkspaceRowsHeightPreferenceKey -> @State workspaceRowsMeasurement -> emptyAreaHeight round trip with SidebarRowsFillLayout, a custom Layout that places the rows at their natural height and stretches the empty drop/tap area to fill the remaining viewport from its own concrete bounds, in one geometry pass with no state writes. The preference write during layout fed a non-converging relayout transaction: main thread pinned 100%+ in GraphHost.flushTransactions -> LazySubviewPlacements.placeSubviews -> LazyStack.place -> ForEachList.applyNodes. A fresh 2026-06-12 capture on stable 0.64.15 (which already contains the mitigations from #5708, #5846, #5855, and #5859) shows the identical signature: 128% CPU, 400 threads, debug socket refusing connections, 3344/3715 main-thread samples inside flushTransactions. The rows-height key is the last live write-during-layout edge in the sidebar after #5325 (frame anchors) and #5708 (row IDs) removed their siblings. Same approach as #5852, re-ported on top of the #5846 pixel-alignment work (contentMinHeight flooring is kept; only the empty-area math moves into the Layout). Fixes #5764. Helps #2586, #5570, #5845. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: create bundled helper directory before install * Sidebar: replace render-item String id with an allocation-free Hashable enum ForEach(renderItems, id: \.id) gathers every row's identifier on each list diff, and the sidebar re-diffs all rows per update. The previous computed String id ("workspace.\(uuid.uuidString)") allocated and formatted a fresh 36-char string per access; SidebarWorkspaceRenderItem.id.getter was the hottest app-owned frame in the #5764 livelock spindump. SidebarWorkspaceRenderItemID is a two-case enum over UUID: identity compare and hash with zero heap allocation, and group headers can never collide with workspace rows on the same UUID (same guarantee the string prefixes gave). Identity values are unchanged in meaning, so row lifetime and animations are unaffected; nothing persisted the string form (the only consumers are the ForEach key path and scrollTo, which targets the explicit inner .id(tab.id) UUIDs, not the ForEach identity). Pure per-pass cost cut for #5764, #5845, #2586; complements the structural loop fix in #6019. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: make rows height-stable under agent churn (no height animation, eager markdown) Two changes that stop agent activity from continuously varying sidebar row heights, which kept re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (#5764, #5845): 1. Remove the three implicit .animation(value:) modifiers on agent-mutable snapshot fields (latestLog, progress, metadataBlocks.count) and reduce the four height-moving .transition(.opacity.combined(.move(edge: .top))) modifiers in TabItemView's log/progress/metadata sections to .transition(.opacity). While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating. Content changes now apply in one discrete layout pass. 2. SidebarMetadataMarkdownBlockRow parsed its markdown in onAppear into @State: a guaranteed nil -> attributed swap (and height change) on every first appearance of every block scrolling in. It now renders inline via a new SidebarMetadataMarkdownRenderer with a bounded (512-entry) memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the SidebarWorkspaceDescriptionText sibling, plus memoization to keep repeat body evals cheap and growth bounded. WWDC backing: lazy rows must be height-stable after appearing; initialize row state in the initializer, not onAppear (WWDC26 "Dive into lazy stacks", 321); keep body cheap / precompute (WWDC23 10160, WWDC25 306). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cache failed parses with updateValue (subscript assignment drops nil values) With [String: AttributedString?], `cache[markdown] = parsed` removes the key when parsed is nil, so unparseable blocks re-parsed on every body eval and appended phantom keys to insertionOrder, mis-evicting valid entries once at capacity. Caught by Greptile on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Byte-bound the metadata markdown cache (autoreview P1) The 512-entry cap bounded entry count but not retained bytes. Metadata blocks are agent/control-socket supplied and uncapped at this boundary, so a key churning large unique markdown could keep hundreds of big payloads alive after the workspace metadata was overwritten or cleared (worse than the old row-local @State, which released on update). Skip caching blocks over 4096 UTF-8 bytes: they parse inline each eval (rare, still attributed from the first frame), and total retained cache bytes are now bounded by capacity * maxCacheableBytes regardless of churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Plain-text fallback for oversized metadata blocks (autoreview P1) Parsing >4KB blocks inline (previous commit) removed the retention but moved the cost to CPU: TabItemView.body re-runs on snapshot changes under agent churn, so a large block reparsed each time. Return nil for oversized blocks instead, so the row falls back to the existing Text(block.markdown) plain path: no parse, no retention, and height-stable (the result never changes for a given block, so no nil->attributed swap). Small blocks still cache and render as markdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Size the sidebar empty area from an explicit viewport, not the layout proposal (autoreview P2) SidebarRowsFillLayout derived its container height from proposal.replacingUnspecifiedDimensions(). A vertical ScrollView leaves the scroll-axis height unspecified, so that fell back to a 10pt placeholder and the empty area collapsed to 0 whenever the rows fit the viewport — dropping the blank area below the last row out of the double-click/drop target. Pass the viewport height (minHeight, the floored content height the call site already computes from the scroll geometry) into the layout explicitly and size the empty area from it. New emptyAreaFillHeight(viewportHeight:rowsHeight:) overload encodes container = max(viewport, rows). Verified at runtime via temporary instrumentation (since removed): rows fit -> viewport=628 rows=421 empty=207 and rows=370 empty=258; rows overflow -> viewport=628 rows=676 empty=0. Added unit coverage for both the fit and overflow viewport paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jun 15, 2026
3 tasks done
ShubhamPatilsd
pushed a commit
to emergent-inc/mosaic
that referenced
this pull request
Jul 9, 2026
…021 + 6026) (#6033) * Sidebar: remove whole-content rows-height measurement (fixes layout livelock) Replace the LazyVStack background GeometryReader -> SidebarWorkspaceRowsHeightPreferenceKey -> @State workspaceRowsMeasurement -> emptyAreaHeight round trip with SidebarRowsFillLayout, a custom Layout that places the rows at their natural height and stretches the empty drop/tap area to fill the remaining viewport from its own concrete bounds, in one geometry pass with no state writes. The preference write during layout fed a non-converging relayout transaction: main thread pinned 100%+ in GraphHost.flushTransactions -> LazySubviewPlacements.placeSubviews -> LazyStack.place -> ForEachList.applyNodes. A fresh 2026-06-12 capture on stable 0.64.15 (which already contains the mitigations from manaflow-ai/cmux#5708, manaflow-ai/cmux#5846, manaflow-ai/cmux#5855, and manaflow-ai/cmux#5859) shows the identical signature: 128% CPU, 400 threads, debug socket refusing connections, 3344/3715 main-thread samples inside flushTransactions. The rows-height key is the last live write-during-layout edge in the sidebar after manaflow-ai/cmux#5325 (frame anchors) and manaflow-ai/cmux#5708 (row IDs) removed their siblings. Same approach as manaflow-ai/cmux#5852, re-ported on top of the manaflow-ai/cmux#5846 pixel-alignment work (contentMinHeight flooring is kept; only the empty-area math moves into the Layout). Fixes manaflow-ai/cmux#5764. Helps manaflow-ai/cmux#2586, manaflow-ai/cmux#5570, manaflow-ai/cmux#5845. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: create bundled helper directory before install * Sidebar: replace render-item String id with an allocation-free Hashable enum ForEach(renderItems, id: \.id) gathers every row's identifier on each list diff, and the sidebar re-diffs all rows per update. The previous computed String id ("workspace.\(uuid.uuidString)") allocated and formatted a fresh 36-char string per access; SidebarWorkspaceRenderItem.id.getter was the hottest app-owned frame in the manaflow-ai/cmux#5764 livelock spindump. SidebarWorkspaceRenderItemID is a two-case enum over UUID: identity compare and hash with zero heap allocation, and group headers can never collide with workspace rows on the same UUID (same guarantee the string prefixes gave). Identity values are unchanged in meaning, so row lifetime and animations are unaffected; nothing persisted the string form (the only consumers are the ForEach key path and scrollTo, which targets the explicit inner .id(tab.id) UUIDs, not the ForEach identity). Pure per-pass cost cut for manaflow-ai/cmux#5764, manaflow-ai/cmux#5845, manaflow-ai/cmux#2586; complements the structural loop fix in manaflow-ai/cmux#6019. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: make rows height-stable under agent churn (no height animation, eager markdown) Two changes that stop agent activity from continuously varying sidebar row heights, which kept re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (manaflow-ai/cmux#5764, manaflow-ai/cmux#5845): 1. Remove the three implicit .animation(value:) modifiers on agent-mutable snapshot fields (latestLog, progress, metadataBlocks.count) and reduce the four height-moving .transition(.opacity.combined(.move(edge: .top))) modifiers in TabItemView's log/progress/metadata sections to .transition(.opacity). While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating. Content changes now apply in one discrete layout pass. 2. SidebarMetadataMarkdownBlockRow parsed its markdown in onAppear into @State: a guaranteed nil -> attributed swap (and height change) on every first appearance of every block scrolling in. It now renders inline via a new SidebarMetadataMarkdownRenderer with a bounded (512-entry) memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the SidebarWorkspaceDescriptionText sibling, plus memoization to keep repeat body evals cheap and growth bounded. WWDC backing: lazy rows must be height-stable after appearing; initialize row state in the initializer, not onAppear (WWDC26 "Dive into lazy stacks", 321); keep body cheap / precompute (WWDC23 10160, WWDC25 306). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cache failed parses with updateValue (subscript assignment drops nil values) With [String: AttributedString?], `cache[markdown] = parsed` removes the key when parsed is nil, so unparseable blocks re-parsed on every body eval and appended phantom keys to insertionOrder, mis-evicting valid entries once at capacity. Caught by Greptile on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Byte-bound the metadata markdown cache (autoreview P1) The 512-entry cap bounded entry count but not retained bytes. Metadata blocks are agent/control-socket supplied and uncapped at this boundary, so a key churning large unique markdown could keep hundreds of big payloads alive after the workspace metadata was overwritten or cleared (worse than the old row-local @State, which released on update). Skip caching blocks over 4096 UTF-8 bytes: they parse inline each eval (rare, still attributed from the first frame), and total retained cache bytes are now bounded by capacity * maxCacheableBytes regardless of churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Plain-text fallback for oversized metadata blocks (autoreview P1) Parsing >4KB blocks inline (previous commit) removed the retention but moved the cost to CPU: TabItemView.body re-runs on snapshot changes under agent churn, so a large block reparsed each time. Return nil for oversized blocks instead, so the row falls back to the existing Text(block.markdown) plain path: no parse, no retention, and height-stable (the result never changes for a given block, so no nil->attributed swap). Small blocks still cache and render as markdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Size the sidebar empty area from an explicit viewport, not the layout proposal (autoreview P2) SidebarRowsFillLayout derived its container height from proposal.replacingUnspecifiedDimensions(). A vertical ScrollView leaves the scroll-axis height unspecified, so that fell back to a 10pt placeholder and the empty area collapsed to 0 whenever the rows fit the viewport — dropping the blank area below the last row out of the double-click/drop target. Pass the viewport height (minHeight, the floored content height the call site already computes from the scroll geometry) into the layout explicitly and size the empty area from it. New emptyAreaFillHeight(viewportHeight:rowsHeight:) overload encodes container = max(viewport, rows). Verified at runtime via temporary instrumentation (since removed): rows fit -> viewport=628 rows=421 empty=207 and rows=370 empty=258; rows overflow -> viewport=628 rows=676 empty=0. Added unit coverage for both the fit and overflow viewport paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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 the unbounded-growth amplifier behind the recurring main-thread hang in #5845 (run loop stuck in
LazyStack/ForEachListview-graph updates with ~37 workspaces + ~30 live agent sessions, footprint climbing to 6–8 GB before force-quit).The sidebar
status/metadatasocket API upserts entries under arbitrary caller-chosen keys.logEntrieswas already capped, butstatusEntriesandmetadataBlocks(@Published [String: …]onWorkspace) had no bound — they only shrank via explicit key removal or full reset. Under ~30 long-running agent/CI integrations over hours, an integration that uses ever-distinct keys grows these dictionaries without limit. That hurts in three compounding ways, all on the main thread:Workspace.sidebarObservationPublisherrebuilds aSidebarObservationStateand runs.removeDuplicates()(a deepEquatablecompare of the full dicts) on every upstream emission, for every active workspace. Cost scales with dictionary size.sidebarStatusEntriesInDisplayOrder()/sidebarMetadataBlocksInDisplayOrder()(O(n log n)) to feed the sidebarLazyVStackview graph.As these grow, the per-update work feeding the view graph grows super-linearly, compounding the never-draining
NSHostingView.beginTransaction → … → LazyStack.place(subviews:) → ForEachList.applyNodestransactions captured in the.hang/.cpu_resourcesamples.Fix
Add a generous per-workspace cap (200) to
statusEntriesandmetadataBlocks, enforced from adidSeton each@Publisheddictionary so every write path (socket commands, feed coordinator, panel lifecycle, restore) is bounded — not just today's call sites. Eviction drops lowest-priority then oldest entries, following the same priority/timestamp ranking as the display-order sort. The structured-agent-hook status keys are a fixed 16-key allowlist that can occupy at most a handful of the 200 slots, and the display re-applies its visibility filter to the survivors, so the shown entries are unaffected in practice. The cap is far above any realistic integration (the collapsed sidebar shows a handful of pills), so it only clamps pathological growth. This mirrors the existinglogEntriescap.Reproduction / evidence
A full live repro needs ~37 workspaces + ~30 agents over hours, so this is driven from the issue's
.hang/.cpu_resourcestack signature and a static analysis of the workspace-sidebarLazyVStackpath (Sources/ContentView.swiftworkspaceRows→TabItemView). That path is already heavily guarded against the #2586 class of livelock (Equatable rows +.equatable(), snapshot boundary,dragStatemade@Observable, gated virtualization-defeating frame readers, 0.5 pt height-jitter tolerance, batched mutations viaTerminalMutationBus). The remaining un-bounded amplifier feeding that graph was the two telemetry dictionaries fixed here.Tests
Two-commit red/green (
cmuxTests/WorkspaceSidebarObservationTests.swift):testStatusEntriesStayBoundedUnderUnboundedDistinctKeys/testMetadataBlocksStayBoundedUnderUnboundedDistinctKeysinsert 3× the cap of distinct keys and assert the dictionary stays bounded and keeps the newest / evicts the oldest. Fails before the fix (count grows to 3×).didSetcap.Added to an already-wired test file (no pbxproj changes). Swift file-length budget refreshed for the added lines. No user-facing strings changed, so no localization audit was required.
Honest scope note / precise follow-ups
This removes a concrete unbounded amplifier (memory + per-tick main-thread cost), but a genuine 30-agent workload can still saturate the main thread through legitimate per-row sidebar refreshes. Two precise, lower-confidence follow-up candidates surfaced during the investigation, deliberately not included here to keep this change low-risk and reviewable:
SidebarObservationStatecarries the fulllogEntriesarray, but the row only renderslogEntries.last. Replacing it withlatestLogEntrywould cut the per-tick compare from O(50) to O(1) and stop firing on eviction-only mutations.SidebarWorkspaceRowsHeightPreferenceKey→ host@State workspaceRowsMeasurement) is the last preference-driven layout-feedback path on the workspaceLazyVStack(sibling of the one removed in Remove sidebar row-ids preference aggregation feeding the layout livelock (#2586) #5708 / Nightly freezes: sidebar LazyVStack layout loop pegs main thread at 100% CPU, deadlocks CLI #2586). Genuine row-height changes from agent telemetry re-run the sidebar host body. Isolating the empty-area sizing into a child view so its@Statewrite no longer invalidates the row-building body is the architectural next step, but it touches delicate scroll/drag/sidebar scrollbar always visible — should only appear when content overflows #3241 code and warrants its own change with a live build to verify.The 6–8 GB footprint is likely also driven by agent-session output buffers / web renderers independent of the sidebar; that should be tracked separately.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Caps per-workspace
statusEntriesandmetadataBlocksat 200 to stop sidebar-driven main-thread hangs and memory spikes (#5845). Retains reserved cmux keys and live agent-backed statuses, and prevents orphaned PID/lifecycle state (including during detached-agent adoption).didSeton both dictionaries (mirrors thelogEntriescap).main.Written for commit 8f357b5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests