Skip to content

fix(claude): bound picker uploads and preserve early replies - #6659

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/picker-upload-upstream-20261006
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/picker-upload-upstream-20261006

Conversation

@luvs01

@luvs01 luvs01 commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Keep the picker's ceiling of 256 active upstream requests and add a shared ceiling of 128 unfinished uploads across HTTP/1.1, HTTP/2 and sessions. Framing selects upload accounting, including HTTP/2 requests whose headers omit END_STREAM.
  • Bound incomplete bodies with a 30-second progress deadline and a five-minute absolute deadline. Body completion frees upload admission and clears both timers while long-lived responses and SSE continue.
  • Close refused headers-only HTTP/2 uploads after the underlying response writable finishes. The compatibility response's finish event depends on stream closure and can otherwise leave cleanup waiting on itself.
  • Preserve early upstream replies through downstream backpressure. Upstream request closure alone no longer cancels unfinished client input after a response has arrived; response writable completion performs that cleanup. Deadlines remain armed while the body is incomplete, and failures/cancellation/shutdown release admission once.
  • Upload deadlines do not impose a lifetime limit on responses after body completion. The upstream counter retains its existing release points and does not cap every buffered downstream response or accepted connection.
  • Add 21 real TLS-listener regressions, register the upload test in both layout maps, and update the Desktop contract and English/Korean guides.

Verification

Local validation used Node v24.19.0 against the actual listener with local verified TLS and a small test-framework/module adapter. Bun 1.4.0 and project dependencies were unavailable, so the following are Node evidence, not Bun execution:

  • Upload suite: 21 passed, 0 failed. Covers 400/503 headers-only refusals without DATA or END_STREAM, byte-exact 8 MiB early replies with H1/H2 backpressure, upload-slot reuse while SSE remains open, failures, cancellation, shutdown, deadlines, and healthy sibling streams. Timer instrumentation observed zero active listener timers after teardown.
  • Negative control: the original implementation delivered 8,290,179 of 8,388,608 response bytes in the HTTP/2 case. The corrected listener delivered every byte. The HTTP/1.1 case passed on both implementations.
  • An explicit Node model of Bun's zero-buffer unread-stream behavior makes both refusal regressions time out on the original implementation; both pass with the correction. This model is not native Bun reproduction.
  • Nearby listener/bootstrap suites: 60 passed, 3 failed on both original and corrected implementations. Node's inbound HTTP/1 overflow reply differs from the Bun assertions, and its H2 header-overflow reset is REFUSED_STREAM rather than ENHANCE_YOUR_CALM.
  • Structure SSOT, privacy scan, file-size ratchet and both layout-map entries passed using the Node adapter. git diff --check passed.

The patch applies cleanly to dev at f92482065217bae891a88e27fd6c00012f212042, preserving its current Desktop CA/ACL documentation.

Published-head verification is now complete in Cross-platform CI for beea8373762e722b3f66193a3868352dbead5c0f. The checked-out merge has the exact proposed tree and ran Bun 1.4.0 (34cbb9a40):

  • Upload regressions: 21 passed, 0 failed, including both headers-only refusals and exact large H1/H2 replies under backpressure.
  • Existing listener tests: 48 passed, 0 failed. The three assertions that differed under Node passed on Bun. Bootstrap tests also passed.
  • All four Linux Bun test shards, root TypeScript and strict contract checks, structure, privacy, documentation build, storage/API-usage, Docker and packaging/keyring smoke checks passed. React Doctor passed separately.

The negative-control reproduction above remains local Node evidence; no baseline Bun reproduction is claimed. Native Windows/macOS full suites were skipped by scope selection. Native Desktop/macOS, live-provider acceptance and production-limit stress were not run.

Review handoff — 2026-10-07

This published head is ready for independent review with the validation scope above. CodeRabbit's completed source review covers beea8373762e722b3f66193a3868352dbead5c0f and found no actionable comments; there are no unresolved inline threads. The earlier author hold was for that review, which has completed. Current-head aggregate CI, TypeScript, hygiene and target-branch checks passed.

The full local Bun suite was unavailable because the validation environment lacked Bun 1.4.0 and project dependencies. The actual-listener Node regressions and controls are retained as such; the completed hosted Bun run supplies the pinned-runtime coverage. The three Node-specific nearby-suite failures are not relabeled as local passes.

Independent maintainer/security review is requested for admission accounting across sessions/protocols, refusal cleanup, early-response backpressure and timer/cancellation ownership. The security checklist remains open until that review is supplied. Native Desktop/platform acceptance and production-limit stress remain unperformed. At this checkpoint the branch is 19 commits behind dev 9099e4bf99166677db0f5fa6df81fe30208dd795; GitHub reports no merge conflict. Existing CI validates the recorded published head and checkout, not a future dev integration. Ready for review does not authorize merge, release or activation.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • New Features
    • The Claude picker relay now limits unfinished uploads to 128 at a time and rejects excess uploads with an empty 503 response.
    • Uploads close after 30 seconds without body progress or five minutes total. These limits stop when the request body finishes, so long-lived responses and SSE subscriptions can continue.
    • The relay cancels unfinished uploads when requests are refused or upstream handling ends.

Bound unfinished upload admission and body deadlines across both relay
protocols. Cancel unread input after downstream writable completion while
keeping completed uploads and long-lived responses independent.

Preserve early replies under backpressure and add focused lifecycle tests.
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 64bb2de0-bd79-4407-8bef-fb65842afdcd
📥 Commits

Reviewing files that changed from the base of the PR and between f924820 and beea837.

📒 Files selected for processing (7)
  • docs-site/src/content/docs/guides/claude-code.md
  • docs-site/src/content/docs/ko/guides/claude-code.md
  • scripts/test-layout/layout.json
  • src/claude/intercept/picker-listener.ts
  • structure/clients/claude-desktop.md
  • tests/claude-integration/claude-picker-upload.test.ts
  • tests/fixtures/test-layout-expected.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The picker relay now tracks unfinished uploads separately from upstream requests, limits upload concurrency, and cancels uploads after idle or absolute deadlines. It releases upload capacity on completion and cleanup. Integration tests cover HTTP/1 and HTTP/2 behavior, and English and Korean guides document the limits.

Changes

Picker upload limits and lifecycle

Layer / File(s) Summary
Upload admission and refusal
src/claude/intercept/picker-listener.ts, structure/clients/claude-desktop.md, tests/claude-integration/claude-picker-upload.test.ts
The listener adds configurable upload limits, with defaults of 128 active uploads, 30 seconds idle, and 300 seconds total. It returns an empty 503 when request or upload capacity is reached. Tests cover refused uploads and capacity across protocols. The client guide describes the separate concurrency bounds and identifies the User-Agent as a routing hint, not authentication.
Upload deadlines and cleanup
src/claude/intercept/picker-listener.ts, tests/claude-integration/claude-picker-upload.test.ts, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json, docs-site/src/content/docs/guides/claude-code.md, docs-site/src/content/docs/ko/guides/claude-code.md, structure/clients/claude-desktop.md
The listener refreshes the idle deadline when it receives nonempty body data; the absolute deadline does not refresh. It stops unfinished uploads and releases their capacity on timeout, completion, or cleanup. Tests cover cancellation, upstream failures, early and large responses, SSE responses, and shutdown. The guides describe the upload limits, and the test-layout mappings include the new integration test.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PickerListener
  participant Upstream
  Client->>PickerListener: Send request with unfinished body
  PickerListener->>Upstream: Forward request
  Client-->>PickerListener: Send nonempty body data
  PickerListener->>PickerListener: Refresh idle deadline
  alt Body completes
    PickerListener->>PickerListener: Release upload slot and stop deadlines
    Upstream-->>PickerListener: Return response
    PickerListener-->>Client: Forward response
  else Deadline expires
    PickerListener->>Upstream: Destroy upstream request
    PickerListener-->>Client: Close unfinished input
  end
Loading

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to beea8

No actionable upload-lifecycle or documentation defect remains established; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 7 functions across 2 files. (5 skipped: 5 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: it bounds picker uploads and preserves early replies.
Full details: Docstring Coverage

Explanation

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 7 functions across 2 files. (5 skipped: 5 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 6, 2026

luvs01 commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

Please perform one review of published head beea8373762e722b3f66193a3868352dbead5c0f. Automatic review was skipped because this PR is Draft.

Author follow-up on 2026-10-06: the head-associated Cross-platform CI run 37418909942 and React Doctor 37418909953 both completed successfully. The existing Verification section distinguishes hosted Bun execution from the local Node negative controls and records native/stress coverage gaps. No new test execution is claimed by this comment.

Please assess the unfinished-upload accounting, refusal cleanup, early-response/backpressure handling and timer ownership. This is a bounded source review request, not merge authorization or a substitute for required independent security review. The PR remains Draft during this review; no settings, trust stores or installed runtime were changed.

@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@luvs01
luvs01 marked this pull request as ready for review October 7, 2026 02:18
@lidge-jun

Copy link
Copy Markdown
Owner

Held for now: the new upload cap and deadlines can reject legitimate slow uploads. The refusal-cleanup part is low impact on its own; it can land separately without the new limits.

lidge-jun added a commit that referenced this pull request Oct 8, 2026
…ct (carry #6659) (#6725)

Carry #6659 without its new upload cap or idle/absolute deadlines. Close refused and early-replied unfinished uploads after the reply finishes: HTTP/1.1 discards remaining input and ends the socket gracefully (immediate destroy truncated large early replies), HTTP/2 sends RST_STREAM NO_ERROR after a complete response and CANCEL otherwise. Completed uploads and bodyless requests keep existing behavior, including keep-alive on generated 502s. Compact the claude-picker layout registrations to stay under the layout ratchet.

Co-authored-by: Epinephrine <luvs01@hanmail.net>
@lidge-jun

Copy link
Copy Markdown
Owner

Thank you, @luvs01. The cleanup from this PR landed on dev through #6725 (squash c0f8165074), with you credited as co-author on every commit and on the squash.

What was carried: closing refused uploads after their empty reply, stopping input relay only after an early reply's downstream writable finishes, the h2 finish handling on the underlying stream, and your real-listener regressions.

What was left out, as noted in the earlier hold: the 128-upload cap and the 30-second idle and five-minute absolute deadlines. This listener carries all Desktop traffic to claude.ai, so a large attachment on a slow link can run past five minutes, and upstream backpressure can pause input data events long enough to trip the idle timer on a healthy upload.

Review of the carry also found and fixed two issues in the HTTP/1.1 path. Destroying the socket right after finish truncated the tail of a large early reply in 3 of 50 runs, so that path now discards remaining input and ends the socket gracefully. Completed uploads also kept Connection: keep-alive on generated 502s, as they do on dev. On HTTP/2, a complete response now ends the stream with RST_STREAM NO_ERROR (RFC 9113 §8.1).

Closing this in favor of #6725.

@lidge-jun lidge-jun closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants