Skip to content

fix(claude): expire idle picker uploads while preserving backpressure - #7784

Merged
lidge-jun merged 7 commits into
lidge-jun:devfrom
luvs01:codex/fix-picker-upload-idle-only-20261014
Oct 27, 2026
Merged

lidge-jun merged 7 commits into
lidge-jun:devfrom
luvs01:codex/fix-picker-upload-idle-only-20261014

Conversation

@luvs01

@luvs01 luvs01 commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Expire unfinished Picker uploads after 60 seconds without body progress, before the first upstream response. Nonempty chunks reset the idle timer, and upstream backpressure pauses it. Completion, response, cancellation and shutdown remove the guard. There is no absolute upload-duration limit, so a slow upload can continue while it makes progress.

HTTP/1.1 expiry closes the underlying socket without an empty success response. HTTP/2 expiry resets only the affected stream with NGHTTP2_CANCEL. The admission slot is released, while sibling streams and existing response cleanup are preserved. Eight localized guides document the behavior.

This replaces the absolute-deadline approach discussed in #7015 with an idle-only proposal. It does not claim an observed live claude.ai incident or a production-verified fix. Whether an additional local bound is useful when the edge normally enforces its own timeout, and whether 60 seconds is the right default, remain review questions.

Verification

Implementation validation at f52b83be, using Bun 1.4.4 on Linux:

bun test tests/claude-integration/claude-picker-upload.test.ts tests/claude-integration/claude-picker-listener.test.ts tests/claude-integration/claude-picker-linger.test.ts
bun run typecheck
bun scripts/structure-ssot.ts
bun scripts/privacy-scan.ts
bun test tests/ci-workflows/file-size-ratchet.test.ts
  • Reported focused TLS validation: 83 passed, 492 assertions. It covers progress, sustained backpressure, stalled HTTP/1.1 and HTTP/2 uploads, cancellation, healthy sibling streams and admission recovery.
  • Typecheck, structure, privacy and whitespace checks passed; size checks passed 9 tests / 85 assertions. The documentation build passed 593 pages / 84,977 links.
  • Current upstream CI completed successfully on exact head f52b83beb7a66ddc68f6f1db118d6101230f6e16, attempt 1: 11 successful jobs / 10 policy skips / 0 failures. Catalog async contracts and the latest target/hygiene/label checks also passed. This upstream run used fast import-graph selection (test changed); the four-shard Ubuntu matrix, desktop shell and native macOS/Windows jobs were policy-skipped. Earlier same-commit four-shard validation remains separate historical evidence.
  • A fresh independent static review found no additional timer, cleanup, backpressure or admission defect.
  • The full local suite was not run under the documented resource exception. Native macOS/Windows jobs were skipped in the prior CI; installed Claude Desktop acceptance, sustained client churn and live-edge behavior remain unverified.

Readiness scope

The unchanged head is conflict-free and five commits behind inspected dev@736fa5087c0b807cc2c7fd54ebae1548e6b2d4cb. The recorded CI checkout ee4eacf828a77847ba9a7e4418951f33f51135b9 combined this head with then-current dev@2606f6f9; it is not a new run on the later dev commit. There are no unresolved upstream inline findings. CodeRabbit skipped the Draft, so this update does not claim completed upstream automated review.

Ready for review of the disclosed idle-only proposal. Live-edge reproduction, whether an additional local bound is useful, and the 60-second default remain open review questions. No code, timeout or live setting changed for this readiness update, and passing checks were not repeated.

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

  • Improvements
    • Picker uploads now stop if they make no progress for 60 seconds before the upstream server’s first response. The timer pauses during upstream backpressure and ends when the upload completes or the first response arrives.
    • An expired upload does not receive a success response. With HTTP/2, only the affected stream is reset. The timeout does not limit total upload duration or apply to response streams.
  • Documentation
    • Updated the Claude Code guides to describe the upload timeout behavior.

github-actions Bot and others added 7 commits October 14, 2026 00:47
Publish the exact locally tested candidate tree. See the draft PR for
focused validation, remaining native/operational coverage and acceptance holds.
Preserve the published PR history while integrating dev 9e48f78. Retain the idle-only policy, request backpressure handling and response cleanup.

Validation: Bun 1.4.4, 83 focused TLS tests; typecheck, structure, privacy, file-size gates and 593-page documentation build passed.
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 2606f6f and f52b83b.

Files selected for processing: 12
  • docs-site/src/content/docs/fr/guides/claude-code.md
  • docs-site/src/content/docs/guides/claude-code.md
  • docs-site/src/content/docs/ja/guides/claude-code.md
  • docs-site/src/content/docs/ko/guides/claude-code.md
  • docs-site/src/content/docs/ru/guides/claude-code.md
  • docs-site/src/content/docs/tr/guides/claude-code.md
  • docs-site/src/content/docs/zh-cn/guides/claude-code.md
  • docs-site/src/content/docs/zh-tw/guides/claude-code.md
  • src/claude/intercept/picker-listener.ts
  • structure/clients/claude-desktop.md
  • tests/claude-integration/claude-picker-upload.test.ts
  • tests/helpers/picker-upload-backpressure.ts

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

Walkthrough

Picker uploads now use a 60-second inactivity timeout before the first upstream response. The timer pauses during upstream backpressure and ends when the upload completes or a response arrives. HTTP/1 and HTTP/2 handling, integration tests, and the client guides also change.

Changes: Picker upload timeout

Layer / File(s) Summary
Idle guard and transport cleanup
src/claude/intercept/picker-listener.ts, structure/clients/claude-desktop.md, docs-site/src/content/docs/.../guides/claude-code.md
The relay adds a configurable idle timeout with a 60-second default. Upload progress resets the timer; upstream backpressure pauses it. Upload completion or the first upstream response clears it. On timeout, the unfinished upload stops without a success response. Cleanup aborts the HTTP/1.1 socket before a complete reply and cancels only the affected HTTP/2 stream. The client structure note and localized guides document the behavior and its limits.
Timeout and backpressure tests
tests/helpers/picker-upload-backpressure.ts, tests/claude-integration/claude-picker-upload.test.ts
The tests cover idle uploads, continuing progress, completed uploads, long-lived responses, and backpressure over HTTP/1 and HTTP/2. The helper observes backpressure, resumes delivery, checks payload bytes, and restores the original pipe method.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PickerListener
  participant Upstream
  Client->>PickerListener: Send upload body
  PickerListener->>Upstream: Relay body
  Upstream-->>PickerListener: Apply backpressure
  PickerListener->>PickerListener: Pause idle timer
  Upstream-->>PickerListener: Return first response
  PickerListener->>PickerListener: Clear idle timer
Loading

Suggested labels: review-ready

Suggested reviewers: lidge-jun, ingwannu

Merge Risk: Minimal · up to f52b8

No merge-blocking issue is established. Installed-client and live-edge behavior remain unverified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: expiring idle Picker uploads while preserving upstream backpressure behavior.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files. (9 skipped: 9 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · 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 added the bug Something isn't working label Oct 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@luvs01
luvs01 marked this pull request as ready for review October 27, 2026 05:30
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration into dev (MAINTAINERS.md, dev-only maintainer integration; not a self-approval).

  • Head merged: f52b83beb7a66ddc68f6f1db118d6101230f6e16. PR CI run 40435413798 (pull_request) concluded success on this exact head: gates, test changed, structure gate, docs site build, api usage, docker smoke, npm-global, storage policy, Catalog async contracts.
  • Independent review (L8 lane, GPT-6.1 Sol): APPROVE, zero blocking findings. Idle expiry, backpressure, response/completion cleanup and admission recovery were checked in src/claude/intercept/picker-listener.ts. The new tests fail against dev source (0 pass / 2 fail: both idle uploads stay open) and pass with this change.
  • Current dev (2b5148b12) merged locally into this head: tests/claude-integration/claude-picker-upload.test.ts plus the test-layout, layout-tooling and file-size-ratchet guards, 71 pass / 0 fail; typecheck passes.
  • No submitted reviews, inline threads or change requests are outstanding.

@lidge-jun
lidge-jun merged commit e72f5d6 into lidge-jun:dev Oct 27, 2026
40 of 41 checks passed
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