fix: replace TCP readiness check with signal file to fix flaky Linux test#59
Conversation
…test Wait-ServerReady used a TCP socket connect check that succeeded as soon as HttpListener.Start() opened the port. On Linux, the background job had not yet called GetContextAsync(), so the test HTTP request arrived before the listener was ready, causing intermittent failures. Replace with a signal file approach: the background job writes a temp file after entering GetContextAsync(), and the parent polls for that file. This is deterministic and eliminates the race condition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> (cherry picked from commit 85de79c)
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 48 minutes and 43 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFileDownload.Integration.tests.ps1 replaces TCP port polling with filesystem signal file polling for HTTP server readiness detection. The Wait-ServerReady helper now waits for a signal file, Start-TestHttpServerJob generates and passes a unique signal file path to the background job, the job writes the file after starting the listener, and the caller waits for the file and removes it. ChangesHTTP Server Readiness Signaling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the integration-test HTTP server “readiness” mechanism to eliminate a race condition on Linux by switching from a TCP connect probe to a signal-file handshake, improving determinism for Invoke-PatFileDownload integration tests.
Changes:
- Replace TCP-port polling readiness check with polling for a temp “ready” signal file.
- Signal readiness from the server job only after entering
GetContextAsync()to prevent request/accept races. - Fail fast if readiness isn’t reached within timeout (instead of warning).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Integration/Private/FileDownload.Integration.tests.ps1`:
- Around line 135-144: Wrap the readiness check and throw in a try/finally so
the signal file is always removed: call Wait-ServerReady (using the existing
$readySignalFile and $Port) inside a try block, throw if $ready is $false, and
perform the Test-Path / Remove-Item cleanup in the finally block to guarantee
removal even on timeout or exceptions; reference the existing symbols
Wait-ServerReady, $readySignalFile, Remove-Item and the throw that currently
short-circuits cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ed63a06-6627-4eb2-a184-8ec6b2a00775
📒 Files selected for processing (1)
tests/Integration/Private/FileDownload.Integration.tests.ps1
Wrap the readiness wait in Start-TestHttpServerJob with try/catch/finally so a startup that times out no longer leaks resources: - The catch stops and removes the background job, releasing the HttpListener and its 30-second GetContextAsync wait instead of orphaning the job. - The finally always removes the readiness signal file, even when the wait times out and throws. - Add ValidateNotNullOrEmpty to the Wait-ServerReady SignalFile parameter. Addresses review feedback on the integration test scaffolding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviving forgotten work
This restores a fix you authored on 2026-02-24 (
85de79c) that was committed onto thepre-commit-ci-update-configbot branch and never merged — it's not inmain. Cherry-picked cleanly onto currentmain(the integration-test file hasn't diverged since).What it does
Replaces a TCP readiness check with a signal-file approach in
Private/FileDownload.Integration.tests.ps1(+24/−21) to fix a flaky Linux integration test.Why now
We hit a flaky download test on Plex #55 (
Failed to download file… connection forcibly closed) while reviewing the test-scaffolding PR — this forgotten fix targets that same FileDownload test flakiness, so it's worth landing.Note
Original work is 3 months old; worth a fresh look to confirm it's still the right approach against current
mainbefore merging. CI (incl. integration, if it runs here) will exercise it.🤖 Generated with Claude Code
Summary by CodeRabbit
Note: This release contains no user-facing changes.