Skip to content

test: isolate hook fixture connections from fake timers - #26369

Merged
brennanb2025 merged 1 commit into
mainfrom
brennanb2025/fix-flaky-tests
Oct 8, 2026
Merged

brennanb2025 merged 1 commit into
mainfrom
brennanb2025/fix-flaky-tests

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​48 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​48
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2

ELI5

The Claude cancellation test keeps cancellation bounded unless a new prompt opens a turn: true failed on Linux in unit shard 1 of the job labeled Node 24 with “The socket connection was closed unexpectedly.” The test file actually ran under Bun. Advancing fake time by 120 seconds expired a real HTTP connection that the test client could still try to reuse for its next Stop-hook request.

There is no user-facing change: this fixes test transport only. Orca's hook handling and agent status behavior stay the same.

What Changed

The shared hook POST fixture now sends Connection: close, matching the existing relay test transport. Every request uses a fresh connection, so fake-time advances cannot leave a stale pooled connection for the next hook. A regression test counts actual server sockets across a 120-second fake-time advance.

This also covers the sibling cancellation and notification-expiry tests that use the same fixture, without changing their assertions or timers.

Why

The tests need fake timers to exercise missing-notification expiry. Keeping those timers and removing connection reuse fixes the race while preserving that coverage. Changing production timeouts, adding retries, or replacing timer advances with clock-only changes would either change application behavior or weaken the tests.

Before the fix, a temporary diagnostic recorded a live HTTP socket, then its timeout and destruction during the fake-time advance. The new regression also failed against the old fixture because three POSTs used only two server sockets.

Linked Issue

No separate issue. Found in the attempt-1 unit shard of #26173: https://github.com/stablyai/orca/actions/runs/37693271510/job/113046014894

Visual Proof

N/A — tests only; no visual or interaction change.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • macOS arm64: 20 sequential runs under Bun 1.4.2, launched through the same wrapper as CI with Node 24.18.0. Each run passed all 61 tests in the wake-up lifecycle, cancellation capture, notification-expiry, and fixture regression files.

  • The same four files passed directly under Node 24.18.0 and Node 26.5.0 (61 tests each).

  • The regression failed with the old helper (two sockets for three POSTs) and passed with the fix (three sockets).

  • tc-gate pnpm tc:node passed.

  • orca-ci-checks --no-typecheck passed all checks immediately after the Node typecheck; the changed-code quality check also passed with the new test staged.

  • Changed-file lint, formatting, and git diff --check passed.

The diagnostic proves fake time destroys the pooled socket; the exact intermittent fetch exception was observed in CI, not reproduced locally. Linux verification is provided by PR CI. No Electron app was launched.

Review

Self-reviewed the test helper, hook listener, timer implementation, sibling tests, and regression. Production files are unchanged.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Author: @BrennanKB5

Platform-independent HTTP headers only. Existing local and relay paths remain covered. No change to SSH execution, folder workspaces, remote protocol compatibility, or production behavior.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review and verification summary

Problem: the cancellation test keeps cancellation bounded unless a new prompt opens a turn: true failed intermittently in unit shard 1 with "The socket connection was closed unexpectedly". That shard runs under Bun on Linux. The shared hook-request test helper reused pooled HTTP connections. When a test fast-forwarded its fake clock by 120 s, that also fired the local hook server's real idle-connection timeout, which closed the pooled socket. The next request could then try to reuse the closed connection.

User-facing change: none. Only test code changed; the hook server is untouched.

Fix: the helper sends Connection: close on every request, matching the existing relay test path. Every hook request now uses a fresh connection, and the fake timers that the notification-expiry tests rely on are kept.

Verified:

  • A new regression test counts the server's sockets across the 120 s fast-forward. It fails with the old helper (2 sockets for 3 requests) and passes with the fix (3).
  • 20 sequential runs under Bun 1.4.2 (the version CI pins), 61 tests each, passed. The same files also passed under Node 24 and Node 26. All of this ran on macOS arm64.
  • A diagnostic run showed the socket being destroyed during the fast-forward.
  • Node typecheck and the repository's static checks passed. PR CI: 19 passed, 16 skipped, 0 failed.
  • I read the diff and the regression test.

Not verified: the exact intermittent error was seen in CI, not reproduced locally. It depends on timing, and only the closed-socket cause was demonstrated.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 8, 2026 00:31
@coderabbitai

coderabbitai Bot commented Oct 8, 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 UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6cd5d75b-e053-4d4f-9776-4285222c452c
📥 Commits

Reviewing files that changed from the base of the PR and between 5a1009f and 7873c3e.

📒 Files selected for processing (2)
  • src/main/agent-hooks/server.test-fixtures.test.ts
  • src/main/agent-hooks/server.test-fixtures.ts

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


📝 Walkthrough

Walkthrough

The hook-event request fixture now sends a Connection: close header. A new test sends three hook events to a server across fake-time advances. It checks that each response has status 204 and that the server observed three request sockets. The test stops the server and restores real timers in a finally block.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 7873c

This test-focused change addresses hook-request connection reuse and adds a regression check. No concrete merge-blocking risk is established by the available evidence.

🚥 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 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the test-focused change: isolating hook fixture connections from fake timers.
Description check ✅ Passed The description is complete and focused. It explains the failure, root cause, implementation, regression coverage, testing, platform considerations, and absence of user-facing changes. The linked issu…
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
  • 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.

@brennanb2025
brennanb2025 merged commit 4da1324 into main Oct 8, 2026
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant