diff --git a/CHANGELOG.md b/CHANGELOG.md index 2baaf7ab8..16228054f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 The clear list is now covered by a test that derives the expected set from `docker-compose.yml` itself, so a forward added without its clear entry fails in CI rather than shipping inert. +- **Restarting from Dashboard > Infrastructure no longer reloads the page into an error.** The restart + modal polls the server and reloads once it answers, but it polled `GET /api/infra/health` — a plain + ping with no knowledge of shutdown. That endpoint keeps answering `200` for the entire drain window + and the engine teardown that follows, so the very first poll, three seconds in, was answered by the + process that had just been asked to exit. The modal declared success and reloaded two seconds later, + by which time the old process was gone and the new one had not yet bound its port — leaving the + operator on a 502/503 from their reverse proxy until they refreshed by hand several times. + + The poll now targets `GET /api/health/ready`, which reports `503` the moment draining starts and + keeps reporting it until both databases answer. The page therefore reloads only once the new process + is genuinely serving, and it waits for the databases too rather than only for the port to open. + + With the poll now waiting for real readiness, its deadline started to matter for the first time. + It was a fixed 60 attempts at one second, while `POST /api/infra/restart` estimates up to 63 seconds + for a restart that brings up PostgreSQL, Redis and MinIO together — so a full-profile restart could + have reported failure while the stack was still coming up correctly. The deadline is now derived + from the estimate the server already returns, with the old 60 as a floor. (#1019) + ## [0.12.2] - 2026-08-01 ### Changed diff --git a/dashboard/src/pages/Infrastructure.tsx b/dashboard/src/pages/Infrastructure.tsx index d59a72847..1c19af7a6 100644 --- a/dashboard/src/pages/Infrastructure.tsx +++ b/dashboard/src/pages/Infrastructure.tsx @@ -15,6 +15,7 @@ import { } from 'lucide-react'; import { infraApi, API_BASE_URL } from '../services/api'; import { copyToClipboard } from '../utils/clipboard'; +import { restartPollAttempts } from '../utils/restartPoll'; import { useDocumentTitle } from '../hooks/useDocumentTitle'; import { useInfraStatusQuery, useInfraConfigQuery, useEnginesQuery, useCurrentEngineQuery } from '../hooks/queries'; import { PageHeader } from '../components/PageHeader'; @@ -451,8 +452,12 @@ export function Infrastructure() { const profilesToRemove = previousProfiles.filter(p => !pendingProfiles.includes(p)); + // Kept outside the try: the poll deadline is derived from it, and the restart call is expected to + // fail sometimes (the server may go down before it answers). + let estimatedTime: number | undefined; try { const response = await infraApi.restart(pendingProfiles, profilesToRemove); + estimatedTime = response.estimatedTime; if (response.estimatedTime) setRestartCountdown(response.estimatedTime); } catch { // Expected — server shutting down @@ -477,12 +482,12 @@ export function Infrastructure() { }); }, 1000); - checkServerHealth(stopCountdown); + checkServerHealth(stopCountdown, estimatedTime); }; - const checkServerHealth = async (stopCountdown?: () => void) => { + const checkServerHealth = async (stopCountdown?: () => void, estimatedTime?: number) => { let attempts = 0; - const maxAttempts = 60; + const maxAttempts = restartPollAttempts(estimatedTime); const check = async () => { try { diff --git a/dashboard/src/services/api.ts b/dashboard/src/services/api.ts index eb86b3317..37ebf95a1 100644 --- a/dashboard/src/services/api.ts +++ b/dashboard/src/services/api.ts @@ -1010,7 +1010,11 @@ export const infraApi = { method: 'POST', body: JSON.stringify({ profiles: profiles || [], profilesToRemove: profilesToRemove || [] }), }), - healthCheck: () => request<{ status: string; timestamp: string }>('/infra/health'), + // Readiness, not the plain /infra/health ping. The restart poll must not be able to latch onto the + // process it just asked to shut down: /infra/health answers 200 for the whole drain and teardown, + // while /health/ready reports 503 as soon as draining starts and stays 503 until both databases + // answer. Public (no API key), like /infra/health. + healthCheck: () => request<{ status: 'ok' | 'error'; details: Record }>('/health/ready'), // Data migration: export all Data-DB tables (call while still on the OLD database, before switching), // then import after the switch + restart. Used by the DB-switch migration guard so data isn't lost. exportData: () => diff --git a/dashboard/src/utils/restartPoll.test.ts b/dashboard/src/utils/restartPoll.test.ts new file mode 100644 index 000000000..f6d45e872 --- /dev/null +++ b/dashboard/src/utils/restartPoll.test.ts @@ -0,0 +1,56 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { restartPollAttempts } from './restartPoll.ts'; + +// The poll runs once a second, so an attempt count IS a deadline in seconds. It used to be a fixed +// 60, which never mattered: the poll hit the still-draining old process and "succeeded" within three +// seconds. Once it waits for real readiness, that 60 becomes the thing that decides whether the +// operator sees the dashboard or an error — and POST /infra/restart estimates up to 63s for a restart +// that brings up postgres + redis + minio. The ceiling must not sit below the server's own estimate. + +test('no estimate: keeps the historical 60-attempt deadline', () => { + assert.equal(restartPollAttempts(undefined), 60); +}); + +test('an estimate the floor already covers does not shorten the deadline', () => { + assert.equal(restartPollAttempts(15), 60); +}); + +test('a full-profile restart gets more room than the server estimated', () => { + // 15 base + 20 postgres + 13 redis + 15 minio — the worst case infra.controller computes. + assert.equal(restartPollAttempts(63), 126); +}); + +test('a fractional estimate rounds up rather than truncating below it', () => { + assert.equal(restartPollAttempts(40.5), 81); +}); + +test('a bogus estimate cannot hang the modal or shorten the deadline', () => { + for (const bogus of [0, -1, NaN, Infinity]) { + assert.equal(restartPollAttempts(bogus), 60, `estimate ${bogus}`); + } + assert.equal(restartPollAttempts(99999), 300); +}); + +// The two assertions below guard the WIRING, which is where the bug actually lived. A correct helper +// that nothing calls, or a correct deadline pointed at an endpoint that lies during shutdown, both +// pass every assertion above while reproducing #1019 in full. The failure is invisible until an +// operator restarts a production stack, so it has to be caught here. +const read = (path: string): string => readFileSync(new URL(path, import.meta.url), 'utf8'); + +test('the restart poll targets readiness, not a ping that survives the drain', () => { + const api = read('../services/api.ts'); + const healthCheck = api.split('\n').find(line => line.trimStart().startsWith('healthCheck: () =>')); + assert.ok(healthCheck, 'infraApi.healthCheck not found — update this guard, do not delete it'); + assert.match( + healthCheck, + /'\/health\/ready'/, + 'the poll must hit /health/ready: /infra/health answers 200 throughout the drain, so the poll ' + + 'latches onto the dying process and reloads the page into a gap (#1019)', + ); +}); + +test('the poll deadline comes from the helper rather than a bare constant', () => { + assert.match(read('../pages/Infrastructure.tsx'), /const maxAttempts = restartPollAttempts\(/); +}); diff --git a/dashboard/src/utils/restartPoll.ts b/dashboard/src/utils/restartPoll.ts new file mode 100644 index 000000000..b99d6e455 --- /dev/null +++ b/dashboard/src/utils/restartPoll.ts @@ -0,0 +1,25 @@ +/** + * How many one-second readiness polls to allow before a restart is declared failed. + * + * The poll used to latch onto the OLD process — `/infra/health` answers 200 throughout the drain and + * the engine teardown that follows — so it "succeeded" in about three seconds and the fixed 60-attempt + * deadline was never approached. Polling readiness instead means the poll genuinely waits for the new + * process, and the deadline becomes the thing that decides whether the operator sees the dashboard or + * an error screen. + * + * `POST /infra/restart` returns its own `estimatedTime`, which reaches 63s when a restart brings up + * postgres, redis and minio together (and more with services to remove). A deadline below the server's + * own estimate would report failure while the stack is still coming up correctly, so the estimate sets + * the floor and is doubled for margin: the estimate is optimistic, and a container recreate plus + * migrations can overrun it. + * + * The value comes from a server response, so it is clamped at both ends — a missing or absurd estimate + * must not shorten the deadline below the historical 60, nor leave the modal waiting indefinitely. + */ +const MIN_ATTEMPTS = 60; +const MAX_ATTEMPTS = 300; + +export function restartPollAttempts(estimatedSeconds?: number): number { + if (!Number.isFinite(estimatedSeconds) || (estimatedSeconds as number) <= 0) return MIN_ATTEMPTS; + return Math.min(MAX_ATTEMPTS, Math.max(MIN_ATTEMPTS, Math.ceil((estimatedSeconds as number) * 2))); +}