Sign up
View stargazersSign up

fix(server): keep remote connections alive during recovery#25053

closed4
main
GitHub fork: fix/restart-connection-readiness
opened17d agobyZZheruel
8 files changed
View on GitHubTwo-way sync enabledPR last imported 11d ago

Keep remote connections alive while the server recovers after a restart. The global HTTP readiness gate previously blocked authentication and WebSocket setup until recovery finished, while failed attempts kept increasing client backoff.

Connection setup, config and lifecycle RPCs now use an earlier readiness gate. Application HTTP requests and RPCs still wait for recovery. Normal tunnel startup uses the earlier gate too.

After losing a session that was stable for 30 s, retry waits stay capped at 5 s for 2 min. Backoff starts fresh when that window ends, so those rapid attempts cannot immediately produce a 5 min wait.

Local QA exercised the production server, temporary SQLite state, authentication and the real client session through ordinary startup, an update trial and recovery failure. Config and probes answered during recovery; application RPCs waited; a cancelled settings write never ran; failed recovery closed the socket. All 62 changed-file tests pass. The only failing local check is the pre-existing unused agent-device dependency.

Not verified: live Cloudflare behavior or the reporter’s production database timings. QA used local stand-ins for relay operations and launcher IPC.

Important

Update trials keep tunnel startup behind the launcher’s commit gate to preserve rollback safety.

Fixes #24661.

Model: gpt-6.1-sol; harness: Codex.

Checks

23 successful · 13 skipped
All checks passed
3a68f8a

Added 3 commits

  • fix(server): keep remote connections alive during recovery36e3773
  • fix(client): restart backoff after the reconnect grace period39a9a8c
  • test(server): trim redundant startup readiness coverage15d683c
CcoderabbitaiBot17d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 15d683c5d67fce8946ba9a4f14df3fdc106b078f and 3a68f8a6ff3cb20f95ae1e6197108ab0c30142b6.

External image Files selected for processing: 6
  • apps/server/src/server.ts
  • apps/server/src/serverRuntimeStartup.test.ts
  • apps/server/src/serverRuntimeStartup.ts
  • apps/server/src/ws.ts
  • packages/client-runtime/src/connection/supervisor.test.ts
  • packages/client-runtime/src/connection/supervisor.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

The server now exposes connection readiness separately from command readiness, applies readiness gates to HTTP routes and RPC methods, and starts the managed tunnel after connection readiness. The client supervisor also caps reconnect delays for 120 seconds after a stable connection.

Changes: Remote connection during recovery

Layer / File(s)Summary
Separate connection readiness from command readiness
apps/server/src/serverRuntimeStartup.ts, apps/server/src/serverRuntimeStartup.test.ts		Startup signals connection readiness after the HTTP listener starts. Startup errors fail both readiness gates. Tests cover slow recovery and settings-start failures.
Apply readiness gates to routes, RPCs, and tunnel startup
apps/server/src/serverReadiness.ts, apps/server/src/server.ts, apps/server/src/ws.ts, apps/server/src/serverReadiness.test.ts		HTTP and RPC middleware let listed connection operations run before command readiness and gate other operations on command readiness. The managed tunnel waits for connection readiness and, for pending updates, activation.
Cap retries after a stable connection
packages/client-runtime/src/connection/supervisor.ts, packages/client-runtime/src/connection/supervisor.test.ts		The supervisor uses a five-second retry-delay cap during the 120-second reconnect window, then returns to the existing 300-second cap. A test checks the delay limits and reconnection.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant HTTPListener
3 participant ServerRuntimeStartup
4 participant ServerReadiness
5 participant HTTPRoute
6 HTTPListener->>ServerRuntimeStartup: listener starts
7 ServerReadiness->>ServerRuntimeStartup: awaitConnectionReady
8 ServerRuntimeStartup-->>ServerReadiness: connection readiness result
9 HTTPRoute->>ServerReadiness: request method and path
10 ServerReadiness->>ServerRuntimeStartup: awaitConnectionReady or awaitCommandReady
11 ServerReadiness-->>HTTPRoute: run request effect when ready

Suggested reviewers: maria-rcks, t3dotgg, juliusmarminge

Merge Risk: External image Minimal · up to 3a68f

Connection endpoints become available during recovery while application requests wait. No concrete merge-blocking defect remains.

Pre-merge checks: Passed 3 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check nameStatusExplanationResolution
Out of Scope Changes checkInconclusiveThe eight files in the PR summary map to #24661: serverReadiness.ts (new), server.ts, serverRuntimeStartup.ts, ws.ts, supervisor.ts and their tests. The incremental diff from 15d683c to 3a68f8a adds c…Provide the whole-PR diff from mergeBase 7195b69 to head 3a68f8a for apps/server/src/server.ts and packages/client-runtime/src/connection/supervisor.ts. Confirm whether the peer, triage, sign-in, secret, document-text, preview-activity, rem…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Title checkPassedThe title is concise, specific, and accurately describes the primary server recovery change.
Description checkPassedThe description clearly covers the problem, implemented changes, linked issue, verification results, and unverified scenarios. It is mostly complete, although it does not include an explicit maintaine…
Linked Issues checkPassed[#24661] The requirements were verified at reviewed head 15d683c. The change to 3a68f8a is a merge of main. The visible incremental hunks keep the readiness wiring. (1) server.ts keeps `Layer.provide(…
Full details: Description check

Explanation

The description clearly covers the problem, implemented changes, linked issue, verification results, and unverified scenarios. It is mostly complete, although it does not include an explicit maintainer approval comment or separate template headings.

Full details: Linked Issues check

Explanation

[#24661] The requirements were verified at reviewed head 15d683c. The change to 3a68f8a is a merge of main. The visible incremental hunks keep the readiness wiring. (1) server.ts keeps Layer.provide(ServerReadiness.layerHttp) in the route layer. (2) serverReadiness.ts at head defines layerHttp. Connection routes (GET /.well-known/t3/environment, /api/auth/*, /oauth/token, /api/t3-connect/*, /api/connect/*, GET /ws) wait on awaitConnectionReady. All other routes wait on awaitCommandReady. layerRpc bypasses command readiness for serverProbe, serverGetConfig, subscribeServerConfig and subscribeServerLifecycle. Application RPCs still wait. (3) Per the previous review and the summary, the managed-tunnel fiber in server.ts waits on awaitConnectionReady. Update trials also wait on activation. The PR description documents this exception for rollback safety. (4) The reconnect window in supervisor.ts (5 s cap for 120 s after a stable session, then the 300 s ladder) was verified at 15d683c. The visible incremental hunks do not alter it. The truncated tail of the supervisor incremental diff was not read. Not implemented: the optional external-connector setting, the optional SIGTERM restart close code, the suggested 'starting/recovering' status, and the planned-restart retry notice. The first two are optional in the issue. The last two are suggestions under Asks 1 and 3. Live Cloudflare timing was not verified.

Full details: Out of Scope Changes check

Explanation

The eight files in the PR summary map to #24661: serverReadiness.ts (new), server.ts, serverRuntimeStartup.ts, ws.ts, supervisor.ts and their tests. The incremental diff from 15d683c to 3a68f8a adds content unrelated to #24661 to server.ts and supervisor.ts. In server.ts it adds peer forwarding and links, RemoteDelegation, TriageService, SourceControlSignIn, SecretResolver, DocumentText, PreviewAgentActivity and a PortScanner layer. In supervisor.ts it adds silent-socket checks and long-resume probe logic. These come from the merged main. The whole-PR summary reports server.ts +8/-12 and supervisor.ts +18/-4, which excludes this content. The supplied mergeBase 7195b69 does not confirm that. The whole-PR diff for these two files was not read, so scope cannot be decided.

Resolution

Provide the whole-PR diff from mergeBase 7195b69 to head 3a68f8a for apps/server/src/server.ts and packages/client-runtime/src/connection/supervisor.ts. Confirm whether the peer, triage, sign-in, secret, document-text, preview-activity, remote-delegation, port-scanner and silent-socket changes appear in that diff. Also provide the truncated tail of the supervisor incremental diff after the probe loop, so the reconnect window code can be confirmed at head.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

Mmaria17d ago

Note

Written by gpt-6.1-sol on behalf of Maria

maintainer edits are now enabled. merged current main into this original head without rebasing and pushed 3a68f8a6ff3cb20f95ae1e6197108ab0c30142b6 to fix/restart-connection-readiness. resolved the import conflict by retaining both readiness middleware and secret resolution. the intended readiness split, update-trial activation gate, and reconnect grace remain intact. the diff remains 8 files, 310 additions and 23 deletions.

blacksmith vp run check:changed github/main passed: formatting, lint, knip, graphql generation, nine affected package typechecks, and 83 tests across six files. two independent read-only reviews found no blocking issues.

github reports this head mergeable. CI run https://github.com/pingdotgg/t3code/actions/runs/41279123808 is action_required before any jobs run, so workflow approval remains necessary. live browser reconnect and remote relay recovery remain unverified. this is not ready to merge yet.

Added 1 commit

  • chore: merge main into restart connection readiness3a68f8a
Mmaria11d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified head 3a68f8a6ff3cb20f95ae1e6197108ab0c30142b6 as-is on a disposable linux vm. installed github/main in a separate worktree and the exact pr head with vp i. ran one background dev server at a time. drove the web app with repo playwright and /usr/bin/google-chrome, created a project for a real git repo through the app, and ran real codex gpt-6.1-sol turns. no mocked providers or seeded fixtures.

with a stable client and the same 45-second backend outage, main produced retry gaps of 2.392, 6.219, 9.343, 18.678 and 36.998 seconds. the client reached the server 27.348 seconds after its listener was ready. on this pr, all 13 measured retry gaps stayed between 2.663 and 4.851 seconds, and the client reached the server 2.532 seconds after its listener was ready. these are two observed runs, not a general latency guarantee.

on the pr, /api/auth/session and /.well-known/t3/environment returned http 200 at 07:45:45.927 and 07:45:45.932 utc, before recovery completed at 07:45:46.037. main answered only after its recovery-completed log. the database was small, so this proves the ordering in a short real recovery window.

thread history survived reconnect. a follow-up codex turn ran pwd and git status --short and returned POST_RESTART_OK. an abrupt backend restart also preserved the thread and resumed its provider path. authenticated websocket probe, config and an empty settings patch succeeded afterward. interrupted sleep commands were not reported as completed.

blacksmith passed 71 focused readiness/startup/supervisor tests and vp run check:changed github/main against pr base 7195b699547b: format, lint, knip, graphql generation, nine package typechecks and tests across six files. github ci is green on this head. no source changes or pushes were needed.

main automatic reconnect during the measured outage, 2x playback

pr automatic reconnect during the matching outage, 2x playback

real codex follow-up completed after reconnect

runtime transcript

Not verified

live t3 connect/cloudflare relay traffic: this development environment has no linked relay. update-trial launcher activation/rollback, minute-long recovery on a production-sized database, pending/cancelled application rpcs through an extended recovery window, and expiration of the 120-second retry grace were not exercised live. native desktop and ios/android clients were not exercised on this linux vm.

Ttheo11d ago

Note

🤖 GPT-6.1 Terra [1m] responding on behalf of Theo

Closing in favor of Maria’s replacement, which preserves the split readiness gates, update-trial gate and bounded reconnect schedule. #25540 #24661