Hibernate idle agents before critical memory pressure panics - #9090
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds critical-memory-pressure safety hibernation alongside scheduled Agent Hibernation. It introduces trigger-aware planning, process-identity validation, bounded runtime teardown, panel lifecycle tracking, synchronous input recording, memory-pressure handling, UI phase transitions, tests, documentation, localization, and build wiring. ChangesCritical Memory Pressure Hibernation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MemoryPressureMonitor
participant AgentHibernationMemoryPressureResponder
participant AgentHibernationController
participant AgentHibernationPlanner
participant TerminalSurfaceRuntimeTeardownCoordinator
participant ProcessExitObservation
MemoryPressureMonitor->>AgentHibernationMemoryPressureResponder: shedMemory(for: snapshot)
AgentHibernationMemoryPressureResponder->>AgentHibernationController: reclaimIdleAgentsForSystemMemoryPressure(...)
AgentHibernationController->>AgentHibernationPlanner: selectedPanelKeys(trigger: .systemMemoryPressure)
AgentHibernationPlanner-->>AgentHibernationController: bounded eligible panels
AgentHibernationController->>TerminalSurfaceRuntimeTeardownCoordinator: reserve and enqueue isolated teardown
AgentHibernationController->>ProcessExitObservation: observeCommittedTermination(...)
ProcessExitObservation-->>AgentHibernationController: exact-generation exit result
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 errors, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@codex review @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Latest HEAD 4cfff09: @codex review @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Latest HEAD 102e733: @codex review @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
|
Latest HEAD 5839b0e: @codex review @coderabbitai review @greptile-apps review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
…wth-panics # Conflicts: # web/messages/en.json # web/messages/ja.json
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/App/AgentHibernationController.swift (1)
265-340: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear expired
.systemMemoryPressureconfirmations so scheduled hibernation can proceed.A stale
.systemMemoryPressureconfirmation survivespruneTrackingStatebecause that only removes.scheduledentries, and later.scheduledcalls block on any existing.systemMemoryPressureconfirmation before creation. The memory-pressure task also leaves them behind on the initialisPressureStillCritical()return, sleepcatch, and subsequentisPressureStillCritical()return before teardown completion. Treat an expired non-in-flight.systemMemoryPressureconfirmation as stale/clearable during.scheduledevaluation, or clear it on every early-exit path of the memory-pressure task.🤖 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/App/AgentHibernationController.swift` around lines 265 - 340, Update evaluateConfirmation so a non-in-flight .systemMemoryPressure confirmation whose dueAt has expired is removed before the existing guard blocks .scheduled evaluation, allowing a new scheduled confirmation to be created. Preserve active confirmations and any teardown-in-flight state, and retain the current behavior for unexpired memory-pressure confirmations.
🤖 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/AgentHibernationProcessTerminationTests.swift`:
- Around line 101-126: Add a test alongside rejectsProcessOutsidePaneScope named
rejectsScopeWithUnrecordedProcessIdentity that supplies a
ProcessTerminationScope whose processIDs includes a PID absent from
processIdentities, then invokes validatedScopedProcessTerminations with valid
providers and asserts the result is nil.
In `@Sources/App/AgentHibernationController`+ProcessTermination.swift:
- Around line 176-220: Move the post-termination validity checks in
commitConfirmedTeardown, especially AgentHibernationTrackingGate.isEnabled(),
ownership, protection, generation, and panel-state checks, to before
terminateScopedProcessesForHibernation sends SIGTERM. Preserve the
post-termination process/index and terminal-input validation, and ensure any
checks that can change during termination are revalidated without leaving a
terminated agent in a non-hibernated state; if post-termination validation still
fails, explicitly reconcile the panel state rather than silently returning
false.
---
Outside diff comments:
In `@Sources/App/AgentHibernationController.swift`:
- Around line 265-340: Update evaluateConfirmation so a non-in-flight
.systemMemoryPressure confirmation whose dueAt has expired is removed before the
existing guard blocks .scheduled evaluation, allowing a new scheduled
confirmation to be created. Preserve active confirmations and any
teardown-in-flight state, and retain the current behavior for unexpired
memory-pressure confirmations.
🪄 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 Plus
Run ID: 27da2b23-ab5d-491c-b5bc-6950ca43c819
📒 Files selected for processing (38)
CLI/cmux.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Runtime/AgentHibernationRecording.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/FakeHibernationRecorder.swiftResources/Localizable.xcstringsSources/App/AgentHibernationController+Confirmation.swiftSources/App/AgentHibernationController+InFlightTeardown.swiftSources/App/AgentHibernationController+MemoryPressure.swiftSources/App/AgentHibernationController+PanelLifecycle.swiftSources/App/AgentHibernationController+ProcessTermination.swiftSources/App/AgentHibernationController+Records.swiftSources/App/AgentHibernationController+Teardown.swiftSources/App/AgentHibernationController.swiftSources/App/AgentHibernationMemoryPressureResponder.swiftSources/App/AgentHibernationPlanner.swiftSources/App/WorkspaceRuntimeSettings.swiftSources/AppDelegate+PaneMemoryGuardrail.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/GhosttyTerminalView.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/TerminalSurfaceRuntimeWiring.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationPlannerSwiftTests.swiftcmuxTests/AgentHibernationProcessTerminationTests.swiftcmuxTests/AgentHibernationTrackingLifecycleTests.swiftcmuxTests/AgentResumeLivenessTests.swiftcmuxTests/CompletedRestoredAgentGenerationTests.swiftcmuxTests/SharedLiveAgentIndexAgentLivenessTests.swiftcmuxUITests/SettingsTerminalBehaviorUITests.swiftdocs/agent-hooks.mddocs/cli-contract.mddocs/configuration.mdweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
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 (4)
Sources/TerminalController.swift (2)
12712-12719: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset snapshots using the same key used when storing them.
panelSnapshotstores entries underterminalPanel.id, butpanelSnapshotResetremovestarget.surfaceID. These are distinct identifiers, so reset leaves the previous snapshot behind and the next capture reports stale pixel changes.Proposed fix
- guard let panelId = resolveSurfaceId(from: panelArg, tab: tab), - let snapshotSurfaceID = tab.terminalInputTarget(forPanelID: panelId)?.surfaceID else { + guard let panelId = resolveSurfaceId(from: panelArg, tab: tab), + let terminalPanel = tab.terminalInputTarget(forPanelID: panelId)?.panel else { result = "ERROR: Surface not found" return } Self.panelSnapshotLock.lock() - Self.panelSnapshots.removeValue(forKey: snapshotSurfaceID) + Self.panelSnapshots.removeValue(forKey: terminalPanel.id)Also applies to: 12847-12851
🤖 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/TerminalController.swift` around lines 12712 - 12719, Update the snapshot reset logic in panelSnapshotReset to remove the entry using the terminal panel identifier used by panelSnapshot when storing snapshots, rather than snapshotSurfaceID. Apply the same key correction to the corresponding reset path around the additional affected location, while preserving the existing panel-resolution validation and locking.
12717-12719: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGive
panelSnapshotsone synchronization owner.The changed code adds manual locking around process-global mutable state while both call paths already execute through
v2MainSync; no rationale explains why an actor orMainActor-owned store cannot own this dictionary. Centralize access behind one actor/MainActor owner, or document and enforce a single lock-protected access path if off-main callers are genuinely required.As per coding guidelines, new manual locks around shared mutable state require a documented reason an actor or
MainActor-isolated model cannot provide synchronization.Also applies to: 12846-12851
🤖 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/TerminalController.swift` around lines 12717 - 12719, Give panelSnapshots a single synchronization owner instead of adding ad hoc locking in the removal and related access paths. Prefer centralizing the dictionary behind the existing v2MainSync/MainActor-isolated flow; if off-main access is required, route every read and mutation through one lock-protected abstraction and document why actor isolation is insufficient. Update both the shown removal path and the corresponding access path around panelSnapshots.Source: Coding guidelines
Sources/GhosttyTerminalView.swift (2)
6307-6329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
#if DEBUG"debug…" accessors are test-only production seams.
debugWordPathSnapshotTerminalPanelID()anddebugCanApplyMountedSearchFieldFocusRequest()are new,#if DEBUG-guarded,debug…-named wrapper accessors added solely to expose private state (wordPathSnapshotTerminalPanel,canApplyMountedSearchFieldFocusRequest) for tests, with no production caller. This matches the pattern the repo's custom lint rule calls out for productionSources/files.As per path instructions: "flag added test-only or debug-only seams:
#if DEBUG(or other test-build-guarded) extensions/members that expose internal state for tests or a debugger with no production caller, members named likedebug…... Prefer reaching internal state from the test target via@testable importafter wideningprivatetointernal; isolate genuinely debug-only facilities in a dedicated debug file or folder."Also applies to: 9749-9752
🤖 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/GhosttyTerminalView.swift` around lines 6307 - 6329, Remove the `#if` DEBUG wrapper accessors debugWordPathSnapshotTerminalPanelID and debugCanApplyMountedSearchFieldFocusRequest. Widen the underlying members wordPathSnapshotTerminalPanel and canApplyMountedSearchFieldFocusRequest from private to internal so tests can access them through `@testable` import, preserving their existing behavior without production debug seams.Source: Path instructions
7735-7735: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWire or remove
onExplicitTerminalInput.
TerminalPanel.configureTerminalPanel(...)assigns the closure, andterminalSurfaceDidReceiveExplicitInput()invokes it, butGhosttyTerminalView.swiftnever wires the assignment into any explicit-input path. Add asetExplicitTerminalInputHandler(_:)/propagation step and the relevantterminalSurfaceDidReceiveExplicitInput()call; otherwise this closure is dead code.🤖 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/GhosttyTerminalView.swift` at line 7735, Wire the onExplicitTerminalInput closure through GhosttyTerminalView: add a setExplicitTerminalInputHandler(_:) propagation method and invoke the handler from terminalSurfaceDidReceiveExplicitInput(). Ensure TerminalPanel.configureTerminalPanel(...) reaches this handler so explicit terminal input triggers the assigned closure; otherwise remove the unused property.
🤖 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/App/AgentHibernationController`+ProcessTermination.swift:
- Around line 292-306: Update the teardown signaling flow around
signalErrorProvider and waitForExactProcessGenerationsToExitWithoutTimeout to
track targets that return non-ESRCH errors after teardownIsCommitted. Exclude
those unsignalable targets from scopedProcessTerminations, or otherwise
transition the panel to a resumable failure state, so the panel cannot remain
permanently .terminating; preserve the existing first-target failure rejection
behavior.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 6307-6329: Remove the `#if` DEBUG wrapper accessors
debugWordPathSnapshotTerminalPanelID and
debugCanApplyMountedSearchFieldFocusRequest. Widen the underlying members
wordPathSnapshotTerminalPanel and canApplyMountedSearchFieldFocusRequest from
private to internal so tests can access them through `@testable` import,
preserving their existing behavior without production debug seams.
- Line 7735: Wire the onExplicitTerminalInput closure through
GhosttyTerminalView: add a setExplicitTerminalInputHandler(_:) propagation
method and invoke the handler from terminalSurfaceDidReceiveExplicitInput().
Ensure TerminalPanel.configureTerminalPanel(...) reaches this handler so
explicit terminal input triggers the assigned closure; otherwise remove the
unused property.
In `@Sources/TerminalController.swift`:
- Around line 12712-12719: Update the snapshot reset logic in panelSnapshotReset
to remove the entry using the terminal panel identifier used by panelSnapshot
when storing snapshots, rather than snapshotSurfaceID. Apply the same key
correction to the corresponding reset path around the additional affected
location, while preserving the existing panel-resolution validation and locking.
- Around line 12717-12719: Give panelSnapshots a single synchronization owner
instead of adding ad hoc locking in the removal and related access paths. Prefer
centralizing the dictionary behind the existing v2MainSync/MainActor-isolated
flow; if off-main access is required, route every read and mutation through one
lock-protected abstraction and document why actor isolation is insufficient.
Update both the shown removal path and the corresponding access path around
panelSnapshots.
🪄 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 Plus
Run ID: a4b8a2a7-c8b4-44a5-9e0f-fad65a9892f0
📒 Files selected for processing (27)
CLI/cmux.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/TerminalSection.swiftResources/Localizable.xcstringsSources/AgentHibernation/AgentHibernationPanelPhase.swiftSources/App/AgentHibernationController+InFlightTeardown.swiftSources/App/AgentHibernationController+PanelLifecycle.swiftSources/App/AgentHibernationController+ProcessTermination.swiftSources/App/AgentHibernationController+Teardown.swiftSources/App/AgentHibernationController+TeardownValidation.swiftSources/App/AgentHibernationController.swiftSources/DockSplitStore+PanelDestruction.swiftSources/DockSplitStore+Reset.swiftSources/DockSplitStore+SessionRestore.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/DockSplitStore.swiftSources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel+AgentHibernation.swiftSources/Panels/TerminalPanel.swiftSources/Panels/TerminalPanelView.swiftSources/TerminalController.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationProcessTerminationTests.swiftcmuxTests/AgentHibernationTrackingLifecycleTests.swiftweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/App/AgentHibernationController+ProcessTermination.swift (2)
40-80: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve rejected scopes in the teardown result dictionary.
scopedProcessTerminationspre-populates entries for empty scopes, but omittedvalidatedScopedProcessTerminations(...)tasks are left absent from the returned dictionary. Since callers useguard scopedProcessTerminationsByPanel[record.key] else { continue }, using a fallback such asresult[key] ?? []would bypass identity/scope validation and allow an empty process set to proceed tocommitConfirmedTeardown.🤖 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/App/AgentHibernationController`+ProcessTermination.swift around lines 40 - 80, Ensure scopedProcessTerminations includes a dictionary entry for every input scope, including scopes whose validatedScopedProcessTerminations task is rejected or produces no result. Preserve the distinction between a present empty array and a missing key so callers’ guarded lookup continues to skip rejected scopes rather than treating them as valid empty terminations; update the task/result aggregation around validatedScopedProcessTerminations accordingly.
137-192: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReclaim ownership on late aborts after the monitor is armed.
In
commitConfirmedTeardown,.rejectedat line 229 and the pre-SIGTERM safety-check failure after line 177 return early without cleanup. Add a cleanup/transfer path here so the monitor/cancellation state is relinquished and the snapshot is retained as recovery (or disposed when it is no longer guarded) rather than leaving tracking state/snapshot ownership behind.🤖 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/App/AgentHibernationController`+ProcessTermination.swift around lines 137 - 192, Update commitConfirmedTeardown so late aborts—both the .rejected branch and the pre-SIGTERM teardownIsStillSafe failure—reclaim all monitor and cancellation state. Reuse the existing snapshot cleanup/transfer helpers to retain the snapshot for recovery while it remains guarded, or dispose it once protection is no longer active, and ensure restoreOwnedSnapshotPaths and tracking state are not left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/App/AgentHibernationController`+ProcessTermination.swift:
- Around line 40-80: Ensure scopedProcessTerminations includes a dictionary
entry for every input scope, including scopes whose
validatedScopedProcessTerminations task is rejected or produces no result.
Preserve the distinction between a present empty array and a missing key so
callers’ guarded lookup continues to skip rejected scopes rather than treating
them as valid empty terminations; update the task/result aggregation around
validatedScopedProcessTerminations accordingly.
- Around line 137-192: Update commitConfirmedTeardown so late aborts—both the
.rejected branch and the pre-SIGTERM teardownIsStillSafe failure—reclaim all
monitor and cancellation state. Reuse the existing snapshot cleanup/transfer
helpers to retain the snapshot for recovery while it remains guarded, or dispose
it once protection is no longer active, and ensure restoreOwnedSnapshotPaths and
tracking state are not left behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4711a97c-db8b-40ac-b291-0fbbd40de8a3
📒 Files selected for processing (1)
Sources/App/AgentHibernationController+ProcessTermination.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Latest HEAD a751a4b: @codex review @coderabbitai review @greptile-apps review |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
|
CodeRabbit review 4803792901 follow-up:
|
|
Latest HEAD 120713c: @codex review @coderabbitai review @greptile-apps review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
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/DockSplitStore.swift`:
- Around line 605-608: Update the reconciliation logic around staleTabIds and
discardPanelStateAndClose so it removes only mappings whose surface IDs are
stale, rather than filtering by stale panel IDs. After removal, close each
affected panel only if no remaining surface-to-panel mapping references it,
preserving ownership for panels still referenced by live tabs.
🪄 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 Plus
Run ID: 9e8a3a9d-d824-4d62-9c2a-4a1acc9a42e5
📒 Files selected for processing (7)
Sources/App/AgentHibernationController+ProcessTermination.swiftSources/App/AgentHibernationController+Teardown.swiftSources/DockSplitStore+PanelDestruction.swiftSources/DockSplitStore+Reset.swiftSources/DockSplitStore.swiftcmuxTests/AgentHibernationProcessTerminationTests.swiftcmuxTests/AgentHibernationRestoreMonitorTests.swift
…wth-panics # Conflicts: # Sources/DockSplitStore+Reset.swift # Sources/DockSplitStore.swift # Sources/Workspace+PanelLifecycle.swift # Sources/Workspace.swift
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/App/AgentHibernationController+Records.swift (1)
61-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local
panelProcessIDs— it means something different from the record field of the same name.The local is
processEntry?.processIDs(used only forhasLiveProcess), whileAgentHibernationRecord.panelProcessIDsis assignedprocessEntry?.hibernationPanelProcessIDs. Same identifier, two different PID sets, three lines apart, in a gate that decides which processes get SIGTERM'd.♻️ Suggested rename
- let panelProcessIDs = processEntry?.processIDs ?? [] + let livenessProcessIDs = processEntry?.processIDs ?? [] @@ - hasLiveProcess: !panelProcessIDs.isEmpty, + hasLiveProcess: !livenessProcessIDs.isEmpty,🤖 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/App/AgentHibernationController`+Records.swift around lines 61 - 80, Rename the local derived from processEntry?.processIDs in the record-building code to clearly distinguish it from AgentHibernationRecord.panelProcessIDs, and update the hasLiveProcess check to use the renamed local. Leave the panelProcessIDs record field assignment sourced from hibernationPanelProcessIDs unchanged.Sources/DockSplitStore+SurfaceTransfer.swift (1)
281-286: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAttach-failure rollbacks don't undo
updateWorkspaceId, so hibernation tracking can be orphaned.Both overloads retarget the panel to the Dock (
terminal.updateWorkspaceId(workspaceId)at Line 255 / Line 344) before the Bonsplit mutation, but the failure branches only roll backpanels,surfaceIdToPanelId, and the cached transfer. The hibernation tracking added in this PR still lives underdetached.sourceWorkspaceId, while the panel now reports the Dock workspace — so a subsequentpanel.close()discards the wrong key and leaves the source-keyed tracking (and its committed-termination observation) behind.Restore the previous workspace id in both rollbacks.
🐛 Suggested rollback fix (split variant shown)
guard let newPane else { surfaceIdToPanelId.removeValue(forKey: tab.id) detachedSurfaceTransfersByPanelId.removeValue(forKey: detached.panelId) panels.removeValue(forKey: detached.panelId) clearSessionRestoreState(panelId: detached.panelId) + if let terminal = panel as? TerminalPanel { + terminal.updateWorkspaceId(detached.sourceWorkspaceId) + } else if let browser = panel as? BrowserPanel { + browser.updateWorkspaceId(detached.sourceWorkspaceId) + } return nil }Also applies to: 374-380
🤖 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/DockSplitStore`+SurfaceTransfer.swift around lines 281 - 286, Update both attach-failure rollback branches in the split and non-split transfer paths to restore the panel’s previous workspace ID after the earlier updateWorkspaceId call. Use detached.sourceWorkspaceId when reverting the affected panel, alongside the existing panels, surface mapping, cached transfer, and session restore cleanup.
🤖 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/AgentHibernationProcessSnapshotCoordinatorTests.swift`:
- Around line 34-47: Replace the non-deterministic await Task.yield() in the
nextSnapshot coalescing test with a real registration signal: await a second
beforeCapture/waiter-registered event, or use a deadline-bounded poll of the
coordinator’s queued-waiter state. Release allowCapture only after the second
waiter is confirmed queued, preserving the captureCount == 1 assertion.
In `@cmuxTests/AgentHibernationProcessTerminationTests.swift`:
- Line 12: Mark the AgentHibernationProcessTerminationTests suite with the
serialized suite trait, matching AgentHibernationRestoreMonitorTests and
AgentHibernationTerminationFailureTests, so tests that observe
AgentHibernationController.shared cannot run concurrently.
- Around line 243-247: Remove the redundant contains assertions on
escalatedTargets in the test, keeping the exact equality assertion to [-101] as
the sole validation of the escalated targets before finishing
postKillDeadline.continuation.
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownCoordinator.swift`:
- Around line 182-190: Replace the preconditionFailure guard in the isolated
hibernation teardown case with an if-let validation of the reservation,
execution slot, and queue index; run the isolated teardown body only when all
resolve successfully, otherwise fall back to the serialized teardown lane.
Ensure the switch case is not skipped and the surface is freed exactly once.
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface`+RuntimeLifecycle.swift:
- Around line 394-415: Release any pending agent hibernation runtime teardown
reservation from the surface teardown lifecycle so it cannot outlive the
surface. Update teardownSurface() to invoke the existing cancellation path
cancelAgentHibernationRuntimeTeardownReservation(), and ensure deinit or the
shared teardown path covers all destruction cases without affecting committed
teardown tickets.
In
`@Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeTeardownCoordinatorTests.swift`:
- Around line 169-183: Reorder the exhaustion assertion in the test so it runs
immediately after acquiring secondReservation and before enqueueRuntimeTeardown
consumes it. Move the reserveIsolatedHibernationTeardown() == nil expectation
ahead of the second ticket enqueue, preserving the existing reservation and
teardown setup otherwise.
In `@Sources/App/AgentHibernationController`+PanelLifecycle.swift:
- Around line 74-95: Update the cleanup Task created in the panel lifecycle flow
so every exit path, including cancellation and failed identity checks, removes
committedTerminationCleanupByPanelID[panelID] when its requestID still matches
cleanupID. Preserve the existing success-path observation removal and completion
handling, and verify no other cleanup path is responsible for pruning this
entry.
In `@Sources/App/AgentHibernationController`+ProcessExitObservation.swift:
- Around line 57-89: Extract the repeated committed-observation identity check
into a private helper named
isCurrentCommittedTerminationObservation(panelID:requestID:) on the relevant
controller. Replace the repeated committedTerminationObservationsByPanelID
requestID comparisons in all three task bodies, including the additional
locations noted, while preserving each existing !Task.isCancelled guard and
return behavior.
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 310-336: Review the `hasCompleteProcessGroups` validation in
`agentHibernationProcessScope` and its downstream `containsUnrelatedProcess`
handling to confirm whether missing process-group leaders should remain a hard
fail-closed veto. If orphaned leaders are valid for otherwise verified
descendant groups, relax this check so live members can pass without requiring
the leader PID to resolve; otherwise preserve the strict behavior and add or
update coverage documenting the intentional tradeoff.
---
Outside diff comments:
In `@Sources/App/AgentHibernationController`+Records.swift:
- Around line 61-80: Rename the local derived from processEntry?.processIDs in
the record-building code to clearly distinguish it from
AgentHibernationRecord.panelProcessIDs, and update the hasLiveProcess check to
use the renamed local. Leave the panelProcessIDs record field assignment sourced
from hibernationPanelProcessIDs unchanged.
In `@Sources/DockSplitStore`+SurfaceTransfer.swift:
- Around line 281-286: Update both attach-failure rollback branches in the split
and non-split transfer paths to restore the panel’s previous workspace ID after
the earlier updateWorkspaceId call. Use detached.sourceWorkspaceId when
reverting the affected panel, alongside the existing panels, surface mapping,
cached transfer, and session restore cleanup.
🪄 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 Plus
Run ID: 64fc3d1a-7382-4a38-add5-c12958808b85
📒 Files selected for processing (57)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownAdmission.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownCompletion.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownCoordinator.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownExecutionLane.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownRequest.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownReservation.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Lifecycle/TerminalSurfaceRuntimeTeardownTicket.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+RuntimeLifecycle.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceRuntimeTeardownCoordinatorTests.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/TerminalSurfaceTeardownCallbackLifetimeTests.swiftResources/Localizable.xcstringsSources/AgentHibernation/AgentHibernationPanelPhase.swiftSources/App/AgentHibernationController+CommittedTerminationCleanup.swiftSources/App/AgentHibernationController+CommittedTerminationObservation.swiftSources/App/AgentHibernationController+InFlightTeardown.swiftSources/App/AgentHibernationController+PanelLifecycle.swiftSources/App/AgentHibernationController+ProcessExitObservation.swiftSources/App/AgentHibernationController+ProcessExitWaiting.swiftSources/App/AgentHibernationController+ProcessSignaling.swiftSources/App/AgentHibernationController+ProcessTermination.swiftSources/App/AgentHibernationController+Records.swiftSources/App/AgentHibernationController+ScopedProcessTerminationResult.swiftSources/App/AgentHibernationController+Teardown.swiftSources/App/AgentHibernationController+TeardownValidation.swiftSources/App/AgentHibernationController.swiftSources/App/AgentHibernationPlanner.swiftSources/App/AgentHibernationProcessExitCompletion.swiftSources/App/AgentHibernationProcessExitEpoch.swiftSources/App/AgentHibernationProcessSnapshotCoordinator.swiftSources/App/AgentHibernationTranscriptGuard+PostTeardownRestore.swiftSources/CmuxTopSnapshot.swiftSources/DockSplitStore+PanelDestruction.swiftSources/DockSplitStore+SessionRestore.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/DockSplitStore.swiftSources/GhosttyTerminalView.swiftSources/Panels/AgentHibernationPlaceholderMode.swiftSources/Panels/TerminalPanel+AgentHibernation.swiftSources/Panels/TerminalPanel.swiftSources/Panels/TerminalPanelView.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/TerminalSurfaceRuntimeWiring.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationPlannerSwiftTests.swiftcmuxTests/AgentHibernationProcessSignalBoundaryTests.swiftcmuxTests/AgentHibernationProcessSnapshotCoordinatorTests.swiftcmuxTests/AgentHibernationProcessTerminationTests.swiftcmuxTests/AgentHibernationRestoreMonitorTests.swiftcmuxTests/AgentHibernationTerminationFailureTests.swiftcmuxTests/AgentHibernationTrackingLifecycleTests.swiftcmuxTests/AgentResumeLivenessTests.swiftcmuxTests/CompletedRestoredAgentGenerationTests.swiftcmuxTests/DockRuntimeParityTests.swift
💤 Files with no reviewable changes (1)
- Sources/App/AgentHibernationController+InFlightTeardown.swift
1 participant
Summary
AgentHibernationControlleras the single MainActor lifecycle owner, with explicitlive → terminating → recovering → terminationFailed/hibernatedphases and one stable committed request identity across exit observation, native teardown, recovery, retry, and panel-close cleanup.ghostty_surface_freecannot strand the second pressure candidate or ordinary panel close. Callback/tee/manual-I/O userdata remains alive until native free returns.mainfont-size-transfer ownership invariants. No bonsplit or Ghostty submodule change is part of this PR.This targets the failure mode in #8997: renderer-only reclamation cannot release memory held by long-lived agent process trees, while a confirmed idle resumable agent can be snapshotted, terminated within an exact process authority, and restored on demand before system pressure reaches WindowServer/watchdog failure.
Regression coverage
The regression commits precede the implementation commits so the PR history shows the failures before the fix. Behavioral coverage includes:
Tests inject signal/exit providers and never signal arbitrary live processes.
Validation
origin/mainmerge.git diff --check,check-pbxproj, test wiring (630 files), workspace package grouping, Package.resolved policy, JSON localization parsing, and canonical cmux policy checks pass.TerminalSurfaceRuntimeTeardownCoordinatorTests5/5 andTerminalSurfaceTeardownCallbackLifetimeTests8/8.ghostty-internal.ainstead oflib…; aftermain's Ghostty API bump, the local generated artifact also lacks the new font-size callback declarations. This PR does not modify the generated XCFramework; GitHub CI/cloud build supplies the matching artifact..github/swift-file-length-budget.tsv,swift-warning-budget.tsv, andscripts/swift_file_length_budget.pyare absent on currentorigin/main; no budget TSV was generated or modified.Localization audit: the new finishing/failure/retry UI copy uses localized keys with English and Japanese entries, matching the existing agent-hibernation catalog coverage. Changed localization JSON parses cleanly and the changed Swift files contain no new bare user-facing English.
Closes #8997
Summary by CodeRabbit