test(ffmpeg-ipc): fix Windows hang in IpcConnection multiplexed tests (drain outbound request)#2016
Conversation
…Windows hang Four IpcConnectionMultiplexedTests started a request via SendAndReceiveAsync but the test server never read the outbound request: ProtocolCorruption_PendingRequestsObserveIOExceptionWithCause, AfterReceiveLoopDies_NewRequestsFailFast, DroppedResponseHandler_Throws_ReceiveLoopSurvives, and DisposeWhileRequestPending_ObservesCancellationNotIOException. On Windows a named-pipe write to a peer that never reads does not complete, so SendAsync's pipe write stayed pending and the request never reached the await-on-response stage these tests exercise. Injecting a malformed frame, disposing, or replying then could not complete the stuck task, so the run hung (ProtocolCorruption / AfterReceiveLoopDies), observed ObjectDisposedException instead of OperationCanceledException (DisposeWhileRequestPending), or timed out (DroppedResponseHandler). On Linux the small write buffers immediately, so CI never caught it; a hang dump (dumpasync) pinned the stuck await at MessageSerializer.WriteMessageAsync inside the initial send. Fix: read (drain) the outbound request from the server so the client's send completes and the request parks on its response TCS, mirroring a real peer that reads the request before responding; assert the drained id while we are at it. Also bound the previously unbounded await-on-fault waits so a future regression fails fast with a clear assertion instead of hanging the whole suite. No production change: a real worker always reads requests, and a broken pipe fails a blocked write, so IpcConnection itself is unaffected. Follow-up to #1826.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated multiplexed IPC tests to make request-state transitions deterministic by draining server reads before cancellation, protocol corruption, and disposal. Added bounded assertions so faulting request tasks must complete instead of hanging. ChangesIpcConnectionMultiplexed timing changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates IpcConnectionMultiplexedTests to avoid a Windows-only deterministic hang by ensuring the test “server” drains outbound requests so the client-side SendAsync can complete and the request reaches the intended “awaiting response” state before fault/cancel/dispose scenarios are exercised.
Changes:
- Drain outbound requests from the server side (and assert the drained request id) so
SendAndReceiveAsyncreliably reaches the pending-response phase on Windows. - Add explicit time bounds around previously unbounded waits for fault propagation so regressions fail fast instead of hanging the suite.
💡 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.
🧹 Nitpick comments (1)
tests/Beutl.FFmpegIpc.Tests/IpcConnectionMultiplexedTests.cs (1)
221-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the drain reads for consistency with the fail-fast goal.
Each new server-side drain (
await MessageSerializer.ReadMessageAsync(server)at Lines 221, 240, 434, 472, 515) is awaited without a timeout. The rest of this PR deliberately replaces unbounded awaits with boundedTask.WhenAny(..., Task.Delay(...))so a regression fails fast instead of hanging the suite. If a future change prevents the client send from reaching the server, these drains would hang indefinitely — the exact failure mode this PR is trying to eliminate. Wrapping the drain in a bounded helper (e.g. anAwaitWithResult-style wrapper) would preserve fail-fast for these reads too.Also applies to: 240-240, 434-434, 472-472, 515-515
🤖 Prompt for 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. In `@tests/Beutl.FFmpegIpc.Tests/IpcConnectionMultiplexedTests.cs` at line 221, The server-side drain reads in IpcConnectionMultiplexedTests still use unbounded awaits on MessageSerializer.ReadMessageAsync(server), which can hang the suite and bypass the fail-fast pattern used elsewhere. Update each drain in the affected test cases to use the same bounded await approach as the rest of the PR, ideally via a shared AwaitWithResult-style helper or Task.WhenAny with Task.Delay, so these reads time out consistently if the client message never arrives.
🤖 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.
Nitpick comments:
In `@tests/Beutl.FFmpegIpc.Tests/IpcConnectionMultiplexedTests.cs`:
- Line 221: The server-side drain reads in IpcConnectionMultiplexedTests still
use unbounded awaits on MessageSerializer.ReadMessageAsync(server), which can
hang the suite and bypass the fail-fast pattern used elsewhere. Update each
drain in the affected test cases to use the same bounded await approach as the
rest of the PR, ideally via a shared AwaitWithResult-style helper or
Task.WhenAny with Task.Delay, so these reads time out consistently if the client
message never arrives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33451c1d-4197-46d6-a31d-af02ae2c8e9c
📒 Files selected for processing (1)
tests/Beutl.FFmpegIpc.Tests/IpcConnectionMultiplexedTests.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99fdb11f29
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… comments in English Review follow-up (#2016). The drain reads added to make these tests pass on Windows were unbounded awaits: if the client send regresses again, they would hang before the later time-bounded waits run. Wrap each drain with AwaitWithResult(... .AsTask(), 5s) so a future regression fails fast with a clear assertion instead of hanging the runner. Also rewrite the newly added explanatory comments in English per the AGENTS.md language policy (pre-existing Japanese comments from #1826 are left untouched).
…ments Keep the Windows "unread pipe write does not return" footgun and the ObjectDisposedException-vs-OperationCanceledException distinction; drop the repeated "bound the drain so a regression fails fast" justification (the 5s timeout already shows it) and the bounded-wait paraphrase (the assertion message already states the intent).
|
No TODO comments were found. |
Minimum allowed line rate is |
Description
Fixes a Windows-only deterministic hang in
IpcConnectionMultiplexedTests(the fixture added in #1826). Four tests started a request viaSendAndReceiveAsyncbut the test server never read the outbound request:ProtocolCorruption_PendingRequestsObserveIOExceptionWithCauseAfterReceiveLoopDies_NewRequestsFailFastDroppedResponseHandler_Throws_ReceiveLoopSurvivesDisposeWhileRequestPending_ObservesCancellationNotIOExceptionOn Windows, a named-pipe write to a peer that never reads does not complete, so
SendAsync's pipe write stayed pending and the request never reached the await-on-response stage these tests exercise. Injecting a malformed frame, disposing, or replying then could not complete the stuck task, so the run hung (ProtocolCorruption / AfterReceiveLoopDies), observedObjectDisposedExceptioninstead ofOperationCanceledException(DisposeWhileRequestPending), or timed out (DroppedResponseHandler). On Linux the small write buffers immediately, so CI never caught it. A hang dump (dumpasync) pinned the stuck await atMessageSerializer.WriteMessageAsyncinside the initial send — i.e. the request never got past send, not receive.Fix: read (drain) the outbound request from the server so the client's send completes and the request parks on its response TCS — mirroring a real peer that reads the request before responding — and assert the drained id. Also bound the previously unbounded await-on-fault waits so a future regression fails fast with a clear assertion instead of hanging the whole suite for hours.
No production change: a real worker always reads requests, and a broken pipe fails a blocked write, so
IpcConnectionitself is unaffected.Affected areas
Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary) — test onlyBreaking changes
None — test-only.
Test plan
tests/Beutl.FFmpegIpc.Tests/IpcConnectionMultiplexedTests.cs: 18/18 green, run 3× consecutively (5s each, no hang) on Windows where it previously hung deterministically at the 60s blame-hang timeout. FullBeutl.FFmpegIpc.Testsproject: 28/28 green.dotnet format --verify-no-changesclean.Fixed issues / References
Summary by CodeRabbit