Skip to content

fix(vscode-ext): anchor the peer-link reclaim check to our own bind - #753

Open
dormouse-bot wants to merge 5 commits into
mainfrom
fix/ci-35764823365
Open

dormouse-bot wants to merge 5 commits into
mainfrom
fix/ci-35764823365

Conversation

@dormouse-bot

@dormouse-bot dormouse-bot commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Build & Test went red on run 35764823365 with settles two windows racing for one corpse into a broker and a client asserting 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 anchors stillOurs to 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 stillOurs exists to make the loser stand down. But it learned what "ours" meant by reading the path after the bind:

const mine = await socketFileIdentity(path);   // a second read of the path
if (!mine) return unstattable;
await delay(RECLAIM_VERIFY_MS);
const now = await socketFileIdentity(path);
return sameSocketFile(now, mine);

That read is an async stat, so the competitor's rm and rebind can complete before it resolves. When they do, the displaced window reads the winner's socket as mine, 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 one burrowId, displacing each other on the relay forever (vscode.md → "A bind is not a role until it is believed").

The fix

tryBind now records the identity of the file its own bind made, and stillOurs compares against that. The capture is synchronous and sits before the first await past the bind: listen binds inside the call and resolves on a nextTick, so a statSync there runs ahead of anything queued on the libuv thread pool. closeServer clears the anchor with server, and Windows is unchanged — a named pipe cannot be stat-ed, so the anchor stays null and stillOurs still reads that as "nothing here can displace us".

Verification

stands down when a competing reclaim displaces it before its verification reads the path pins the fix deterministically: it forces the displacement just before stillOurs reads 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_UNIX is 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 typecheck in vscode-ext is clean, and the 11 vscode-ext test files that don't need a unix socket pass (103 tests).
  • The nextTick ordering the fix depends on, probed directly: with an fs.promises write already queued, await-ing a listen resolves and a following statSync completes before that write's callback runs.
  • The displacement itself, forced rather than raced — A binds, B unlinks and rebinds, then both run the verification. With the old second-read anchoring both windows confirmed as broker in 25/25 trials; with the bind-time anchor the displaced one stood down in 25/25.

Related, not fixed here

Closing a server unlinks its path even when another window has rebound it (libuv's uv__pipe_close), so closeServer(false) — the reclaim stand-down — removes the winner's socket, not only closeServer(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 bind and the statSync. The comment at tryBind and the rationale say so.

Specs updated: vscode.md states the anchoring as part of the existing stillOurs rule, with the evidence in vscode.rationale.md. The word budget for vscode.md is re-baselined in the same commit, per AGENTS.md.

…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.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 22, 2026

Copy link
Copy Markdown

Deploying mouseterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 47ba3ac
Status: ✅  Deploy successful!
Preview URL: https://ff892a83.mouseterm.pages.dev
Branch Preview URL: https://fix-ci-35764823365.mouseterm.pages.dev

View logs

@dormouse-bot dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread vscode-ext/src/peer-link.ts Outdated
…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 dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread vscode-ext/test/peer-link.test.ts Outdated
Comment thread vscode-ext/test/peer-link.test.ts Outdated
Comment thread vscode-ext/test/peer-link.test.ts
@nedtwigg
nedtwigg marked this pull request as ready for review September 23, 2026 00:30
…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 dormouse-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread vscode-ext/src/peer-link.ts Outdated
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants