Repository navigation
fix(claude): close refused picker uploads and keep early replies intact - #6725
Conversation
Carry #6659 (beea837) without its new unfinished-upload cap or idle/absolute deadlines, which can reject legitimate slow uploads. Preserve the existing 256-request aggregate ceiling and its activeUpstreams release paths. Close refused input after the empty downstream reply finishes. Preserve early reply bytes under backpressure by cancelling unfinished input only after the downstream writable finishes or closes, or upstream closes without responding. Detach cleanup listeners when the body completes or the request closes. Retain protocol, failure, cancellation, SSE and shutdown regressions, with keep-alive HTTP/1.1 clients observing socket closure. Register the focused suite in both layout inventories and describe only the carried behavior. Co-authored-by: Epinephrine <luvs01@hanmail.net>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe Claude picker relay identifies unfinished request bodies and manages their cleanup during refusals, upstream failures, and early responses over HTTP/1.1 and HTTP/2. Integration tests and documentation cover upload handling and response delivery. ChangesClaude picker upload lifecycle
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The reviewed changes preserve early replies while cleaning up unfinished picker uploads, and the new test is mapped to its existing integration directory. No material merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28aa101f49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| req.once("end", finishUpload); | ||
| req.once("close", finishUpload); | ||
| upReq.once("close", onUpstreamClose); | ||
| responseWritable.once("finish", stopUnfinishedUpload); |
There was a problem hiding this comment.
When an HTTP/1.1 upstream returns before the client finishes its framed upload, this finish handler now destroys the entire client socket, but the relayed response—unlike refuse()—does not include Connection: close. A keep-alive client can therefore treat a fully delimited early response (including the generated 502 path) as reusable and dispatch queued work onto a socket the proxy immediately destroys. Add the close header before sending any response that will trigger this HTTP/1.1 cleanup, or drain the request instead of destroying the connection.
Useful? React with 👍 / 👎.
Unpipe unfinished uploads after the downstream writable finishes, discard remaining HTTP/1.1 input and end the socket gracefully. Immediate request destruction can truncate the last TLS response records despite finish. Advertise Connection: close for refusals, early replies and generated 502s. Use HTTP/2 NO_ERROR after complete replies and retain CANCEL for incomplete responses. Keep upload cleanup free of new deadlines and byte caps. Synchronize the large-reply regression on observed backpressure and readable buffer state. Consume the fixture's known upload prefix before responding to avoid an independent upstream reset, while leaving client input unfinished. Cover buffered refusals and byte-exact replies; update the owning contract. Verification: upload suite 40/40 repetitions (760 passing cases); nearby picker 194/194; layout and ratchet 27/27; test:changed 7310 pass, 31 skip, 0 fail; typecheck, structure and privacy pass. Destroy-only negative control fails 5/50; origin/dev negative control fails 12/19. Co-authored-by: Epinephrine <luvs01@hanmail.net>
Co-authored-by: Epinephrine <luvs01@hanmail.net>
…tchet Co-authored-by: Epinephrine <luvs01@hanmail.net>
1 participant
Summary
Carries #6659 by @luvs01 without its new upload limits. The maintainer review on #6659 held the 128-upload cap, the 30-second idle deadline and the five-minute absolute deadline because they can cut legitimate slow uploads. For example, a 30 MB attachment on a link below about 0.8 Mbps runs past five minutes, and upstream backpressure can pause input
dataevents long enough to trip the idle timer. The cleanup from that PR lands here:Connection: closeand ends the socket with a FIN. An immediatedestroy()truncated the last ~16 KiB of a large early reply in 3 of 50 runs. HTTP/2 sendsRST_STREAM NO_ERRORafter a complete response (RFC 9113 §8.1) and keepsCANCELwhen no complete response was sent.No timers or caps are added. The 256-request aggregate ceiling and its release paths are unchanged.
structure/clients/claude-desktop.mddocuments the carried behavior; there is no user-visible limit to document indocs-site.Supersedes #6659.
Co-authored-by: Epinephrine luvs01@hanmail.net
Verification
tests/claude-integration/claude-picker-upload.test.tsrun 40 times: 40/40 files, 760 tests passed. Covers headers-only 400/503 refusals on both protocols, byte-exact 8 MiB early replies under H1/H2 backpressure (synchronized on observed buffering, no fixed sleeps), completed uploads with SSE, cancellation, upstream failure, shutdown and healthy sibling streams.destroy()on finish the early-reply case passed 47/50 and 45/50; with the graceful end 50/50. HTTP/2 withNO_ERROR: 50/50.bun run test:changed: 7,310 pass, 31 skip, 0 fail. Test-layout and file-size ratchet: 27 pass.bun run typecheck,bun run structure:check,bun run privacy:scanexit 0.picker-listener.tsreverted todev, 12 of the upload regressions fail.Checklist
Summary by CodeRabbit