http: publish to http.server.* diagnostics_channel channels - #29588
Closed
robobun wants to merge 11 commits into
Closed
http: publish to http.server.* diagnostics_channel channels#29588robobun wants to merge 11 commits into
robobun wants to merge 11 commits into
Claude / Claude Code Review
completed
Jun 18, 2026 in 29m 57s
Code review found 2 potential issues
Found 3 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/js/node/_http_server.ts:683-688 |
response.finish publishes after socket.end() runs (listener-ordering Node divergence) |
| 🟡 Nit | src/js/node/_http_server.ts:1518-1522 |
http2 allowHTTP1 fallback emits response.created but not request.start/response.finish |
Annotations
Check warning on line 688 in src/js/node/_http_server.ts
claude / Claude Code Review
response.finish publishes after socket.end() runs (listener-ordering Node divergence)
The `emitResponseFinishChannel` listener is registered (line 685) *after* `endSocketOnFinishIfNeeded` (line 666), so on the `kMustCloseConnection` path (HTTP/1.0, `Connection: close`, `shouldKeepAlive=false`, close-delimited body, etc.) `socket.end()` runs before the channel publishes — a subscriber sees `payload.socket.writableEnded === true` where Node shows `false` (Node's `resOnFinish` publishes as its first action, before `socket.destroySoon()`). Practical impact is near-zero, but moving th
Check warning on line 1522 in src/js/node/_http_server.ts
claude / Claude Code Review
http2 allowHTTP1 fallback emits response.created but not request.start/response.finish
Moving the `response.created` publish into the `ServerResponse` constructor is correct per Node, but it now also fires from `connectionListenerHTTP1` in `src/js/node/http2.ts:5526` (the `Http2SecureServer({allowHTTP1: true})` HTTP/1 fallback). That path doesn't go through `onNodeHTTPRequest`, so it never publishes `http.server.request.start` or attaches the `response.finish` listener — an APM subscriber pairing created↔finish will now see orphaned `response.created` events on this path (Node rou
Loading