From f46329b0af7be6fe7885cf55b119ea351ba42a1c Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:31:00 +0000 Subject: [PATCH 1/3] fix(vscode-ext): wait out a half-created peer token instead of adopting an empty one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureToken` returned `(await readFile(path)).trim()` without checking the result was non-empty. `writeFile(..., { flag: 'wx' })` creates the file before it writes the bytes, so a second window reading in that gap gets `''`. If that window then wins the bind, `serverToken = ''` and `onServerFrame`'s `!serverToken` rejects every hello — permanently, since a broker never re-reads the token, while every other window retries at 1 Hz and is never served. An empty read is now treated as "not written yet" and waited out; a file that stays empty past the wait takes the existing stand-down path, which logs the reason rather than silently brokering for nobody. --- docs/specs/vscode.md | 2 +- vscode-ext/src/peer-link.ts | 42 ++++++++++++++++++++++++++----- vscode-ext/test/peer-link.test.ts | 38 ++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/docs/specs/vscode.md b/docs/specs/vscode.md index 4668a538..b7794c63 100644 --- a/docs/specs/vscode.md +++ b/docs/specs/vscode.md @@ -281,7 +281,7 @@ The invariants are what make this simpler than the heartbeat lease it replaced: *The directory.* On unix the sockets live in a `dormouse-peer-` directory created 0700, and before every bind and every connect it is `lstat`-ed and required to be a directory, owned by this uid, at exactly mode 0700, and not a symlink. A loose directory we own is tightened; anything else is somebody else's, no retry makes it ours, and the peer link stands down for good rather than spinning against it (callers waiting on the contention are released rather than left hanging). Windows named pipes carry their own ACL and skip this layer. -*The handshake.* The shared secret is a mode-0600 `remote-host.peer-token` in `globalStorageUri`, created once with an exclusive `wx` write rather than a rename so two windows starting together agree — the loser reads the winner's token instead of overwriting it under a client that already read the old one. A `globalStorageUri` where it can be neither read nor created latches the same permanent stand-down as an unsafe socket directory, for the same reason: it is not a transient failure, and retrying at `RETRY_MS` forever would make every command wait out its whole queue budget on every attempt instead of being told there is nothing to reach. It **never crosses the socket**. Instead three frames prove mutual knowledge of it: +*The handshake.* The shared secret is a mode-0600 `remote-host.peer-token` in `globalStorageUri`, created once with an exclusive `wx` write rather than a rename so two windows starting together agree — the loser reads the winner's token instead of overwriting it under a client that already read the old one. `wx` creates the file before it writes the bytes, so the loser's read can land on a zero-length file; an empty read is therefore treated as *not yet written* and waited out (`TOKEN_WRITE_ATTEMPTS` × `TOKEN_WRITE_POLL_MS`) rather than taken as the token. That distinction is load-bearing rather than cosmetic: an empty `serverToken` fails the hello check below for every peer, and a broker never re-reads the token, so a window that adopted `''` would refuse the whole installation for its lifetime while every other window retried at `RETRY_MS` forever. A file that stays empty past the wait — a crash mid-write — takes the stand-down path instead, which at least names the reason in the log. A `globalStorageUri` where the token can be neither read nor created latches the same permanent stand-down as an unsafe socket directory, for the same reason: it is not a transient failure, and retrying at `RETRY_MS` forever would make every command wait out its whole queue budget on every attempt instead of being told there is nothing to reach. It **never crosses the socket**. Instead three frames prove mutual knowledge of it: 1. `challenge { nonce }` — the *server* speaks first, on accept. A client that has not yet seen proof of the token must not volunteer one into whatever bound the path. 2. `hello { nonce, proof }` — the client answers with `HMAC-SHA256(token, "client:" + serverNonce)` and a fresh nonce of its own. diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index 36c67ca5..136c7003 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -125,6 +125,14 @@ export function configurePeerLink(next: PeerLinkDeps): void { const TOKEN_FILE = 'remote-host.peer-token'; +/** + * How long a window losing the exclusive create will wait for the winner's + * bytes ({@link readSharedToken}). The write is one `writeFile` of a UUID, so + * the gap it covers is microseconds — this is sized to be unmissable, not tuned. + */ +const TOKEN_WRITE_ATTEMPTS = 10; +const TOKEN_WRITE_POLL_MS = 20; + /** Floor between contention attempts, so a refused hello cannot become a spin. */ const RETRY_MS = 1_000; @@ -212,6 +220,21 @@ function socketPath(): string | null { : join(peerDirPath(), `${id}.sock`); } +/** + * Read the shared token, treating a file that is present but empty as *not yet + * written* rather than as a token. + * + * `writeFile(..., { flag: 'wx' })` creates the file before it writes the bytes, + * so a window reading in that gap sees zero length. Returning `''` from there + * would be unrecoverable rather than merely wrong: an empty `serverToken` makes + * {@link onServerFrame}'s `!serverToken` reject every hello, and a broker never + * re-reads the token, so every other window retries at 1 Hz and is never served. + */ +async function readSharedToken(path: string): Promise { + const raw = await readFile(path, 'utf8').catch(() => null); + return raw && raw.trim() ? raw.trim() : null; +} + /** * The shared secret, created once per installation and reused forever. Written * with an exclusive create rather than a rename, so two windows starting @@ -221,11 +244,8 @@ function socketPath(): string | null { async function ensureToken(): Promise { const path = tokenPath(); if (!path) throw new Error('peer link has no storage location'); - try { - return (await readFile(path, 'utf8')).trim(); - } catch { - // Missing (the common first run) — fall through and create it. - } + const existing = await readSharedToken(path); + if (existing) return existing; await mkdir(join(path, '..'), { recursive: true }).catch(() => {}); const token = randomUUID(); try { @@ -234,7 +254,17 @@ async function ensureToken(): Promise { await writeFile(path, token, { encoding: 'utf8', mode: 0o600, flag: 'wx' }); return token; } catch { - return (await readFile(path, 'utf8')).trim(); + // `EEXIST` — another window created it, and the empty read above may have + // been that same window mid-write, so wait the bytes out. Bounded, because + // a token file left zero-length by a crash never fills in: throwing hands + // the caller its existing stand-down path, which at least says so in the + // log, rather than a broker that silently refuses every peer forever. + for (let attempt = 0; attempt < TOKEN_WRITE_ATTEMPTS; attempt++) { + const written = await readSharedToken(path); + if (written) return written; + await delay(TOKEN_WRITE_POLL_MS); + } + throw new Error('peer link token file is empty'); } } diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index 6eea4b2e..b46f82d9 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -286,6 +286,44 @@ describe('bind-as-lease', () => { expect(mod.isPeerBroker()).toBe(false); }); + it('waits for the winner’s bytes rather than reading a half-created token as empty', async () => { + // `writeFile(..., { flag: 'wx' })` creates the file before it writes the + // bytes. A window reading in that gap used to take `''` as the token, and + // an empty `serverToken` rejects every hello for the life of that broker — + // which never re-reads it, so the installation does not recover. + const path = join(dir, 'remote-host.peer-token'); + await writeFile(path, '', { mode: 0o600 }); + + const mod = await openWindow(fakeWindow()); + const settled = mod.ensurePeerNet(() => {}); + // Not yet decided, so the bytes below land inside the wait rather than + // before the read — without which this would pass vacuously. + await tick(); + expect(mod.isPeerLinkSettled()).toBe(false); + await writeFile(path, 'the-winners-token', { mode: 0o600 }); + await settled; + + expect(mod.isPeerBroker()).toBe(true); + // The broker serves over the token that was actually written, so a hello + // proved with it is accepted. + expect(await readToken()).toBe('the-winners-token'); + }, 30_000); + + it('stands down rather than brokering on a token file that stays empty', async () => { + // A zero-length token left by a crash never fills in. Binding anyway makes + // this window the broker every other one dials, and it then refuses all of + // them silently; the stand-down at least names the reason in the log. + await writeFile(join(dir, 'remote-host.peer-token'), '', { mode: 0o600 }); + + const mod = await openWindow(fakeWindow()); + const roles: boolean[] = []; + await mod.ensurePeerNet((held) => roles.push(held)); + + expect(roles).toEqual([]); + expect(mod.isPeerBroker()).toBe(false); + expect(mod.isPeerLinkSettled()).toBe(true); + }, 30_000); + it('makes the first window to bind the broker and the second a client', async () => { const { broker, peer } = await linkedPair(); expect(broker.isPeerBroker()).toBe(true); From 4d24493bed8879ee70ab5bc30d1d7f9fba1e6dee Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:40:27 +0000 Subject: [PATCH 2/3] fix(vscode-ext): re-derive why the token wait was exhausted instead of asserting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `open(O_CREAT|O_EXCL)` on a token path that is a directory fails with EEXIST, not EISDIR, so an unreadable globalStorageUri reaches the wait loop and used to throw "is empty" — swallowing the cause the caller's log line exists to name. --- docs/specs/vscode.md | 2 +- vscode-ext/src/peer-link.ts | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/docs/specs/vscode.md b/docs/specs/vscode.md index b7794c63..562293e4 100644 --- a/docs/specs/vscode.md +++ b/docs/specs/vscode.md @@ -281,7 +281,7 @@ The invariants are what make this simpler than the heartbeat lease it replaced: *The directory.* On unix the sockets live in a `dormouse-peer-` directory created 0700, and before every bind and every connect it is `lstat`-ed and required to be a directory, owned by this uid, at exactly mode 0700, and not a symlink. A loose directory we own is tightened; anything else is somebody else's, no retry makes it ours, and the peer link stands down for good rather than spinning against it (callers waiting on the contention are released rather than left hanging). Windows named pipes carry their own ACL and skip this layer. -*The handshake.* The shared secret is a mode-0600 `remote-host.peer-token` in `globalStorageUri`, created once with an exclusive `wx` write rather than a rename so two windows starting together agree — the loser reads the winner's token instead of overwriting it under a client that already read the old one. `wx` creates the file before it writes the bytes, so the loser's read can land on a zero-length file; an empty read is therefore treated as *not yet written* and waited out (`TOKEN_WRITE_ATTEMPTS` × `TOKEN_WRITE_POLL_MS`) rather than taken as the token. That distinction is load-bearing rather than cosmetic: an empty `serverToken` fails the hello check below for every peer, and a broker never re-reads the token, so a window that adopted `''` would refuse the whole installation for its lifetime while every other window retried at `RETRY_MS` forever. A file that stays empty past the wait — a crash mid-write — takes the stand-down path instead, which at least names the reason in the log. A `globalStorageUri` where the token can be neither read nor created latches the same permanent stand-down as an unsafe socket directory, for the same reason: it is not a transient failure, and retrying at `RETRY_MS` forever would make every command wait out its whole queue budget on every attempt instead of being told there is nothing to reach. It **never crosses the socket**. Instead three frames prove mutual knowledge of it: +*The handshake.* The shared secret is a mode-0600 `remote-host.peer-token` in `globalStorageUri`, created once with an exclusive `wx` write rather than a rename so two windows starting together agree — the loser reads the winner's token instead of overwriting it under a client that already read the old one. `wx` creates the file before it writes the bytes, so the loser's read can land on a zero-length file; an empty read is therefore treated as *not yet written* and waited out (`TOKEN_WRITE_ATTEMPTS` × `TOKEN_WRITE_POLL_MS`) rather than taken as the token. That distinction is load-bearing rather than cosmetic: an empty `serverToken` fails the hello check below for every peer, and a broker never re-reads the token, so a window that adopted `''` would refuse the whole installation for its lifetime while every other window retried at `RETRY_MS` forever. A file still unreadable past the wait takes the stand-down path instead, and latches it: the exclusive create fails with `EEXIST` for a token path that is a *directory* as much as for one another window owns, so exhausting the wait means either a crash-left zero-length file or a `globalStorageUri` this process cannot read — neither of which a retry fixes. The throw re-derives which it was, because the caller's log line is the only diagnosis any of them gets. A `globalStorageUri` where the token can be neither read nor created latches the same permanent stand-down as an unsafe socket directory, for the same reason: it is not a transient failure, and retrying at `RETRY_MS` forever would make every command wait out its whole queue budget on every attempt instead of being told there is nothing to reach. It **never crosses the socket**. Instead three frames prove mutual knowledge of it: 1. `challenge { nonce }` — the *server* speaks first, on accept. A client that has not yet seen proof of the token must not volunteer one into whatever bound the path. 2. `hello { nonce, proof }` — the client answers with `HMAC-SHA256(token, "client:" + serverNonce)` and a fresh nonce of its own. diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index 136c7003..659cdaa7 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -264,7 +264,15 @@ async function ensureToken(): Promise { if (written) return written; await delay(TOKEN_WRITE_POLL_MS); } - throw new Error('peer link token file is empty'); + // Not only the crash-left empty file: `open(O_CREAT|O_EXCL)` on a token + // path that is a directory — or that we cannot read — is `EEXIST` too, so + // those land here as well. The caller's log line is the only diagnosis any + // of them gets, so re-derive which it was rather than asserting one. + const why = await readFile(path, 'utf8').then( + () => 'is empty', + (error: unknown) => `could not be read: ${String(error)}`, + ); + throw new Error(`peer link token file ${path} ${why}`); } } From 04ff87cd7f80380e045c7cfbc3ffcd2d13a20034 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:43:20 +0000 Subject: [PATCH 3/3] test(vscode-ext): lock in the stand-down reason, and name the create error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The re-derived reason had no coverage: the directory case and the crash-left-empty case share one branch, and only the log tells them apart, so the tests now read the output channel and assert each one names its own cause. Reverting the re-derivation fails the directory assertion with 'peer link token file is empty'. The read-failure arm also carries the create error now — an unwritable globalStorageUri fails the create with EACCES and then the read with ENOENT, and the read alone names the missing file rather than why. --- vscode-ext/src/peer-link.ts | 8 ++++++-- vscode-ext/test/peer-link.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/vscode-ext/src/peer-link.ts b/vscode-ext/src/peer-link.ts index 659cdaa7..94f7b394 100644 --- a/vscode-ext/src/peer-link.ts +++ b/vscode-ext/src/peer-link.ts @@ -253,7 +253,7 @@ async function ensureToken(): Promise { // installation's terminals, so it is never briefly world-readable. await writeFile(path, token, { encoding: 'utf8', mode: 0o600, flag: 'wx' }); return token; - } catch { + } catch (writeError) { // `EEXIST` — another window created it, and the empty read above may have // been that same window mid-write, so wait the bytes out. Bounded, because // a token file left zero-length by a crash never fills in: throwing hands @@ -270,7 +270,11 @@ async function ensureToken(): Promise { // of them gets, so re-derive which it was rather than asserting one. const why = await readFile(path, 'utf8').then( () => 'is empty', - (error: unknown) => `could not be read: ${String(error)}`, + // The create error rides along only here: an unwritable `globalStorageUri` + // fails the create with `EACCES` and then the read with `ENOENT`, and the + // read alone would name the missing file rather than why it is missing. + (error: unknown) => + `could not be read: ${String(error)}; creating it failed with ${String(writeError)}`, ); throw new Error(`peer link token file ${path} ${why}`); } diff --git a/vscode-ext/test/peer-link.test.ts b/vscode-ext/test/peer-link.test.ts index b46f82d9..7681ce2a 100644 --- a/vscode-ext/test/peer-link.test.ts +++ b/vscode-ext/test/peer-link.test.ts @@ -34,6 +34,20 @@ import { type LinkModule = typeof import('../src/peer-link'); +/** + * The output channel is the only diagnosis a permanent stand-down gets, so the + * tests below read it. Hoisted, because `freshModule` resets the registry and + * the factory re-runs — the array has to outlive that. + */ +const logged = vi.hoisted(() => [] as string[]); +vi.mock('../src/log', () => ({ + log: { + init() {}, + info: (...args: unknown[]) => void logged.push(`[info] ${args.map(String).join(' ')}`), + error: (...args: unknown[]) => void logged.push(`[error] ${args.map(String).join(' ')}`), + }, +})); + let dir: string; /** Peer sockets live in the temp dir; point that at this test's own storage. */ let realTmp: string | undefined; @@ -118,6 +132,7 @@ beforeEach(async () => { dir = await tempStorageDir(); realTmp = process.env.TMPDIR; process.env.TMPDIR = dir; + logged.length = 0; }); afterEach(async () => { @@ -284,6 +299,11 @@ describe('bind-as-lease', () => { expect(mod.isPeerLinkSettled()).toBe(true); await mod.ensurePeerNet(() => {}); expect(mod.isPeerBroker()).toBe(false); + // A directory fails the exclusive create with `EEXIST`, same as a window + // that got there first, so this arrives by way of the mid-write wait. The + // reason still has to name what is actually wrong with the path rather + // than the empty-file case that shares the branch. + expect(logged.join('\n')).toContain('EISDIR'); }); it('waits for the winner’s bytes rather than reading a half-created token as empty', async () => { @@ -322,6 +342,9 @@ describe('bind-as-lease', () => { expect(roles).toEqual([]); expect(mod.isPeerBroker()).toBe(false); expect(mod.isPeerLinkSettled()).toBe(true); + // The readable-but-empty case is the one that really is empty, and it says + // so — the two arrivals at this branch are told apart in the log. + expect(logged.join('\n')).toContain('is empty'); }, 30_000); it('makes the first window to bind the broker and the second a client', async () => {