Skip to content

fix(ssh): restart managed server after runner update#4290

Open
Kron00 wants to merge 2 commits into
pingdotgg:mainfrom
Kron00:fix/ssh-managed-runner-restart
Open

fix(ssh): restart managed server after runner update#4290
Kron00 wants to merge 2 commits into
pingdotgg:mainfrom
Kron00:fix/ssh-managed-runner-restart

Conversation

@Kron00

@Kron00 Kron00 commented Jul 22, 2026

Copy link
Copy Markdown

What Changed

  • Recover the actual T3 server PID from server-runtime.json when the SSH state is managed and the saved/runtime ports match.
  • Persist the runtime PID after launching a managed server instead of retaining an exited npm/npx wrapper PID.
  • Avoid falling back to an unrelated runtime PID when stale managed state does not match.
  • Add an end-to-end shell regression test covering runner changes with a dead wrapper PID and a live managed server.

Why

The SSH launcher can save the npm/npx wrapper PID while the real T3 server continues under a different PID. When the runner changes, signaling the exited wrapper leaves the old server alive, and the launcher then reuses it as external. That keeps the remote environment on the previous T3 version.

The runtime descriptor is authoritative only when the existing managed marker and saved port correlate it with the launcher state, which lets the launcher restart its server without risking an unrelated process.

Addresses #3598.

Verification

  • pnpm --filter @t3tools/ssh test -- --run
  • pnpm --filter @t3tools/ssh typecheck
  • pnpm exec vp lint packages/ssh/src/tunnel.ts packages/ssh/src/tunnel.test.ts
  • pnpm exec vp fmt --check packages/ssh/src/tunnel.ts packages/ssh/src/tunnel.test.ts

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • This PR has no UI changes
  • This PR has no animation or interaction changes

Note

Fix managed SSH tunnel server to restart after runner update

  • The remote launch script in tunnel.ts previously scheduled the running managed server for termination when the runner updated, even if the server was still healthy on the correct port.
  • Now, if the saved port matches the default and the existing pid is alive, the script adopts the running process (writes its pid to the pid file) instead of stopping it.
  • When a port mismatch does occur, the script targets only $REMOTE_PID for termination rather than a fallback value.
  • After starting a new managed server, the pid file is updated to the actual runtime pid resolved via resolve_default_runtime_port.
  • A live integration test in tunnel.test.ts verifies the full restart flow, including that the old server exits and the wrapper process survives.

Macroscope summarized cae6e56.


Note

Medium Risk
Changes remote process kill/reuse logic on SSH hosts; incorrect PID handling could affect running servers, though scope is narrowed with port correlation and a regression test.

Overview
Fixes SSH remote launch so managed sessions track the real T3 server PID from server-runtime.json instead of a dead npm/npx wrapper, so a runner update actually restarts the old server instead of reusing it as external.

The launch script now saves SAVED_REMOTE_PORT before applying the default runtime port. When reuse is possible, it only adopts DEFAULT_RUNTIME_PID if managed state, the saved port, and the runtime port all match and that PID is alive—then it rewrites pid. Otherwise it stops REMOTE_PID (no fallback to the runtime PID) before demoting to external. After a fresh managed start, it resolves the runtime again and persists the server PID when the port matches.

Adds a live shell test that simulates a live server, a stale wrapper PID in state, and a runner change; it asserts the old server exits and a new managed process replaces it.

Reviewed by Cursor Bugbot for commit cae6e56. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1cef2bfb-3ea9-47f6-8ae3-87bbfee19679

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list. labels Jul 22, 2026
@Kron00
Kron00 marked this pull request as ready for review July 22, 2026 17:51
@macroscopeapp

macroscopeapp Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR modifies process lifecycle management logic in the SSH tunnel code, changing when managed servers are stopped and restarted. While the diff is small and includes comprehensive tests, changes to process management behavior warrant human review due to potential edge cases.

You can customize Macroscope's approvability policy. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant