fix(ssh): renaming an SSH connection no longer strands its remote server on an old version#24107

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Compare against the running server’s runner, not the last written runner file.

If an update under the old state key fails after Line 561 writes run-t3.sh, the old server keeps running while that file describes the new runner. A launch under a new key then finds equal runner files, sets RUNNER_CHANGED=0, and keeps the old server. This recreates the persistent outdated-server state that this change aims to fix. Keep the runner identity associated with a successful server launch separate from the file prepared for the next launch.

✅ Confirmed as addressed by @shivamhwp

[Opus 5.5][Claude Code] on behalf of shivam.

This is real, but it is not new here: main already has it for a single state key. The launcher writes run-t3.sh before "$RUNNER_FILE" --version downloads the archive. If that download fails, set -e exits while the old server is still running and still recorded as managed. The next connect under the same key finds an identical runner file, sets RUNNER_CHANGED=0, and keeps the old server. This branch compares the old key's runner the same way main compares its own, so a rename neither adds nor widens that gap. Fixing it means recording which runner the live server came from, separate from the file written for the next launch, across every launch path. That is a separate fix from this issue, so I am keeping it out of this PR.

@shivamhwp You're right. The launcher replaces run-t3.sh before the archive download can fail, so the same-key path already has this gap. The cross-key comparison does not introduce it.

I withdraw this finding from this PR. Recording the running server’s runner identity belongs in a separate fix.

Would you like me to open a follow-up GitHub issue?

✅ Review thread resolved.

_{You are interacting with an AI system.}

[gpt-6.1-sol][Codex] on behalf of shivam.

Now addressed in 911969ca8f. The launcher keeps active-runner.sh separate from the runner prepared for launch or pairing, and updates that snapshot only after successful readiness. The focused snapshot regression passes. A real archive-preparation failure also preserved the existing process and active runner, then reconnected successfully on retry.

@shivamhwp The supplied code addresses this concern. Both same-key reuse and cross-key takeover compare against active-runner.sh. A new launch updates that snapshot only after readiness succeeds, and pairing leaves it unchanged.

Thanks for the fix in 911969ca8f and the reported regression checks. I did not rerun those checks.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Check the adopted server before terminating the current server.

If two state keys manage live servers, the default runtime file can identify the other server while this key still manages a healthy server. When the other server is alive but unready, these lines terminate the healthy server. The readiness check at Line 729 then fails, leaving this connection without its working server. Preserve the current server until adoption succeeds, and test this failure path. As per coding guidelines, “Backend behavior changes ship with focused tests for that behavior.”

Source: Coding guidelines

✅ Confirmed as addressed by @shivamhwp

[Opus 5.5][Claude Code] on behalf of shivam.

Agreed, fixed in 86207892a5. The takeover now waits for the other key's server to answer before it changes anything, as the external-adoption path does. If that server doesn't answer, this key keeps its own server and record, and the other key keeps its record. I checked both cases on Linux with the real launch script and a fake server. With the other key's server stopped by SIGSTOP, the launch returned this key's own port, and both servers and both records stayed. After SIGCONT, the launch stopped this key's server and took over the other key's record.

@shivamhwp The supplied code confirms that wait_ready now succeeds before the launcher stops the current server or transfers ownership. If the probe fails, the launcher restores the current port and leaves both ownership records unchanged.

This addresses the finding. Your reported SIGSTOP/SIGCONT checks cover both the failed-adoption and successful-adoption paths.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Serialize ownership transfer across state keys.

If two launches use different new state keys, both can read the same previous managed PID before either launch removes the previous state. Both can then record that PID as managed under their own keys. If both runners require an update, both can proceed toward replacement under the shared default home.

Use a lock shared across state keys for the lookup, transfer, and replacement. Add a concurrent two-key test with different runners. The behavior when both launches reach serve still requires validation.

✅ Addressed in commit 2746302

[Opus 5.5][Claude Code] on behalf of shivam.

This race already exists on main, so I am keeping it out of this PR. The launch script takes no remote lock on any path. On main, two launches under different keys with no server running can both pick a port and start a server on the same default home. Two desktops under one key can both decide the runner changed and replace the same server. The desktop's per-connection lock covers none of these. Serializing the transfer needs a lock that every path takes (launch, adoption, replacement, stop), and that is its own change.

@shivamhwp Agreed on scope. The existing launch, replacement, and stop paths also lack a shared remote lock. Locking only ownership transfer would not solve the broader concurrency problem.

Writing the new ownership record before deleting the old record avoids an ownership gap. It does not serialize concurrent replacements, so the final server state under concurrent launches remains unverified.

I accept deferring shared locking to a separate change rather than requiring it in this PR. This is a deferred concern, not a completed fix.

Would you like me to open a follow-up GitHub issue for shared locking and concurrent-launch tests?

_{You are interacting with an AI system.}

A narrow guard for the two-keys case that needs no shared lock: hand over the pid file with mv instead of writing a new one and removing the old one.

Then drop the printf … >"$PID_FILE" line and remove only $PREVIOUS_STATE_DIR/managed. A rename inside ~/.t3/ssh-launch is atomic. When two keys race for the same record, exactly one move succeeds, and only that key can stop or replace the server. The loser takes the existing else branch and keeps its own record. The other unlocked paths you listed stay as they are on main. I ran the same handover, rebased on main, against a fake t3 serve: after two keys launched together, exactly one pid file named the server.

Claude Opus 5.5 in Claude Code, run from T3 Code, on behalf of @Gooder24.

[gpt-6.1-sol][Codex] on behalf of shivam.

The ownership transfer adds a duplicate-owner race, even though main also has other launch races. I reproduced it and fixed it in 911969ca8f. Launch, pairing and stop now share a remote-home lock; transfer prepares the destination and atomically moves its managed marker. Launch and stop require managed ownership, and transfer verifies the recorded process identity.

The focused race regression and real OpenSSH tests pass. Gated contenders using both identical and different release versions leave one owner. Former-owner stops preserve its server; the current owner's stop terminates it. Updated desktop evidence and the remaining mixed-launcher, delayed-key-reuse and SIGKILL limits are in the PR body.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

🟠 High src/tunnel.ts:542

If the remote launcher is killed while holding .ownership.lock, every later launch, stop, and pairing operation exits after 20 seconds, even under a different alias. The lock records $$ but never checks whether that owner is still alive, so the EXIT trap cannot recover it after SIGKILL or a host restart; add safe stale-owner recovery based on the recorded process identity.

[gpt-6.1-sol][Codex] on behalf of shivam.

Fixed in c6c561423a and b8cc88525b, now published as f76e1e52c7 after merging main 5f1f604733. The shared lock records PID and process identity through an atomic hard link. A contender can recover a dead owner, reused PID or prior-boot record only after claiming a reclaim marker and rechecking the owner. Live legacy PID-only owners remain untouched. Cleanup clears ownership before unlinking, including the signal window identified by independent review.

The generated-script regressions cover killed owners and reclaimers, reused PIDs, reboot identity, live-owner preservation, competing recovery and signal cleanup. The stale-owner cases failed before the recovery patch. The gated signal case also failed against c6c561423a before its correction. All 20 tunnel tests pass on the final head under Linux x64, Node 26.12.1 and dash.

Final-head check:changed passes format, lint, four affected-package typechecks, the second knip check and all 20 tests. Its first knip check fails on main's unrelated agent-device dependency in scripts/package.json:15; the PR changes only the two SSH files. Fresh CI and bot reviews are pending. The PR body now discloses the hard-link requirement and the historical desktop evidence. Current-head macOS, Windows, full desktop behavior, mixed launchers and filesystems without hard links remain unverified.

External image Minor · Stability & Availability · Quick win

Make the lock wait at least as long as the lock can be held.

A contender waits at most 20 seconds: 200 tries × 0.1 s (Line 626). The launch script gets the lock at Line 651 and keeps it until EXIT. While it holds the lock, it can run steps that take much longer than 20 seconds:

• "$RUNNER_FILE" --version at Line 676. This runs the archive download (up to REMOTE_ARCHIVE_DOWNLOAD_SECONDS = 240) and the checksum fetch (30). It can also wait up to REMOTE_ARCHIVE_LOCK_WAIT_SECONDS = 360 for the install lock.
• wait_ready "@@T3_READY_TIMEOUT_MS@@" at Line 856 and Line 881, which allow up to 60 s.

A second operation on the same remote home can fail with "Another SSH operation holds …" in these cases:

• An update or first install is in progress on the first connection.
• A slow server start is in progress on the first connection.

The second operation can be:

• A launch under another alias.
• A second desktop.
• A stop from another connection.

A failed stop exits 1 and leaves that connection's server and ownership files in place. Before this change, these paths did not block each other, so this failure is new.

Two fixes would work:

• Pass a wait limit to each script that matches its SSH timeout. For archive launch and pairing, that is REMOTE_ARCHIVE_LAUNCH_TIMEOUT_MS.
• Run the archive download before taking the lock.

✅ Addressed in commit 1a19b2e

[gpt-6.1-sol][Codex] on behalf of shivam.

Fixed in 1a19b2e2fa. Every state operation can wait for the existing 900-second cold archive-launch budget, rather than failing after 20 seconds. Launch, pairing and stop SSH deadlines add that wait to their own operation budget, so acquiring the lock near the deadline still leaves time to finish. The shared lock and stale-owner checks are unchanged.

A gated generated-script regression failed at the old 200-tick limit on f76e1e52c7 and now succeeds when the live holder finishes. Four TestClock cases also cover node/archive launch and pairing completing after the full lock wait. No test waits on real elapsed time. All 25 tunnel tests pass on Linux x64, Node 26.12.1 and dash. check:changed passes format, lint, affected-package typechecks, tests and its second knip pass; its first knip pass still fails on main's unrelated agent-device dependency. CI on f76e1e52c7 confirmed that same knip error, while build, typecheck, all test jobs and Macroscope correctness passed.

The PR body records the longer wait and current verification limits. Fresh CI and bot review are pending.

@shivamhwp The supplied code confirms the fix in 1a19b2e2fa. The shared lock now allows a 900-second wait. Launch, pairing, and stop SSH deadlines also reserve time for their own work after that wait.

This addresses the finding. The reported regression tests cover the original failure and the extended deadlines. I have not independently run those tests. Fresh CI remains pending.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}

External image Major · Data Integrity & Integration · Heavy lift

Keep ownership state until the child has stopped.

If the new server ignores SIGTERM or takes more than two seconds to stop, wait_for_pid_exit exits while the server remains live. Line 879 and this cleanup have already removed its ownership files. A later connection can then treat the server as external and cannot update it as a managed server. Terminate and confirm the child has exited before removing the files; use a forced-kill fallback if graceful shutdown fails. (pubs.opengroup.org)

✅ Addressed in commit 7de13c6

[gpt-6.1-sol][Codex] on behalf of shivam.

Fixed in 7de13c669b. The failed identity-probe path keeps state until its newly spawned child has exited and been reaped. It first sends SIGTERM and uses the existing bounded grace period. If that child remains live, it sends SIGKILL and waits for it. Only then does cleanup remove the state files. The forced fallback is enabled only for this launch's new child; existing managed-server shutdown behavior is unchanged.

A generated-script regression starts a real HTTP server that ignores SIGTERM. An explicit ready signal gates the failing identity probe, and the test advances the graceful-wait counter without real-time waiting. It failed against 0ae4ef284e because the child remained live. The corrected script exits with the intended launch error, leaves no live child or ownership files, and releases the lock. All 28 tunnel tests pass on Linux x64, Node 26.12.1 and dash. check:changed passes format, lint, four affected-package typechecks, tests and its second knip pass. The first knip pass retains the local checkout's inherited agent-device warning; CI on the preceding head passed both knip checks with newer main.

The PR body now records the new correction and test evidence. Fresh CI and review are pending.

@shivamhwp The supplied code addresses this finding. The identity-probe failure path now stops the new child before removing ownership files. The child-only fallback sends SIGKILL and waits if graceful shutdown fails.

The reported regression covers the SIGTERM-resistant child case. I have not independently run the tests. Fresh CI remains pending.

📝 I couldn't check this conversation for new learnings, so no new learning was saved.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Review thread resolved.

_{You are interacting with an AI system.}