fix: prevent race conditions when starting concurrent sessions (Node.js & Python)#690
fix: prevent race conditions when starting concurrent sessions (Node.js & Python)#690Fu-Jie wants to merge 1 commit intogithub:mainfrom
Conversation
Fu-Jie
force-pushed
the
fix/concurrent-session-start-race
branch
from
991bc8f to
44ac913
Compare
| return; | ||
| } | ||
|
|
||
| this.state = "connecting"; | ||
| if (this.startPromise) { | ||
| return this.startPromise; | ||
| } |
There was a problem hiding this comment.
start() now returns a Promise in some branches but returns nothing in the early-return path shown here. If start() is no longer declared async, callers doing await client.start() may not reliably await anything. Consider ensuring start() is declared async (so return; becomes Promise<void>), or return a resolved promise in the early-return path (e.g., return this.startPromise ?? Promise.resolve();) to keep the return type consistent.
| this.startPromise = (async () => { | ||
| this.state = "connecting"; | ||
|
|
||
| try { |
There was a problem hiding this comment.
startPromise is cleared on error and on explicit stop/cleanup, but it is never cleared on a successful start. Keeping a resolved promise around works, but it can become a stale gate if any other code path later transitions the client back to "disconnected" without also clearing startPromise. Consider limiting startPromise to only represent an in-flight start (e.g., set it back to null in a finally after the connection attempt completes successfully), relying on this.state === "connected" for the fast-path afterward.
| this.state = "connected"; | ||
| } catch (error) { | ||
| this.state = "error"; | ||
| this.startPromise = null; | ||
| throw error; | ||
| } | ||
| })(); | ||
|
|
||
| return this.startPromise; |
There was a problem hiding this comment.
startPromise is cleared on error and on explicit stop/cleanup, but it is never cleared on a successful start. Keeping a resolved promise around works, but it can become a stale gate if any other code path later transitions the client back to "disconnected" without also clearing startPromise. Consider limiting startPromise to only represent an in-flight start (e.g., set it back to null in a finally after the connection attempt completes successfully), relying on this.state === "connected" for the fast-path afterward.
| """ | ||
| if self._state == "connected": | ||
| return | ||
| async with self._start_lock: |
There was a problem hiding this comment.
This acquires/awaits the lock even when already connected (the common steady-state). A small optimization is to add a fast-path check before entering the lock, and keep the check inside the lock as well (double-checked locking pattern) to preserve correctness while avoiding unnecessary lock contention/awaits.
| """ | |
| if self._state == "connected": | |
| return | |
| async with self._start_lock: | |
| """ | |
| # Fast-path: avoid acquiring the start lock if we're already connected. | |
| if self._state == "connected": | |
| return | |
| async with self._start_lock: | |
| # Double-check under the lock to ensure correctness with concurrent callers. |
Summary
Fixes a race condition where executing multiple
createSession()calls concurrently (withautoStart: trueenabled) triggered multiple underlyingcopilot serverprocesses to spawn instead of sharing one.When
autoStartis enabled, the checkif (this.state === "connected") return;could be bypassed by parallel invocations before the first one completed connecting. This resulted in background zombie CLI processes.Go and .NET are not affected — Go already guards with
sync.RWMutex, and .NET uses_connectionTask ??= StartCoreAsync(...)which is atomic by design.Changes
nodejs/src/client.ts: Cached the initialization into astartPromise. Subsequent concurrent calls await the same promise instead of spawning a new process.python/copilot/client.py: Secured the initialization inside anasyncio.Lock()to guarantee a single daemon process spawn..skipand@pytest.mark.skip(along with the stale TODO comment) from bothnodejs/test/e2e/session.test.tsandpython/e2e/test_session.py. The concurrent instantiation tests now pass cleanly across both languages.