Skip to content

fix: clean up the remote server update reconnect flow#4485

Open
t3dotgg wants to merge 5 commits into
mainfrom
t3code/fix-remote-reconnect-flow
Open

fix: clean up the remote server update reconnect flow#4485
t3dotgg wants to merge 5 commits into
mainfrom
t3code/fix-remote-reconnect-flow

Conversation

@t3dotgg

@t3dotgg t3dotgg commented Jul 24, 2026

Copy link
Copy Markdown
Member

Clicking Update server on a relay-connected server showed a spinner, then a disconnected banner with a raw transport error, then hung until the page was refreshed.

Three separate causes, fixed independently — the last two improve every reconnect, not just updates.

1. The update RPC never acknowledged on the systemd path

selfUpdate.ts ran systemctl --user restart synchronously inside the RPC handler, so the process died before the acknowledgement flushed. Every successful update reached the client as an interrupt, which ServerUpdateAction released quietly — no success toast, and nothing told the connection layer a restart was coming. (The respawn path already deferred its exit by 2s, so it usually won that race.)

The restart is now deferred the same way. The trade-off: a rejected restart can no longer be reported through the already-acknowledged RPC, so rollback and the in-flight reset move onto the detached fiber and log instead. Rollback behavior itself is unchanged and still covered by tests.

2. Backoff left the client asleep when the server returned

Reconnect backoff escalates 1s → 2s → 4s → 8s → 16s, and only resets after 30s of stable connection — so a restart could even inherit backoff debt from earlier flakiness. The client was frequently asleep in a 16s delay exactly when the server came back. Refreshing reset the failure count, which is why refreshing "fixed" it.

A restart-expected overlay (serverRestartStore.ts) is armed before dispatch and retries on a flat 2s cadence while active. It's deliberately not a new supervisor phase — the transport state machine stays transport-only and this is UI-side intent that expires on its own after 90s. Two safeguards worth noting:

  • It only pokes retryNow while retryAt is set (supervisor sleeping in backoff); waking a live attempt would cancel and restart it.
  • Arming before dispatch, rather than treating an interrupt as proof of acceptance, avoids showing "restarting" for a request the server never accepted. An explicit RPC failure withdraws it, since that proves the server stayed up.

3. Reconnect threw away the cached DPoP token

Two consecutive websocket-ticket failures evicted the cached token, forcing a full relay bootstrap + token exchange during reconnect. Eviction now only happens for failures that suggest a stale endpoint; plain unreachability (network/timeout) — the exact shape of a restart — keeps the token, so reconnect is a single request.

Also: raw transport errors no longer reach the banner

Failed to fetch remote environment endpoint https://prod-…t3coderelay.com/api/auth/websocket-ticket (HttpClientError: …) was flowing verbatim from rpc/http.ts into the composer. Presentation now maps each failure reason to a stable summary and carries reason for callers; the raw message stays on the supervisor state for connection diagnostics. Authored blocked-failure messages (auth/permission/config) pass through unchanged since they're already actionable.

Result

Click Update → button stays "Updating…" → one calm "bb-1 is restarting" banner → reconnects the moment the server is back, typically 10–20s, no refresh. The error UI still appears if the server genuinely never returns.

Testing

  • Full suite green: 4,218 passing, 0 failures. Typecheck and lint clean across all 15 projects.
  • New coverage: deferred boot-service restart + rollback-after-deferred-failure, transport-message sanitization, blocked-message passthrough, and cached-token retention under pure unreachability.
  • Not verified by running the flow end-to-end against a live relay server — the behavior above is reasoned from the code and covered by unit tests. Worth a manual update against a real remote before merging.

🤖 Generated with Claude Code


Note

Medium Risk
Touches self-update restart timing, auth token caching, and connection retry behavior—important for remote server updates and reconnects, but changes are scoped with tests and rollback paths.

Overview
Fixes the Update server flow on relay-connected hosts where a successful update looked like a failed RPC, the client slept through exponential backoff, and reconnect banners showed raw relay URLs.

Server: Boot-service self-update now defers systemctl restart (like the respawn path) so the update RPC can return before the process dies. Failed or defective deferred restarts roll back the unit, log, and clear the in-flight guard so another update can run; ServerSelfUpdateError exposes isAlreadyRunning via a shared constant for clients.

Web: New serverRestartStore arms a per-environment 90s “restart expected” window around self-update. ServerUpdateAction sets/re-arms it on click, success, and interrupt, clears it on real failures (but not “already in progress”), and re-enables the button when restart resolves without a new version. ChatView shows a calm restarting banner, suppresses the version-mismatch action during restart, pokes reconnect every 2s only while the supervisor is in backoff (retryAt set), and clears the window after disconnect→reconnect.

Client runtime: Cached DPoP tokens are not evicted on network/timeout unreachability—only endpoint-stale reasons—so restart reconnects stay a single ticket request. Connection presentation adds reason and retryAt, and maps transport failures to stable user-facing summaries (blocked auth messages unchanged).

Mobile and tests update connection fixture shapes for the new fields.

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

Note

Fix remote server update reconnect flow to defer restart and manage UI restart state

  • The server's update RPC now returns success before attempting the systemd restart, which is deferred to a background fiber. A rejected restart triggers unit-file rollback and releases the in-flight guard so the user can retry; a fiber defect also releases the guard and logs the error.
  • A new serverRestartStore (Zustand) tracks per-environment restart-expectation windows with auto-expiry, used by the UI to distinguish expected restarts from real outages.
  • ServerUpdateAction arms the restart window before dispatch, re-arms it on success or RPC interrupt, and releases the pending state only once the environment disconnects and reconnects or the window lapses.
  • ChatViewContent shows a neutral "restarting" info banner during an expected restart and auto-retries the environment connection every 2 seconds instead of waiting on backoff.
  • authorization/service no longer evicts cached tokens on pure unreachability (network/timeout), reducing unnecessary relay re-bootstrap during restarts.
  • presentConnectionState now exposes reason and retryAt fields so consumers can tailor retry cadence and error messages.
  • Behavioral Change: interrupting the update RPC (e.g. boot-service disconnect) no longer shows an error toast; it re-arms the restart window instead.

Macroscope summarized 85b8a38.

Clicking "Update server" on a relay-connected server showed a spinner,
then a disconnected banner with a raw transport error, then hung until
the page was refreshed. Three separate causes:

1. The boot-service (systemd) update path ran `systemctl --user restart`
   synchronously inside the RPC handler, so the process died before the
   acknowledgement flushed. Every successful update reached the client as
   an interrupt, which ServerUpdateAction released quietly — no success
   toast, and nothing told the connection layer a restart was coming.
   The restart is now deferred like the respawn path. Rollback and the
   in-flight reset move onto the detached fiber and log instead of
   failing the (already-acknowledged) RPC.

2. Reconnect backoff escalates to 16s, so the client was frequently
   asleep exactly when the server came back; refreshing reset the
   failure count, which is why refreshing "fixed" it. A restart-expected
   overlay is now armed before dispatch and retries on a flat 2s cadence
   while it is active, and only while the supervisor is sleeping in
   backoff so a live attempt is never interrupted. It expires on its own
   after 90s and is withdrawn on an explicit RPC failure, which proves
   the server stayed up.

3. Two consecutive websocket-ticket failures evicted the cached DPoP
   token, forcing a full relay bootstrap and token exchange during
   reconnect. Eviction is now limited to failures suggesting a stale
   endpoint; plain unreachability (network/timeout) — the shape of a
   restart — keeps the token.

Also stops piping raw transport messages (which embed internal endpoint
URLs) into the composer banner: presentation maps each failure reason to
a stable summary and carries the reason for callers, while the raw
message remains on the supervisor state for diagnostics.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 24, 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 Plus

Run ID: c088a18a-82c4-4e06-830a-994469cfc37f

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
  • Commit unit tests in branch t3code/fix-remote-reconnect-flow

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 vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 24, 2026
Comment thread apps/web/src/components/ServerUpdateAction.tsx
Comment thread apps/web/src/components/ServerUpdateAction.tsx Outdated
Comment thread apps/server/src/cloud/selfUpdate.ts
Comment thread apps/web/src/components/ServerUpdateAction.tsx Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces significant runtime behavior changes: a new client-side store for tracking restart expectations, deferred server restart execution, modified reconnection retry logic, and changes to token caching during transient errors. The scope of behavioral changes across server and client warrants human review.

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

Three defects found by Macroscope and Cursor Bugbot:

- The restart window was armed only at click time, but an npm install can
  consume most of the 10-minute request window, so a slow-but-successful
  update restarted after the window had lapsed and was presented as an
  outage. Re-arm from the acknowledgement and from the interrupt path,
  matching what keepPendingForRestart already does for the button expiry.

- Effect.catch on the detached restart fiber only sees typed failures, so
  an unexpected defect skipped the inFlight reset and left a live process
  refusing every further update until restarted by other means. Release
  the guard from Effect.onExit, outside the typed handler. Success still
  deliberately keeps it set: systemd is about to stop the process and a
  second update would race the handoff.

- "A server update is already in progress" is a typed failure, so it
  cleared a restart overlay that the earlier still-pending update needed,
  restoring the harsh reconnect UI mid-restart. That reason now lives in
  contracts as SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON with an
  isAlreadyRunning helper, so the client can distinguish it structurally
  instead of matching prose.

Each fix has a regression test; the defect-path test was confirmed to
fail without the onExit change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@t3dotgg

t3dotgg commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Thanks — all three findings were real and are fixed in 7aecc5d.

1. Restart window armed only at click time (Macroscope + Bugbot both flagged this)

Correct, and the component's own comment about installs consuming the request window is what made it likely in practice. The window is now re-armed from the acknowledgement and from the interrupt path — mirroring what keepPendingForRestart already did for the button expiry. The pre-dispatch arming stays, since it's what covers the boot-service ack losing the race against its own disconnect.

2. Deferred restart defects leave inFlight set

Correct — Effect.catch only sees typed failures, so a defect skipped the reset and left a live process rejecting every further update. The guard is now released from Effect.onExit, outside the typed handler.

One deliberate asymmetry: on success the guard is intentionally left set, because systemd is about to stop the process and a second update in that window would race the handoff. Only failure/defect/interrupt releases it.

I verified the new regression test actually catches this — with the onExit block removed it fails with exactly ServerSelfUpdateError: A server update is already in progress., the stranded-guard symptom described.

3. In-progress error clears restart overlay

Correct, and the sharpest of the three: that failure is the one case where a restart genuinely is still pending. Rather than match on the message text, the reason moved into contracts as SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON with an isAlreadyRunning helper, so server and client agree structurally and a future wording change can't silently break the check.

Full suite green (4,891 passing), typecheck and lint clean. Each fix has a regression test.

Still worth a manual update against a live remote before merge — the end-to-end flow isn't something the unit tests can cover.

An independent gpt-5.6-sol review found that the restart overlay never
actually engaged in the normal flow — the feature was inert.

ChatView cleared the expectation on any render where the connection was
"connected". But the window is armed while the old server is still
connected (that is the point: the acknowledgement races its own
disconnect), so the store update triggered a render that destroyed the
window within milliseconds, long before the server went away. Both
re-arm points had the same problem. The user therefore still got the
outage banner and escalating backoff.

The window now tracks whether the restart's disconnect was actually
observed. A connected environment only ends the window once that has
happened; before it, "connected" just means the restart has not started
yet. Re-arming preserves an already-observed disconnect.

Also fixes the follow-on the same review found: because a rejected
restart can no longer be reported through the acknowledged RPC, the
update action could stay disabled for the full 12-minute safety window
with the mismatch unresolved. A settled restart window (disconnect seen,
window closed) now releases it so the user can retry immediately.

The restarting/settled decisions are pure exported predicates, since the
component test harness stubs useEffect and cannot exercise
effect-driven behavior. Both are tested, and both tests were confirmed
to fail against the old logic.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@t3dotgg

t3dotgg commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

An independent gpt-5.6-sol review round found a defect the bots and I both missed, and it was the important one: the restart overlay never actually engaged. Fixed in 02abc2c.

The overlay was inert

ChatView cleared the expectation on any render where the connection was connected. But the window is armed while the old server is still connected — that's the entire point, since the acknowledgement races its own disconnect. So arming the window triggered a render that destroyed it within milliseconds, long before the server went away. Both re-arm points had the same problem, and the server's deliberate 2s RESTART_DELAY gave React ample time to clear it again.

Net effect: every code path in this PR was in place, and the user still got the outage banner and escalating backoff. The feature was decorative.

The window now tracks whether the restart's disconnect was actually observed. A connected environment only ends the window once that's happened; before it, "connected" just means the restart hasn't started yet. Re-arming preserves an already-seen disconnect, so a late-acknowledging slow install doesn't reset that.

Worth noting this is exactly the class of bug the unit tests couldn't catch — ServerUpdateAction's tests mock the restart store, so they verified the arming calls while the real interaction lived in a different component.

Follow-on: action stuck disabled after a rejected restart

Same review, also confirmed. Since a rejected restart can no longer report through the already-acknowledged RPC, the update action could sit disabled for the full 12-minute safety window with the mismatch still valid and the server answering again. A settled restart window (disconnect seen, window since closed) now releases it for an immediate retry.

On testability

The restarting/settled decisions are pure exported predicates rather than inline effect logic, because the component test harness stubs useEffect to a no-op and cannot exercise effect-driven behavior. I'd rather have the decision covered than a test that renders and asserts nothing. Both predicates are tested, and I verified both tests fail against the old logic — reintroducing the connected-clears-immediately behavior fails does not claim a still-connected environment is restarting.

Full suite green (4,901 passing), typecheck and lint clean.

The earlier caveat stands and now matters more: this needs one manual update against a live remote before merge. Three review rounds found three real defects in the client-side coordination, and the end-to-end path is the part no test here covers.

Comment thread apps/web/src/components/ServerUpdateAction.tsx Outdated
Comment thread apps/web/src/components/ChatView.tsx Outdated
Comment thread apps/web/src/components/ChatView.tsx
Cursor found three defects (one High) that shared a root cause: the
restart window had been given a second job — releasing the update
action — while the window itself deleted the evidence that job depended
on. Patching each symptom would have kept the coupling, so sawDisconnect
is gone.

The window is now write-once-and-expire. It is armed while the old
server is still connected, so nothing observed on the connection can
reliably end it early; presentation gates on the environment actually
being unavailable, which makes a window outliving its restart inert.
That removes both "a brief blip during install poisons the flag" and
"reconnecting clears the flag before the action can read it".

Releasing the action is now its own signal, independent of the window,
with the two shapes a rejected restart can take:
  - the environment went away and came back, so something restarted but
    did not deliver the new version; or
  - the window lapsed with the connection never dropping, so no restart
    ever happened (systemd refused it and rolled back).
The second is the High-severity case: previously nothing set the flag,
so the action stayed disabled for the full 12-minute window.

The decision is a pure exported predicate because the component harness
stubs useEffect. Verified by removing the lapsed-window branch, which
fails exactly the test covering that case.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@t3dotgg

t3dotgg commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

All three fixed in 3c9f1db — but not individually, because they shared a root cause worth naming.

Root cause: the window had two jobs

I'd given the restart window a second responsibility — releasing the update action — while the window itself deleted the evidence that job depended on. That's why three separate symptoms appeared:

  • Settled never releases failed restart (High): a restart that fails without dropping the connection never sets sawDisconnect, so nothing releases the action.
  • Reconnect clears before settle: clearing on reconnect drops sawDisconnect before the action can read it.
  • Install blip clears restart window: any transient non-connected phase poisons the flag, and the subsequent reconnect then clears the window before the real restart.

Patching each would have preserved the coupling and invited a fourth. So sawDisconnect is gone.

The window is now write-once-and-expire

It's armed while the old server is still connected, so nothing observed on the connection can reliably end it early — that was the false premise in my previous fix. Instead, presentation gates on the environment actually being unavailable, which makes a window outliving its restart inert rather than wrong. That removes the blip and clear-before-settle problems structurally; there's no flag left to poison.

Releasing the action is its own signal

Independent of the window, covering both shapes a rejected restart can take:

  1. The environment went away and came back → something restarted but the skew is still here, so it didn't carry the new version.
  2. The window lapsed with the connection never dropping → no restart ever happened (systemd refused and rolled back).

#2 is the High-severity case — previously unreachable, since nothing set the flag.

Verification

isExpectedRestartResolved is a pure exported predicate (the component harness stubs useEffect, so effect logic isn't testable in place). I removed the lapsed-window branch and confirmed it fails exactly resolves when the window lapses without the connection ever dropping, then restored.

Full suite green (4,927 passing), typecheck and lint clean.


Tally so far: 4 review rounds, 10 confirmed defects, all in the client-side coordination this PR adds. Three of those rounds found something the previous round missed, and one found that the feature was entirely inert. I'd treat the server-side changes (deferred restart, DPoP eviction, error sanitization) as solid — they're small and well covered — but the restart-overlay coordination has earned skepticism, and it still has not been run once against a live remote. Please do that before merging rather than trusting the green checks.

Comment thread apps/web/src/components/ServerUpdateAction.tsx

@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 high effort and found 2 potential issues.

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 3c9f1db. Configure here.

Comment thread apps/web/src/components/ServerUpdateAction.tsx
Comment thread apps/web/src/serverRestartStore.ts Outdated
Two more review findings, plus a self-correction.

Stale wentAwayRef (Macroscope): the ref persists for the component's
lifetime, so an outage from before the click satisfied the resolve
predicate on the next render and re-enabled the button mid-update,
allowing a second dispatch. Reset both refs when an attempt is armed so
only outages during that attempt count.

Unrelated outage shown as a restart (Bugbot): making the window
write-once-and-expire last round traded one bug for another — any
disconnect within 90s was dressed up as an intentional restart. The
window now ends once the environment has gone away and come back, i.e.
the restart it was armed for has been seen through. That is the narrow
version of what an earlier revision got wrong by ending it on any healthy
connection; the difference is requiring the disconnect first.

The observation is used only for the banner. The update action's release
path stays independent of it — coupling those is what produced the
earlier family of bugs where each consumer invalidated the other's
evidence.

Also removed a test committed in the previous change that passed with and
without its fix, so it verified nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@t3dotgg

t3dotgg commented Jul 25, 2026

Copy link
Copy Markdown
Member Author

Both fixed in 85b8a38, plus a correction to my own previous round.

Stale wentAwayRef (Macroscope) — confirmed, and the suggested fix was right. The ref lives for the component's lifetime, so an outage from before the click satisfied the resolve predicate on the next render, re-enabled the button mid-update, and allowed a second dispatch. Both refs now reset when an attempt is armed, so only outages during that attempt count.

Unrelated outage shown as a restart (Bugbot) — also confirmed, and this one is my regression. Making the window write-once-and-expire last round traded one bug for another: any disconnect inside the 90s was dressed up as an intentional restart.

The window now ends once the environment has gone away and come back — the restart it was armed for has been seen through. That's the narrow version of what my first attempt got wrong by ending it on any healthy connection; the difference is requiring the disconnect first. The observation feeds only the banner. The update action's release path stays independent, because coupling those is exactly what produced the earlier family of bugs where each consumer destroyed the other's evidence.

Two things I got wrong that reviews didn't catch

I committed a test that verified nothing. The regression test in the previous commit passed identically with and without its fix — the component harness stubs useEffect, so the effect under test never ran. Removed rather than left as false assurance. Worth flagging because the "19 passing" number in my earlier comments was carrying it.

I started implementing a backoff change that was wrong, and reverted it. Reviewing my own plan I noticed item 4 — resetting failureCount on post-connection drops — was never implemented. On writing it, keeps escalating backoff when a newly opened session flaps failed, and that test is right: a flapping session should escalate or the client hammers a sick server. The 2s retry loop already covers the restart case without weakening that protection, so the supervisor is deliberately unchanged in this PR. Flagging it as a scoped-out item rather than a silent omission.

Full suite green (4,928 passing), typecheck and lint clean, supervisor diff empty.


Five review rounds, twelve confirmed defects, every one in the client-side coordination this PR adds. Twice now a fix of mine has introduced the next finding. That pattern is the actual signal here: the server-side changes are small and well covered, but this overlay logic should not be merged on the strength of green CI. It needs one real Update click against a live remote, watching the banner, the button, and the reconnect timing. I can't do that from here.

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

Labels

size:L 100-499 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