Skip to content

feat(threads): graceful drain hook for in-flight work before worker shutdown#1621

Merged
kriszyp merged 4 commits into
mainfrom
kris/blob-send-drain-core
Jul 8, 2026
Merged

feat(threads): graceful drain hook for in-flight work before worker shutdown#1621
kriszyp merged 4 commits into
mainfrom
kris/blob-send-drain-core

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

Adds a generic per-worker shutdown-drain registry (components/shutdownDrain.ts, mirroring scopeShutdown.ts) that the worker shutdown path awaits before closeServers(). It's the core half of fix (2) of the deploy-reload blob-divergence root cause: harper-pro registers a drain that lets an in-flight replication blob send finish instead of being torn down mid-stream by a rolling http_workers restart (which leaves the peer's copy diverged). Paired harper-pro PR bumps core to this commit.

Core stays deliberately generic — it knows nothing about blobs. A component registers a { hasWork, drain } hook; runShutdownDrains(deadline) awaits them (failure-isolated, never rejects, hard-bounded by the deadline).

What changed

  • components/shutdownDrain.ts (new): registerShutdownDrain / shutdownDrainsHaveWork / runShutdownDrains, plus getShutdownDrainCeilingMs() (coerced config read) and the pure boundedTerminateDelay() used to clamp the terminate timer.
  • server/threads/threadServer.js: on SHUTDOWN, run registered drains before closeServers(). Extend the termination backstops only when a drain reports progressing work, then restore the normal short timeout once draining completes.
  • server/threads/manageThreads.js: extendShutdownDeadline / restoreShutdownDeadline (worker) push out / restore the self-exit backstop and post EXTEND_SHUTDOWN_DEADLINE so the main thread re-arms its worker.terminate() timer to match — clamped at the ceiling.
  • utility/hdbTerms.ts: new replication_blobSendDrainTimeout config (default 600000 = 10 min; 0 disables).

Where to look / lower-confidence areas

  • The ITC deadline flow across three handlers (manageThreads main + worker, threadServer). The self-exit extension is order-independent via selfExitDrainDeadline (the two worker SHUTDOWN listeners race). The main-thread terminate timer is armed synchronously in the same tick as the SHUTDOWN post, so the worker's async EXTEND reply can only arrive after it exists.
  • Hang-recovery is preserved: backstops extend only while a drain is progressing, and are restored to the normal timeout once the drain returns — so a worker that hangs for an unrelated reason (or after draining) is still force-killed on the normal threadTerminationTimeout window. The main thread caps any worker-supplied deadline at the ceiling.

Review notes (open items, per the cross-model review)

  • Deploy-rollout latency: restartWorkers' await Promise.race(waitingToFinish) throttle now waits on a draining worker's exit, so a large progressing send can delay the next worker's restart up to the ceiling. Bounded (force-terminate still fires at ceiling + 2×threadTerminationTimeout; capacity is preserved by the Multi-worker HTTP rolling restart produces ~0.6–1.2s whole-pool connection-refused gap #1417 pre-started replacement) and deliberate — surfaced so operators know a deploy can wait on in-flight sends. A follow-up could stop counting a draining worker against maxWorkersDown.
  • Correctness of any unfinished send is already guaranteed by fix (1) on the receiver (harper-pro#527) — this drain is a bounded optimization, not a correctness requirement.

Unit tests cover the registry, config coercion, and the boundedTerminateDelay clamp/guard arithmetic. Cross-model reviewed (Codex + Harper-domain adjudication; the Gemini/agy leg hung out and was skipped).

Generated by Claude (Opus 4.8).

@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

📖 Docs: HarperFast/documentation#569 documents the new replication.blobSendDrainTimeout option. (comment by Claude/KrAIs)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a graceful shutdown drain mechanism to allow workers to complete in-flight work before exiting. It adds a new shutdownDrain component, updates thread management to support extending shutdown deadlines, and integrates these drains into the worker shutdown sequence. The feedback focuses on robustly handling empty, null, or extremely large values for the drain timeout configuration to prevent silent disabling or setTimeout overflows, clearing lingering timers in runShutdownDrains to avoid open handles in tests, and adding corresponding unit tests.

Comment thread components/shutdownDrain.ts
Comment thread components/shutdownDrain.ts Outdated
Comment thread unitTests/components/shutdownDrain.test.js Outdated
Comment thread unitTests/components/shutdownDrain.test.js Outdated
@kriszyp kriszyp force-pushed the kris/blob-send-drain-core branch from 1b62a23 to 5b9b499 Compare July 6, 2026 20:07
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

…hutdown

Adds a per-worker shutdown-drain registry (components/shutdownDrain.ts, mirroring
scopeShutdown.ts) that the worker shutdown path awaits before closeServers. The
force-terminate backstops (worker self-exit in manageThreads + the main-thread
worker.terminate() timer) are extended only while a registered drain reports
progressing work, bounded by the new replication_blobSendDrainTimeout config
(default 10m, coerced), then restored to the normal short timeout once draining
completes — so an unrelated worker hang is still force-killed on the normal
timeout, and the main thread caps any worker-supplied deadline at the ceiling.

harper-pro registers the replication blob-send drain against this hook: fix (2)
of the deploy-reload blob-divergence root cause (don't tear down an in-flight
replication blob SEND mid-stream during a rolling http_worker restart).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp kriszyp force-pushed the kris/blob-send-drain-core branch from 5b9b499 to d8a63d6 Compare July 6, 2026 20:23
kriszyp added a commit to HarperFast/documentation that referenced this pull request Jul 6, 2026
New replication config option (HarperFast/harper#1621, HarperFast/harper-pro#529):
bounds how long a worker drains in-flight blob sends before shutting down during a
restart, so a rolling restart doesn't interrupt a transfer in progress.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp

kriszyp commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

CI note (pre-existing failures, not from this PR):

  • Unit Test (subscriptionReplay.test.js) — failing on main too (see main run 28818847554: same Subscription replay / "expected at least 1 event for id=1, got 0" at subscriptionReplay.test.js:1048). This PR's own shutdownDrain suite passes.
  • Integration Tests 3/6 — the failing cases are device-type schemas visible in describe_all / Per-device-type LMDB database sharding (schema-propagation/setup timing); main's Integration Tests are green, so this reads as a flaky shard. No causal path from a shutdown-timer change.

This PR's diff is 5 files (shutdown-drain hook + timers + config); neither failing suite touches that path.

(comment generated by Claude/KrAIs)

@kriszyp kriszyp marked this pull request as ready for review July 6, 2026 20:25
…core

# Conflicts:
#	server/threads/manageThreads.js
@kriszyp kriszyp added the patch label Jul 8, 2026
@kriszyp kriszyp requested review from Devin-Holland and removed request for kylebernhardy July 8, 2026 15:59
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Patch cherry-pick: conflict

Cherry-pick onto v5.1 produced conflicts on commit(s): 9018760e480a61e54d280eb07e93fa26c96c9a0b

The conflict markers are committed on branch cherry-pick/v5.1/pr-1621.
A pull request has been opened to land this patch: #1728

Comment thread components/shutdownDrain.ts Outdated

@Devin-Holland Devin-Holland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a per-worker graceful drain-before-shutdown hook registry so in-flight work (notably a replication blob send streaming to a peer) can reach a safe point before a worker restart, rather than being torn down mid-stream. Careful and fail-safe.

  • runShutdownDrains never rejects (a throwing or hanging drain is logged and abandoned), is bounded by an absolute deadline via an unref'd timer that is always cleared, and no-ops with zero drains — so it can't wedge the shutdown sequence that follows.
  • The backstop extension is gated on shutdownDrainsHaveWork(), so a worker hung for an unrelated reason is still force-killed on the normal short timeout. Worker-requested deadlines are clamped to the configured ceiling and to a finite value via the pure, unit-tested boundedTerminateDelay, so a rogue/buggy message can't defer the force-kill unboundedly.
  • Ordering: the SHUTDOWN handler and the drain-extension race across listeners, and armSelfExit honors selfExitDrainDeadline regardless of which lands first. Worker self-exits (1x timeout headroom) before the main thread force-terminates (2x) — correct backstop order.
  • Config coercion handles the string-from-YAML case and the blank-not-zero trap (a blank value must not read as Number('')===0 and silently disable draining), clamps oversized values below the 2^31 setTimeout-overflow cliff, and lets an explicit 0 disable draining. All covered by tests.

One thing worth putting on the radar: because the draining worker keeps its listening sockets up for the drain window (before closeServers), sustained normal traffic that keeps opening connections to it could keep it "busy" and push each restart closer to the ceiling — which would make routine maintenance / rolling restarts drag under load. Whether that actually happens hinges on the harper-pro blob-send drain hook's semantics:

  • If the hook snapshots the set of active sends when the drain begins and only waits on those, new connections arriving mid-drain are irrelevant — it drains what was already streaming and resolves. Non-issue.
  • If it treats any active send as work, a busy peer could hold a draining worker near the cap (never beyond it — the deadline is fixed at SHUTDOWN receipt and the force-terminate backstops still fire; SO_REUSEPORT also spreads new connections across the already-accepting replacement).

Worth confirming which semantics the hook uses, so operators know whether reboot time under load is bounded by work-in-flight or by ongoing traffic. Not a blocker.

Approving.

kriszyp added 2 commits July 8, 2026 14:55
…iling

boundedTerminateDelay clamped ceilingMs to MAX_TIMER_MS, but adding baseMs
headroom on top of that clamp could still push the sum back over the max
setTimeout delay at the extreme end of a configured ceiling (~24.8 days),
which Node silently coerces to ~1ms — firing the backstop almost
immediately instead of honoring the drain. Clamp the final sum too.

Mirrors the same gap in armSelfExit (server/threads/manageThreads.js),
which does the equivalent arithmetic inline for the worker-side backstop.

Addresses cb1kenobi's review comment on #1621.
github-actions Bot pushed a commit that referenced this pull request Jul 8, 2026
…iling

boundedTerminateDelay clamped ceilingMs to MAX_TIMER_MS, but adding baseMs
headroom on top of that clamp could still push the sum back over the max
setTimeout delay at the extreme end of a configured ceiling (~24.8 days),
which Node silently coerces to ~1ms — firing the backstop almost
immediately instead of honoring the drain. Clamp the final sum too.

Mirrors the same gap in armSelfExit (server/threads/manageThreads.js),
which does the equivalent arithmetic inline for the worker-side backstop.

Addresses cb1kenobi's review comment on #1621.
@kriszyp kriszyp merged commit 9018760 into main Jul 8, 2026
49 checks passed
@kriszyp kriszyp deleted the kris/blob-send-drain-core branch July 8, 2026 22:56
github-actions Bot pushed a commit that referenced this pull request Jul 8, 2026
feat(threads): graceful drain hook for in-flight work before worker shutdown
@kriszyp kriszyp added patch and removed patch labels Jul 9, 2026
Ethan-Arrowood pushed a commit to HarperFast/documentation that referenced this pull request Jul 9, 2026
New replication config option (HarperFast/harper#1621, HarperFast/harper-pro#529):
bounds how long a worker drains in-flight blob sends before shutting down during a
restart, so a rolling restart doesn't interrupt a transfer in progress.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants