Problem
Raised by Copilot on the v2.10.0 milestone merge (#2637, round 3), and traced to tryReserveAuthFlow in clients/mcpdo/src/connection/auth-helper.ts.
When the per-server .lock is stale (a crashed reserver), two stealers can interleave:
- A renames the stale lock to
…claim-A, finds it stale, removes it, and create()s a fresh lock. A now holds the flow.
- B, which also saw the old lock as stale, now renames A's fresh lock to
…claim-B. It correctly sees the lock is fresh (claimedFresh) and backs off, but it does so with fs.rmSync(claimPath), which deletes A's lock.
- A third connect finds no lock, wins
create(), and spawns a second sign-in helper. The two helpers contend for the OAuth callback port.
The comment at the claimedFresh branch calls the window "un-injectable", but the back-off branch should not destroy the lock it just identified as live.
Expected
On claimedFresh, put the lock back instead of deleting it: for example fs.linkSync(claimPath, lockPath), which fails if a new lock exists, and then remove the claim. That way only a lock confirmed stale is ever discarded. Add a test that injects the interleaving through the fs seams.
Effect
Needs a crashed reserver plus three concurrent non-TTY connects to the same server within the same instant. The worst case is two helpers contending for the callback port and one failing. Nothing leaks.
Priority
Low (rubric total 5): Severity 2, Urgency 1, +1 bug, +1 milestoned.
Problem
Raised by Copilot on the v2.10.0 milestone merge (#2637, round 3), and traced to
tryReserveAuthFlowinclients/mcpdo/src/connection/auth-helper.ts.When the per-server
.lockis stale (a crashed reserver), two stealers can interleave:…claim-A, finds it stale, removes it, andcreate()s a fresh lock. A now holds the flow.…claim-B. It correctly sees the lock is fresh (claimedFresh) and backs off, but it does so withfs.rmSync(claimPath), which deletes A's lock.create(), and spawns a second sign-in helper. The two helpers contend for the OAuth callback port.The comment at the
claimedFreshbranch calls the window "un-injectable", but the back-off branch should not destroy the lock it just identified as live.Expected
On
claimedFresh, put the lock back instead of deleting it: for examplefs.linkSync(claimPath, lockPath), which fails if a new lock exists, and then remove the claim. That way only a lock confirmed stale is ever discarded. Add a test that injects the interleaving through thefsseams.Effect
Needs a crashed reserver plus three concurrent non-TTY
connects to the same server within the same instant. The worst case is two helpers contending for the callback port and one failing. Nothing leaks.Priority
Low (rubric total 5): Severity 2, Urgency 1, +1
bug, +1 milestoned.