Skip to content

fix(server): prevent duplicate servers for one state directory - #9652

Open
t3dotgg wants to merge 6 commits into
mainfrom
t3code/prevent-duplicate-server-starts
Open

t3dotgg wants to merge 6 commits into
mainfrom
t3code/prevent-duplicate-server-starts

Conversation

@t3dotgg

@t3dotgg t3dotgg commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

T3 Connect could start a background service while an SSH-launched server still used the same state directory. The old server's shutdown could then delete the new server's runtime record.

Acquire an OS-backed SQLite lock for the resolved state directory before server startup. Hold it through shutdown and check a unique owner ID before removing the runtime record. Service setup refuses an unmanaged takeover and gives an explicit stop-and-retry step. SSH reconnect uses the current server address without stopping a saved PID. Update backup and rollback use the same lock.

Verification

  • 146 focused tests cover concurrent starts, symlink aliases, separate directories, crashes, stale records, old-owner cleanup, the project CLI race, SSH reconnect, service installation, and update rollback.
  • The incident release reproduced two full servers sharing temporary state and old shutdown deleting the newer record. The fixed bundle rejects the duplicate and permits a replacement after an explicit stop.
  • A real service launcher started the replacement with a temporary configuration. Node and Bun respected the same ownership lock.
  • Server and SSH typechecks, targeted lint, and the server bundle build pass.

All process tests ran locally on Linux with temporary state. No live machines or relay services changed. macOS and Windows process behavior was not tested locally. Older binaries do not implement the lock, so a live legacy runtime record blocks takeover until that server stops.

The initial relay startup error remains unproven and is not attributed to this bug.

Created with GPT-6 Astra (preview) in Codex.

Takeover

Two regressions for existing users were fixed on top of the original commits, after real-process checks on Linux.

  • A desktop backend that lost the lock exited with code 1, so the desktop restart loop retried every few seconds with no message. The refused server now exits with code 78. The desktop stops the loop on that code and shows a dialog that names the cause.
  • Desktop SSH no longer replaced its own remote server after an app update, so the remote CLI stayed on the old version forever. Reconnect now replaces a server only when the saved PID and process start time prove it is the launcher's own and the bundled runner changed. Any other server is reused as is. Verified end to end with the real launch and stop scripts against a real server: replace on runner change, reuse otherwise, stop on disconnect.
  • A failed runtime record publish after startup is a logged warning instead of a server shutdown.

Real-process checks on the lock itself: duplicate start refused, crash releases the lock, graceful stop clears the record, legacy record with a live PID refused, reused legacy PID allowed, node --watch restart works, and Bun and Node respect each other's lock.

Not verified: macOS and Windows process behavior, and the desktop dialog in a packaged build.

Original work by GPT-6 Astra (preview) in Codex. Takeover by Claude Fable 5.1 in Claude Code.

Note

Prevent duplicate servers for one state directory with SQLite ownership lock

  • Adds acquireServerOwnership in serverOwnership.ts backed by an exclusive SQLite transaction in the runtime-state directory; the lock is held for the server lifecycle and released on exit
  • Server startup acquires ownership before HTTP activation and publishes an owner-tagged runtime record; a second server attempting to start in the same directory exits with SERVER_EXIT_CODE_STATE_DIR_OWNED (78)
  • Wraps backupDatabaseOnce and restoreDatabaseBackup in serviceLauncher.ts with the same ownership lock so concurrent update/restore operations are mutually exclusive
  • CLI connect, project discovery, and bootService install now check ownership before proceeding; the desktop backend in DesktopBackendManager.ts stops permanently instead of restarting when the backend exits with the ownership status
  • SSH launch and stop scripts in tunnel.ts validate a saved PID via process start-time matching before signaling it, preventing termination of reused or foreign PIDs
  • isProcessAlive in serverRuntimeState.ts returns false for non-positive or non-integer PIDs; persistServerRuntimeState and clearPersistedServerRuntimeState are removed in favor of the ownership module
  • Risk: PersistedServerRuntimeState gains an optional ownerId field and ServerRuntimeStateError no longer covers persistence/clear operations — any out-of-tree callers of the removed functions or the old error cases will break

Macroscope summarized 4700334.


Note

High Risk
Changes core server lifecycle, discovery records, service install, and SSH cleanup across a shared state directory; mis-ownership could still allow duplicate servers or block legitimate restarts until legacy servers stop.

Overview
Introduces SQLite-backed state-directory ownership so only one T3 Code server can use a given home at a time. Startup acquires an exclusive lock (server-owner.sqlite on the realpath-resolved userdata dir), publishes server-runtime.json with an ownerId, and only removes the discovery file on shutdown when that ID still matches—fixing races where Connect/service install could run a second server and an old process could delete the newer runtime record.

CLI and service behavior now refuse silent takeover: t3 connect saves auth but skips background install if a server still holds the directory; boot service install calls requireServerStopped before first install and again after stopping the unit. Project commands treat dead PIDs as offline and no longer clear the runtime file when a live-server request fails.

SSH remote launch prefers the current server-runtime.json endpoint without killing saved PIDs, records process start time for managed launches, and only stops or blocks reconnect when start times match. The service launcher takes the same lock during SQLite backup/restore so updates do not race a foreground server.

Docs and broad integration tests cover concurrent starts, legacy records, and the original incident scenario.

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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 4, 2026
Comment thread apps/server/src/cloud/bootService.ts
Comment thread apps/server/src/cli/project.ts
@github-actions

github-actions Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.5 KiB 13.6 KiB +62 B (+0.4%) 15.1 KiB ✅
Codex Thread snapshot wire 7.0 KiB 7.0 KiB +8 B (+0.1%) 7.3 KiB ✅
Codex Live turn WebSocket wire 6.5 KiB 6.6 KiB +54 B (+0.8%) 7.8 KiB ✅
Codex Live turn WebSocket decoded 57.0 KiB 57.1 KiB +88 B (+0.2%) 66.4 KiB ✅
Codex Live turn messages 8 10 +2 (+25.0%) 21 ✅
Claude Total thread wire 13.5 KiB 13.6 KiB +25 B (+0.2%) 15.1 KiB ✅
Claude Thread snapshot wire 7.0 KiB 7.0 KiB −7 B (−0.1%) 7.3 KiB ✅
Claude Live turn WebSocket wire 6.5 KiB 6.5 KiB +32 B (+0.5%) 7.8 KiB ✅
Claude Live turn WebSocket decoded 57.8 KiB 57.8 KiB +44 B (+0.1%) 66.4 KiB ✅
Claude Live turn messages 8 9 +1 (+12.5%) 21 ✅

Baseline: d924fe2 · PR result: 4700334 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 113.8 KiB
  • Claude decoded thread snapshot: 114.5 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Comment thread apps/server/src/cloud/bootService.ts
Comment thread apps/server/src/serverOwnership.ts Outdated
Comment thread apps/server/src/serverOwnership.ts
Comment thread apps/server/src/serverOwnership.ts Outdated
Comment thread apps/server/src/serverOwnership.ts Outdated
Comment thread apps/server/src/serverOwnership.ts
Comment thread packages/ssh/src/tunnel.ts Outdated
Comment thread apps/server/src/serverOwnership.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces cross-process state-directory locking and changes server startup, service installation, desktop restart, database recovery, and SSH process-management behavior across several production paths. Unresolved findings describe risks of terminating a reused PID and tearing down a live server after state publication fails, while the change also adds static-analysis suppressions.

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.

@t3dotgg
t3dotgg force-pushed the t3code/prevent-duplicate-server-starts branch from fd8bd47 to 449ae8e Compare September 4, 2026 19:51
CURRENT_STARTED_AT=""
case "$REMOTE_PID" in
''|*[!0-9]*) ;;
*) CURRENT_STARTED_AT="$(LC_ALL=C ps -p "$REMOTE_PID" -o lstart= 2>/dev/null || true)" ;;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High src/tunnel.ts:624

The stop script can kill an unrelated process when the original server's PID is reused within the same second. ps -o lstart= only has whole-second precision, so CURRENT_STARTED_AT can equal the saved REMOTE_STARTED_AT for the replacement process; store and compare a higher-precision process start identity instead.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/ssh/src/tunnel.ts around line 624:

The stop script can kill an unrelated process when the original server's PID is reused within the same second. `ps -o lstart=` only has whole-second precision, so `CURRENT_STARTED_AT` can equal the saved `REMOTE_STARTED_AT` for the replacement process; store and compare a higher-precision process start identity instead.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 449ae8e. Configure here.

Comment thread apps/server/src/server.ts
if (typeof address === "string" || !("port" in address)) return;
const state = yield* makePersistedServerRuntimeState({ config, port: address.port });
yield* ownership.publish(state);
}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Publish failure tears down live server

Medium Severity

ownership.publish now runs after activation with no failure handling. A disk or rename error that used to only skip the discovery record now fails runtimeStateLayer and tears down an already-listening server. Pairing and T3 Connect go down with it even though the process already owns the state directory.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 449ae8e. Configure here.

t3dotgg and others added 6 commits September 5, 2026 21:27
…cked

The ownership lock had two regressions for existing users. A desktop backend
that lost the lock exited with code 1, so the desktop restart loop retried
forever with no message. Desktop SSH stopped replacing its own remote server
when the runner script changed, so an app update no longer updated the remote
CLI.

The refused server now exits with code 78. The desktop stops the restart loop
on that code and shows a dialog that names the cause. SSH reconnect replaces
a running server only when the saved PID and start time prove it is the
launcher's own and the bundled runner changed. Any other server is reused as
is. A publish failure after startup is a warning instead of a shutdown.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

size:XL 500-999 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant