From 48610bcadb21e8182316df18da546bcb7121fce1 Mon Sep 17 00:00:00 2001 From: Yudhi Armyndharis Date: Sat, 1 Aug 2026 19:26:54 +0700 Subject: [PATCH] fix(dashboard): poll readiness after a restart instead of a ping that outlives it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The restart modal reloads the page once the server answers again, but it polled GET /api/infra/health — a plain ping with no knowledge of shutdown. That endpoint answers 200 for the whole drain window and the engine teardown after it, so the first poll at t+3s was answered by the very process that had just been asked to exit. The modal declared success and reloaded two seconds later, when 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. Poll GET /api/health/ready instead. It reports 503 as soon as draining starts — markShuttingDown() runs synchronously before the restart response is written, so there is no window in which the dying process can answer 200 — and stays 503 until both databases respond. The reload therefore happens only once the new process is genuinely serving. Confirmed against the shipped image: through a 10s drain, /api/infra/health answered 200 every second while /api/health/ready answered 503. /health/ready is already @Public() and already covered by a drain test, so nothing on the gateway changes. With the poll finally waiting, its deadline started to matter. It was a fixed 60 attempts at 1s, while POST /api/infra/restart estimates up to 63s for a restart bringing up postgres + redis + minio — so a full-profile restart could report failure while the stack was still coming up. restartPollAttempts derives the deadline from the estimate the server already returns, keeping 60 as the floor and clamping a missing or absurd value at both ends. Two of the tests assert the wiring rather than the helper: a correct helper nothing calls, or a correct deadline pointed at an endpoint that lies during shutdown, both reproduce the bug while passing every behavioural assertion. Closes #1019 --- CHANGELOG.md | 18 ++++++++ dashboard/src/pages/Infrastructure.tsx | 11 +++-- dashboard/src/services/api.ts | 6 ++- dashboard/src/utils/restartPoll.test.ts | 56 +++++++++++++++++++++++++ dashboard/src/utils/restartPoll.ts | 25 +++++++++++ 5 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 dashboard/src/utils/restartPoll.test.ts create mode 100644 dashboard/src/utils/restartPoll.ts 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))); +}