Repository navigation
fix: stop StdioTransportTest background sender when send is rejected during close - #138
Merged
Merged
Conversation
…during close Co-authored-by: Junie <junie@jetbrains.com>
Rizzen
approved these changes
Oct 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
masterCI fails at random inStdioTransportTestwithkotlinx.coroutines.channels.ClosedSendChannelException. The tests that fail all use thetestWhileBackgroundSendhelper:should handle close of source gracefully(StdioTransportTest.kt:184)should handle close of sink gracefully(StdioTransportTest.kt:176)should cancel scope gracefully(StdioTransportTest.kt:200)Root cause
This is a race in the test helper. It is not a bug in
StdioTransport.StdioTransport.close()setsCLOSING, closes the send queue, runs the close handler (closes theSourceandSink), cancels the child scope, and then setsCLOSED. Since #127,send()callssendChannel.trySend(encoded).getOrThrow(). It therefore throwsClosedSendChannelExceptionas soon as shutdown begins, while the state is stillCLOSING. TheTransport.sendKDoc documents this behavior.testWhileBackgroundSendsends in a loop until the state isCLOSED. It does not expect the rejection duringCLOSING. In three tests,close()runs on aDispatchers.IOthread (read job, write job, or scope cancellation). If the loop sends in theCLOSINGwindow, the exception goes out oflaunchand failsrunBlocking. The window is very short, so the failure occurs only on a busy runner.should handle close of transport gracefullycannot fail this way, because it callsclose()on the same thread as the loop.Fix
Test-only change in
acp/src/jvmTest/kotlin/com/agentclientprotocol/transport/StdioTransportTest.kt:testWhileBackgroundSendstops its loop whensend()throwsClosedSendChannelException. Thestate != CLOSEDloop condition stays.beforeSinkClosehook. The default hook does nothing.should stop background send when send is rejected while closing. ACountDownLatchin the sink hook holds the transport inCLOSING, so the background loop always sends in the rejected-send window. The test also asserts that a directsend()inCLOSINGthrowsClosedSendChannelException, then releases the latch and expectsCLOSED. The latch is released infinallyon all paths. The test uses no sleeps.StdioTransportbehavior does not change. Sends continue to throw duringCLOSINGandCLOSED. There are no public API or.apidump changes.Verification
Red, with the regression test and fixture hook but without the helper catch:
Result: fails with
ClosedSendChannelExceptionfromStdioTransport.send(StdioTransport.kt:168)in thetestWhileBackgroundSendsend job. This is the same failure as in CI.Green, with the helper catch:
Results:
:acp:jvmTestpasses (147 tests, 0 failures).:acp:check,apiCheck,jvmTest,jsNodeTest,jsBrowserTest, andwasmJsNodeTestpass.linkDebugTestIosSimulatorArm64failed locally withMissingXcodeException, because Xcode is not installed on the machine (xcrun xcodebuild -versionfails). The iOS tests therefore did not run locally. The change is injvmTestonly. CI covers the iOS targets.