From 1f521d3db1760ad44f2a9f2bbf80ce7387df325a Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Tue, 22 Sep 2026 18:29:58 +0000 Subject: [PATCH 1/5] fix(vscode-ext): anchor the reclaim check to our own bind, not a later read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/specs/vscode.md | 2 +- docs/specs/vscode.rationale.md | 2 +- scripts/spec-word-budgets.json | 2 +- vscode-ext/src/peer-link.ts | 38 +++++++++++++++++++++++++++++----- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/docs/specs/vscode.md b/docs/specs/vscode.md index 08f3bc72..9dcb92dd 100644 --- a/docs/specs/vscode.md +++ b/docs/specs/vscode.md @@ -280,7 +280,7 @@ Source of truth: `VsCodeBurrowStateStore` in `vscode-ext/src/burrow-store.ts`, ` - **Roles never flip downward.** There is no `onRole(false)` after a `true`, and a client only ever changes role *upward*, which makes a TTL lease's mid-transition races unrepresentable rather than handled (rationale). - **Contend on broker death, not on a timer.** When the broker exits, every client's socket closes and they all race to bind; exactly one wins, because `bind` is the arbiter. No TTL, no heartbeat file, no filesystem watcher. -- **A corpse is cleared, then the bind is re-checked.** **Never unlink on the first refusal**, and **`stillOurs` compares full filesystem identity — device, inode, and nanosecond change timestamp, never inode alone**. A window whose socket identity was replaced, **or whose path has gone entirely**, stands down and the loop re-runs (jitter, re-dial and per-platform reasoning at `attempt` and `stillOurs`; rationale). +- **A corpse is cleared, then the bind is re-checked.** **Never unlink on the first refusal**, and **`stillOurs` compares full filesystem identity — device, inode, and nanosecond change timestamp, never inode alone, anchored at our own bind rather than a later read of the path**. A window whose socket identity was replaced, **or whose path has gone entirely**, stands down and the loop re-runs (jitter, re-dial and per-platform reasoning at `attempt` and `stillOurs`; rationale). - **A bind is not a role until it is believed.** Everything that answers "is this window the broker" — `ensurePeerNet`'s shortcut, `isPeerBroker`, `isPeerLinkSettled`, `remoteNotifyPeerChange` — reads `brokerConfirmed`, set only where `settle(true)` runs and cleared by `closeServer`. **Unverified reads as unsettled**, so a command landing in the `RECLAIM_VERIFY_MS` window (an `enroll`, a `secrets.onDidChange`) is held for the verdict rather than told "broker" (rationale). - **Attempts are spaced.** The loop waits `RETRY_MS` between rounds so a refused hello cannot spin, and a bind or connect landing after disposal is undone rather than left to outlive its window. - **Errors after `listen` are logged, not thrown** — an `EventEmitter` with no `'error'` listener rethrows out of a libuv callback and takes the extension host down, so `listenServer` installs a permanent logging listener the moment the bind succeeds (rationale). diff --git a/docs/specs/vscode.rationale.md b/docs/specs/vscode.rationale.md index bf29e05f..a6dd00ef 100644 --- a/docs/specs/vscode.rationale.md +++ b/docs/specs/vscode.rationale.md @@ -76,7 +76,7 @@ Rows 1–2 are why a blanket second press is wrong; `Press Ctrl-C again` was abs **Why the reclaim is jittered and re-dialled.** Every client of a dead broker reaches the `ECONNREFUSED` at the same instant. Unlinking immediately means several of them unlink, and unlinking a *live* broker's socket — one that rebound the path while we waited — strands every window dialling it. -**Why `stillOurs` compares full filesystem identity.** Two windows can find the same corpse, both unlink, and the second bind silently displaces the first, leaving the loser serving a socket no client can reach; nothing on the bind path detects that. Inode alone is reused too readily to distinguish "still ours" from "replaced". +**Why `stillOurs` compares full filesystem identity.** Two windows can find the same corpse, both unlink, and the second bind silently displaces the first, leaving the loser serving a socket no client can reach; nothing on the bind path detects that. Inode alone is reused too readily to distinguish "still ours" from "replaced". And the identity has to be anchored to our own bind rather than to a first read of the path: the displacing unlink and rebind can land before that read, so both windows read the winner's socket, both find it unchanged, and both confirm — two brokers, which is what `settles two windows racing for one corpse into a broker and a client` caught intermittently (observed 2026-09-22). `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. **What an unverified bind would cost.** During `RECLAIM_VERIFY_MS` the socket is bound but may still be given up. A command landing inside that window and told "broker" would start a service the stand-down path never tears down — two Burrows under one burrowId, and the endless relay displacement above. diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 9695d709..2d698d5f 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -34,7 +34,7 @@ "docs/specs/tiling-engine.md": 4650, "docs/specs/transport.md": 5500, "docs/specs/tutorial.md": 1900, - "docs/specs/vscode.md": 7200, + "docs/specs/vscode.md": 7250, "docs/specs/webgl-text.md": 1200, "docs/specs/website-docs.md": 4800 } diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index 59d114ad..0a8a76dd 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -5,6 +5,7 @@ */ import { chmod, lstat, mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises'; +import { statSync, type BigIntStats } from 'node:fs'; import { createHash, randomUUID } from 'node:crypto'; import { createConnection, createServer, type Server, type Socket } from 'node:net'; import { tmpdir } from 'node:os'; @@ -287,6 +288,17 @@ let server: Server | null = null; let brokerConfirmed = false; /** Claimed and cleared with `server`; the two always move together. */ let serverToken: string | null = null; +/** + * The socket file this window's own bind created, claimed and cleared with + * `server`. + * + * {@link stillOurs} compares the path against *this*, never against a second + * read of the path: a competing window that cleared the same corpse may have + * rebound it before the first read lands, and then both reads name that + * window's socket, the two agree, and every window that bound believes it + * won. + */ +let boundSocketFile: SocketFileIdentity | null = null; const clients = new Set(); /** Provider-local route handle → the peer window that owns it. */ const routes = new Map(); @@ -783,6 +795,11 @@ async function tryBind(path: string, token: string): Promise { return false; } server = nextServer; + // Synchronous, and before the first `await` past the bind: `listen` binds + // inside the call and resolves on a nextTick, so nothing queued on the thread + // pool — a competing window's unlink among it — can run in between, and this + // reads the file our own bind made ({@link boundSocketFile}). + boundSocketFile = socketFileIdentitySync(path); // Provisional until the caller settles it: a reclaimed bind may still be // displaced (see {@link brokerConfirmed}). brokerConfirmed = false; @@ -1243,11 +1260,21 @@ interface SocketFileIdentity { ctimeNs: bigint; } +function toSocketFileIdentity(value: BigIntStats | null | undefined): SocketFileIdentity | null { + return value ? { dev: value.dev, ino: value.ino, ctimeNs: value.ctimeNs } : null; +} + async function socketFileIdentity(path: string): Promise { - const value = await stat(path, { bigint: true }).catch(() => null); - return value - ? { dev: value.dev, ino: value.ino, ctimeNs: value.ctimeNs } - : null; + return toSocketFileIdentity(await stat(path, { bigint: true }).catch(() => null)); +} + +/** {@link socketFileIdentity} without yielding — see {@link boundSocketFile}. */ +function socketFileIdentitySync(path: string): SocketFileIdentity | null { + try { + return toSocketFileIdentity(statSync(path, { bigint: true, throwIfNoEntry: false })); + } catch { + return null; + } } function sameSocketFile(left: SocketFileIdentity, right: SocketFileIdentity): boolean { @@ -1256,7 +1283,7 @@ function sameSocketFile(left: SocketFileIdentity, right: SocketFileIdentity): bo async function stillOurs(path: string): Promise { const unstattable = process.platform === 'win32'; - const mine = await socketFileIdentity(path); + const mine = boundSocketFile; if (!mine) return unstattable; await delay(RECLAIM_VERIFY_MS); const now = await socketFileIdentity(path); @@ -1296,6 +1323,7 @@ async function closeServer(unlink: boolean): Promise { server = null; brokerConfirmed = false; serverToken = null; + boundSocketFile = null; for (const peer of [...clients]) dropClient(peer); if (!closing) return; if (closing.listening) closing.close(); From 42e2f7738719313d913f93e678fdff19416c10ba Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 16:42:35 -0700 Subject: [PATCH 2/5] test(vscode-ext): pin the bind-time reclaim anchor; say it narrows the 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) --- docs/specs/vscode.rationale.md | 2 +- vscode-ext/src/peer-link.ts | 11 +++-- vscode-ext/test/peer-link.test.ts | 68 +++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/docs/specs/vscode.rationale.md b/docs/specs/vscode.rationale.md index a6dd00ef..d5febf23 100644 --- a/docs/specs/vscode.rationale.md +++ b/docs/specs/vscode.rationale.md @@ -76,7 +76,7 @@ Rows 1–2 are why a blanket second press is wrong; `Press Ctrl-C again` was abs **Why the reclaim is jittered and re-dialled.** Every client of a dead broker reaches the `ECONNREFUSED` at the same instant. Unlinking immediately means several of them unlink, and unlinking a *live* broker's socket — one that rebound the path while we waited — strands every window dialling it. -**Why `stillOurs` compares full filesystem identity.** Two windows can find the same corpse, both unlink, and the second bind silently displaces the first, leaving the loser serving a socket no client can reach; nothing on the bind path detects that. Inode alone is reused too readily to distinguish "still ours" from "replaced". And the identity has to be anchored to our own bind rather than to a first read of the path: the displacing unlink and rebind can land before that read, so both windows read the winner's socket, both find it unchanged, and both confirm — two brokers, which is what `settles two windows racing for one corpse into a broker and a client` caught intermittently (observed 2026-09-22). `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. +**Why `stillOurs` compares full filesystem identity.** Two windows can find the same corpse, both unlink, and the second bind silently displaces the first, leaving the loser serving a socket no client can reach; nothing on the bind path detects that. Inode alone is reused too readily to distinguish "still ours" from "replaced". And the identity has to be anchored to our own bind rather than to a first read of the path: the displacing unlink and rebind can land before that read, so both windows read the winner's socket, both find it unchanged, and both confirm — two brokers, which is what `settles two windows racing for one corpse into a broker and a client` caught intermittently (observed 2026-09-22). `listen` binds inside the call and resolves on a nextTick, so a `statSync` before the next `await` runs ahead of anything queued on this process's thread pool. The competitor is another extension host, so its unlink and rebind can still land in the microseconds between our `bind` and that `statSync`: the anchor narrows the race rather than closing it. Closing it would take an atomic create-only publish — bind a unique path, then `link()` it onto the fixed one, which fails `EEXIST` instead of displacing — a redesign of the arbitration. **What an unverified bind would cost.** During `RECLAIM_VERIFY_MS` the socket is bound but may still be given up. A command landing inside that window and told "broker" would start a service the stand-down path never tears down — two Burrows under one burrowId, and the endless relay displacement above. diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index 0a8a76dd..beac72eb 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -294,9 +294,10 @@ let serverToken: string | null = null; * * {@link stillOurs} compares the path against *this*, never against a second * read of the path: a competing window that cleared the same corpse may have - * rebound it before the first read lands, and then both reads name that + * rebound it before an async read lands, and then both reads name that * window's socket, the two agree, and every window that bound believes it - * won. + * won. Captured synchronously after the bind, which narrows that gap without + * closing it (see `tryBind`). */ let boundSocketFile: SocketFileIdentity | null = null; const clients = new Set(); @@ -796,8 +797,10 @@ async function tryBind(path: string, token: string): Promise { } server = nextServer; // Synchronous, and before the first `await` past the bind: `listen` binds - // inside the call and resolves on a nextTick, so nothing queued on the thread - // pool — a competing window's unlink among it — can run in between, and this + // inside the call and resolves on a nextTick, so no thread-pool callback of + // *this* process can interleave. A competing window is a separate extension + // host, so its unlink and rebind can still land here — this narrows the gap + // to a few microseconds of straight-line code rather than closing it, and // reads the file our own bind made ({@link boundSocketFile}). boundSocketFile = socketFileIdentitySync(path); // Provisional until the caller settles it: a reclaimed bind may still be diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index 6d3a593e..a52e1b64 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -48,12 +48,34 @@ vi.mock('../src/log', () => ({ }, })); +/** + * Runs once, just before the next `stat` of the peer socket path — the only + * `stat` `peer-link` makes. Lets a test land a competing window's displacement + * at an exact point in the reclaim verification. Hoisted for the same reason as + * {@link logged}. + */ +const beforeSocketStat = vi.hoisted(() => ({ hook: null as null | (() => Promise), path: '' })); +vi.mock('node:fs/promises', async (importOriginal) => { + const real = await importOriginal(); + const stat = (async (...args: Parameters) => { + const { hook, path } = beforeSocketStat; + if (hook && String(args[0]) === path) { + beforeSocketStat.hook = null; + await hook(); + } + return real.stat(...args); + }) as typeof real.stat; + return { ...real, stat }; +}); + let dir: string; /** Peer sockets live in the temp dir; point that at this test's own storage. */ let realTmp: string | undefined; const opened: LinkModule[] = []; const derivedSocketPath = (): string => socketPathFor(dir); +/** `RECLAIM_VERIFY_MS` in `peer-link.ts`. */ +const RECLAIM_VERIFY_MS = 250; interface SocketFileIdentity { dev: bigint; @@ -133,6 +155,7 @@ beforeEach(async () => { realTmp = process.env.TMPDIR; process.env.TMPDIR = dir; logged.length = 0; + beforeSocketStat.hook = null; }); afterEach(async () => { @@ -457,6 +480,51 @@ describe('bind-as-lease', () => { expect(firstRoles.concat(secondRoles)).toEqual([true]); }, 30_000); + it('stands down when a competing reclaim displaces it before its verification reads the path', async () => { + // The interleaving the racing test above reaches only by luck, forced: a + // competing window's unlink and rebind land after our bind but before + // `stillOurs` first reads the path. Anchored to that read rather than to + // our own bind, both windows would name the competitor's socket as "ours" + // and both would broker. + const path = derivedSocketPath(); + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const corpse = spawn(process.execPath, [ + '-e', + `require('node:net').createServer().listen(${JSON.stringify(path)})`, + ]); + await waitForFile(path); + corpse.kill('SIGKILL'); + await new Promise((resolve) => corpse.on('exit', resolve)); + + // The competitor holds the path and never speaks, so a dial to it neither + // connects nor reads as refused: this window can only wait for it to go. + const held: Socket[] = []; + const competitor = createServer((socket) => void held.push(socket)); + let displaced = false; + beforeSocketStat.path = path; + beforeSocketStat.hook = async () => { + await rm(path, { force: true }); + await new Promise((resolve) => competitor.listen(path, resolve)); + displaced = true; + }; + + const mod = await openWindow(fakeWindow()); + const roles: boolean[] = []; + const settled = mod.ensurePeerNet((held) => roles.push(held)); + await waitFor(() => displaced, 15_000); + // Past the whole verification: a window that confirmed would be broker now. + await tick(RECLAIM_VERIFY_MS * 2); + expect(mod.isPeerBroker()).toBe(false); + expect(roles).toEqual([]); + + // Once the competitor is gone the path is free, and the loop takes it. + const gone = new Promise((resolve) => competitor.close(resolve)); + for (const socket of held) socket.destroy(); + await gone; + await settled; + expect(mod.isPeerBroker()).toBe(true); + }, 30_000); + it('collects directory entries from the other window', async () => { const peerSide = fakeWindow({ entries: [{ surfaceId: 'far-1' }, { surfaceId: 'far-2' }] }); const { broker } = await linkedPair(fakeWindow(), peerSide); From 6e1b3921b4cfd417183cc744b9ba464d5536c390 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:35:29 -0700 Subject: [PATCH 3/5] test(vscode-ext): judge the forced reclaim by whose socket the path names 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) --- vscode-ext/test/peer-link.test.ts | 45 ++++++++++++++++--------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index a52e1b64..15514ce5 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -74,8 +74,6 @@ let realTmp: string | undefined; const opened: LinkModule[] = []; const derivedSocketPath = (): string => socketPathFor(dir); -/** `RECLAIM_VERIFY_MS` in `peer-link.ts`. */ -const RECLAIM_VERIFY_MS = 250; interface SocketFileIdentity { dev: bigint; @@ -496,33 +494,36 @@ describe('bind-as-lease', () => { corpse.kill('SIGKILL'); await new Promise((resolve) => corpse.on('exit', resolve)); - // The competitor holds the path and never speaks, so a dial to it neither - // connects nor reads as refused: this window can only wait for it to go. + // The competitor holds the path and never speaks, standing in for a window + // that won the reclaim. const held: Socket[] = []; const competitor = createServer((socket) => void held.push(socket)); - let displaced = false; + let theirs: SocketFileIdentity | null = null; beforeSocketStat.path = path; beforeSocketStat.hook = async () => { await rm(path, { force: true }); await new Promise((resolve) => competitor.listen(path, resolve)); - displaced = true; + theirs = await socketFileIdentity(path); }; - - const mod = await openWindow(fakeWindow()); - const roles: boolean[] = []; - const settled = mod.ensurePeerNet((held) => roles.push(held)); - await waitFor(() => displaced, 15_000); - // Past the whole verification: a window that confirmed would be broker now. - await tick(RECLAIM_VERIFY_MS * 2); - expect(mod.isPeerBroker()).toBe(false); - expect(roles).toEqual([]); - - // Once the competitor is gone the path is free, and the loop takes it. - const gone = new Promise((resolve) => competitor.close(resolve)); - for (const socket of held) socket.destroy(); - await gone; - await settled; - expect(mod.isPeerBroker()).toBe(true); + try { + const mod = await openWindow(fakeWindow()); + const roles: boolean[] = []; + await mod.ensurePeerNet((broker) => roles.push(broker)); + expect(theirs).not.toBeNull(); + // Settled as broker either way — standing down closes our server, which + // unlinks the path (libuv does, whatever it names now), so the next round + // binds uncontested. What tells the two apart is whose socket the path + // names at the verdict: a window that confirmed on the competitor's + // socket is a broker nobody can reach. + expect(roles).toEqual([true]); + expect(sameSocketFile(await socketFileIdentity(path), theirs!)).toBe(false); + const peer = await openWindow(fakeWindow({ entries: [{ surfaceId: 'far-1' }] })); + await peer.ensurePeerNet(() => {}); + expect(peer.isPeerBroker()).toBe(false); + } finally { + for (const socket of held) socket.destroy(); + if (competitor.listening) await new Promise((resolve) => competitor.close(resolve)); + } }, 30_000); it('collects directory entries from the other window', async () => { From abb4e261c9556364ebebed8384d81d8e8496f299 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 17:41:49 -0700 Subject: [PATCH 4/5] test(vscode-ext): point the unlink-on-close note at #756 Co-Authored-By: Claude Opus 5.5 (1M context) --- vscode-ext/test/peer-link.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index 15514ce5..1913b019 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -511,8 +511,8 @@ describe('bind-as-lease', () => { await mod.ensurePeerNet((broker) => roles.push(broker)); expect(theirs).not.toBeNull(); // Settled as broker either way — standing down closes our server, which - // unlinks the path (libuv does, whatever it names now), so the next round - // binds uncontested. What tells the two apart is whose socket the path + // unlinks the path (libuv does, whatever it names now; #756), so the next + // round binds uncontested. What tells the two apart is whose socket the path // names at the verdict: a window that confirmed on the competitor's // socket is a broker nobody can reach. expect(roles).toEqual([true]); From 47ba3ac34c6d4633cc7e963871493fd9b45d49a3 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 22 Sep 2026 18:01:31 -0700 Subject: [PATCH 5/5] fix(vscode-ext): take no bind anchor on Windows by construction Co-Authored-By: Claude Opus 5.5 (1M context) --- vscode-ext/src/peer-link.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index beac72eb..64ab6712 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -801,8 +801,10 @@ async function tryBind(path: string, token: string): Promise { // *this* process can interleave. A competing window is a separate extension // host, so its unlink and rebind can still land here — this narrows the gap // to a few microseconds of straight-line code rather than closing it, and - // reads the file our own bind made ({@link boundSocketFile}). - boundSocketFile = socketFileIdentitySync(path); + // reads the file our own bind made ({@link boundSocketFile}). Skipped on + // Windows: a named pipe is not a filesystem object, so there is no anchor + // to take and nothing there can displace us ({@link stillOurs}). + boundSocketFile = process.platform === 'win32' ? null : socketFileIdentitySync(path); // Provisional until the caller settles it: a reclaimed bind may still be // displaced (see {@link brokerConfirmed}). brokerConfirmed = false;