diff --git a/docs/specs/vscode.md b/docs/specs/vscode.md index 08f3bc725..9dcb92dd1 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 bf29e05fc..d5febf23c 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 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/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 9695d7096..2d698d5f2 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 59d114adc..64ab6712b 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,18 @@ 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 an async read lands, and then both reads name that + * window's socket, the two agree, and every window that bound believes it + * won. Captured synchronously after the bind, which narrows that gap without + * closing it (see `tryBind`). + */ +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 +796,15 @@ 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 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}). 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; @@ -1243,11 +1265,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 +1288,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 +1328,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(); diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index 6d3a593ec..1913b0199 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -48,6 +48,26 @@ 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; @@ -133,6 +153,7 @@ beforeEach(async () => { realTmp = process.env.TMPDIR; process.env.TMPDIR = dir; logged.length = 0; + beforeSocketStat.hook = null; }); afterEach(async () => { @@ -457,6 +478,54 @@ 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, standing in for a window + // that won the reclaim. + const held: Socket[] = []; + const competitor = createServer((socket) => void held.push(socket)); + let theirs: SocketFileIdentity | null = null; + beforeSocketStat.path = path; + beforeSocketStat.hook = async () => { + await rm(path, { force: true }); + await new Promise((resolve) => competitor.listen(path, resolve)); + theirs = await socketFileIdentity(path); + }; + 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; #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]); + 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 () => { const peerSide = fakeWindow({ entries: [{ surfaceId: 'far-1' }, { surfaceId: 'far-2' }] }); const { broker } = await linkedPair(fakeWindow(), peerSide);