Skip to content

Hub shutdown: drain in-flight requests then force-close lingering streams - #479

Merged
TheGreatAxios merged 3 commits into
mainfrom
cl-7210-hub-shutdown-drain
Aug 31, 2026
Merged

Hub shutdown: drain in-flight requests then force-close lingering streams#479
TheGreatAxios merged 3 commits into
mainfrom
cl-7210-hub-shutdown-drain

Conversation

@TheGreatAxios

@TheGreatAxios TheGreatAxios commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

apps/hub called server.stop() with no argument on shutdown, which waits for every open connection to close on its own — including a workbench SSE bridge and an idle sidecar websocket. Those never close by themselves, so the drain always timed out and the hub exited non-zero on any deploy with a live tab open.

This replaces the previous approach (a vendored hasPendingRepoWrites export from @intx/hub-sessions, which only covered git-on-disk writes). Shutdown now:

  • Counts in-flight Hono handlers in hub middleware (a request that has not yet returned a Response — including one mid-Postgres-transaction).
  • Waits for that count to hit zero inside the existing drain bound.
  • Then server.stop(true) so lingering SSE/websocket connections cannot hang the drain or fail it.

A live stream at shutdown is not a drain fault.

Test plan

  • bun run typecheck in apps/hub
  • bun test in apps/hub (in-flight tracker, drain helper, real Bun.serve SSE/websocket)
  • bunx prettier --check on touched hub files
  • CI (gh pr checks)

@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Code-review pass on the two-phase drain. Linear checkboxes (CL-7210) verified against the code, not ticked on trust — all three genuinely met: server.stop() completes within the drain window with a live SSE/websocket connection (grace + force-close), that no longer reports a fault and exits non-zero on an ordinary deploy, and there's real test coverage (a real Bun.serve() with an open websocket, both phases).

Comment-accuracy verdict on the disk-write claim: the mechanical claim is true, checked against Bun's own docs, not assumed. Fetched Bun's server.stop() reference directly: server.stop() (no argument) waits for in-flight requests to complete — "connections with a request in flight close once the server has sent their response" — while server.stop(true) "immediately terminate[s] all connections," aborting a handler mid-async-operation. So the new comment's claim that a bounded request "return[s] as soon as that write finishes" before anything is force-closed is accurate, unlike the prior version's flatly false "no handler writes to hubDataDir." I also traced the actual write surface past what the comment named: asset and agent-definition writes go through assetService, but AgentRepoStore's agent-state and workflow-run pack receives write too, and all of it funnels through the same RepoStore per-repo lock (withRepoLock in vendor/intx/hub-sessions/src/repo-store/store.ts) — confirmed by reading agent-repo.ts's use of the same createRepoStore module, which shares its lock map at module scope.

Timing coherence: was fine (grace 6s < overall bound 10s, force-close phase gets ~4s), but coherent timing wasn't sufficient — reachability is a genuine bug, now fixed. A write slow enough to still be running when the 6s grace window elapses (a large tarball, or a write queued behind another writer's per-repo lock) was still torn: the old code called server.stop(true) unconditionally on timeout, and stop(true) aborts every open connection regardless of in-flight state. This window was real and reachable, not theoretical — the shared withRepoLock map already tracks exactly this state, it just wasn't being read.

Fix pushed (two commits):

  1. hub-sessions: export hasPendingRepoWrites for shutdown-safe force-close — a vendored-package edit, called out explicitly since it's easy to miss: vendor/intx/hub-sessions/src/repo-store/store.ts now exports hasPendingRepoWrites(), a one-line read of the module-level withRepoLock occupancy map that already existed. Re-exported through repo-store/index.ts and the package's index.ts. Per VENDORED.md's rules for a vendored edit, this commit also updates: VENDORED.md's hub-sessions row (appends the new delta to its existing description), the package's own VENDORED-FROM local-modifications note, and scripts/checks/kill-dates.txt's recorded content hash (recomputed via the same hashDirectory() check:killdates uses) — check:structural fails without that last one, confirmed locally.
  2. hub shutdown: skip force-close while a repo write is in flightapps/hub/src/shutdown.ts gains a waitUntil(predicate, deadlineMs, pollMs) helper; the shutdown block now polls !hasPendingRepoWrites() after the grace timeout, bounded by the existing SHUTDOWN_DRAIN_MS, and only calls server.stop(true) once that's confirmed safe. If a write is still running when the overall budget runs out, the connection is left alone and shutdownHub's outer bound reports the fault and exits non-zero — an honest failure instead of a silent tear.

New regression test in apps/hub/test/shutdown-drain.test.ts: a real Bun.serve() with a 300ms write, grace window at 100ms — proves the write is still running when the window elapses (reproduces the race, not just the already-finished case the existing tests cover) and that force-close waits for it. apps/hub/src/shutdown.test.ts gets direct unit coverage for waitUntil.

Verified locally, all foreground:

  • WORKBENCH_CHECK_SINCE=origin/main bun run typecheck — clean (includes @intx/hub-sessions)
  • bun run lint — 0 errors (8 pre-existing unrelated warnings)
  • bun test apps/hub — 187 pass, 0 fail
  • bun run check:structural — clean, including check:killdates against the updated hash and check:licenses
  • bunx prettier --check on every touched file — clean (VENDORED.md's diff looks large only because prettier reflows the whole markdown table's column widths once one cell changes length — cosmetic, not a rewrite)

Not merging — leaving for CI and final review.

@TheGreatAxios
TheGreatAxios force-pushed the cl-7210-hub-shutdown-drain branch 3 times, most recently from da13203 to 60ce792 Compare August 31, 2026 02:13
@TheGreatAxios

Copy link
Copy Markdown
Contributor Author

Holding this rather than fixing the check:killdates hash — the approach needs rework first.

hasPendingRepoWrites() reads the module-private locks map inside vendor/intx/hub-sessions/src/repo-store/store.ts. Two problems:

It edits vendored code. Every line of that delta is re-pin tax: it has to be hand-reapplied at each future pin (CL-7107), and it grows the vendored surface rather than shrinking what we re-create. VENDORED-FROM records it correctly, but the right question is whether the delta needs to exist at all.

It is also less correct than tracking requests directly. That locks map only covers git-on-disk writes to hubDataDir. A request mid-Postgres-transaction, or waiting on inference, holds no repo lock — so the drain force-closes exactly the requests it should wait for, while reaching into storage internals to do it.

shutdown.ts already takes drain: () => Promise<void> as a callback. Counting in-flight requests in Hono middleware — increment on entry, decrement on response — answers the real question ("is anything still running?") without caring what the request is doing. Fewer lines, no vendored delta, no kill-dates hash churn, and it covers every backend rather than one.

Suggested: drop the vendor/intx/hub-sessions changes and the VENDORED-FROM line, and drain on a hub-owned in-flight counter.

The rest of the PR (the bounded drain in shutdown.ts, its tests) looks right and is worth keeping. Rebased onto current main already; typecheck/e2e/lint/build-test all pass — structural is red only on the unrecorded tree hash, which this rework removes the need for.

@TheGreatAxios
TheGreatAxios force-pushed the cl-7210-hub-shutdown-drain branch from 60ce792 to a132990 Compare August 31, 2026 06:44
@TheGreatAxios TheGreatAxios changed the title Hub shutdown: drain in-flight requests before force-closing lingering connections Hub shutdown: drain in-flight requests then force-close lingering streams Aug 31, 2026
@TheGreatAxios
TheGreatAxios merged commit 4bbc8a9 into main Aug 31, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant