From b462ae5f2f7893fd0382bd452ab1169cc5edada1 Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Fri, 24 Jul 2026 16:35:49 -0700 Subject: [PATCH 1/5] fix: clean up the remote server update reconnect flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/features/review/ReviewSheet.tsx | 2 + .../terminal/ThreadTerminalRouteScreen.tsx | 2 + apps/mobile/src/state/workspaceModel.test.ts | 2 + apps/server/src/cloud/selfUpdate.test.ts | 42 +++++++-- apps/server/src/cloud/selfUpdate.ts | 69 ++++++++------- apps/web/src/components/ChatView.tsx | 53 +++++++++++- .../web/src/components/ServerUpdateAction.tsx | 10 +++ ...dEnvironmentConnectionPresentation.test.ts | 2 +- apps/web/src/serverRestartStore.ts | 80 +++++++++++++++++ .../src/authorization/layer.test.ts | 36 ++++++++ .../src/authorization/service.ts | 23 +++++ .../src/connection/presentation.test.ts | 63 +++++++++++++- .../src/connection/presentation.ts | 85 ++++++++++++++++--- 13 files changed, 414 insertions(+), 55 deletions(-) create mode 100644 apps/web/src/serverRestartStore.ts diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index 1ebb3aaf7b1..82269bade54 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -771,7 +771,9 @@ export function ReviewSheet(props: ReviewSheetProps) { environment.presentation?.connection ?? { phase: "available", error: null, + reason: null, traceId: null, + retryAt: null, } } resourceName="review" diff --git a/apps/mobile/src/features/terminal/ThreadTerminalRouteScreen.tsx b/apps/mobile/src/features/terminal/ThreadTerminalRouteScreen.tsx index cb281bf4aed..5192599915b 100644 --- a/apps/mobile/src/features/terminal/ThreadTerminalRouteScreen.tsx +++ b/apps/mobile/src/features/terminal/ThreadTerminalRouteScreen.tsx @@ -1209,7 +1209,9 @@ export function ThreadTerminalRouteScreen(props: ThreadTerminalRouteScreenProps) environment.presentation?.connection ?? { phase: "available", error: null, + reason: null, traceId: null, + retryAt: null, } } resourceName="terminal" diff --git a/apps/mobile/src/state/workspaceModel.test.ts b/apps/mobile/src/state/workspaceModel.test.ts index e51273d57de..0fe2c2c4b7b 100644 --- a/apps/mobile/src/state/workspaceModel.test.ts +++ b/apps/mobile/src/state/workspaceModel.test.ts @@ -40,7 +40,9 @@ function environment( connection: { phase, error: phase === "error" ? "Connection failed." : null, + reason: phase === "error" ? "authentication" : null, traceId: phase === "error" ? "trace-1" : null, + retryAt: null, }, serverConfig: null, }; diff --git a/apps/server/src/cloud/selfUpdate.test.ts b/apps/server/src/cloud/selfUpdate.test.ts index 9d6e3801704..7619206470a 100644 --- a/apps/server/src/cloud/selfUpdate.test.ts +++ b/apps/server/src/cloud/selfUpdate.test.ts @@ -26,6 +26,30 @@ import * as SelfUpdate from "./selfUpdate.ts"; const NODE_PATH = "/usr/local/bin/node"; +/** + * The deferred restart runs on a detached fiber, so its rollback performs + * real filesystem I/O that advancing the TestClock does not await. Poll on + * real time until the expected content lands. + */ +const eventuallyFileString = Effect.fn("test.eventuallyFileString")(function* ( + filePath: string, + expected: string, +) { + const fs = yield* FileSystem.FileSystem; + let last = ""; + for (let iteration = 0; iteration < 1_000; iteration += 1) { + last = yield* fs.readFileString(filePath); + if (last === expected) { + return last; + } + // Yielding (rather than sleeping) keeps this on the real event loop: + // TestClock would only advance virtual time and never let the + // detached fiber's filesystem writes land. + yield* Effect.yieldNow; + } + return yield* Effect.die(new Error(`Expected file contents were not observed at ${filePath}.`)); +}); + interface RecordedCommand { readonly command: string; readonly args: ReadonlyArray; @@ -490,7 +514,7 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { }).pipe(Effect.provide(TestClock.layer())), ); - it.effect("rewrites the systemd unit and restarts the boot service", () => + it.effect("rewrites the systemd unit and defers the boot-service restart", () => Effect.gen(function* () { const context = yield* makeContext({ bootService: true }); const result = yield* context.service.update({ targetVersion: "0.0.29" }); @@ -504,12 +528,15 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { context.path.join(context.home, ".config", "systemd", "user", "t3code.service"), ); assert.include(unit, `ExecStart=${NODE_PATH} ${pinnedEntry} serve`); + // The restart is deferred so the RPC acknowledgement flushes before + // systemd stops this process. assert.deepEqual( context.commands.map((entry) => entry.command), - ["npm", NODE_PATH, "systemctl", "systemctl"], + ["npm", NODE_PATH, "systemctl"], ); assert.deepEqual(context.commands[2]?.args, ["--user", "daemon-reload"]); + yield* TestClock.adjust(Duration.seconds(10)); assert.deepEqual(context.commands[3], { command: "systemctl", args: ["--user", "restart", "t3code.service"], @@ -520,7 +547,7 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { }).pipe(Effect.provide(TestClock.layer())), ); - it.effect("restores the previous unit and permits a retry when systemd restart fails", () => + it.effect("restores the previous unit and permits a retry when the deferred restart fails", () => Effect.gen(function* () { let failRestart = true; const context = yield* makeContext({ @@ -542,9 +569,12 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { ); const previousUnit = yield* context.fs.readFileString(unitPath); - const first = yield* context.service.update({ targetVersion: "0.0.29" }).pipe(Effect.flip); - assert.include(first.reason, "Restarting the systemd boot service failed"); - assert.equal(yield* context.fs.readFileString(unitPath), previousUnit); + // The RPC acknowledges before the restart runs, so a rejected + // restart can only roll back and log — never fail the request. + const first = yield* context.service.update({ targetVersion: "0.0.29" }); + assert.deepEqual(first, { targetVersion: "0.0.29", method: "boot-service" }); + yield* TestClock.adjust(Duration.seconds(10)); + yield* eventuallyFileString(unitPath, previousUnit); assert.deepEqual( context.commands.slice(-2).map((entry) => entry.args), [ diff --git a/apps/server/src/cloud/selfUpdate.ts b/apps/server/src/cloud/selfUpdate.ts index 62bd07fbbc8..4dd3da27a67 100644 --- a/apps/server/src/cloud/selfUpdate.ts +++ b/apps/server/src/cloud/selfUpdate.ts @@ -349,37 +349,46 @@ export const make = Effect.fn("cloud.server_self_update.make")(function* (option yield* Effect.logInfo("Server self-update installed; restarting boot service.", { targetVersion, }); - // A successful systemd restart stops this process, so the RPC is - // interrupted and the reconnecting client observes the new version. - // A rejected restart returns while the old process is still alive; - // restore the previous unit and report that failure through the RPC. - yield* Effect.gen(function* () { - const restart = yield* runner - .run({ - command: "systemctl", - args: ["--user", "restart", BOOT_SERVICE_UNIT_FILE], - }) - .pipe( - Effect.mapError((cause) => - failWith("Could not restart the systemd boot service.", cause), - ), - ); - if (restart.code !== 0) { - return yield* failWith( - `Restarting the systemd boot service failed (exit code ${String(restart.code)}).`, - ); - } - }).pipe( - Effect.catch((restartError) => - writeUnitAtomically(unitPath, previousUnit).pipe( - Effect.andThen(reloadSystemd()), - Effect.mapError((rollbackError) => - failWith("Could not restore the previous systemd unit.", { - restartError, - rollbackError, - }), + // Deferred like the respawn path: a synchronous systemctl restart + // stops this process mid-RPC, so every successful update used to + // surface to the client as an interrupted call indistinguishable + // from a dropped connection. Deferring lets the acknowledgement + // flush first. The trade: a rejected restart can no longer be + // reported through this RPC — it rolls back, logs, and clears the + // in-flight guard so a retry works. + yield* scheduleRestart( + Effect.gen(function* () { + const restart = yield* runner + .run({ + command: "systemctl", + args: ["--user", "restart", BOOT_SERVICE_UNIT_FILE], + }) + .pipe( + Effect.mapError((cause) => + failWith("Could not restart the systemd boot service.", cause), + ), + ); + if (restart.code !== 0) { + return yield* failWith( + `Restarting the systemd boot service failed (exit code ${String(restart.code)}).`, + ); + } + }).pipe( + Effect.catch((restartError) => + Effect.logError( + "Server self-update restart was rejected; restoring the previous systemd unit.", + ).pipe( + Effect.annotateLogs({ targetVersion, error: restartError.reason }), + Effect.andThen( + writeUnitAtomically(unitPath, previousUnit).pipe(Effect.andThen(reloadSystemd())), + ), + Effect.catch((rollbackError) => + Effect.logError( + "Server self-update could not restore the previous systemd unit after a rejected restart.", + ).pipe(Effect.annotateLogs({ targetVersion, error: String(rollbackError) })), + ), + Effect.ensuring(Ref.set(inFlight, false)), ), - Effect.andThen(Effect.fail(restartError)), ), ), ); diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index ab1256cddb3..dc26d3d25e2 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -25,6 +25,7 @@ import { connectionStatusTitle, type EnvironmentConnectionPresentation, } from "@t3tools/client-runtime/connection"; +import { clearExpectedServerRestart, useServerRestartExpected } from "~/serverRestartStore"; import { effectiveSettled, effectiveSnoozed } from "@t3tools/client-runtime/state/thread-settled"; import { parseScopedThreadKey, @@ -275,6 +276,7 @@ import { RightPanelSheet } from "./RightPanelSheet"; import { previewEnvironment } from "../state/preview"; import { useAtomCommand } from "../state/use-atom-command"; import { Button } from "./ui/button"; +import { Spinner } from "./ui/spinner"; import { AlertDialog, AlertDialogClose, @@ -1619,6 +1621,37 @@ function ChatViewContent(props: ChatViewProps) { connection: activeEnvironment.connection, }; }, [activeEnvironment, activeEnvironmentUnavailable, activeEnvironmentUnavailableLabel]); + // While a server self-update restart is expected, disconnects are routine: + // present them calmly and retry fast instead of letting exponential + // backoff strand the client asleep when the server comes back. + const activeEnvironmentId = activeEnvironment?.environmentId ?? null; + const activeEnvironmentRestartExpected = useServerRestartExpected(activeEnvironmentId); + const activeEnvironmentRestarting = + activeEnvironmentRestartExpected && activeEnvironmentUnavailable; + useEffect(() => { + if (activeEnvironmentId === null || !activeEnvironmentRestartExpected) { + return; + } + if (activeEnvironmentConnectionPhase === "connected") { + clearExpectedServerRestart(activeEnvironmentId); + return; + } + // Only wake the supervisor out of a backoff sleep; while an attempt is + // live, retryNow would interrupt and restart it. + if (activeEnvironment?.connection.retryAt == null) { + return; + } + const timer = setTimeout(() => { + void retryEnvironment(activeEnvironmentId); + }, 2_000); + return () => clearTimeout(timer); + }, [ + activeEnvironment?.connection.retryAt, + activeEnvironmentConnectionPhase, + activeEnvironmentId, + activeEnvironmentRestartExpected, + retryEnvironment, + ]); const handleReconnectActiveEnvironment = useCallback( async (environmentId: EnvironmentId) => { const result = await retryEnvironment(environmentId); @@ -1832,7 +1865,19 @@ function ChatViewContent(props: ChatViewProps) { const versionMismatchSelfUpdate = resolveServerSelfUpdateCapability(serverConfig); const systemComposerBannerItems = useMemo(() => { const items: ComposerBannerStackItem[] = []; - if (activeEnvironmentUnavailableState) { + if (activeEnvironmentUnavailableState && activeEnvironmentRestarting) { + // An expected self-update restart: the disconnect and the transient + // connection errors behind it are routine, so keep the tone neutral + // and let the fast auto-retry loop do the work instead of asking the + // user to reconnect by hand. + items.push({ + id: `environment-restarting:${activeEnvironmentUnavailableState.environmentId}`, + variant: "info", + icon: , + title: `${activeEnvironmentUnavailableState.label} is restarting`, + description: "Finishing the server update — reconnecting automatically.", + }); + } else if (activeEnvironmentUnavailableState) { const connection = activeEnvironmentUnavailableState.connection; const isReconnecting = connection.phase === "connecting" || connection.phase === "reconnecting"; @@ -1872,7 +1917,10 @@ function ChatViewContent(props: ChatViewProps) { showVersionMismatchBanner && versionMismatch && versionMismatchDismissKey && - versionMismatchEnvironmentId + versionMismatchEnvironmentId && + // The mismatch data predates the disconnect; during an expected + // restart it is stale and its update action would double-trigger. + !activeEnvironmentRestarting ) { items.push({ id: `version-mismatch:${versionMismatchDismissKey}`, @@ -1906,6 +1954,7 @@ function ChatViewContent(props: ChatViewProps) { } return items; }, [ + activeEnvironmentRestarting, activeEnvironmentUnavailableState, handleReconnectActiveEnvironment, navigate, diff --git a/apps/web/src/components/ServerUpdateAction.tsx b/apps/web/src/components/ServerUpdateAction.tsx index 5e82c4e4d93..a97a6ab67a7 100644 --- a/apps/web/src/components/ServerUpdateAction.tsx +++ b/apps/web/src/components/ServerUpdateAction.tsx @@ -6,6 +6,7 @@ import { } from "@t3tools/client-runtime/state/runtime"; import { useCopyToClipboard } from "~/hooks/useCopyToClipboard"; +import { clearExpectedServerRestart, expectServerRestart } from "~/serverRestartStore"; import { serverEnvironment } from "~/state/server"; import { useAtomCommand } from "~/state/use-atom-command"; import { manualServerUpdateCommand } from "~/versionSkew"; @@ -116,6 +117,13 @@ export function ServerUpdateAction({ expiry = armExpiry(); } }; + // Armed before dispatch: the boot-service path acknowledges and then + // drops every connection, and even the respawn acknowledgement can lose + // the race against the disconnect. An explicit RPC failure proves the + // server stayed alive, so the expectation is withdrawn there; on + // success or interrupt it stays armed and expires on its own if the + // server never returns. + expectServerRestart(environmentId); void Promise.resolve() .then(() => updateServer({ @@ -133,6 +141,7 @@ export function ServerUpdateAction({ if (isAtomCommandInterrupted(result)) { return; } + clearExpectedServerRestart(environmentId); toastManager.add({ type: "error", title: "Server update failed", @@ -152,6 +161,7 @@ export function ServerUpdateAction({ }) .catch((error: unknown) => { if (!ownsAttempt()) return; + clearExpectedServerRestart(environmentId); toastManager.add({ type: "error", title: "Server update failed", diff --git a/apps/web/src/components/cloud/cloudEnvironmentConnectionPresentation.test.ts b/apps/web/src/components/cloud/cloudEnvironmentConnectionPresentation.test.ts index 654c2462a6f..bdc867b35dd 100644 --- a/apps/web/src/components/cloud/cloudEnvironmentConnectionPresentation.test.ts +++ b/apps/web/src/components/cloud/cloudEnvironmentConnectionPresentation.test.ts @@ -7,7 +7,7 @@ function connection( phase: EnvironmentConnectionPresentation["phase"], error: string | null = null, ): EnvironmentConnectionPresentation { - return { phase, error, traceId: null }; + return { phase, error, reason: null, traceId: null, retryAt: null }; } describe("saved cloud environment connection presentation", () => { diff --git a/apps/web/src/serverRestartStore.ts b/apps/web/src/serverRestartStore.ts new file mode 100644 index 00000000000..a439c811685 --- /dev/null +++ b/apps/web/src/serverRestartStore.ts @@ -0,0 +1,80 @@ +/** + * Environment-scoped "a server restart is expected" overlay. + * + * Set around a server self-update so the connection UI can tell an + * intentional restart apart from a genuine outage: while the window is + * active, disconnects present as a calm "restarting" notice and the client + * retries on a fast flat cadence instead of escalating backoff. This is + * deliberately not a connection-supervisor phase — the supervisor state + * machine stays transport-only, and this store is UI-side intent that + * expires on its own if the server never comes back. + */ +import type { EnvironmentId } from "@t3tools/contracts"; +import { useEffect, useReducer } from "react"; +import { create } from "zustand"; + +/** + * How long after the last update signal (dispatch, RPC acknowledgement, or + * interrupted RPC) a disconnect is still attributed to the restart. Covers + * the respawn shim (~3s), boot-service systemd swap, server boot, and relay + * re-registration with generous margin; after it lapses the normal error + * presentation returns. + */ +export const SERVER_RESTART_EXPECTED_WINDOW_MS = 90_000; + +interface ServerRestartState { + readonly expectedUntilByEnvironmentId: Readonly>; + readonly expect: (environmentId: EnvironmentId) => void; + readonly clear: (environmentId: EnvironmentId) => void; +} + +export const useServerRestartStore = create()((set) => ({ + expectedUntilByEnvironmentId: {}, + expect: (environmentId) => + set((state) => ({ + expectedUntilByEnvironmentId: { + ...state.expectedUntilByEnvironmentId, + [environmentId]: Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, + }, + })), + clear: (environmentId) => + set((state) => { + if (!(environmentId in state.expectedUntilByEnvironmentId)) { + return state; + } + const next = { ...state.expectedUntilByEnvironmentId }; + delete next[environmentId]; + return { expectedUntilByEnvironmentId: next }; + }), +})); + +export function expectServerRestart(environmentId: EnvironmentId): void { + useServerRestartStore.getState().expect(environmentId); +} + +export function clearExpectedServerRestart(environmentId: EnvironmentId): void { + useServerRestartStore.getState().clear(environmentId); +} + +/** + * Whether a restart window is currently active for the environment. + * Re-renders when the window is set, cleared, or lapses. + */ +export function useServerRestartExpected(environmentId: EnvironmentId | null): boolean { + const expiresAt = useServerRestartStore((state) => + environmentId === null ? null : (state.expectedUntilByEnvironmentId[environmentId] ?? null), + ); + const [, tick] = useReducer((count: number) => count + 1, 0); + useEffect(() => { + if (expiresAt === null) { + return; + } + const remaining = expiresAt - Date.now(); + if (remaining <= 0) { + return; + } + const timer = setTimeout(tick, remaining + 50); + return () => clearTimeout(timer); + }, [expiresAt]); + return expiresAt !== null && expiresAt > Date.now(); +} diff --git a/packages/client-runtime/src/authorization/layer.test.ts b/packages/client-runtime/src/authorization/layer.test.ts index 1d2c6c6cca7..13531609d63 100644 --- a/packages/client-runtime/src/authorization/layer.test.ts +++ b/packages/client-runtime/src/authorization/layer.test.ts @@ -316,6 +316,42 @@ describe("RemoteEnvironmentAuthorization", () => { }), ); + it.effect("keeps a cached token when the endpoint is merely unreachable", () => + Effect.gen(function* () { + const cached = new TokenStore.RemoteDpopAccessToken({ + environmentId: ENVIRONMENT_ID, + label: DESCRIPTOR.label, + endpoint: ENDPOINT, + accessToken: "cached-access-token", + expiresAtEpochMs: Number.MAX_SAFE_INTEGER, + dpopThumbprint: "thumbprint-1", + }); + // A restarting server refuses connections outright rather than + // answering; re-bootstrapping would not produce a better endpoint, so + // the token must survive to keep reconnects to a single request. + const harness = yield* makeHarness({ + initialToken: cached, + responses: [], + }); + + yield* Effect.gen(function* () { + const remote = yield* RemoteEnvironmentAuthorization.RemoteEnvironmentAuthorization; + for (let attempt = 0; attempt < 3; attempt++) { + const failure = yield* remote + .authorizeDpop({ + expectedEnvironmentId: ENVIRONMENT_ID, + obtainBootstrap: harness.obtainBootstrap, + }) + .pipe(Effect.flip); + expect(failure._tag).toBe("ConnectionTransientError"); + } + }).pipe(Effect.provide(harness.layer)); + + expect((yield* Ref.get(harness.tokens)).get(ENVIRONMENT_ID)).toBe(cached); + expect(yield* Ref.get(harness.bootstrapCalls)).toBe(0); + }), + ); + it.effect("does not persist a refreshed token until its websocket ticket succeeds", () => Effect.gen(function* () { const harness = yield* makeHarness({ diff --git a/packages/client-runtime/src/authorization/service.ts b/packages/client-runtime/src/authorization/service.ts index 624ecf7f672..29fea911278 100644 --- a/packages/client-runtime/src/authorization/service.ts +++ b/packages/client-runtime/src/authorization/service.ts @@ -60,6 +60,24 @@ export class RemoteEnvironmentAuthorization extends Context.Service< const TOKEN_EXPIRY_SAFETY_MARGIN_MS = 60_000; const CACHED_ENDPOINT_FAILURE_THRESHOLD = 2; +/** + * Transient reasons that suggest the *cached endpoint itself* is stale — + * something answered and rejected or could not route the request — so + * re-bootstrapping may hand back a different endpoint. + * + * `network` and `timeout` are excluded on purpose: they only prove the + * endpoint was unreachable right now, which is the normal shape of a server + * restart or a brief outage. Evicting on those threw away a perfectly good + * token and forced a full relay bootstrap + token exchange on every + * reconnect, adding latency exactly when the server was coming back. + */ +const CACHED_ENDPOINT_STALE_REASONS: ReadonlySet = new Set([ + "endpoint-unavailable", + "remote-unavailable", + "relay-unavailable", + "transport", +]); + function mapDpopSocketError(error: RemoteEnvironmentAuthError | ConnectionAttemptError) { return error._tag === "ConnectionTransientError" || error._tag === "ConnectionBlockedError" ? error @@ -215,6 +233,11 @@ export const make = Effect.gen(function* () { } const mappedFailure = mapDpopSocketError(cachedSocket.failure); if (mappedFailure._tag === "ConnectionTransientError") { + if (!CACHED_ENDPOINT_STALE_REASONS.has(mappedFailure.reason)) { + // Pure unreachability: keep the cached token so the next attempt + // is a single ticket request rather than a full re-bootstrap. + return yield* mappedFailure; + } const failureCount = yield* recordCachedEndpointFailure(input.expectedEnvironmentId); if (failureCount < CACHED_ENDPOINT_FAILURE_THRESHOLD) { return yield* mappedFailure; diff --git a/packages/client-runtime/src/connection/presentation.test.ts b/packages/client-runtime/src/connection/presentation.test.ts index e13638a2b41..332fc3053cf 100644 --- a/packages/client-runtime/src/connection/presentation.test.ts +++ b/packages/client-runtime/src/connection/presentation.test.ts @@ -5,6 +5,7 @@ import * as Option from "effect/Option"; import { BearerConnectionProfile, type ConnectionCatalogEntry } from "./catalog.ts"; import { BearerConnectionTarget, + ConnectionBlockedError, ConnectionTransientError, type SupervisorConnectionState, } from "./model.ts"; @@ -59,7 +60,9 @@ describe("connection presentation", () => { expect(presentConnectionState(supervisorState({ phase: "connecting", attempt: 1 }))).toEqual({ phase: "connecting", error: null, + reason: null, traceId: null, + retryAt: null, }); expect( presentConnectionState( @@ -75,8 +78,10 @@ describe("connection presentation", () => { ), ).toEqual({ phase: "reconnecting", - error: "Socket closed.", + error: "The connection to the environment was interrupted.", + reason: "transport", traceId: "trace-previous", + retryAt: null, }); expect( presentConnectionState( @@ -93,8 +98,48 @@ describe("connection presentation", () => { ), ).toEqual({ phase: "reconnecting", - error: "Disconnected.", + error: "The connection to the environment was interrupted.", + reason: "transport", traceId: "trace-1", + retryAt: 1, + }); + }); + + it("summarizes transport failures without exposing endpoint internals", () => { + const presented = presentConnectionState( + supervisorState({ + phase: "backoff", + attempt: 2, + retryAt: 5, + lastFailure: new ConnectionTransientError({ + reason: "network", + detail: + "Failed to fetch remote environment endpoint https://prod-6e7fc14fddd8f0fb.t3coderelay.com/api/auth/websocket-ticket (HttpClientError: Transport error).", + }), + }), + ); + expect(presented.error).toBe("The environment could not be reached."); + expect(presented.error).not.toContain("t3coderelay.com"); + expect(presented.reason).toBe("network"); + }); + + it("keeps authored blocked-failure messages, which are already actionable", () => { + expect( + presentConnectionState( + supervisorState({ + phase: "blocked", + lastFailure: new ConnectionBlockedError({ + reason: "authentication", + detail: "The environment credential is invalid.", + }), + }), + ), + ).toEqual({ + phase: "error", + error: "The environment credential is invalid.", + reason: "authentication", + traceId: null, + retryAt: null, }); }); @@ -106,7 +151,7 @@ describe("connection presentation", () => { stage: "opening", attempt: 2, lastFailure: new ConnectionTransientError({ - reason: "transport", + reason: "timeout", detail: "Relay connection timed out.", traceId: "trace-retry", }), @@ -114,8 +159,10 @@ describe("connection presentation", () => { ), ).toEqual({ phase: "reconnecting", - error: "Relay connection timed out.", + error: "The environment did not respond in time.", + reason: "timeout", traceId: "trace-retry", + retryAt: null, }); }); @@ -127,7 +174,9 @@ describe("connection presentation", () => { const connection = { phase: "reconnecting", error: "Relay request timed out.", + reason: "timeout", traceId: "trace-retry", + retryAt: null, } as const; expect(connectionStatusText(connection)).toBe( "Failed to connect. Reconnecting... Reason: Relay request timed out.", @@ -147,7 +196,9 @@ describe("connection presentation", () => { ).toEqual({ phase: "offline", error: null, + reason: null, traceId: null, + retryAt: null, }); }); @@ -163,7 +214,9 @@ describe("connection presentation", () => { ).toEqual({ phase: "connected", error: null, + reason: null, traceId: null, + retryAt: null, }); }); @@ -181,7 +234,9 @@ describe("connection presentation", () => { ).toEqual({ phase: "available", error: null, + reason: null, traceId: null, + retryAt: null, }); }); }); diff --git a/packages/client-runtime/src/connection/presentation.ts b/packages/client-runtime/src/connection/presentation.ts index 168443deceb..dfd511581da 100644 --- a/packages/client-runtime/src/connection/presentation.ts +++ b/packages/client-runtime/src/connection/presentation.ts @@ -2,7 +2,7 @@ import type { ServerConfig } from "@t3tools/contracts"; import * as Option from "effect/Option"; import type { ConnectionCatalogEntry } from "./catalog.ts"; -import type { NetworkStatus, SupervisorConnectionState } from "./model.ts"; +import type { ConnectionAttemptError, NetworkStatus, SupervisorConnectionState } from "./model.ts"; export type EnvironmentConnectionPhase = | "available" @@ -12,10 +12,18 @@ export type EnvironmentConnectionPhase = | "connected" | "error"; +export type EnvironmentConnectionFailureReason = ConnectionAttemptError["reason"]; + export interface EnvironmentConnectionPresentation { readonly phase: EnvironmentConnectionPhase; readonly error: string | null; + readonly reason: EnvironmentConnectionFailureReason | null; readonly traceId: string | null; + /** Epoch ms of the next scheduled attempt while the supervisor sleeps in + backoff; null while an attempt is live. Callers that want to retry + early (retryNow) should only do so while this is set — waking an + in-flight attempt cancels and restarts it. */ + readonly retryAt: number | null; } export interface EnvironmentPresentation { @@ -24,38 +32,91 @@ export interface EnvironmentPresentation { readonly serverConfig: ServerConfig | null; } +/** + * Transport errors carry internal endpoint URLs and raw causes (see + * rpc/http.ts) that belong in diagnostics, not user-facing banners. Present + * a stable per-reason summary instead; the raw message stays available on + * the supervisor state for connection diagnostics. + */ +export function connectionFailureSummary(failure: ConnectionAttemptError): string { + switch (failure.reason) { + case "network": + return "The environment could not be reached."; + case "transport": + return "The connection to the environment was interrupted."; + case "timeout": + return "The environment did not respond in time."; + case "endpoint-unavailable": + case "remote-unavailable": + return "The environment is not accepting connections yet."; + case "relay-unavailable": + return "The relay is temporarily unavailable."; + // Blocked reasons are actionable, and their details are authored + // messages (errors.ts) rather than raw transport dumps. + case "authentication": + case "permission": + case "configuration": + case "unsupported": + return failure.message; + } +} + +function presentFailure(failure: ConnectionAttemptError | null): { + readonly error: string | null; + readonly reason: EnvironmentConnectionFailureReason | null; + readonly traceId: string | null; +} { + if (failure === null) { + return { error: null, reason: null, traceId: null }; + } + return { + error: connectionFailureSummary(failure), + reason: failure.reason, + traceId: failure.traceId ?? null, + }; +} + export function presentConnectionState( state: SupervisorConnectionState, ): EnvironmentConnectionPresentation { switch (state.phase) { case "available": - return { phase: "available", error: null, traceId: null }; + return { phase: "available", ...presentFailure(null), retryAt: null }; case "offline": - return { phase: "offline", error: null, traceId: null }; + return { phase: "offline", ...presentFailure(null), retryAt: null }; case "connecting": return { phase: state.attempt <= 1 && state.lastFailure === null ? "connecting" : "reconnecting", - error: state.lastFailure?.message ?? null, - traceId: state.lastFailure?.traceId ?? null, + ...presentFailure(state.lastFailure), + retryAt: null, }; case "connected": - return { phase: "connected", error: null, traceId: null }; + return { phase: "connected", ...presentFailure(null), retryAt: null }; case "backoff": return { phase: "reconnecting", - error: state.lastFailure?.message ?? null, - traceId: state.lastFailure?.traceId ?? null, + ...presentFailure(state.lastFailure), + retryAt: state.retryAt, }; case "blocked": return { phase: "error", - error: state.lastFailure?.message ?? null, - traceId: state.lastFailure?.traceId ?? null, + ...presentFailure(state.lastFailure), + retryAt: null, }; } } -export function connectionStatusText(connection: EnvironmentConnectionPresentation): string { +/** The status helpers only read phase and error, so callers holding a phase + and a message do not have to synthesize a whole presentation. traceId is + accepted (and ignored) because callers commonly carry it alongside. */ +export interface ConnectionStatusTextInput { + readonly phase: EnvironmentConnectionPhase; + readonly error: string | null; + readonly traceId?: string | null; +} + +export function connectionStatusText(connection: ConnectionStatusTextInput): string { switch (connection.phase) { case "available": return "Available"; @@ -76,7 +137,7 @@ export function connectionStatusText(connection: EnvironmentConnectionPresentati } } -export function connectionStatusTitle(connection: EnvironmentConnectionPresentation): string { +export function connectionStatusTitle(connection: ConnectionStatusTextInput): string { if (connection.phase === "reconnecting" && connection.error) { return "Failed to connect. Reconnecting..."; } From 7aecc5d47a02a0a549eefea11172ce4ea72b78de Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Fri, 24 Jul 2026 16:47:51 -0700 Subject: [PATCH 2/5] fix: address review findings on the reconnect flow 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) --- apps/server/src/cloud/selfUpdate.test.ts | 33 ++++++++ apps/server/src/cloud/selfUpdate.ts | 19 ++++- .../components/ServerUpdateAction.test.tsx | 84 ++++++++++++++++++- .../web/src/components/ServerUpdateAction.tsx | 43 ++++++++-- packages/contracts/src/server.ts | 11 +++ 5 files changed, 181 insertions(+), 9 deletions(-) diff --git a/apps/server/src/cloud/selfUpdate.test.ts b/apps/server/src/cloud/selfUpdate.test.ts index 7619206470a..a81649e07f7 100644 --- a/apps/server/src/cloud/selfUpdate.test.ts +++ b/apps/server/src/cloud/selfUpdate.test.ts @@ -62,6 +62,8 @@ const makeRecordingRunnerLayer = ( readonly stdoutFor?: | ((command: string, args: ReadonlyArray) => string | undefined) | undefined; + /** Raises a defect (not a typed failure) to exercise unexpected-error paths. */ + readonly dieWhen?: ((command: string, args: ReadonlyArray) => boolean) | undefined; }, ) => Layer.succeed( @@ -70,6 +72,9 @@ const makeRecordingRunnerLayer = ( run: (input) => Effect.sync(() => { commands.push({ command: input.command, args: input.args }); + if (options?.dieWhen?.(input.command, input.args) === true) { + throw new Error(`${input.command} died unexpectedly`); + } const failed = options?.failWhen?.(input.command, input.args) === true; const versionFromPath = input.command === NODE_PATH && input.args[1] === "--version" @@ -281,6 +286,7 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { readonly failWhen?: (command: string, args: ReadonlyArray) => boolean; readonly stdoutFor?: (command: string, args: ReadonlyArray) => string | undefined; readonly failSpawn?: boolean; + readonly dieWhen?: (command: string, args: ReadonlyArray) => boolean; }) { const fs = yield* FileSystem.FileSystem; const path = yield* Path.Path; @@ -353,6 +359,7 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { makeRecordingRunnerLayer(commands, { failWhen: options?.failWhen, stdoutFor: options?.stdoutFor, + dieWhen: options?.dieWhen, }), configLayer, ), @@ -588,6 +595,32 @@ it.layer(NodeServices.layer)("ServerSelfUpdate.update", (it) => { }).pipe(Effect.provide(TestClock.layer())), ); + it.effect("permits a retry when the deferred restart dies unexpectedly", () => + Effect.gen(function* () { + let dieOnRestart = true; + const context = yield* makeContext({ + bootService: true, + dieWhen: (command, args) => { + if (command !== "systemctl" || args[1] !== "restart" || !dieOnRestart) { + return false; + } + dieOnRestart = false; + return true; + }, + }); + + const first = yield* context.service.update({ targetVersion: "0.0.29" }); + assert.deepEqual(first, { targetVersion: "0.0.29", method: "boot-service" }); + yield* TestClock.adjust(Duration.seconds(10)); + + // A defect bypasses the typed rollback handler, so the in-flight guard + // has to be released outside it — otherwise this live process would + // reject every further update until restarted by other means. + const retry = yield* context.service.update({ targetVersion: "0.0.30" }); + assert.deepEqual(retry, { targetVersion: "0.0.30", method: "boot-service" }); + }).pipe(Effect.provide(TestClock.layer())), + ); + it.effect("restores the previous systemd unit when daemon-reload fails", () => Effect.gen(function* () { const context = yield* makeContext({ diff --git a/apps/server/src/cloud/selfUpdate.ts b/apps/server/src/cloud/selfUpdate.ts index 4dd3da27a67..f8d0b532b2b 100644 --- a/apps/server/src/cloud/selfUpdate.ts +++ b/apps/server/src/cloud/selfUpdate.ts @@ -3,6 +3,7 @@ // detached fire-and-forget child that outlives this process, while Effect's // ChildProcessSpawner ties every child to a scope that kills it. import { + SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON, ServerSelfUpdateError, type ServerSelfUpdateCapability, type ServerSelfUpdateInput, @@ -18,6 +19,7 @@ import * as NodeChildProcess from "node:child_process"; import * as Context from "effect/Context"; import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Path from "effect/Path"; @@ -246,7 +248,7 @@ export const make = Effect.fn("cloud.server_self_update.make")(function* (option const alreadyRunning = yield* Ref.getAndSet(inFlight, true); if (alreadyRunning) { - return yield* failWith("A server update is already in progress."); + return yield* failWith(SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON); } return yield* Effect.gen(function* () { @@ -387,9 +389,24 @@ export const make = Effect.fn("cloud.server_self_update.make")(function* (option "Server self-update could not restore the previous systemd unit after a rejected restart.", ).pipe(Effect.annotateLogs({ targetVersion, error: String(rollbackError) })), ), + // The process survived a rejected restart, so it must stay + // updatable — leaving the guard set would reject every retry + // until the server was restarted by other means. Effect.ensuring(Ref.set(inFlight, false)), ), ), + // The typed catch above cannot see defects or interrupts, which + // would otherwise strand the guard on a live process. A success + // deliberately leaves it set: systemd is about to stop this + // process and a second update would race the handoff. + Effect.onExit((exit) => + Exit.isSuccess(exit) + ? Effect.void + : Effect.logError("Server self-update restart fiber ended unexpectedly.").pipe( + Effect.annotateLogs({ targetVersion, exit: String(exit) }), + Effect.ensuring(Ref.set(inFlight, false)), + ), + ), ), ); } else { diff --git a/apps/web/src/components/ServerUpdateAction.test.tsx b/apps/web/src/components/ServerUpdateAction.test.tsx index c0ba1c693d4..7d5b4724e0c 100644 --- a/apps/web/src/components/ServerUpdateAction.test.tsx +++ b/apps/web/src/components/ServerUpdateAction.test.tsx @@ -2,13 +2,22 @@ import type { Dispatch, ReactElement, SetStateAction } from "react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; import * as Cause from "effect/Cause"; import { AsyncResult } from "effect/unstable/reactivity"; -import type { EnvironmentId } from "@t3tools/contracts"; +import { + SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON, + ServerSelfUpdateError, + type EnvironmentId, +} from "@t3tools/contracts"; const testState = vi.hoisted(() => ({ updateServer: vi.fn(), toast: vi.fn(), })); +const restartStore = vi.hoisted(() => ({ + expect: vi.fn(), + clear: vi.fn(), +})); + const hooks = vi.hoisted(() => { let cursor = 0; let slots: unknown[] = []; @@ -78,6 +87,10 @@ vi.mock("~/state/use-atom-command", () => ({ vi.mock("./ui/toast", () => ({ toastManager: { add: testState.toast }, })); +vi.mock("~/serverRestartStore", () => ({ + expectServerRestart: restartStore.expect, + clearExpectedServerRestart: restartStore.clear, +})); import { ServerUpdateAction } from "./ServerUpdateAction"; @@ -118,6 +131,8 @@ describe("ServerUpdateAction", () => { hooks.reset(); testState.updateServer.mockReset(); testState.toast.mockReset(); + restartStore.expect.mockReset(); + restartStore.clear.mockReset(); }); afterEach(() => { @@ -195,4 +210,71 @@ describe("ServerUpdateAction", () => { expect.objectContaining({ title: "Server update failed" }), ); }); + + it("re-arms the restart window when a slow install finally succeeds", async () => { + const update = + deferred< + ReturnType> + >(); + testState.updateServer.mockReturnValue(update.promise); + + renderAction().props.onClick?.(); + expect(restartStore.expect).toHaveBeenCalledTimes(1); + + // An install can outlast the 90s restart window armed at click time, so + // the acknowledgement must restart the clock or the ensuing disconnect + // would be presented as an outage. + await vi.advanceTimersByTimeAsync(5 * 60_000); + update.resolve(AsyncResult.success({ targetVersion: "0.0.29", method: "boot-service" })); + await flushPromises(); + + expect(restartStore.expect).toHaveBeenCalledTimes(2); + expect(restartStore.clear).not.toHaveBeenCalled(); + }); + + it("re-arms the restart window on an interrupted restart RPC", async () => { + testState.updateServer.mockResolvedValue(AsyncResult.failure(Cause.interrupt())); + + renderAction().props.onClick?.(); + await flushPromises(); + + expect(restartStore.expect).toHaveBeenCalledTimes(2); + expect(restartStore.clear).not.toHaveBeenCalled(); + }); + + it("withdraws the restart expectation when the server reports a real failure", async () => { + testState.updateServer.mockResolvedValue( + AsyncResult.failure( + Cause.fail( + new ServerSelfUpdateError({ reason: "Could not install the requested t3 version." }), + ), + ), + ); + + renderAction().props.onClick?.(); + await flushPromises(); + + // The server answered, so it is still alive and no restart is coming. + expect(restartStore.clear).toHaveBeenCalledTimes(1); + expect(testState.toast).toHaveBeenCalledWith( + expect.objectContaining({ title: "Server update failed" }), + ); + }); + + it("keeps the restart expectation when an update is already in progress", async () => { + testState.updateServer.mockResolvedValue( + AsyncResult.failure( + Cause.fail( + new ServerSelfUpdateError({ reason: SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON }), + ), + ), + ); + + renderAction().props.onClick?.(); + await flushPromises(); + + // An earlier request is still installing, so a restart really is pending + // — clearing here would restore the harsh reconnect UI mid-restart. + expect(restartStore.clear).not.toHaveBeenCalled(); + }); }); diff --git a/apps/web/src/components/ServerUpdateAction.tsx b/apps/web/src/components/ServerUpdateAction.tsx index a97a6ab67a7..e186e3f8518 100644 --- a/apps/web/src/components/ServerUpdateAction.tsx +++ b/apps/web/src/components/ServerUpdateAction.tsx @@ -1,5 +1,10 @@ import { useEffect, useRef, useState } from "react"; -import type { EnvironmentId, ServerSelfUpdateCapability } from "@t3tools/contracts"; +import { + ServerSelfUpdateError, + type EnvironmentId, + type ServerSelfUpdateCapability, +} from "@t3tools/contracts"; +import * as Schema from "effect/Schema"; import { isAtomCommandInterrupted, squashAtomCommandFailure, @@ -25,6 +30,18 @@ function updateFailureMessage(error: unknown): string { return error instanceof Error ? error.message : "Server update failed."; } +/** + * The server rejects a second update while one is already installing. That + * failure means a restart is still pending from the earlier request, so the + * restart expectation must survive it — unlike every other failure, which + * proves nothing is coming. + */ +const isServerSelfUpdateError = Schema.is(ServerSelfUpdateError); + +function isUpdateAlreadyRunning(error: unknown): boolean { + return isServerSelfUpdateError(error) && error.isAlreadyRunning; +} + /** * The call-to-action for a version-skewed server, matched to the update path * it advertises: a one-click install-and-restart for servers that can update @@ -116,13 +133,17 @@ export function ServerUpdateAction({ clearTimeout(expiry); expiry = armExpiry(); } + // The install can consume most of the request window, so the window + // armed at click time may already have lapsed by the time the restart + // actually begins. Re-arm from the handoff, not from the click. + expectServerRestart(environmentId); }; // Armed before dispatch: the boot-service path acknowledges and then // drops every connection, and even the respawn acknowledgement can lose // the race against the disconnect. An explicit RPC failure proves the // server stayed alive, so the expectation is withdrawn there; on - // success or interrupt it stays armed and expires on its own if the - // server never returns. + // success or interrupt it is re-armed from that point and expires on its + // own if the server never returns. expectServerRestart(environmentId); void Promise.resolve() .then(() => @@ -137,15 +158,21 @@ export function ServerUpdateAction({ // An interrupt may be the expected boot-service disconnect, but it // can also be client-side cancellation before restart was accepted. // Release the action quietly; version sync will remove it when a - // successful replacement reconnects. + // successful replacement reconnects. Re-arm from here for the same + // reason as the success path: a long install may have outlasted the + // window armed at click time. if (isAtomCommandInterrupted(result)) { + expectServerRestart(environmentId); return; } - clearExpectedServerRestart(environmentId); + const failure = squashAtomCommandFailure(result); + if (!isUpdateAlreadyRunning(failure)) { + clearExpectedServerRestart(environmentId); + } toastManager.add({ type: "error", title: "Server update failed", - description: updateFailureMessage(squashAtomCommandFailure(result)), + description: updateFailureMessage(failure), }); return; } @@ -161,7 +188,9 @@ export function ServerUpdateAction({ }) .catch((error: unknown) => { if (!ownsAttempt()) return; - clearExpectedServerRestart(environmentId); + if (!isUpdateAlreadyRunning(error)) { + clearExpectedServerRestart(environmentId); + } toastManager.add({ type: "error", title: "Server update failed", diff --git a/packages/contracts/src/server.ts b/packages/contracts/src/server.ts index 69699c7a839..49ac828f4ff 100644 --- a/packages/contracts/src/server.ts +++ b/packages/contracts/src/server.ts @@ -590,6 +590,12 @@ export const ServerSelfUpdateResult = Schema.Struct({ }); export type ServerSelfUpdateResult = typeof ServerSelfUpdateResult.Type; +/** Rejection reason when an update is dispatched while one is already + installing. Shared so clients can tell "nothing is happening" apart from + "a restart is already pending from an earlier request", which the update + UI must not treat as the end of the flow. */ +export const SERVER_SELF_UPDATE_ALREADY_RUNNING_REASON = "A server update is already in progress."; + export class ServerSelfUpdateError extends Schema.TaggedErrorClass()( "ServerSelfUpdateError", { @@ -600,4 +606,9 @@ export class ServerSelfUpdateError extends Schema.TaggedErrorClass Date: Fri, 24 Jul 2026 16:59:17 -0700 Subject: [PATCH 3/5] fix: make the restart window survive until the restart is observed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/web/src/components/ChatView.tsx | 27 +++- .../components/ServerUpdateAction.test.tsx | 16 ++- .../web/src/components/ServerUpdateAction.tsx | 28 +++- apps/web/src/serverRestartStore.test.ts | 123 ++++++++++++++++++ apps/web/src/serverRestartStore.ts | 102 +++++++++++++-- 5 files changed, 272 insertions(+), 24 deletions(-) create mode 100644 apps/web/src/serverRestartStore.test.ts diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index dc26d3d25e2..26857b79fb5 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -25,7 +25,12 @@ import { connectionStatusTitle, type EnvironmentConnectionPresentation, } from "@t3tools/client-runtime/connection"; -import { clearExpectedServerRestart, useServerRestartExpected } from "~/serverRestartStore"; +import { + clearExpectedServerRestart, + isServerRestarting, + markServerRestartDisconnected, + useServerRestartExpectation, +} from "~/serverRestartStore"; import { effectiveSettled, effectiveSnoozed } from "@t3tools/client-runtime/state/thread-settled"; import { parseScopedThreadKey, @@ -1625,17 +1630,28 @@ function ChatViewContent(props: ChatViewProps) { // present them calmly and retry fast instead of letting exponential // backoff strand the client asleep when the server comes back. const activeEnvironmentId = activeEnvironment?.environmentId ?? null; - const activeEnvironmentRestartExpected = useServerRestartExpected(activeEnvironmentId); - const activeEnvironmentRestarting = - activeEnvironmentRestartExpected && activeEnvironmentUnavailable; + const activeEnvironmentRestartExpectation = useServerRestartExpectation(activeEnvironmentId); + const activeEnvironmentRestartExpected = activeEnvironmentRestartExpectation.expected; + const activeEnvironmentRestartSawDisconnect = activeEnvironmentRestartExpectation.sawDisconnect; + const activeEnvironmentRestarting = isServerRestarting({ + expectation: activeEnvironmentRestartExpectation, + environmentUnavailable: activeEnvironmentUnavailable, + }); useEffect(() => { if (activeEnvironmentId === null || !activeEnvironmentRestartExpected) { return; } if (activeEnvironmentConnectionPhase === "connected") { - clearExpectedServerRestart(activeEnvironmentId); + // The window is armed while the old server is still connected, so a + // healthy connection only ends it once the restart's disconnect has + // actually been seen. Clearing on any connected render would destroy + // the window before the restart it exists to cover. + if (activeEnvironmentRestartSawDisconnect) { + clearExpectedServerRestart(activeEnvironmentId); + } return; } + markServerRestartDisconnected(activeEnvironmentId); // Only wake the supervisor out of a backoff sleep; while an attempt is // live, retryNow would interrupt and restart it. if (activeEnvironment?.connection.retryAt == null) { @@ -1650,6 +1666,7 @@ function ChatViewContent(props: ChatViewProps) { activeEnvironmentConnectionPhase, activeEnvironmentId, activeEnvironmentRestartExpected, + activeEnvironmentRestartSawDisconnect, retryEnvironment, ]); const handleReconnectActiveEnvironment = useCallback( diff --git a/apps/web/src/components/ServerUpdateAction.test.tsx b/apps/web/src/components/ServerUpdateAction.test.tsx index 7d5b4724e0c..fbdf8125127 100644 --- a/apps/web/src/components/ServerUpdateAction.test.tsx +++ b/apps/web/src/components/ServerUpdateAction.test.tsx @@ -16,6 +16,7 @@ const testState = vi.hoisted(() => ({ const restartStore = vi.hoisted(() => ({ expect: vi.fn(), clear: vi.fn(), + expectation: vi.fn(() => ({ expected: false, sawDisconnect: false })), })); const hooks = vi.hoisted(() => { @@ -87,10 +88,15 @@ vi.mock("~/state/use-atom-command", () => ({ vi.mock("./ui/toast", () => ({ toastManager: { add: testState.toast }, })); -vi.mock("~/serverRestartStore", () => ({ - expectServerRestart: restartStore.expect, - clearExpectedServerRestart: restartStore.clear, -})); +vi.mock("~/serverRestartStore", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + expectServerRestart: restartStore.expect, + clearExpectedServerRestart: restartStore.clear, + useServerRestartExpectation: restartStore.expectation, + }; +}); import { ServerUpdateAction } from "./ServerUpdateAction"; @@ -133,6 +139,8 @@ describe("ServerUpdateAction", () => { testState.toast.mockReset(); restartStore.expect.mockReset(); restartStore.clear.mockReset(); + restartStore.expectation.mockReset(); + restartStore.expectation.mockReturnValue({ expected: false, sawDisconnect: false }); }); afterEach(() => { diff --git a/apps/web/src/components/ServerUpdateAction.tsx b/apps/web/src/components/ServerUpdateAction.tsx index e186e3f8518..1f5e721d2c3 100644 --- a/apps/web/src/components/ServerUpdateAction.tsx +++ b/apps/web/src/components/ServerUpdateAction.tsx @@ -11,7 +11,12 @@ import { } from "@t3tools/client-runtime/state/runtime"; import { useCopyToClipboard } from "~/hooks/useCopyToClipboard"; -import { clearExpectedServerRestart, expectServerRestart } from "~/serverRestartStore"; +import { + clearExpectedServerRestart, + expectServerRestart, + isServerRestartSettled, + useServerRestartExpectation, +} from "~/serverRestartStore"; import { serverEnvironment } from "~/state/server"; import { useAtomCommand } from "~/state/use-atom-command"; import { manualServerUpdateCommand } from "~/versionSkew"; @@ -98,6 +103,27 @@ export function ServerUpdateAction({ [], ); + // The restart is scheduled after the RPC is acknowledged, so a rejected + // restart (or a replacement that comes back on the old version) can no + // longer be reported through the call. Without this, the mismatch banner + // would keep its only action disabled for the full safety window. Once the + // restart window has closed and this component is still mounted — meaning + // the skew is unresolved and the server is answering again — release the + // action so the user can retry immediately. + const restartSettled = isServerRestartSettled(useServerRestartExpectation(environmentId)); + useEffect(() => { + if (!restartSettled || !inFlightRef.current) { + return; + } + if (expiryRef.current !== null) { + clearTimeout(expiryRef.current); + expiryRef.current = null; + } + attemptRef.current += 1; + inFlightRef.current = false; + setPending(false); + }, [restartSettled]); + const handleUpdate = () => { // Synchronous re-entry guard: setPending is async, so a rapid // double-click would otherwise dispatch two updates. diff --git a/apps/web/src/serverRestartStore.test.ts b/apps/web/src/serverRestartStore.test.ts new file mode 100644 index 00000000000..939687ce9b1 --- /dev/null +++ b/apps/web/src/serverRestartStore.test.ts @@ -0,0 +1,123 @@ +import type { EnvironmentId } from "@t3tools/contracts"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +import { + SERVER_RESTART_EXPECTED_WINDOW_MS, + clearExpectedServerRestart, + expectServerRestart, + isServerRestartSettled, + isServerRestarting, + markServerRestartDisconnected, + useServerRestartStore, +} from "./serverRestartStore"; + +const ENVIRONMENT_ID = "env-test" as EnvironmentId; + +function windowFor(environmentId: EnvironmentId) { + return useServerRestartStore.getState().windowByEnvironmentId[environmentId] ?? null; +} + +describe("serverRestartStore", () => { + beforeEach(() => { + vi.useFakeTimers(); + useServerRestartStore.setState({ windowByEnvironmentId: {} }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("arms a window that has not yet seen the restart's disconnect", () => { + expectServerRestart(ENVIRONMENT_ID); + + // The window is armed while the old server is still connected, so the + // consumer must be able to tell "waiting for the restart" apart from + // "the restart already happened". + expect(windowFor(ENVIRONMENT_ID)?.sawDisconnect).toBe(false); + expect(windowFor(ENVIRONMENT_ID)?.expiresAt).toBe( + Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, + ); + }); + + it("remembers an observed disconnect across a re-arm", () => { + expectServerRestart(ENVIRONMENT_ID); + markServerRestartDisconnected(ENVIRONMENT_ID); + + // A slow install acknowledging late re-arms the window; forgetting the + // disconnect here would strand the restarting UI until the window lapsed. + vi.advanceTimersByTime(30_000); + expectServerRestart(ENVIRONMENT_ID); + + expect(windowFor(ENVIRONMENT_ID)?.sawDisconnect).toBe(true); + expect(windowFor(ENVIRONMENT_ID)?.expiresAt).toBe( + Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, + ); + }); + + it("ignores a disconnect for an environment with no armed window", () => { + markServerRestartDisconnected(ENVIRONMENT_ID); + expect(windowFor(ENVIRONMENT_ID)).toBeNull(); + }); + + it("clears only the requested environment", () => { + const other = "env-other" as EnvironmentId; + expectServerRestart(ENVIRONMENT_ID); + expectServerRestart(other); + + clearExpectedServerRestart(ENVIRONMENT_ID); + + expect(windowFor(ENVIRONMENT_ID)).toBeNull(); + expect(windowFor(other)).not.toBeNull(); + }); + + describe("isServerRestarting", () => { + it("does not claim a still-connected environment is restarting", () => { + // The window is armed before dispatch, while the old server is still + // connected. Presenting that as "restarting" would fire the moment the + // user clicks, before anything has actually gone away. + expect( + isServerRestarting({ + expectation: { expected: true, sawDisconnect: false }, + environmentUnavailable: false, + }), + ).toBe(false); + }); + + it("presents an armed window with an away environment as restarting", () => { + expect( + isServerRestarting({ + expectation: { expected: true, sawDisconnect: true }, + environmentUnavailable: true, + }), + ).toBe(true); + }); + + it("falls back to the outage UI once the window lapses", () => { + // The server never came back; the calm banner must give way. + expect( + isServerRestarting({ + expectation: { expected: false, sawDisconnect: true }, + environmentUnavailable: true, + }), + ).toBe(false); + }); + }); + + describe("isServerRestartSettled", () => { + it("does not treat a live window as settled", () => { + expect(isServerRestartSettled({ expected: true, sawDisconnect: true })).toBe(false); + }); + + it("treats an observed restart whose window has closed as settled", () => { + // Covers a rejected systemd restart: the RPC already acknowledged, so + // nothing else can tell the update action to release itself. + expect(isServerRestartSettled({ expected: false, sawDisconnect: true })).toBe(true); + }); + + it("does not settle a window that never saw a disconnect", () => { + // The restart simply never happened; the explicit-failure path owns + // that case, and settling here would release the action early. + expect(isServerRestartSettled({ expected: false, sawDisconnect: false })).toBe(false); + }); + }); +}); diff --git a/apps/web/src/serverRestartStore.ts b/apps/web/src/serverRestartStore.ts index a439c811685..0c519e71b49 100644 --- a/apps/web/src/serverRestartStore.ts +++ b/apps/web/src/serverRestartStore.ts @@ -22,29 +22,60 @@ import { create } from "zustand"; */ export const SERVER_RESTART_EXPECTED_WINDOW_MS = 90_000; +interface ServerRestartWindow { + readonly expiresAt: number; + /** + * Whether the environment has actually gone away since the window was + * armed. The window is armed while the old server is still connected — that + * is the point, since the acknowledgement races its own disconnect — so a + * connected environment must not end the window until the restart it is + * waiting for has been observed. + */ + readonly sawDisconnect: boolean; +} + interface ServerRestartState { - readonly expectedUntilByEnvironmentId: Readonly>; + readonly windowByEnvironmentId: Readonly>; readonly expect: (environmentId: EnvironmentId) => void; + readonly markDisconnected: (environmentId: EnvironmentId) => void; readonly clear: (environmentId: EnvironmentId) => void; } export const useServerRestartStore = create()((set) => ({ - expectedUntilByEnvironmentId: {}, + windowByEnvironmentId: {}, expect: (environmentId) => set((state) => ({ - expectedUntilByEnvironmentId: { - ...state.expectedUntilByEnvironmentId, - [environmentId]: Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, + windowByEnvironmentId: { + ...state.windowByEnvironmentId, + [environmentId]: { + expiresAt: Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, + // Re-arming (a slow install acknowledging late) must not forget a + // disconnect that was already observed. + sawDisconnect: state.windowByEnvironmentId[environmentId]?.sawDisconnect ?? false, + }, }, })), + markDisconnected: (environmentId) => + set((state) => { + const current = state.windowByEnvironmentId[environmentId]; + if (current === undefined || current.sawDisconnect) { + return state; + } + return { + windowByEnvironmentId: { + ...state.windowByEnvironmentId, + [environmentId]: { ...current, sawDisconnect: true }, + }, + }; + }), clear: (environmentId) => set((state) => { - if (!(environmentId in state.expectedUntilByEnvironmentId)) { + if (!(environmentId in state.windowByEnvironmentId)) { return state; } - const next = { ...state.expectedUntilByEnvironmentId }; + const next = { ...state.windowByEnvironmentId }; delete next[environmentId]; - return { expectedUntilByEnvironmentId: next }; + return { windowByEnvironmentId: next }; }), })); @@ -52,18 +83,58 @@ export function expectServerRestart(environmentId: EnvironmentId): void { useServerRestartStore.getState().expect(environmentId); } +/** Records that the expected restart's disconnect has now been observed. */ +export function markServerRestartDisconnected(environmentId: EnvironmentId): void { + useServerRestartStore.getState().markDisconnected(environmentId); +} + export function clearExpectedServerRestart(environmentId: EnvironmentId): void { useServerRestartStore.getState().clear(environmentId); } +export interface ServerRestartExpectation { + /** An unexpired restart window is armed for this environment. */ + readonly expected: boolean; + /** The restart's disconnect has been observed, so a reconnect ends it. */ + readonly sawDisconnect: boolean; +} + +/** + * Whether an armed restart window has run its course: the disconnect was + * observed and the window has since closed. Callers use this to recover UI + * that was held for the restart — the update RPC is acknowledged before the + * restart runs, so a rejected restart cannot report itself through the call. + * + * Note that a window which never saw a disconnect is not "settled": the + * restart simply never happened, and other paths (an explicit RPC failure) + * own that case. + */ +export function isServerRestartSettled(expectation: ServerRestartExpectation): boolean { + return expectation.sawDisconnect && !expectation.expected; +} + +/** + * Whether the environment should currently be presented as restarting rather + * than as an outage: a live window plus an environment that is actually away. + */ +export function isServerRestarting(input: { + readonly expectation: ServerRestartExpectation; + readonly environmentUnavailable: boolean; +}): boolean { + return input.expectation.expected && input.environmentUnavailable; +} + /** - * Whether a restart window is currently active for the environment. - * Re-renders when the window is set, cleared, or lapses. + * The restart window state for an environment. Re-renders when the window is + * armed, observes its disconnect, is cleared, or lapses. */ -export function useServerRestartExpected(environmentId: EnvironmentId | null): boolean { - const expiresAt = useServerRestartStore((state) => - environmentId === null ? null : (state.expectedUntilByEnvironmentId[environmentId] ?? null), +export function useServerRestartExpectation( + environmentId: EnvironmentId | null, +): ServerRestartExpectation { + const window = useServerRestartStore((state) => + environmentId === null ? null : (state.windowByEnvironmentId[environmentId] ?? null), ); + const expiresAt = window?.expiresAt ?? null; const [, tick] = useReducer((count: number) => count + 1, 0); useEffect(() => { if (expiresAt === null) { @@ -76,5 +147,8 @@ export function useServerRestartExpected(environmentId: EnvironmentId | null): b const timer = setTimeout(tick, remaining + 50); return () => clearTimeout(timer); }, [expiresAt]); - return expiresAt !== null && expiresAt > Date.now(); + return { + expected: expiresAt !== null && expiresAt > Date.now(), + sawDisconnect: window?.sawDisconnect === true, + }; } From 3c9f1db60fc80658dacae919718a6d4c85b52d02 Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Fri, 24 Jul 2026 17:13:30 -0700 Subject: [PATCH 4/5] fix: decouple update-action release from the restart window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/web/src/components/ChatView.tsx | 37 ++--- .../components/ServerUpdateAction.test.tsx | 17 +- .../web/src/components/ServerUpdateAction.tsx | 52 ++++-- apps/web/src/serverRestartStore.test.ts | 117 +++++++------- apps/web/src/serverRestartStore.ts | 150 ++++++++---------- 5 files changed, 186 insertions(+), 187 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 26857b79fb5..c1745d7d36d 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -25,12 +25,7 @@ import { connectionStatusTitle, type EnvironmentConnectionPresentation, } from "@t3tools/client-runtime/connection"; -import { - clearExpectedServerRestart, - isServerRestarting, - markServerRestartDisconnected, - useServerRestartExpectation, -} from "~/serverRestartStore"; +import { isServerRestarting, useServerRestartExpected } from "~/serverRestartStore"; import { effectiveSettled, effectiveSnoozed } from "@t3tools/client-runtime/state/thread-settled"; import { parseScopedThreadKey, @@ -1630,28 +1625,25 @@ function ChatViewContent(props: ChatViewProps) { // present them calmly and retry fast instead of letting exponential // backoff strand the client asleep when the server comes back. const activeEnvironmentId = activeEnvironment?.environmentId ?? null; - const activeEnvironmentRestartExpectation = useServerRestartExpectation(activeEnvironmentId); - const activeEnvironmentRestartExpected = activeEnvironmentRestartExpectation.expected; - const activeEnvironmentRestartSawDisconnect = activeEnvironmentRestartExpectation.sawDisconnect; + const activeEnvironmentRestartExpected = useServerRestartExpected(activeEnvironmentId); const activeEnvironmentRestarting = isServerRestarting({ - expectation: activeEnvironmentRestartExpectation, + restartExpected: activeEnvironmentRestartExpected, environmentUnavailable: activeEnvironmentUnavailable, }); + // While the restart is expected, poke the supervisor on a flat cadence so a + // 16-second backoff sleep cannot outlast the server's return. The window is + // never cleared here: it is armed while the old server is still connected, + // so nothing observed on the connection distinguishes "before the restart" + // from "after it". Presentation already gates on the environment being + // away, which makes a lingering window inert once the replacement is back. useEffect(() => { - if (activeEnvironmentId === null || !activeEnvironmentRestartExpected) { - return; - } - if (activeEnvironmentConnectionPhase === "connected") { - // The window is armed while the old server is still connected, so a - // healthy connection only ends it once the restart's disconnect has - // actually been seen. Clearing on any connected render would destroy - // the window before the restart it exists to cover. - if (activeEnvironmentRestartSawDisconnect) { - clearExpectedServerRestart(activeEnvironmentId); - } + if ( + activeEnvironmentId === null || + !activeEnvironmentRestartExpected || + activeEnvironmentConnectionPhase === "connected" + ) { return; } - markServerRestartDisconnected(activeEnvironmentId); // Only wake the supervisor out of a backoff sleep; while an attempt is // live, retryNow would interrupt and restart it. if (activeEnvironment?.connection.retryAt == null) { @@ -1666,7 +1658,6 @@ function ChatViewContent(props: ChatViewProps) { activeEnvironmentConnectionPhase, activeEnvironmentId, activeEnvironmentRestartExpected, - activeEnvironmentRestartSawDisconnect, retryEnvironment, ]); const handleReconnectActiveEnvironment = useCallback( diff --git a/apps/web/src/components/ServerUpdateAction.test.tsx b/apps/web/src/components/ServerUpdateAction.test.tsx index fbdf8125127..093b22a5136 100644 --- a/apps/web/src/components/ServerUpdateAction.test.tsx +++ b/apps/web/src/components/ServerUpdateAction.test.tsx @@ -16,7 +16,11 @@ const testState = vi.hoisted(() => ({ const restartStore = vi.hoisted(() => ({ expect: vi.fn(), clear: vi.fn(), - expectation: vi.fn(() => ({ expected: false, sawDisconnect: false })), + expected: vi.fn(() => false), +})); + +const environmentState = vi.hoisted(() => ({ + useEnvironment: vi.fn(() => ({ connection: { phase: "connected" } })), })); const hooks = vi.hoisted(() => { @@ -94,9 +98,12 @@ vi.mock("~/serverRestartStore", async (importOriginal) => { ...actual, expectServerRestart: restartStore.expect, clearExpectedServerRestart: restartStore.clear, - useServerRestartExpectation: restartStore.expectation, + useServerRestartExpected: restartStore.expected, }; }); +vi.mock("~/state/environments", () => ({ + useEnvironment: environmentState.useEnvironment, +})); import { ServerUpdateAction } from "./ServerUpdateAction"; @@ -139,8 +146,10 @@ describe("ServerUpdateAction", () => { testState.toast.mockReset(); restartStore.expect.mockReset(); restartStore.clear.mockReset(); - restartStore.expectation.mockReset(); - restartStore.expectation.mockReturnValue({ expected: false, sawDisconnect: false }); + restartStore.expected.mockReset(); + restartStore.expected.mockReturnValue(false); + environmentState.useEnvironment.mockReset(); + environmentState.useEnvironment.mockReturnValue({ connection: { phase: "connected" } }); }); afterEach(() => { diff --git a/apps/web/src/components/ServerUpdateAction.tsx b/apps/web/src/components/ServerUpdateAction.tsx index 1f5e721d2c3..e8a8bbfb4a7 100644 --- a/apps/web/src/components/ServerUpdateAction.tsx +++ b/apps/web/src/components/ServerUpdateAction.tsx @@ -14,9 +14,10 @@ import { useCopyToClipboard } from "~/hooks/useCopyToClipboard"; import { clearExpectedServerRestart, expectServerRestart, - isServerRestartSettled, - useServerRestartExpectation, + isExpectedRestartResolved, + useServerRestartExpected, } from "~/serverRestartStore"; +import { useEnvironment } from "~/state/environments"; import { serverEnvironment } from "~/state/server"; import { useAtomCommand } from "~/state/use-atom-command"; import { manualServerUpdateCommand } from "~/versionSkew"; @@ -104,17 +105,46 @@ export function ServerUpdateAction({ ); // The restart is scheduled after the RPC is acknowledged, so a rejected - // restart (or a replacement that comes back on the old version) can no - // longer be reported through the call. Without this, the mismatch banner - // would keep its only action disabled for the full safety window. Once the - // restart window has closed and this component is still mounted — meaning - // the skew is unresolved and the server is answering again — release the - // action so the user can retry immediately. - const restartSettled = isServerRestartSettled(useServerRestartExpectation(environmentId)); + // restart — or a replacement that comes back on the old version — can no + // longer be reported through the call. Without a release path the mismatch + // banner would keep its only action disabled for the full safety window. + // + // The signal is the environment going away and then answering again while + // this component is still mounted: still mounted means the skew is + // unresolved, so whatever restarted did not deliver the new version. + // Deliberately independent of the restart window, which is UI presentation + // state and can lapse or be re-armed for unrelated reasons. + // Two ways that resolves, because a rejected restart may or may not have + // taken the connection down with it: + // - the environment went away and came back, so something restarted but + // did not deliver the new version; or + // - the restart window lapsed with the connection never dropping, so no + // restart ever happened (systemd refused it and rolled back). + const connectionPhase = useEnvironment(environmentId)?.connection.phase ?? null; + const connectionUnavailable = connectionPhase !== null && connectionPhase !== "connected"; + const restartExpected = useServerRestartExpected(environmentId); + const wentAwayRef = useRef(false); + const sawWindowRef = useRef(false); + if (connectionUnavailable) { + wentAwayRef.current = true; + } + if (restartExpected) { + // Guards against reading a not-yet-rendered window as "already lapsed" + // in the render between the click and the store update landing. + sawWindowRef.current = true; + } + const restartResolved = isExpectedRestartResolved({ + sawRestartWindow: sawWindowRef.current, + restartExpected, + wentAway: wentAwayRef.current, + environmentUnavailable: connectionUnavailable, + }); useEffect(() => { - if (!restartSettled || !inFlightRef.current) { + if (!restartResolved || !inFlightRef.current) { return; } + wentAwayRef.current = false; + sawWindowRef.current = false; if (expiryRef.current !== null) { clearTimeout(expiryRef.current); expiryRef.current = null; @@ -122,7 +152,7 @@ export function ServerUpdateAction({ attemptRef.current += 1; inFlightRef.current = false; setPending(false); - }, [restartSettled]); + }, [restartResolved]); const handleUpdate = () => { // Synchronous re-entry guard: setPending is async, so a rapid diff --git a/apps/web/src/serverRestartStore.test.ts b/apps/web/src/serverRestartStore.test.ts index 939687ce9b1..36ab446c06c 100644 --- a/apps/web/src/serverRestartStore.test.ts +++ b/apps/web/src/serverRestartStore.test.ts @@ -5,58 +5,40 @@ import { SERVER_RESTART_EXPECTED_WINDOW_MS, clearExpectedServerRestart, expectServerRestart, - isServerRestartSettled, + isExpectedRestartResolved, isServerRestarting, - markServerRestartDisconnected, useServerRestartStore, } from "./serverRestartStore"; const ENVIRONMENT_ID = "env-test" as EnvironmentId; -function windowFor(environmentId: EnvironmentId) { - return useServerRestartStore.getState().windowByEnvironmentId[environmentId] ?? null; +function expiresAtFor(environmentId: EnvironmentId) { + return useServerRestartStore.getState().expiresAtByEnvironmentId[environmentId] ?? null; } describe("serverRestartStore", () => { beforeEach(() => { vi.useFakeTimers(); - useServerRestartStore.setState({ windowByEnvironmentId: {} }); + useServerRestartStore.setState({ expiresAtByEnvironmentId: {} }); }); afterEach(() => { vi.useRealTimers(); }); - it("arms a window that has not yet seen the restart's disconnect", () => { + it("arms a window that expires on its own", () => { expectServerRestart(ENVIRONMENT_ID); - - // The window is armed while the old server is still connected, so the - // consumer must be able to tell "waiting for the restart" apart from - // "the restart already happened". - expect(windowFor(ENVIRONMENT_ID)?.sawDisconnect).toBe(false); - expect(windowFor(ENVIRONMENT_ID)?.expiresAt).toBe( - Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, - ); + expect(expiresAtFor(ENVIRONMENT_ID)).toBe(Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS); }); - it("remembers an observed disconnect across a re-arm", () => { + it("extends the window when a slow install acknowledges late", () => { expectServerRestart(ENVIRONMENT_ID); - markServerRestartDisconnected(ENVIRONMENT_ID); - - // A slow install acknowledging late re-arms the window; forgetting the - // disconnect here would strand the restarting UI until the window lapsed. - vi.advanceTimersByTime(30_000); + vi.advanceTimersByTime(60_000); expectServerRestart(ENVIRONMENT_ID); - expect(windowFor(ENVIRONMENT_ID)?.sawDisconnect).toBe(true); - expect(windowFor(ENVIRONMENT_ID)?.expiresAt).toBe( - Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, - ); - }); - - it("ignores a disconnect for an environment with no armed window", () => { - markServerRestartDisconnected(ENVIRONMENT_ID); - expect(windowFor(ENVIRONMENT_ID)).toBeNull(); + // Re-arming from the acknowledgement is what keeps a restart that begins + // minutes after the click from being presented as an outage. + expect(expiresAtFor(ENVIRONMENT_ID)).toBe(Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS); }); it("clears only the requested environment", () => { @@ -66,58 +48,71 @@ describe("serverRestartStore", () => { clearExpectedServerRestart(ENVIRONMENT_ID); - expect(windowFor(ENVIRONMENT_ID)).toBeNull(); - expect(windowFor(other)).not.toBeNull(); + expect(expiresAtFor(ENVIRONMENT_ID)).toBeNull(); + expect(expiresAtFor(other)).not.toBeNull(); }); describe("isServerRestarting", () => { it("does not claim a still-connected environment is restarting", () => { // The window is armed before dispatch, while the old server is still - // connected. Presenting that as "restarting" would fire the moment the - // user clicks, before anything has actually gone away. - expect( - isServerRestarting({ - expectation: { expected: true, sawDisconnect: false }, - environmentUnavailable: false, - }), - ).toBe(false); + // connected. Gating on availability is what keeps that from showing the + // restarting banner the instant the user clicks — and what makes a + // window outliving its restart harmless. + expect(isServerRestarting({ restartExpected: true, environmentUnavailable: false })).toBe( + false, + ); }); it("presents an armed window with an away environment as restarting", () => { - expect( - isServerRestarting({ - expectation: { expected: true, sawDisconnect: true }, - environmentUnavailable: true, - }), - ).toBe(true); + expect(isServerRestarting({ restartExpected: true, environmentUnavailable: true })).toBe( + true, + ); }); it("falls back to the outage UI once the window lapses", () => { // The server never came back; the calm banner must give way. + expect(isServerRestarting({ restartExpected: false, environmentUnavailable: true })).toBe( + false, + ); + }); + }); + + describe("isExpectedRestartResolved", () => { + const base = { + sawRestartWindow: true, + restartExpected: true, + wentAway: false, + environmentUnavailable: false, + }; + + it("does not resolve while the environment is still away", () => { expect( - isServerRestarting({ - expectation: { expected: false, sawDisconnect: true }, - environmentUnavailable: true, - }), + isExpectedRestartResolved({ ...base, wentAway: true, environmentUnavailable: true }), ).toBe(false); }); - }); - describe("isServerRestartSettled", () => { - it("does not treat a live window as settled", () => { - expect(isServerRestartSettled({ expected: true, sawDisconnect: true })).toBe(false); + it("resolves once the environment comes back after going away", () => { + // Something restarted but the skew is still here, so whatever came back + // did not carry the new version. + expect(isExpectedRestartResolved({ ...base, wentAway: true })).toBe(true); }); - it("treats an observed restart whose window has closed as settled", () => { - // Covers a rejected systemd restart: the RPC already acknowledged, so - // nothing else can tell the update action to release itself. - expect(isServerRestartSettled({ expected: false, sawDisconnect: true })).toBe(true); + it("resolves when the window lapses without the connection ever dropping", () => { + // systemd refused the restart and rolled back: the RPC already returned + // success, so nothing else can release the action. + expect(isExpectedRestartResolved({ ...base, restartExpected: false })).toBe(true); }); - it("does not settle a window that never saw a disconnect", () => { - // The restart simply never happened; the explicit-failure path owns - // that case, and settling here would release the action early. - expect(isServerRestartSettled({ expected: false, sawDisconnect: false })).toBe(false); + it("does not resolve while the restart is still expected", () => { + expect(isExpectedRestartResolved(base)).toBe(false); + }); + + it("does not resolve before a window has been observed", () => { + // Guards the render between the click and the store update landing, + // which would otherwise look like an already-lapsed window. + expect( + isExpectedRestartResolved({ ...base, sawRestartWindow: false, restartExpected: false }), + ).toBe(false); }); }); }); diff --git a/apps/web/src/serverRestartStore.ts b/apps/web/src/serverRestartStore.ts index 0c519e71b49..2db7cf62ab2 100644 --- a/apps/web/src/serverRestartStore.ts +++ b/apps/web/src/serverRestartStore.ts @@ -8,6 +8,12 @@ * deliberately not a connection-supervisor phase — the supervisor state * machine stays transport-only, and this store is UI-side intent that * expires on its own if the server never comes back. + * + * The window is intentionally write-once-and-expire: it is armed while the + * old server is still connected (the acknowledgement races its own + * disconnect), and nothing observed on the connection can reliably end it + * early. Presentation gates on the environment actually being unavailable, + * so a lingering window is inert once the replacement is back. */ import type { EnvironmentId } from "@t3tools/contracts"; import { useEffect, useReducer } from "react"; @@ -22,60 +28,29 @@ import { create } from "zustand"; */ export const SERVER_RESTART_EXPECTED_WINDOW_MS = 90_000; -interface ServerRestartWindow { - readonly expiresAt: number; - /** - * Whether the environment has actually gone away since the window was - * armed. The window is armed while the old server is still connected — that - * is the point, since the acknowledgement races its own disconnect — so a - * connected environment must not end the window until the restart it is - * waiting for has been observed. - */ - readonly sawDisconnect: boolean; -} - interface ServerRestartState { - readonly windowByEnvironmentId: Readonly>; + readonly expiresAtByEnvironmentId: Readonly>; readonly expect: (environmentId: EnvironmentId) => void; - readonly markDisconnected: (environmentId: EnvironmentId) => void; readonly clear: (environmentId: EnvironmentId) => void; } export const useServerRestartStore = create()((set) => ({ - windowByEnvironmentId: {}, + expiresAtByEnvironmentId: {}, expect: (environmentId) => set((state) => ({ - windowByEnvironmentId: { - ...state.windowByEnvironmentId, - [environmentId]: { - expiresAt: Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, - // Re-arming (a slow install acknowledging late) must not forget a - // disconnect that was already observed. - sawDisconnect: state.windowByEnvironmentId[environmentId]?.sawDisconnect ?? false, - }, + expiresAtByEnvironmentId: { + ...state.expiresAtByEnvironmentId, + [environmentId]: Date.now() + SERVER_RESTART_EXPECTED_WINDOW_MS, }, })), - markDisconnected: (environmentId) => - set((state) => { - const current = state.windowByEnvironmentId[environmentId]; - if (current === undefined || current.sawDisconnect) { - return state; - } - return { - windowByEnvironmentId: { - ...state.windowByEnvironmentId, - [environmentId]: { ...current, sawDisconnect: true }, - }, - }; - }), clear: (environmentId) => set((state) => { - if (!(environmentId in state.windowByEnvironmentId)) { + if (!(environmentId in state.expiresAtByEnvironmentId)) { return state; } - const next = { ...state.windowByEnvironmentId }; + const next = { ...state.expiresAtByEnvironmentId }; delete next[environmentId]; - return { windowByEnvironmentId: next }; + return { expiresAtByEnvironmentId: next }; }), })); @@ -83,58 +58,18 @@ export function expectServerRestart(environmentId: EnvironmentId): void { useServerRestartStore.getState().expect(environmentId); } -/** Records that the expected restart's disconnect has now been observed. */ -export function markServerRestartDisconnected(environmentId: EnvironmentId): void { - useServerRestartStore.getState().markDisconnected(environmentId); -} - export function clearExpectedServerRestart(environmentId: EnvironmentId): void { useServerRestartStore.getState().clear(environmentId); } -export interface ServerRestartExpectation { - /** An unexpired restart window is armed for this environment. */ - readonly expected: boolean; - /** The restart's disconnect has been observed, so a reconnect ends it. */ - readonly sawDisconnect: boolean; -} - /** - * Whether an armed restart window has run its course: the disconnect was - * observed and the window has since closed. Callers use this to recover UI - * that was held for the restart — the update RPC is acknowledged before the - * restart runs, so a rejected restart cannot report itself through the call. - * - * Note that a window which never saw a disconnect is not "settled": the - * restart simply never happened, and other paths (an explicit RPC failure) - * own that case. + * Whether an unexpired restart window is armed for the environment. + * Re-renders when the window is armed, cleared, or lapses. */ -export function isServerRestartSettled(expectation: ServerRestartExpectation): boolean { - return expectation.sawDisconnect && !expectation.expected; -} - -/** - * Whether the environment should currently be presented as restarting rather - * than as an outage: a live window plus an environment that is actually away. - */ -export function isServerRestarting(input: { - readonly expectation: ServerRestartExpectation; - readonly environmentUnavailable: boolean; -}): boolean { - return input.expectation.expected && input.environmentUnavailable; -} - -/** - * The restart window state for an environment. Re-renders when the window is - * armed, observes its disconnect, is cleared, or lapses. - */ -export function useServerRestartExpectation( - environmentId: EnvironmentId | null, -): ServerRestartExpectation { - const window = useServerRestartStore((state) => - environmentId === null ? null : (state.windowByEnvironmentId[environmentId] ?? null), +export function useServerRestartExpected(environmentId: EnvironmentId | null): boolean { + const expiresAt = useServerRestartStore((state) => + environmentId === null ? null : (state.expiresAtByEnvironmentId[environmentId] ?? null), ); - const expiresAt = window?.expiresAt ?? null; const [, tick] = useReducer((count: number) => count + 1, 0); useEffect(() => { if (expiresAt === null) { @@ -147,8 +82,47 @@ export function useServerRestartExpectation( const timer = setTimeout(tick, remaining + 50); return () => clearTimeout(timer); }, [expiresAt]); - return { - expected: expiresAt !== null && expiresAt > Date.now(), - sawDisconnect: window?.sawDisconnect === true, - }; + return expiresAt !== null && expiresAt > Date.now(); +} + +/** + * Whether the environment should be presented as restarting rather than as an + * outage. Requires the environment to actually be away, which is why a window + * that outlives its restart is harmless: a reconnected environment reports + * false regardless of the window. + */ +export function isServerRestarting(input: { + readonly restartExpected: boolean; + readonly environmentUnavailable: boolean; +}): boolean { + return input.restartExpected && input.environmentUnavailable; +} + +/** + * Whether an expected restart has resolved without delivering a new version, + * so UI held for it should be released. + * + * The update RPC is acknowledged before the restart runs, so a rejected + * restart cannot report itself through the call. Two shapes resolve it, + * because a rejected restart may or may not have dropped the connection: + * the environment went away and came back (something restarted, but the skew + * is still here), or the window lapsed while the connection never dropped + * (no restart ever happened). + */ +export function isExpectedRestartResolved(input: { + /** A restart window was observed as armed at some point. */ + readonly sawRestartWindow: boolean; + /** A restart window is armed right now. */ + readonly restartExpected: boolean; + /** The environment has been unavailable at some point since arming. */ + readonly wentAway: boolean; + readonly environmentUnavailable: boolean; +}): boolean { + if (input.environmentUnavailable) { + return false; + } + if (input.wentAway) { + return true; + } + return input.sawRestartWindow && !input.restartExpected; } From 85b8a38455b99b1d814ee6414097f2418a4e9b53 Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Fri, 24 Jul 2026 17:23:35 -0700 Subject: [PATCH 5/5] fix: end the restart window when its restart has been seen through MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/web/src/components/ChatView.tsx | 35 +++++++++++++------ .../web/src/components/ServerUpdateAction.tsx | 5 +++ apps/web/src/serverRestartStore.test.ts | 16 +++++++++ apps/web/src/serverRestartStore.ts | 20 ++++++++--- 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index c1745d7d36d..c75a35dbcc1 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -25,7 +25,11 @@ import { connectionStatusTitle, type EnvironmentConnectionPresentation, } from "@t3tools/client-runtime/connection"; -import { isServerRestarting, useServerRestartExpected } from "~/serverRestartStore"; +import { + clearExpectedServerRestart, + isServerRestarting, + useServerRestartExpected, +} from "~/serverRestartStore"; import { effectiveSettled, effectiveSnoozed } from "@t3tools/client-runtime/state/thread-settled"; import { parseScopedThreadKey, @@ -1631,19 +1635,28 @@ function ChatViewContent(props: ChatViewProps) { environmentUnavailable: activeEnvironmentUnavailable, }); // While the restart is expected, poke the supervisor on a flat cadence so a - // 16-second backoff sleep cannot outlast the server's return. The window is - // never cleared here: it is armed while the old server is still connected, - // so nothing observed on the connection distinguishes "before the restart" - // from "after it". Presentation already gates on the environment being - // away, which makes a lingering window inert once the replacement is back. + // 16-second backoff sleep cannot outlast the server's return. + // + // The window also has to end at the right moment. Ending it on any healthy + // connection is wrong — it is armed while the old server is still connected, + // so that clears it before the restart it exists to cover. Never ending it + // is also wrong — an unrelated outage later in the window would be dressed + // up as an intentional restart. So it ends once this environment has gone + // away and come back: the restart it was armed for has been seen through. + const restartSawDisconnectRef = useRef(false); useEffect(() => { - if ( - activeEnvironmentId === null || - !activeEnvironmentRestartExpected || - activeEnvironmentConnectionPhase === "connected" - ) { + if (activeEnvironmentId === null || !activeEnvironmentRestartExpected) { + restartSawDisconnectRef.current = false; + return; + } + if (activeEnvironmentConnectionPhase === "connected") { + if (restartSawDisconnectRef.current) { + restartSawDisconnectRef.current = false; + clearExpectedServerRestart(activeEnvironmentId); + } return; } + restartSawDisconnectRef.current = true; // Only wake the supervisor out of a backoff sleep; while an attempt is // live, retryNow would interrupt and restart it. if (activeEnvironment?.connection.retryAt == null) { diff --git a/apps/web/src/components/ServerUpdateAction.tsx b/apps/web/src/components/ServerUpdateAction.tsx index e8a8bbfb4a7..e10bfaab648 100644 --- a/apps/web/src/components/ServerUpdateAction.tsx +++ b/apps/web/src/components/ServerUpdateAction.tsx @@ -161,6 +161,11 @@ export function ServerUpdateAction({ return; } inFlightRef.current = true; + // Only outages observed during this attempt may resolve it. An earlier + // unrelated blip would otherwise satisfy the resolve predicate on the + // very next render and re-enable the button mid-update. + wentAwayRef.current = false; + sawWindowRef.current = false; const attempt = attemptRef.current + 1; attemptRef.current = attempt; const ownsAttempt = () => attemptRef.current === attempt; diff --git a/apps/web/src/serverRestartStore.test.ts b/apps/web/src/serverRestartStore.test.ts index 36ab446c06c..53ed4b2fe06 100644 --- a/apps/web/src/serverRestartStore.test.ts +++ b/apps/web/src/serverRestartStore.test.ts @@ -75,6 +75,22 @@ describe("serverRestartStore", () => { false, ); }); + + it("does not dress up an unrelated outage as a restart once cleared", () => { + // The consumer clears the window after seeing the restart through, so a + // later ordinary outage inside the original 90s must read as an outage. + expectServerRestart(ENVIRONMENT_ID); + clearExpectedServerRestart(ENVIRONMENT_ID); + vi.advanceTimersByTime(10_000); + + expect(expiresAtFor(ENVIRONMENT_ID)).toBeNull(); + expect( + isServerRestarting({ + restartExpected: expiresAtFor(ENVIRONMENT_ID) !== null, + environmentUnavailable: true, + }), + ).toBe(false); + }); }); describe("isExpectedRestartResolved", () => { diff --git a/apps/web/src/serverRestartStore.ts b/apps/web/src/serverRestartStore.ts index 2db7cf62ab2..4c294dc6ae6 100644 --- a/apps/web/src/serverRestartStore.ts +++ b/apps/web/src/serverRestartStore.ts @@ -9,11 +9,21 @@ * machine stays transport-only, and this store is UI-side intent that * expires on its own if the server never comes back. * - * The window is intentionally write-once-and-expire: it is armed while the - * old server is still connected (the acknowledgement races its own - * disconnect), and nothing observed on the connection can reliably end it - * early. Presentation gates on the environment actually being unavailable, - * so a lingering window is inert once the replacement is back. + * Ending the window is the subtle part. It is armed while the old server is + * still connected (the acknowledgement races its own disconnect), so a + * connected environment cannot by itself mean the restart is over — that is + * how an earlier revision made the whole feature inert. But never ending it + * is also wrong: an unrelated outage later in the window would be dressed up + * as an intentional restart. So the window ends when the environment has + * gone away and come back, i.e. the restart it was armed for has been seen. + * + * A blip during a long install can consume the window early; the + * acknowledgement re-arms it, which is what covers that case. + * + * Note this observation is only used for presentation. The update action's + * own release path deliberately does not depend on it — coupling the two is + * what produced a family of bugs where each consumer invalidated the other's + * evidence. */ import type { EnvironmentId } from "@t3tools/contracts"; import { useEffect, useReducer } from "react";