fix(vscode-ext): anchor the peer-link reclaim check to our own bind - #753
dormouse-bot wants to merge 5 commits into
Conversation
…r read Two windows that clear the same corpse can both bind, the second unlinking the first's socket file. `stillOurs` was meant to catch that, but it read the path *after* the bind to learn what "ours" was — and the displacing unlink and rebind can land before that read. Both windows then read the winner's socket, both find it unchanged 250 ms later, and both confirm as broker. Capture the identity synchronously in `tryBind` instead. `listen` binds inside the call and resolves on a nextTick, so a `statSync` before the next `await` runs ahead of anything queued on the thread pool and reads the file our own bind made.
Deploying mouseterm with
|
| Latest commit: |
47ba3ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff892a83.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-ci-35764823365.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
The anchor is the right shape, and the ordering argument behind it holds within one process — on Node 22.23.2 a thread-pool fs completion queued before listen does not run before the await resumes, and the Server.listen stack confirms setupListenHandle binds synchronously inside the call. But the comment at tryBind and the matching rationale sentence present that as closing the race, and the competitor is not in this process: docs/specs/vscode.md → "Which window: bind-as-lease" states "One extension host runs per window", so window B's rm and bind are syscalls in a different process and were never on this process's thread pool. They can still land between our bind() returning and the statSync on the next line: A captures B's inode as boundSocketFile, finds the path unchanged 250 ms later, and settle(true)s alongside B — the same two-broker outcome, from a window narrowed from a stat's full round trip to a few microseconds of straight-line code.
The narrowing is worth having and it makes the in-process test deterministic, which is what was flaking. The finding is the claim rather than the change: as written, a later reader concludes the displacement is now impossible and stops guarding for it. Suggested wording inline. If the residual is meant to be closed rather than narrowed, the usual shape is an atomic create-only publish — bind a unique path, then link() it onto the fixed one, which fails EEXIST instead of silently displacing — but that is a redesign of the arbitration, not this PR.
Separately, nothing in the suite pins the new behavior. The forced-displacement trials in the description ran in a throwaway harness, and settles two windows racing for one corpse into a broker and a client reaches the interleaving only by luck — the property that made it flake to begin with. Reverting const mine = boundSocketFile to the old post-bind read would go green on most runs. The harness's forced shape (A binds, B unlinks and rebinds, then both verify) is deterministic in-process and would be the regression guard.
Feedback on work in progress, not a merge verdict.
…e race The forced-displacement shape from the PR description becomes a regression test: a competing reclaim lands just before stillOurs first reads the path. It fails with the old second-read anchor and passes with the bind-time one. The comment at tryBind and the rationale no longer claim the race is closed. The nextTick ordering only covers this process, and the competing window is a separate extension host, so its unlink and rebind can still land between our bind and the statSync. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dormouse-bot
left a comment
There was a problem hiding this comment.
The new test pins the behavior, which was the gap — but its wait is clocked off a copy of the source constant, and that copy is what decides whether it guards anything. const RECLAIM_VERIFY_MS = 250 in the test mirrors the unexported const RECLAIM_VERIFY_MS = 250; in peer-link.ts. Raise the source value and await tick(RECLAIM_VERIFY_MS * 2) lands mid-verification instead of past it: isPeerBroker() is then false because no verdict has been reached, not because the window stood down. Trace the rest with the pre-fix anchoring restored and the constant drifted — mine reads the competitor's socket, the assertion window elapses early, settled resolves true once the real delay ends, and the closing expect(mod.isPeerBroker()).toBe(true) passes too. The test goes green on exactly the bug it was written to catch, with nothing to show it stopped guarding.
The stand-down is observable without a clock: a window that confirmed as broker never dials, so the competitor accepting a connection is the stand-down. Suggested inline, along with dropping the mirrored constant — the two go together, since the constant is only dead once the first lands. The suggestion also renames the ensurePeerNet callback parameter, which currently shadows the held socket array the new wait reads.
Second, the competitor server is never torn down on a failing path. Every other test in this file that listens a net.Server — the EMFILE-after-listen test, both squatter tests, and the rawBroker fan-out test — wraps its assertions in try { … } finally { await new Promise((resolve) => server.close(resolve)) }. Here competitor.close() sits after the three assertions, so if expect(mod.isPeerBroker()).toBe(false) fires — the regression this test exists to catch — the server and its accepted sockets stay open as libuv handles for the rest of the worker's life, and the real failure surfaces alongside a hanging teardown.
Feedback on work in progress, not a merge verdict.
…ames The previous wait was clocked off a mirrored RECLAIM_VERIFY_MS. It now asserts, with no clock, that the settled broker is not sitting on the competitor's socket and that a peer can reach it. It fails with the old second-read anchor. The competitor is closed in a finally, and the mirrored constant is dropped. The stand-down cannot be observed by the competitor being dialled. Closing a displaced unix server unlinks the path whatever it now names, so the loser rebinds uncontested instead of dialling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dormouse-bot
left a comment
There was a problem hiding this comment.
The anchor capture is unconditional, so it runs on Windows too — where socketPath() returns \\.\pipe\dormouse-peer-<id> and the value is never read: stillOurs short-circuits on !mine before its own stat, so a null anchor already produces the only outcome Windows can have. If statSync against a pipe path ever succeeds, that short-circuit stops firing and the 250 ms verification runs instead, comparing two NPFS-reported identities that were never meant to distinguish anything. The description's "Windows is unchanged — a named pipe cannot be stat-ed, so the anchor stays null" is what holds that shut, and it's an assumption about libuv rather than something the code states.
Two things argue for closing it by construction. The old stat was reachable only through the reclaim path, after two refused connects; the new one is on every successful bind, including the uncontested first one — which is the path a lone Windows window takes. And nothing exercises it: pnpm test runs in Build & Test on ubuntu-latest, and the Windows entry of the Standalone Platform Check matrix runs cargo test only, so no job in the repo has run this module's suite against a named pipe. The module already branches on process.platform === 'win32' before touching the filesystem — peerDirIsSafe opens with if (process.platform === 'win32') return true;, and stillOurs computes unstattable as its first line. Suggested inline; it also drops the extra syscall from the common bind path.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Build & Testwent red on run 35764823365 withsettles two windows racing for one corpse into a broker and a clientasserting one broker and getting two. Nothing in the triggering commit (a react-router bump) touches the extension host — this is a real race in the bind-as-lease reclaim that had been passing by luck. The fix anchorsstillOursto the socket file our own bind created rather than to a read of the path taken after it.The race
Both windows find the same dead socket, both unlink it, and the second bind silently displaces the first — that much the code already expects, and
stillOursexists to make the loser stand down. But it learned what "ours" meant by reading the path after the bind:That read is an async
stat, so the competitor'srmand rebind can complete before it resolves. When they do, the displaced window reads the winner's socket asmine, finds it unchanged 250 ms later, and confirms — while the winner confirms too. Two brokers, which the spec calls out as the outcome that matters: two Burrows under oneburrowId, displacing each other on the relay forever (vscode.md → "A bind is not a role until it is believed").The fix
tryBindnow records the identity of the file its own bind made, andstillOurscompares against that. The capture is synchronous and sits before the firstawaitpast the bind:listenbinds inside the call and resolves on a nextTick, so astatSyncthere runs ahead of anything queued on the libuv thread pool.closeServerclears the anchor withserver, and Windows is unchanged — a named pipe cannot be stat-ed, so the anchor stays null andstillOursstill reads that as "nothing here can displace us".Verification
stands down when a competing reclaim displaces it before its verification reads the pathpins the fix deterministically: it forces the displacement just beforestillOursreads the path, fails with the old second-read anchor, and passes with the bind-time one.The peer-link suite cannot run in this CI sandbox —
AF_UNIXis refused outright (listen EAFNOSUPPORT), which fails 43 of its 47 tests regardless of this change. So the reproduction here is the mechanism rather than the suite, and PR CI is what runs the actual test.What was checked locally
pnpm typecheckinvscode-extis clean, and the 11 vscode-ext test files that don't need a unix socket pass (103 tests).fs.promiseswrite already queued,await-ing alistenresolves and a followingstatSynccompletes before that write's callback runs.Related, not fixed here
Closing a server unlinks its path even when another window has rebound it (libuv's
uv__pipe_close), socloseServer(false)— the reclaim stand-down — removes the winner's socket, not onlycloseServer(true)on disposal. That can still end in two confirmed brokers through a sub-millisecond interleaving. Tracked in #756.The fix here also only narrows the cross-process race rather than closing it: another window's unlink and rebind can still land between our
bindand thestatSync. The comment attryBindand the rationale say so.Specs updated: vscode.md states the anchoring as part of the existing
stillOursrule, with the evidence in vscode.rationale.md. The word budget forvscode.mdis re-baselined in the same commit, per AGENTS.md.