fix(dashboard): poll readiness after a restart instead of a ping that outlives it - #1041
Merged
Merged
Conversation
… outlives it 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
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What happens today
Dashboard > Infrastructure > Restart reloads the page once the server answers again. It polls
GET /api/infra/health, which is a plain ping:It knows nothing about shutdown, and it keeps answering
200for the entire drain window and theengine teardown that follows. Meanwhile the poll starts at
t+3sandSHUTDOWN_DELAY_MSdefaults to3000, so the first poll lands on the process that has just been asked to exit — and is told everything
is fine.
The modal declares success and calls
window.location.reload()two seconds later. By then the oldprocess has gone and the new one has not yet bound its port, so the browser gets a 502/503 from the
reverse proxy. The operator refreshes by hand a few times until it comes back. That is the report in
#1019.
Measured, not inferred
A throwaway container from the shipped image,
SHUTDOWN_DELAY_MSwidened to 20s so the drain isobservable,
SIGTERMatt=0, both endpoints polled once a second:with the process logging
Entering draining state — readiness now reports 503.The endpoint the dashboard polls reports success for every second of a shutdown in progress.
The change
Poll
GET /api/health/readyinstead. It already does exactly the right thing:503as soon as draining begins —markShuttingDown()runs synchronously before therestart response is written, so there is no window in which the dying process can answer
200;503until both databases respond, so the reload waits for a server that is genuinelyusable rather than one that has merely opened a port;
@Public(), and its drain behaviour is already covered by a test(
health.controller.spec.ts, "throws 503 while draining, without even probing the DBs").Nothing on the gateway changes.
/api/infra/healthkeeps its current contract, so no documentedendpoint changes behaviour.
The deadline had to move with it
With the poll finally waiting for something real, its ceiling started to matter for the first time.
It was a fixed 60 attempts at one second.
POST /api/infra/restartcomputes its own estimate:That reaches 63s — the poller's own deadline. A restart bringing up all three could therefore report
failure while the stack was still coming up correctly. Before this change the ceiling was unreachable,
because the poll always "succeeded" within three seconds.
restartPollAttemptsderives the deadline from the estimate the response already carries, keeps theold 60 as a floor so nothing gets shorter, and clamps at both ends so a missing or absurd value can
neither shorten the deadline nor leave the modal waiting forever.
Tests
Five cover the helper. Two cover 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
reproduce #1019 in full while passing every behavioural assertion.
Mutation-tested, both arms:
'/health/ready'→'/infra/health'restartPollAttempts(estimatedTime)→60The helper tests were watched failing first (
60 !== 126,60 !== 81,60 !== 300).Verification
tsc --noEmit— all clean, nothing on the gateway touchedCloses #1019