sdk: make client startup single-flight - #2585
Conversation
Concurrent createSession()/resumeSession() on a fresh client each auto-start the CLI; without an in-flight guard the second spawn overwrote this.cliProcess and orphaned the first, which stop() never terminates. Guard start() with a shared startPromise so concurrent callers join one spawn; clear it on completion so a failed start can retry. Adds regression tests (single-flight + retry-after-failure). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Serialize Python startup attempts and clean partial resources before retrying after failures or cancellation. Atomically publish one .NET connection task, clear failed attempts before notifying waiters, and cover concurrent, failed, and cancelled startup behavior with regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DonJayamanne
changed the title
|
Addressed the cross-SDK consistency review in
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await connect_started.wait() | ||
| first_start.cancel() | ||
| with pytest.raises(asyncio.CancelledError): | ||
| await first_start |
This comment has been minimized.
This comment has been minimized.
Update the CLI startup failure E2E for retryable client startup. A subsequent lazy start now launches a fresh process and reports the same invalid argument instead of interacting with a stale pipe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fixed the Python matrix regression in The .NET jobs are a separate pre-compilation restore failure from the existing |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2585 · copilot · sonnet50 · 56.7 AIC · ⌖ 12.4 AIC · ⊞ 8.3K
| catch (Exception ex) | ||
| { | ||
| // Clear before waking waiters so continuations can immediately retry. | ||
| _ = Interlocked.CompareExchange( | ||
| ref _connectionTask, | ||
| null, | ||
| startCompletion.Task); | ||
| startCompletion.TrySetException(ex); | ||
| } |
| else if (ffiHost is not null) | ||
| { | ||
| try { ffiHost.Dispose(); } | ||
| catch (Exception cleanupError) { AddCleanupError(null, cleanupError, _logger); } |
This comment has been minimized.
This comment has been minimized.
Cross-SDK consistency reviewThis PR fixes a startup race/leak in Node.js, .NET, and Python: Findings:
Overall the PR maintains good parity for the languages it touches (Node/.NET/Python), with matching behavior and test coverage across all three. The one actionable gap is the Java retry-after-failure behavior noted above.
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2585 · copilot · sonnet50 · 125 AIC · ⌖ 12.4 AIC · ⊞ 8.3K
2 participants
Summary
CopilotClient.start()share one in-flight CLI startupWhy
Concurrent lazy-start callers could pass the initial state check before startup was published, causing duplicate CLI processes or connections. The language-specific guards now establish one startup owner while preserving safe retry behavior after failure.
Validation
ty check copilot: passedClientSessionLifetimeTests: 125 passednet8.0,net10.0, andnetstandard2.0: passed locally with NuGet audit disabled because of the upstream advisory belowgit diff --check: passedCI note
The current .NET jobs stop during restore, before compiling this diff, because the existing
Microsoft.SourceLink.GitHub 10.0.102dependency now triggers GHSA-23fw-v26w-5fgq. This PR does not update that unrelated dependency or suppress repository CI audits.