Skip to content

test(claude): end the linger upload pump when the socket closes - #7180

Merged
lidge-jun merged 2 commits into
devfrom
fix/picker-linger-macos
Oct 15, 2026
Merged

lidge-jun merged 2 commits into
devfrom
fix/picker-linger-macos

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Summary

h1 refusal reaches a client that keeps uploading; the drop is armed once with the production budget in tests/claude-integration/claude-picker-linger.test.ts timed out at 60s on macos 1/3 in run 38699018462. It normally passes in 20–80 ms. Over the last 400 Cross-platform CI runs, this is the only failure.

The hang is in the test's cleanup. The product behaves correctly. The test's upload pump writes one TLS chunk at a time and settles pumped only from the write callback or from its next tick. When the test fires the armed drop, the picker front destroys the connection and the client gets ECONNRESET. Bun drops a TLS write callback that is still pending when the peer resets the socket: a standalone probe on Bun 1.4.3 lost it in 40 of 40 runs. Even a local destroy() lost it in 1 of 40. If a chunk is in flight at that moment, pumped never settles, and stop() in finally waits until Bun's 60s test timeout. A loaded macOS runner is the most likely place to catch a write in flight.

The fix settles pumped on socket close as well. Neither test's assertions or budget change.

Verification

  • A stress harness repeated the exact scenario with a trace of every client event. Before the fix, iterations stuck at stop with the trace c.end | armed | c.error ECONNRESET | c.close | await stop: 9 of 3,200 finished iterations, and two of the six processes never finished. After the fix, 4,800 of 4,800 passed.
  • Real file, six parallel processes with --rerun-each 250 --timeout 10000: the unfixed file had 14 timeouts in 1,500 runs of exactly this test name; the fixed file passed 3,000 of 3,000 (both tests).
  • bun run typecheck, bun run test:changed (2 pass).
  • Cross-platform CI lane=all: linked below once complete.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (test-only change; none needed)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (no product code touched)

Summary by CodeRabbit

  • Tests
    • Improved reliability of a TLS integration test when the peer resets the socket, preventing the test from waiting indefinitely for upload completion.

Bun drops a TLS write callback still pending when the peer resets the socket.
The armed drop resets the uploading client, so a chunk in flight at that
moment left the pump unsettled and stop() hung until the 60s test timeout
(macos 1/3, run 38699018462). Settle the pump on close as well.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 15, 2026 11:35
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T11:37:20.312536Z 4e8f9c0 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 15, 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: 22361a97-f500-4255-ac59-275fdaca500f
📥 Commits

Reviewing files that changed from the base of the PR and between c37161c and 4e8f9c0.

📒 Files selected for processing: 1
  • tests/claude-integration/claude-picker-linger.test.ts

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

📝 Walkthrough

The TLS test socket’s close handler now resolves the upload-pump completion promise. A comment explains that a TLS write callback may remain pending when the peer resets the socket.

Changes: TLS test socket handling

Layer / File(s) Summary
Resolve pump completion on close
tests/claude-integration/claude-picker-linger.test.ts
At lines 46–61, the socket close handler now resolves the pump completion promise, in addition to recording closure and resolving closed. The comment notes that a pending TLS write callback can otherwise leave pump completion waiting.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4e8f9

This change adjusts test cleanup so socket closure releases the upload-pump wait. No actionable merge-blocking risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed 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 1 functions across 1 files. 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: ending the linger upload pump when the TLS test socket closes. This matches the change in tests/claude-integration/claude-picker-linger.test.…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration into dev (MAINTAINERS.md dev maintainer-integration policy; batch integration authorized by @lidge-jun on 2026-10-15).

  • Head: e824856a7e6c7477fd1065ede01e9e7bfc56e76d (tree 2512e093f2a71ad79c58cdeb3a41b14cfcf36d00) = this branch merged with origin/dev 3f6c696eef.
  • Exact-head evidence: GitHub-hosted Actions for this account have not been assigning runners since 2026-10-15 13:30 UTC, so by owner decision the evidence is a full bun run test (Bun 1.4.3 test runner, root and GUI dependencies installed, GUI built) of this exact tree on a maintainer host (Linux x86_64): exit 1, 7 min, 31 failing test(s), each also failing in the same host's baseline run of dev 60bf4c7 (environment-only): Codex shim install readiness > a refused install exits unsuccessfully and preserves its re, Codex shim install readiness > keeps install advisory when the Codex config cannot be read, Codex shim install readiness > the install command surfaces readiness warnings without lea, GUI pairing client > sends one bodyless origin-bound capability and redacts rejected bodie, Node launcher PATH fallback activation > broken bundle reaches validated PATH Bun, Node launcher PATH fallback activation > inspection bundle reaches validated PATH Bun, Node launcher PATH fallback activation > missing bundle reaches validated PATH Bun, a secondary ensure parent preserves shared Grok, Codex and Claude agent bytes, ….
  • Local gates on the same tree: bun run typecheck, file-size ratchet, test-layout and structure:check.
  • Hosted CI checks for this head may still be queued; they are not claimed as evidence.

@lidge-jun
lidge-jun merged commit 041241a into dev Oct 15, 2026
5 checks passed
@lidge-jun
lidge-jun deleted the fix/picker-linger-macos branch October 15, 2026 17:37
@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Oct 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant