fix: don't crash the CLI when the upstream dev server dies mid-response - #8386
fix: don't crash the CLI when the upstream dev server dies mid-response#8386NMiller3862 wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe proxy error handler now checks whether the response headers were sent or the writable stream ended. When either condition is true, it destroys the response socket and returns instead of sending a 500 response. Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/proxy.ts (2)
560-564: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the explanatory comments in this branch.
The condition,
res.destroy(), andreturnalready express the behavior. Remove the explanatory block, or retain only a short issue reference if project policy requires one.As per coding guidelines:
**/*.{ts,tsx}files must not contain comments describing what the code does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/proxy.ts` around lines 560 - 564, Remove the multi-line explanatory comment in the upstream-response failure branch of the proxy handler, leaving the existing condition, res.destroy(), and return behavior unchanged. Retain only a brief issue reference if required by project policy.Source: Coding guidelines
559-567: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the committed-response guard.
Cover an upstream error after headers are sent and after the response has ended. Assert that the handler destroys the response, returns before
res.writeHead(500, ...), and does not raiseERR_HTTP_HEADERS_SENT. Also preserve coverage for the uncommitted-response 500 fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/proxy.ts` around lines 559 - 567, Add regression tests for the proxy error handler covering upstream failures after headers are sent and after the response has ended: assert that res.destroy() is called, execution returns before res.writeHead(500, ...) and no ERR_HTTP_HEADERS_SENT is raised. Retain coverage asserting the existing 500 fallback for uncommitted responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/proxy.ts`:
- Around line 560-564: Remove the multi-line explanatory comment in the
upstream-response failure branch of the proxy handler, leaving the existing
condition, res.destroy(), and return behavior unchanged. Retain only a brief
issue reference if required by project policy.
- Around line 559-567: Add regression tests for the proxy error handler covering
upstream failures after headers are sent and after the response has ended:
assert that res.destroy() is called, execution returns before res.writeHead(500,
...) and no ERR_HTTP_HEADERS_SENT is raised. Retain coverage asserting the
existing 500 fallback for uncommitted responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 60bed017-34f0-4045-a1f8-8aa75a95cc9f
📒 Files selected for processing (1)
src/utils/proxy.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
netlify/blueprints(manual)
1 participant
Summary
Fixes the crash class tracked in #5917 (also #7060, #5676): when the framework dev server that
netlify devproxies to dies mid-response, the proxy's error handler callsres.writeHead(500, …)on a response whose headers are already on the wire.ERR_HTTP_HEADERS_SENTis thrown out of an event handler, and the whole CLI terminates:One condition fixes it: if
res.headersSent || res.writableEnded, the response is unsalvageable — destroy the socket and return, instead of throwing. The one in-flight request was doomed either way; the CLI survives, and requests after the upstream recovers succeed.Deterministic reproduction
This crash has historically been hard to pin ("sometimes when I save a file", #5917). Next.js provides a deterministic trigger: its dev server restarts itself when the V8 heap passes 80% of its limit (
next/dist/server/lib/start-server.js→process.exit(RESTART_EXIT_CODE)), expecting its supervisor to bring it back — which it does, in ~3s. Any request mid-flight through netlify-cli's proxy at that moment hits this handler with headers already sent.Repro:
netlify devwrapping a large Next 16 app, drive sustained browser traffic (we used a ~480-test Playwright suite) until Next logsServer is approaching the used memory threshold, restarting.... Unpatched: the CLI died on this line in 2 of 2 runs at that moment. Patched (same workload, same default heap): 2 restarts occurred, CLI survived both, zeroERR_CONNECTION_REFUSEDin the suite — 457/486 tests passed vs. a cascade of 289 spurious failures when the CLI dies.Notes
writableEndedis included alongsideheadersSentbecauseres.end()after end also errors; ended implies headers sent for HTTP/1, so this is belt-and-braces.writeHeadcall sites.