node:http: defer server 'close' until every tracked connection has ended - #35837
Open
robobun wants to merge 7 commits into
Open
node:http: defer server 'close' until every tracked connection has ended#35837robobun wants to merge 7 commits into
robobun wants to merge 7 commits into
Claude / Claude Code Review
completed
Jul 26, 2026 in 35m 1s
Code review found 2 important issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 0 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/js/node/_http_server.ts:128-132 |
close(cb) can now stall indefinitely because closeAllConnections()/closeIdleConnections() are no-ops after close() |
| 🔴 Important | src/js/node/_http_server.ts:128-133 |
kPendingDrainClose is not reset on re-listen; stale flag can fire 'close' on a listening server |
Annotations
Check failure on line 132 in src/js/node/_http_server.ts
claude / Claude Code Review
close(cb) can now stall indefinitely because closeAllConnections()/closeIdleConnections() are no-ops after close()
Deferring `'close'` until `kTrackedConnections` drains is correct, but `close()` sets `this[serverSymbol] = undefined` (line 523), which makes `closeAllConnections()` (line 488) and `closeIdleConnections()` (line 510) no-ops. So the documented Node deadline pattern — `server.close(cb); setTimeout(() => server.closeAllConnections(), N)` — now hangs where pre-PR it (incorrectly, but terminatingly) fired `cb` early. When `serverSymbol` is already cleared, both methods should fall back to iterating
Check failure on line 133 in src/js/node/_http_server.ts
claude / Claude Code Review
kPendingDrainClose is not reset on re-listen; stale flag can fire 'close' on a listening server
`kPendingDrainClose` is set to `true` here but never reset in `listen()`/`kRealListen`, and `emitCloseServer` doesn't check `self[serverSymbol]`. If the user re-listens while the flag is armed, the old keep-alive connection's `#onClose` will schedule `emitCloseServer` and fire `'close'` (plus the stored close callback) on an actively-listening server. Node's `_emitCloseIfDrained` — which the comment says this mirrors — guards this with `if (this._handle || this._connections) return`; add the equ
Loading