[Bugfix #1261] Hold Tower API requests until boot wiring completes#1263
Merged
Conversation
Tower bound its port and started serving before initInstances() had wired up tower-instances._deps, so DELETE /api/terminals/:id returned 404 for a terminal that plainly existed while GET on the same id returned 200. The window scaled with disk state — the #1227 husk sweep and the #1238 session-log retention scan both ran between the bind and the wiring — so a log-heavy machine failed deterministically while CI stayed green. - Hold each request in http.createServer until bootSequence() opens the readiness gate; a boot that exceeds 20s answers 503 + Retry-After rather than hanging clients. The port still binds first: it is the single-Tower mutex and what every readiness probe connects to. - Move initInstances()/initCron() ahead of the two maintenance sweeps and initTunnel(), which now run after readiness. Gating local readiness on a remote tunnel connect would make an unreachable cloud endpoint look like a broken Tower. - Add instancesReady() so the terminal-delete paths can answer 503 instead of 404 (or a lying 204) if the gate is ever bypassed. Also makes 'afx tower start' honest: it polls /api/status for a 200, which the pre-fix Tower returned during the window because getInstances() returns [] when _deps is null.
…ndow
The race was invisible to the suite for two reasons, both addressed here:
the e2e helper polls the port every 200ms, which usually lands after boot
finishes, and the window's width depends on the machine's process table and
session-log volume.
- waitForPortImmediate() / startTower({returnAtBind: true}) return at the
instant of the bind instead of on the next poll tick.
- AF_TEST_BOOT_DELAY_MS widens the pre-wiring window to a known duration so
the race is deterministic rather than machine-dependent.
- tower-startup-readiness.e2e.test.ts asserts DELETE returns 204 (not a
spurious 404) for a request issued at bind time, and that /api/status
does not answer 200 until wiring is complete.
- A tower-routes unit test covers the instancesReady() 503 guard.
Verified both e2e tests fail with the gate disabled.
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.
Summary
Tower bound its HTTP port and started answering requests before its internal dependency wiring was complete.
DELETE /api/terminals/:idreturned 404 for a terminal that existed — whileGETon the same id returned 200, because the GET path never touches_deps.This makes the port-bind and "ready to serve" two different things: the port still binds first, but requests are held until the boot sequence has wired up everything a request handler depends on.
Fixes #1261
Root Cause
The entire boot sequence lived inside the
server.listen()callback, andinitInstances()— which setstower-instances.ts's_deps— was its last step. Anything arriving before it got whatever a half-wired Tower could produce:killTerminalWithShellper()(tower-instances.ts) opens withif (!_deps) return false.falseas "no such terminal" and answers 404.Two aggravating factors, both confirmed while investigating:
initInstances(). On a machine with ~1,400 session logs the window exceeds 100ms; on a fresh CI runner it closes before any test's first request lands. That asymmetry is whytower-terminals.e2e.test.ts > DELETEfailed deterministically on a log-heavy dev machine while CI stayed green.afx tower start's readiness signal was dishonest. It polls/api/statusand treats a 200 as "Tower is up" (commands/tower.ts), but/api/statusanswered 200 during the window —getInstances()returns[]when_depsis null. So the CLI reported success mid-window, which is what makesafx tower start && <immediate afx command>racy.Reproduced deterministically before touching anything, with an empty log dir, by connecting in a tight loop and firing the first request at the instant of the bind:
The window is not exotic. It just needs a client that doesn't sleep before its first request.
Fix
1. Readiness gate (
tower-server.ts). The listen callback now only logs and kicks off a namedbootSequence();http.createServerholds each request until that sequence callsmarkBootComplete(). The port still binds first on purpose — it is the single-Tower mutex (a secondafx tower startneedsEADDRINUSE) and what every readiness probe connects to. A boot that exceedsBOOT_READY_TIMEOUT_MS(20s) answers 503 +Retry-Afterrather than hanging clients forever. A throw during boot still exits the process, exactly as before (it was an unhandled rejection when this code was the listen callback).2. Boot reorder.
initInstances()+initCron()move up to just afterkillOrphanedShellpers(); the two maintenance sweeps andinitTunnel()now run after the gate opens. Readiness should depend on wiring, not on housekeeping — and gating it oninitTunnel()in particular would make an unreachable cloud endpoint look like a broken local Tower. Both sweeps keep their real ordering constraint (afterreconcileTerminalSessions), and neither can touch a session created by an incoming request: the husk predicate requires a shellper aged past a 1h grace, and the log sweep only unlinks logs older than the retention window. Measured effect: ready at ~90ms instead of after the disk scans.3. Second line of defence. New
instancesReady()lets routes tell "not wired yet" apart from "no such thing". The DELETE route answers 503 instead of 404, and the workspace tab-delete path — which discards the kill result and answers 204 either way — refuses instead of reporting a close it never performed. Unreachable while the gate holds, and deliberately so: the failure mode if the gate is ever bypassed should be honest rather than misleading.Test Plan
New
tower-startup-readiness.e2e.test.ts. The race was invisible to the suite for two separate reasons, both addressed:waitForPortImmediate()/startTower({returnAtBind: true})return at the instant of the bind.AF_TEST_BOOT_DELAY_MSwidens it to a known duration, so the test asserts an invariant instead of a timing accident.Two e2e tests: a request issued at bind time gets
204(not a spurious 404) and the terminal really is gone afterwards; and/api/statusdoes not answer 200 until wiring completes, which is the signalafx tower startdepends on. Plus atower-routesunit test for theinstancesReady()503 guard.Verified the tests catch the regression: with the gate disabled in
dist, both fail — DELETE503(the second-line guard firing) and the status-hold assertion at 2ms against a required ≥1200ms.vitest run)vitest.e2e.config.ts)tsc --noEmitNotes for review
setupUpgradeHandlerattaches its own'upgrade'listener, so it bypasses the HTTP gate. Not reachable in practice — every client makes an HTTP call before opening a socket, so by then the gate is open — and the tower: reconcile terminal sessions before serving requests (kill the restart successor-lookup race) #997whenStartupReconcileSettled()barrier already covers the WS terminal-upgrade routes for reconcile's output specifically. Left alone rather than silently widening scope; happy to file a follow-up if you'd rather close it explicitly.whenStartupReconcileSettled()is now always settled for HTTP requests — but it still does real work for WS upgrades, so nothing was removed.arch.md's canonical boot-order table was stale as of this change; it's updated, along with a new "Boot Readiness Gate (Tower serves API requests before internal wiring completes — terminal DELETE returns spurious 404 during startup window #1261)" section stating the invariant for future startup work: anything a request handler depends on must be wired beforemarkBootComplete(), anything else goes after it.