fix(server): preserve newer runtime state on shutdown

[reubengrussell]

What Changed

• Return the persisted runtime owner from the server resource acquisition.
• Remove runtime state only when pid and startedAt still match that owner.
• Keep unconditional cleanup for callers that deliberately clear confirmed stale state.
• Add tests for matching-owner cleanup and newer-owner preservation.

Why

Two T3 servers can overlap briefly during an SSH reconnect. An older server previously removed the shared server-runtime.json during shutdown, even after a newer server replaced it. This left endpoint discovery without the current server record.

The owner check keeps the newer record while preserving existing stale-state cleanup behavior.

Checklist

• This PR is small and focused
• I explained what changed and why
• UI screenshots are not applicable
• Animation or interaction video is not applicable

Validation:

• vp test run apps/server/src/serverRuntimeState.test.ts: 8 tests passed.
• vp run --filter t3 typecheck: passed with existing suggestions in unrelated files.
• Focused lint passed.
• Context documentation is not needed. This change does not add domain language or a durable product decision.

---

Note

Medium Risk
Changes shared runtime-state lifecycle on shutdown; incorrect owner matching could leave stale files or skip needed cleanup, though behavior is covered by new tests and CLI paths stay unconditional.

Overview
Fixes a race where an older server process could delete server-runtime.json on shutdown after a newer server had already written it (e.g. brief overlap on SSH reconnect), breaking endpoint discovery.

clearPersistedServerRuntimeState now accepts an optional owner (pid + startedAt). When provided, it reads the file and skips removal unless the on-disk record still matches that owner. Callers that omit the owner (e.g. CLI stale cleanup) still delete unconditionally.

The server acquireRelease hook returns the persisted state (or null when no port was written) and passes it into shutdown cleanup so only the stopping instance clears its own record.

^{Reviewed by Cursor Bugbot for commit 0d22f86. Bugbot is set up for automated code reviews on this repo. Configure here.}

Note

Fix clearPersistedServerRuntimeState to skip clearing newer server state on shutdown

• On shutdown the server now passes the state it acquired at startup as an expectedOwner to clearPersistedServerRuntimeState(path, expectedOwner) so it only deletes the persisted file when pid and startedAt still match the current instance
• If the server had no numeric port/address, the release phase receives null and skips the clear entirely
• Adds tests verifying the file is removed on owner match and preserved when the stored state belongs to a different/newer server
• Behavioral Change: clearPersistedServerRuntimeState signature gains a second optional expectedOwner parameter; callers that omit it keep the old unconditional-delete behavior

📊 Macroscope summarized 0d22f86. 2 files reviewed, 2 issues evaluated, 1 issue filtered, 1 comment posted

🗂️ Filtered Issues

apps/server/src/serverRuntimeState.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 96: The owner check is a time-of-check/time-of-use race: after readPersistedServerRuntimeState(path) confirms that the old instance owns the file, a newer server can atomically replace the state before fs.remove(path) runs. The old shutdown then unlinks the newer server's record, recreating the endpoint-discovery failure this guard is intended to prevent. [ Cross-file consolidated ]

[macroscopeapp]

🟠 High src/server.ts:518

The old server can delete a newer server's runtime-state file: clearPersistedServerRuntimeState verifies ownership with readPersistedServerRuntimeState, then performs fs.remove separately, allowing an overlapping startup to replace the file between those operations. This removes the active endpoint record and breaks discovery; the check-and-remove must be atomic (or otherwise revalidate ownership immediately before deletion).

Also found in 1 other location(s)

apps/server/src/serverRuntimeState.ts:96

The owner check is a time-of-check/time-of-use race: after readPersistedServerRuntimeState(path) confirms that the old instance owns the file, a newer server can atomically replace the state before fs.remove(path) runs. The old shutdown then unlinks the newer server's record, recreating the endpoint-discovery failure this guard is intended to prevent.

🤖 Copy this AI Prompt to have your agent fix this:

In file @apps/server/src/server.ts around line 518:

The old server can delete a newer server's runtime-state file: `clearPersistedServerRuntimeState` verifies ownership with `readPersistedServerRuntimeState`, then performs `fs.remove` separately, allowing an overlapping startup to replace the file between those operations. This removes the active endpoint record and breaks discovery; the check-and-remove must be atomic (or otherwise revalidate ownership immediately before deletion).

Also found in 1 other location(s):
- apps/server/src/serverRuntimeState.ts:96 -- The owner check is a time-of-check/time-of-use race: after `readPersistedServerRuntimeState(path)` confirms that the old instance owns the file, a newer server can atomically replace the state before `fs.remove(path)` runs. The old shutdown then unlinks the newer server's record, recreating the endpoint-discovery failure this guard is intended to prevent.

[macroscopeapp]

Approvability

Verdict: Would Approve

Macroscope's review found this PR approvable — This is a focused three-file shutdown bug fix that preserves newer runtime-state records while retaining existing unconditional cleanup for other callers, with targeted tests and no schema, deployment, security, billing, or default changes. An unresolved High-severity finding still identifies a possible check-then-remove race in the ownership guard, which is a separate blocking correctness concern.

Not approved because:

• 1 blocking correctness issue found at or above your repo's Minimum Blocking Severity

^{Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.}

[ericsngyun]

This fixes one of the triggers in #5749. There, three reports (Sep 15, 24 and 26) describe a stopped server deleting the live service's server-runtime.json, which leads the next SSH reconnect to start a second server. I checked this PR against that sequence end to end, with real servers.

Setup: two worktrees: main at 8ed276c24 and main with this PR merged (clean merge, and packages/ssh is identical in both). I ran real node apps/server/src/bin.ts serve processes. The launch and stop scripts came from buildRemoteLaunchScript / buildRemoteStopScript in the same checkout. HOME was an isolated directory under /tmp. Ubuntu 24.04 x86_64, Node 24.19.

Sequence:

1. The launcher starts a managed server.
2. A service-style server starts on the same base dir and records itself in server-runtime.json.
3. The launcher runs again. It adopts the service as external and stops its own managed server.
4. The desktop treats the tunnel as stale, so the stop script runs, then the launch script.

	main	main + this PR
server-runtime.json after step 3	missing (the managed server's shutdown removed the service's record)	kept (service PID)
Step 4 result	serverKind: "managed": a second server starts next to the live service	serverKind: "external": the service is adopted

Each build gave the same result in 3 out of 3 runs.

This matches what I saw on a real host (0.0.45, macOS desktop → SSH → systemd service). The launcher adopted the service at 14:28, and the old managed server shut down at 14:28:58. At 14:40 a stale-tunnel stop and relaunch started a managed server on 3774, while the service was still healthy (NRestarts=0).

Not covered:

• The check-then-remove race Macroscope flagged. These runs don't overlap a startup with a shutdown.
• I used a placeholder apps/web/dist/index.html so that the launcher's GET / readiness probe returns 200, as it does with the release binary.
• I only tested the SSH launcher path.