HarperFast / HarperFast/harper
Worker-restart replacement can race a drain-delayed port release on Bun/Windows/macOS (EADDRINUSE for worker-owned listeners)
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 205
Description
## Summary
`restartWorkers()` starts the replacement HTTP worker immediately after posting `SHUTDOWN` to the
old one whenever the platform can't pre-start a SO_REUSEPORT-sharing replacement
(`canPreStartReplacement === false`: Windows, macOS, and Bun — see
`server/threads/manageThreads.js:453` and the `worker.startCopy()` call at
`server/threads/manageThreads.js:548`). That ordering assumes the old worker releases its
worker-owned listeners (e.g. MQTT, which binds exclusively rather than via SO_REUSEPORT) "well
before the replacement finishes booting and binds" (see the comment above `canPreStartReplacement`).
The shutdown-drain feature added in #1621 (`components/shutdownDrain.ts`,
`server/threads/threadServer.js:194-209`) breaks that assumption: a worker with in-flight drain
work now delays `closeServers()` (which releases its ports) until the drain settles or the
configured ceiling is hit — `replication.blobSendDrainTimeout`, which defaults to **10 minutes**
(`DEFAULT_DRAIN_CEILING_MS` in `components/shutdownDrain.ts`). On the affected platforms, the
replacement worker can now start and attempt to bind its own copy of those exclusive listeners
long before the old worker actually frees them.
## Evidence
CI run for HarperFast/harper#1791 (Bun job, "Integration Tests 5/6 (Bun)"), instance log for
`integrationTests/components/shutdown-drain-e2e.test.ts`:
```
19:05:38.092Z [main/0] restart_service http_workers requested (2nd restart, old worker tid=3)
19:05:39.079Z [job/4] Starting job (drives the restart)
19:05:39.559Z [http/5] new replacement worker boots (tid=5) — started immediately, well before
tid=3's drain (configured 5s ceiling in the test) has run
19:05:40.672Z [http/5] [error]: Failed to bind TLS listener for component 'mqtt' to port
127.0.0.2:8883: address already in use by another process
19:05:40.672Z [http/5] [error]: Failed to bind TCP listener for component 'mqtt' to port
127.0.0.2:1883: address already in use by another process
19:05:49.243Z [job/4] restart_service job completes (tid=3 finally exits ~11s after shutdown)
```
`tid=3` (the old worker) is still bound to the mqtt ports when `tid=5` (its replacement) tries to
claim them, because `tid=3`'s shutdown is mid-drain and hasn't reached `closeServers()` yet. The
bind failure itself is caught and logged as an "external conflict" (`listenOnPortsBun()`'s
EADDRINUSE handler in `server/threads/threadServer.js` around line 545) rather than crashing the
worker, but the net effect is a worker-owned listener (mqtt, in this case) going unbound on the
new worker — the same failure mode the `canPreStartReplacement` comment set out to avoid, just
triggered by drain-added latency rather than the original "brief gap" it was written for. In the
observed run, the subsequent restart-completion polling in the test also failed to observe the new
worker within a generous 35s window, suggesting downstream effects beyond just the logged bind
conflict (not fully root-caused here).
## Suggested directions (not investigated in depth)
- Don't start the replacement worker until the old worker's `closeServers()` has actually
completed (i.e. gate `worker.startCopy()` on an ITC signal from the old worker, not just on
having posted `SHUTDOWN`) when `!canPreStartReplacement`.
- Or: have the drain path close the worker's *exclusive* listeners immediately (before the drain
runs), independent of the rest of the shutdown sequence, since those don't need to stay open
during a blob-send drain.
## Repro
`integrationTests/components/shutdown-drain-e2e.test.ts` (added in #1791) reproduces this
reliably on Bun; it currently skips on Bun (see that PR) pending this fix, to avoid blocking CI on
a known, understood gap rather than a flake.
Contributor guide
Research direction
Read server/threads/manageThreads.js around lines 453 and 548, then follow shutdown handling in components/shutdownDrain.ts and server/threads/threadServer.js:194-209 and around line 545. Run integrationTests/components/shutdown-drain-e2e.test.ts on Bun to reproduce the EADDRINUSE failure. Done means replacement workers no longer lose worker-owned listeners while the old worker is still draining.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- backend, networking, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100