Fix SSH authentication cleanup and production env fixture - #9703
lawrencecchen wants to merge 134 commits into
Conversation
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. |
|
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:
📝 WalkthroughWalkthroughThe PR replaces recursive SSH process cleanup with identity-validated, snapshot-based group termination. It adds owned-group lifecycle handling, bounded recovery, temporary state directories, startup-script integration, and tests for cleanup, signals, deadlines, and recovery. ChangesSSH authentication group cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SSHAttachScript
participant ForegroundAuthentication
participant OwnedGroupState
participant ProcessSnapshot
SSHAttachScript->>OwnedGroupState: Create and export authentication-group directory
SSHAttachScript->>ForegroundAuthentication: Start foreground authentication
ForegroundAuthentication->>OwnedGroupState: Publish owned process-group identity
OwnedGroupState->>ProcessSnapshot: Snapshot and validate process identities
ProcessSnapshot-->>OwnedGroupState: Return owned descendants and groups
OwnedGroupState->>ForegroundAuthentication: Freeze, reap, or terminate validated processes
SSHAttachScript->>OwnedGroupState: Remove state files and group directory
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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 |
lawrencecchen
changed the title
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift (2)
146-210: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRequire a confirmed frozen snapshot before killing members.
If all eight calls to
cmux_ssh_stop_auth_tree_membersreturn nonzero, Line 202 still creates a kill order from a snapshot that contains running members. A running parent can fork after that snapshot. Its new child is absent from the kill list and can survive cleanup.Only build
cmux_ssh_auth_tree_kill_orderafter a snapshot confirms that every rooted member is stopped. If freeze confirmation fails, use a safe failure path instead of killing an unconfirmed list.🤖 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 `@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift` around lines 146 - 210, Require successful freeze confirmation before constructing or using cmux_ssh_auth_tree_kill_order: after the retry loop, proceed only when cmux_ssh_stop_auth_tree_members has confirmed every rooted member is stopped. If all eight attempts fail, resume cmux_ssh_auth_tree_frozen_members and exit through the safe failure path instead of killing the unconfirmed snapshot.
174-185: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBuild the descendant tree without repeated full-process scans.
The
while (cmux_discovered)loop scans every process in the globalpssnapshot for every tree depth. Its cost isO(P × D)per freeze attempt, wherePis all host processes andDis tree depth. The outer retry loop can run eight times.Build a
children[parentPID]index during the input pass. Then traverse only rooted descendants with a queue. This makes discoveryO(P + D)before the required descendant sort.As per coding guidelines, production code must avoid nested full-collection scans over scalable collections.
🤖 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 `@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift` around lines 174 - 185, Replace the repeated full-process discovery loop around cmux_discovered with a children-by-parent index built during the initial cmux_process input pass, then traverse only descendants rooted at the target process using a queue. Preserve the existing depth and zombie filtering behavior, and retain the required descendant sort after traversal; update the surrounding symbols cmux_parent, cmux_depth, and cmux_process without scanning the global process collection once per depth.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift`:
- Around line 249-259: The cleanup logic in both descendant traversals must keep
the authentication root stopped until the force scan completes. In
SSHForegroundAuthenticationRetryPolicy.swift lines 249-259 and 282-289, update
the cleanup-expired handling around cmux_ssh_terminate_auth_process and
cmux_ssh_auth_cleanup_has_time so stopped-parent propagation is preserved,
including when the parent is the authentication root; apply the same change at
both sites.
---
Outside diff comments:
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift`:
- Around line 146-210: Require successful freeze confirmation before
constructing or using cmux_ssh_auth_tree_kill_order: after the retry loop,
proceed only when cmux_ssh_stop_auth_tree_members has confirmed every rooted
member is stopped. If all eight attempts fail, resume
cmux_ssh_auth_tree_frozen_members and exit through the safe failure path instead
of killing the unconfirmed snapshot.
- Around line 174-185: Replace the repeated full-process discovery loop around
cmux_discovered with a children-by-parent index built during the initial
cmux_process input pass, then traverse only descendants rooted at the target
process using a queue. Preserve the existing depth and zombie filtering
behavior, and retain the required descendant sort after traversal; update the
surrounding symbols cmux_parent, cmux_depth, and cmux_process without scanning
the global process collection once per depth.
🪄 Autofix
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: f9667b43-0657-4f9e-8bc8-4f8a3babe1f6
📒 Files selected for processing (2)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d6632e1. Configure here.
| fi | ||
| cmux_ssh_auth_recovery_unlock | ||
| fi | ||
| if [ "$cmux_ssh_auth_recovery_sweep_ready" != 1 ]; then exit 0; fi |
There was a problem hiding this comment.
Sweep worker flock wait can time out
Medium Severity
The coalesced recovery worker waits for owner publication by calling cmux_ssh_auth_recovery_lock while the parent still holds that same flock. That helper hard-caps the wait at one second, so a slow post-fork owner publish makes the child exit before work starts; the parent then sees a dead owner and aborts the schedule. Failed auth-group recovery is skipped until a later startup or cleanup schedules again.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d6632e1. Configure here.
| "$cmux_ssh_auth_stale_lock/generation" \ | ||
| "$cmux_ssh_auth_stale_lock/generation.new" \ | ||
| "$cmux_ssh_auth_stale_lock/pending" \ | ||
| "$cmux_ssh_auth_stale_lock/pending.new" 2>/dev/null || true |
There was a problem hiding this comment.
Release helper omits pending markers
Low Severity
cmux_ssh_auth_release_reaper_lock_if_current still deletes only owner, publisher, and generation files, not the new pending markers. The sweep worker’s EXIT trap uses that helper, so an abnormal exit after another scheduler wrote pending leaves sweep.lock behind because rmdir fails.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d6632e1. Configure here.
lawrencecchen
added
the
stale-revisit
2 participants
Summary
mainThese changes must land together because current
mainrequires the production environment fixture before the SSH regression suite can pass its speculative merge gate. This PR supersedes #9712 after merge.Mechanism
Authentication runs in a cmux-owned process group. Cleanup freezes and snapshots the rooted tree, revalidates process identity, terminates children before parents, and keeps durable state when it cannot prove safe completion. A lock owner records PID plus start time so later attempts can distinguish a live reaper from stale state. Attach startup injects the SSH executable through one shared builder, and the app-host tests exercise the exact
ssh -tt ... attachpath.This is a principled ownership fix. It models process lifetime and identity explicitly instead of extending timeouts or sending signals to unverified PIDs.
Validation
d5a9a783b4d0940ce481c3cf9ecc76cb7d18716cbun test tests/client-config-env.test.tsfromweb/: 17 passedSSHPTYAttachRetryScriptBuilderTests: 6 passedSSHForegroundAuthenticationMarkerCleanupTests: 5 passed, including process-tree death, retry limit, marker removal, and signal-trap coveragesgf5-0c12f0b27fc0,BUILD_OKMerge state
$autoreviewreached this exact head but the required Codex account is quota-limited until August 12, 2026 PT. That review remains a hard merge blocker.Safety
Cleanup fails closed on identity mismatch, never sends a bare unvalidated PID
KILL, resumes processes if it cannot confirm a frozen snapshot, and preserves state when the hard deadline cannot prove safe cleanup.Dictionary
Note
High Risk
Large generated shell surface around process signals, identity checks, and temp-dir locking on the SSH authentication path; regressions could leave processes stopped, leak auth state, or signal wrong PIDs.
Overview
Replaces ad hoc foreground SSH authentication teardown with owned process groups, durable temp state, and bounded recovery so cleanup cannot outlive the session or signal unverified PIDs.
Each auth attempt gets a
CMUX_SSH_AUTH_GROUP_DIRcreated at startup/attach; the classifier publishes an isolated PTY anchor and group identity there. Cleanup snapshots and journals STOP/KILL in transactions (500 ms work budget, 2 s hard deadline), validates stable identity (group + kernel start time) before signals, and rolls back STOP journals when it cannot prove every target is frozen. Failed or incomplete cleanup launches a background reaper and enqueues the directory for a per-user recovery sweep; SSH startup always schedules one recovery pass so stale groups from prior sessions are reclaimed.CLI startup and PTY attach retry scripts now wire
cmux_ssh_remove_auth_group_dirinto session end, signal handlers, and post-auth wait paths, and install termination helpers on every startup (not only when a one-time auth command exists).classifyingTransientFailureruns auth inside the owned group when a directory is present.Also fixes the reusable shell startup wrapper to decode base64 via a
cmux_payloadvariable instead of repeating the encoded literal.Reviewed by Cursor Bugbot for commit d6632e1. Bugbot is set up for automated code reviews on this repo. Configure here.