fix: bound polling loop timeouts by remaining deadline time#216
Merged
Conversation
…ssionMetadataById The Copilot CLI persists session state asynchronously, so querying session metadata or the last session ID immediately after sendAndWait() is a race condition. On Linux (CI) and Windows the I/O completes fast enough to mask the issue, but on macOS the tests fail consistently. Align the Java tests with the .NET and Node.js reference implementations: - testShouldGetLastSessionId: close the session before calling getLastSessionId() (matches .NET DisposeAsync-then-query pattern), and poll with a 10-second deadline and 50ms intervals (matches Node.js client_lifecycle.e2e.test.ts polling pattern). - testShouldGetSessionMetadataById: poll getSessionMetadata() with a 10-second deadline and 50ms intervals until it returns non-null (matches .NET WaitForConditionAsync pattern). Signed-off-by: Ed Burns <[email protected]>
Co-authored-by: edburns <[email protected]>
… deadline The 10s polling deadline in testShouldGetLastSessionId and testShouldGetSessionMetadataById was not actually enforced because each iteration could block up to 30s on the Future.get() call. Now each iteration uses Math.max(1, deadline - currentTimeMillis) as the timeout so the overall wait is bounded by the intended 10s deadline. Co-authored-by: edburns <[email protected]>
Copilot
AI
changed the title
[WIP] Fix code based on review comment 3268776043
fix: bound polling loop timeouts by remaining deadline time
May 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates two polling-based tests in CopilotSessionTest so that per-iteration Future.get() timeouts are bounded by the remaining overall deadline, preventing a single stalled RPC from blocking longer than intended.
Changes:
- Replaced fixed per-iteration
Future.get(30s)timeouts withremaining-based timeouts against a 10s deadline. - Added polling loops (with short sleeps) for
getLastSessionId()andgetSessionMetadata()to accommodate asynchronous persistence behavior.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/github/copilot/sdk/CopilotSessionTest.java | Bounds polling loop get() timeouts by remaining deadline time for last-session-id and session-metadata tests. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/test/java/com/github/copilot/sdk/CopilotSessionTest.java:870
- Same issue as the getLastSessionId polling loop:
get(remaining, ...)can throw TimeoutException whenremainingbecomes small (down to 1ms), causing a premature test failure instead of continuing to poll until the 10s deadline. Handle TimeoutException inside the loop (retry until deadline) and consider capping the per-iteration timeout to avoid ultra-short timeouts near the end.
long deadline = System.currentTimeMillis() + 10_000;
while (System.currentTimeMillis() < deadline) {
long remaining = Math.max(1, deadline - System.currentTimeMillis());
metadata = client.getSessionMetadata(sessionId).get(remaining, TimeUnit.MILLISECONDS);
if (metadata != null) {
break;
}
Thread.sleep(50);
- Files reviewed: 1/1 changed files
- Comments generated: 1
…eout Co-authored-by: edburns <[email protected]>
edburns
approved these changes
May 19, 2026
edburns
added a commit
to github/copilot-sdk
that referenced
this pull request
May 19, 2026
edburns
added a commit
to github/copilot-sdk
that referenced
this pull request
May 19, 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.
Before the change?
testShouldGetLastSessionIdandtestShouldGetSessionMetadataByIdused a fixed 30s timeout perFuture.get()call inside a loop with a 10s deadline. If the RPC stalled, a single iteration could block 3× longer than the intended overall deadline.After the change?
remaining = Math.max(1, deadline - currentTimeMillis())and passes it as the per-iteration timeout, ensuring the overall wait is bounded by the 10s deadline:Pull request checklist
mvn spotless:applyhas been run to format the codemvn clean verifypasses locallyDoes this introduce a breaking change?