From 8ca9c7064156385f5c1b6123fc3c72ea73522bf0 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5.1)" Date: Thu, 3 Sep 2026 15:27:20 +0200 Subject: [PATCH 1/2] fix: a registered server is trusted only once it has said which repository it serves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A registry knows only its own data directory, so the port a dead server left behind is the first one a prepare server — running in a data directory of its own — picks up. The global registry still named that port for the dead server's worktree, and both the inbox's open route and the agent CLI's server lookup checked no more than that something answered there: one pull request's page came up serving another's checkout and prepared review, with the first one's bundle imported into it. An entry is now used only when its process is alive and the server on its port reports that repository root; a stale one is dropped from the registry. A freshly spawned server is waited for by its own pid, never by a row another server left for the same worktree. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- package-lock.json | 12 ++--- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/cli/src/agent-session.ts | 11 ++--- packages/cli/src/agent.ts | 6 +-- packages/cli/src/inbox/open-session.ts | 33 +++++++++----- packages/cli/src/registry.ts | 45 +++++++++++++++++++ .../tests/agent-session-precedence.test.ts | 8 +++- packages/cli/tests/agent-session.test.ts | 25 +++++++++++ packages/cli/tests/inbox-open.test.ts | 22 ++++++++- packages/cli/tests/registry.test.ts | 44 ++++++++++++++++++ packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 15 files changed, 183 insertions(+), 35 deletions(-) diff --git a/package-lock.json b/package-lock.json index 53ffeff0..71a471a1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.15", + "version": "0.10.16", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.15", + "version": "0.10.16", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.15", + "version": "0.10.16", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.15", + "version": "0.10.16", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.15", + "version": "0.10.16", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.15", + "version": "0.10.16", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index 7f07def8..859fb563 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.15", + "version": "0.10.16", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 8a12172b..6276ecc6 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.15", + "version": "0.10.16", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/cli/src/agent-session.ts b/packages/cli/src/agent-session.ts index 1d5ec846..97dfae00 100644 --- a/packages/cli/src/agent-session.ts +++ b/packages/cli/src/agent-session.ts @@ -1,7 +1,7 @@ import { createHash } from 'node:crypto'; import { AGENT_TRAFFIC_HEADER, type ReviewSession } from '@diffity/api'; import { getRepoRoot } from '@diffity/git'; -import { findInstanceForRepo, type RegistryEntry } from './registry.js'; +import { findServingInstance, type RegistryEntry } from './registry.js'; import { getCurrentSession, getSessionById } from './session.js'; /** How the server tells the agent's own traffic from a page somebody is looking at. */ @@ -10,12 +10,13 @@ export const AGENT_HEADER: Record = { [AGENT_TRAFFIC_HEADER]: '1 /** The registry's own health probe allows the same; a wedged server must not hang every command. */ export const SERVER_TIMEOUT_MS = 2000; -export function findRunningInstance(): RegistryEntry | null { +/** The server registered for this repository, once it has confirmed it is serving this repository. */ +export function findServingInstanceForCwd(): Promise { const repoRoot = getRepoRoot(); if (!repoRoot) { - return null; + return Promise.resolve(null); } - return findInstanceForRepo(createHash('sha256').update(repoRoot).digest('hex').slice(0, 12)); + return findServingInstance(createHash('sha256').update(repoRoot).digest('hex').slice(0, 12), repoRoot); } /** @@ -84,7 +85,7 @@ export async function resolveAgentSession(explicitId?: string): Promise', `How long to wait before giving up (each poll caps at ${CLIENT_WAIT_CAP_SECONDS}s and returns; call again to keep waiting)`, '900') .action(async (opts: { timeout: string }) => { const session = await requireSession(agent.opts().session); - const instance = findRunningInstance(); + const instance = await findServingInstanceForCwd(); if (!instance) { console.error(pc.red('No diffity is running for this repository — start one first')); process.exitCode = 1; @@ -481,7 +481,7 @@ Examples: .option('--json', 'Output as JSON') .action(async (opts: { json?: boolean }) => { await requireSession(agent.opts().session); - const instance = findRunningInstance(); + const instance = await findServingInstanceForCwd(); const status = instance ? await fetchLiveStatus(instance.port) : null; if (opts.json) { diff --git a/packages/cli/src/inbox/open-session.ts b/packages/cli/src/inbox/open-session.ts index f4ea43c0..fc9d1e04 100644 --- a/packages/cli/src/inbox/open-session.ts +++ b/packages/cli/src/inbox/open-session.ts @@ -2,7 +2,7 @@ import { spawn, execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { readFileSync, realpathSync } from 'node:fs'; import { createHash } from 'node:crypto'; -import { checkInstanceHealth, findInstanceForRepo } from '../registry.js'; +import { checkInstanceHealth, findServingInstance, readRegistry } from '../registry.js'; /** * Brings a prepared review up as a live diffity session the reviewer can open: a server over the @@ -55,11 +55,11 @@ export function realOpenSessionDeps(nodePath: string, entry: string): OpenSessio } export async function ensureServer(nodePath: string, entry: string, worktree: string, ref: string, prNumber: number | undefined, waitMs = 30_000): Promise { - const hash = repoHash(worktree); - // A healthy server already on this worktree is reused as-is. The worktree lives under the inbox's - // own directory and is only ever served at the pull request's base, so its ref is the one wanted. - const existing = findInstanceForRepo(hash); - if (existing && await checkInstanceHealth(existing.port)) { + // A server already registered for this worktree is reused only once it has confirmed it is the + // one registered: alive, and serving this worktree. The worktree lives under the inbox's own + // directory and is only ever served at the pull request's base, so a genuine one is the one wanted. + const existing = await findServingInstance(repoHash(worktree), resolvedRoot(worktree)); + if (existing) { return existing.port; } @@ -69,9 +69,11 @@ export async function ensureServer(nodePath: string, entry: string, worktree: st const deadline = Date.now() + waitMs; while (Date.now() < deadline) { await sleep(400); - const entryRow = findInstanceForRepo(hash); - if (entryRow && await checkInstanceHealth(entryRow.port)) { - return entryRow.port; + // The child's own registration, by pid: a row for the same worktree left by an earlier server + // must not stand in for it. + const own = readRegistry().find(row => row.pid === child.pid); + if (own && await checkInstanceHealth(own.port)) { + return own.port; } } try { if (child.pid) process.kill(child.pid, 'SIGTERM'); } catch { /* already gone */ } @@ -89,9 +91,16 @@ export function serverArgs(entry: string, worktree: string, ref: string, prNumbe /** The server registers under the hash of its resolved repo root, so resolve symlinks before hashing. */ export function repoHash(worktree: string): string { - let root = worktree; - try { root = realpathSync(worktree); } catch { /* not yet on disk; hash the path as given */ } - return createHash('sha256').update(root).digest('hex').slice(0, 12); + return createHash('sha256').update(resolvedRoot(worktree)).digest('hex').slice(0, 12); +} + +/** The path as the server will report it; a worktree not yet on disk is taken as given. */ +function resolvedRoot(worktree: string): string { + try { + return realpathSync(worktree); + } catch { + return worktree; + } } function sleep(ms: number): Promise { diff --git a/packages/cli/src/registry.ts b/packages/cli/src/registry.ts index e67d9514..fb02dc77 100644 --- a/packages/cli/src/registry.ts +++ b/packages/cli/src/registry.ts @@ -252,6 +252,51 @@ export function checkInstanceHealth(port: number): Promise { }); } +/** + * Whether the server on a port is the one an entry describes: it answers /api/info naming that + * repository root. Ports are reused, and a registry that knows only its own data directory can hand + * a freed port to another server — so an entry is trusted only once the server on its port has said + * whose it is. + */ +export function instanceServes(port: number, repoRoot: string): Promise { + return new Promise((resolve) => { + const req = get(`http://localhost:${port}/api/info`, { headers: { [AGENT_TRAFFIC_HEADER]: '1' } }, (res) => { + let body = ''; + res.setEncoding('utf-8'); + res.on('data', (chunk: string) => { body += chunk; }); + res.on('end', () => { + try { + resolve(res.statusCode === 200 && (JSON.parse(body) as { root?: unknown }).root === repoRoot); + } catch { + resolve(false); + } + }); + }); + req.on('error', () => resolve(false)); + req.setTimeout(2000, () => { + req.destroy(); + resolve(false); + }); + }); +} + +/** + * The registered server for a repository, if it is still that server: alive, and serving that root. + * A stale entry — a dead pid, or a port another server has taken since — is dropped from the + * registry, so nothing else trusts it either. + */ +export async function findServingInstance(repoHash: string, repoRoot: string): Promise { + const entry = findInstanceForRepo(repoHash); + if (!entry) { + return null; + } + if (entryIsAlive(entry) && await instanceServes(entry.port, repoRoot)) { + return entry; + } + deregisterInstance(entry.pid); + return null; +} + export function killInstance(entry: RegistryEntry): void { // A reused pid is somebody else's process; deregistering is all there is left to do. if (entryIsAlive(entry)) { diff --git a/packages/cli/tests/agent-session-precedence.test.ts b/packages/cli/tests/agent-session-precedence.test.ts index 5b5ce512..a093cfa3 100644 --- a/packages/cli/tests/agent-session-precedence.test.ts +++ b/packages/cli/tests/agent-session-precedence.test.ts @@ -105,6 +105,12 @@ describe('resolveAgentSession', () => { expect(getCurrentSession()?.id).toBe(decoy.id); stub = createServer((req, res) => { + // A server is trusted only once its info route names the repository it serves. + if (req.url === '/api/info' && req.method === 'GET') { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ name: 'repo', root: repoDir })); + return; + } if (req.url === '/api/sessions/ensure' && req.method === 'POST') { res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(other)); @@ -133,7 +139,7 @@ describe('resolveAgentSession', () => { stub = createServer((req, res) => { if (req.url === '/api/info' && req.method === 'GET') { res.writeHead(200, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ name: 'repo', sessionId: named.id })); + res.end(JSON.stringify({ name: 'repo', root: repoDir, sessionId: named.id })); return; } // An old server answers an unknown route with the page and a 200. diff --git a/packages/cli/tests/agent-session.test.ts b/packages/cli/tests/agent-session.test.ts index 96a258b6..ec44b778 100644 --- a/packages/cli/tests/agent-session.test.ts +++ b/packages/cli/tests/agent-session.test.ts @@ -79,4 +79,29 @@ describe('getSessionById', () => { expect(after.id).not.toBe(before.id); expect(getSessionById(before.id)?.id).toBe(after.id); }); + + it('does not take a session from a server that turns out to serve another repository', async () => { + const { createHash } = await import('node:crypto'); + const { createServer } = await import('node:http'); + const { registerInstance, readRegistry } = await import('../src/registry.js'); + const { resolveAgentSession } = await import('../src/agent-session.js'); + const { getRepoRoot } = await import('@diffity/git'); + + const impostor = createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ id: 'not-yours', ref: 'work', headHash: 'x', root: '/somewhere/else' })); + }); + await new Promise(resolve => impostor.listen(0, '127.0.0.1', () => resolve())); + const port = (impostor.address() as { port: number }).port; + const repoRoot = getRepoRoot()!; + const repoHash = createHash('sha256').update(repoRoot).digest('hex').slice(0, 12); + registerInstance({ pid: process.pid, port, repoRoot, repoHash, repoName: 'repo', ref: 'work', description: '', startedAt: new Date().toISOString() }); + try { + const session = await resolveAgentSession(); + expect(session?.id).not.toBe('not-yours'); + expect(readRegistry().find(e => e.port === port)).toBeUndefined(); + } finally { + impostor.close(); + } + }); }); diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index b4dc7a8a..23682805 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { request } from 'node:http'; import { execFileSync, spawn, type ChildProcess } from 'node:child_process'; -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; +import { createServer } from 'node:http'; import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; @@ -149,14 +150,31 @@ describe('the real ensureServer', () => { execFileSync('git', ['add', '.'], { cwd: repo, stdio: 'pipe' }); execFileSync('git', ['commit', '-m', 'init'], { cwd: repo, stdio: 'pipe' }); + // Another server — alive, answering — has taken the port a registered entry for this repo still + // names, which is how one pull request's page came to show another's code. + const impostor = createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ name: 'other', branch: 'main', root: join(root, 'somebody-else') })); + }); + await new Promise(resolve => impostor.listen(0, '127.0.0.1', () => resolve())); + const impostorPort = (impostor.address() as { port: number }).port; + registerInstance({ + pid: process.pid, port: impostorPort, repoRoot: realpathSync(repo), repoHash: repoHash(repo), repoName: 'repo', + ref: 'work', description: '', startedAt: new Date().toISOString(), + }); + let port = 0; try { port = await ensureServer(process.execPath, ENTRY, repo, 'work', undefined, 20_000); - // The entry the server registered must carry the hash open-session looks it up by. + expect(port).not.toBe(impostorPort); + // The entry the server registered must carry the hash open-session looks it up by, and the + // impostor's row is gone. const entry = readRegistry().find(e => e.port === port); expect(entry).toBeDefined(); expect(entry!.repoHash).toBe(repoHash(repo)); + expect(readRegistry().find(e => e.port === impostorPort)).toBeUndefined(); } finally { + impostor.close(); const entry = readRegistry().find(e => e.port === port); if (entry) { try { process.kill(entry.pid, 'SIGKILL'); } catch { /* gone */ } } if (prev === undefined) delete process.env.DIFFITY_DATA_DIR; else process.env.DIFFITY_DATA_DIR = prev; diff --git a/packages/cli/tests/registry.test.ts b/packages/cli/tests/registry.test.ts index 2f6ec775..ee5fc594 100644 --- a/packages/cli/tests/registry.test.ts +++ b/packages/cli/tests/registry.test.ts @@ -115,4 +115,48 @@ describe('the registry lock', () => { expect(wall).toBeGreaterThanOrEqual(2500); expect((cpu.user + cpu.system) / 1000).toBeLessThan(wall / 2); }); + +describe('findServingInstance', () => { + async function serveInfo(root: string): Promise<{ port: number; close(): void }> { + const { createServer } = await import('node:http'); + const server = createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ name: 'x', branch: 'main', root })); + }); + await new Promise(resolve => server.listen(0, '127.0.0.1', () => resolve())); + const { port } = server.address() as { port: number }; + return { port, close: () => server.close() }; + } + + it('trusts an alive entry whose server names that root', async () => { + const { registerInstance, findServingInstance, deregisterInstance } = await import('../src/registry.js'); + const info = await serveInfo('/tmp/repo'); + registerInstance(entry({ port: info.port, repoHash: 'serving1' })); + try { + expect((await findServingInstance('serving1', '/tmp/repo'))?.port).toBe(info.port); + } finally { + info.close(); + deregisterInstance(process.pid); + } + }); + + it('drops an entry whose port another server has taken', async () => { + const { registerInstance, findServingInstance, readRegistry } = await import('../src/registry.js'); + const info = await serveInfo('/tmp/somebody-else'); + registerInstance(entry({ port: info.port, repoHash: 'serving2' })); + try { + expect(await findServingInstance('serving2', '/tmp/repo')).toBeNull(); + expect(readRegistry().find(e => e.repoHash === 'serving2')).toBeUndefined(); + } finally { + info.close(); + } + }); + + it('drops an entry whose process is gone', async () => { + const { registerInstance, findServingInstance, readRegistry } = await import('../src/registry.js'); + registerInstance(entry({ pid: deadPid(), port: 1, repoHash: 'serving3' })); + expect(await findServingInstance('serving3', '/tmp/repo')).toBeNull(); + expect(readRegistry().find(e => e.repoHash === 'serving3')).toBeUndefined(); + }); +}); }); diff --git a/packages/git/package.json b/packages/git/package.json index 0daebbc4..b8fe39c8 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.15", + "version": "0.10.16", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 13b2b56e..dd1b285a 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.15", + "version": "0.10.16", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index ce76889e..fdc9ac1b 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.15", + "version": "0.10.16", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 62b56750..9cd5d6aa 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.15", + "version": "0.10.16", "type": "module", "private": true, "scripts": { From 50fd95e9a4c5b810602f8e35bd08f720badb2362 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5.1)" Date: Thu, 3 Sep 2026 15:29:20 +0200 Subject: [PATCH 2/2] fix: only a certainly stale row is dropped, and a server that dies at once fails the open at once A live server that did not answer within two seconds is busy, not gone; its row is kept and it is simply not reused this time. A row is dropped when its process is gone or its port answered for another repository. The spawn wait watches the child's exit instead of polling the registry for the full deadline. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- packages/cli/src/inbox/open-session.ts | 5 ++++ packages/cli/src/registry.ts | 36 +++++++++++++++----------- packages/cli/tests/inbox-open.test.ts | 14 ++++++++++ packages/cli/tests/registry.test.ts | 17 ++++++++++++ 4 files changed, 57 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/inbox/open-session.ts b/packages/cli/src/inbox/open-session.ts index fc9d1e04..ded0a728 100644 --- a/packages/cli/src/inbox/open-session.ts +++ b/packages/cli/src/inbox/open-session.ts @@ -65,10 +65,15 @@ export async function ensureServer(nodePath: string, entry: string, worktree: st const child = spawn(nodePath, serverArgs(entry, worktree, ref, prNumber), { detached: true, stdio: 'ignore' }); child.unref(); + let exited: string | null = null; + child.once('exit', (code, signal) => { exited = signal ? `signal ${signal}` : `exit code ${code}`; }); const deadline = Date.now() + waitMs; while (Date.now() < deadline) { await sleep(400); + if (exited) { + throw new Error(`diffity ended with ${exited} before registering for ${worktree}`); + } // The child's own registration, by pid: a row for the same worktree left by an earlier server // must not stand in for it. const own = readRegistry().find(row => row.pid === child.pid); diff --git a/packages/cli/src/registry.ts b/packages/cli/src/registry.ts index fb02dc77..999ef081 100644 --- a/packages/cli/src/registry.ts +++ b/packages/cli/src/registry.ts @@ -253,12 +253,12 @@ export function checkInstanceHealth(port: number): Promise { } /** - * Whether the server on a port is the one an entry describes: it answers /api/info naming that - * repository root. Ports are reused, and a registry that knows only its own data directory can hand - * a freed port to another server — so an entry is trusted only once the server on its port has said - * whose it is. + * What the server on a port says about itself: `serves` when /api/info names that repository root, + * `other` when it names a different one, `silent` when nothing usable came back in time. Ports are + * reused, and a registry that knows only its own data directory can hand a freed port to another + * server — so an entry is trusted only once the server on its port has said whose it is. */ -export function instanceServes(port: number, repoRoot: string): Promise { +export function instanceServes(port: number, repoRoot: string): Promise<'serves' | 'other' | 'silent'> { return new Promise((resolve) => { const req = get(`http://localhost:${port}/api/info`, { headers: { [AGENT_TRAFFIC_HEADER]: '1' } }, (res) => { let body = ''; @@ -266,35 +266,41 @@ export function instanceServes(port: number, repoRoot: string): Promise res.on('data', (chunk: string) => { body += chunk; }); res.on('end', () => { try { - resolve(res.statusCode === 200 && (JSON.parse(body) as { root?: unknown }).root === repoRoot); + const root = (JSON.parse(body) as { root?: unknown }).root; + resolve(res.statusCode !== 200 || typeof root !== 'string' ? 'silent' : root === repoRoot ? 'serves' : 'other'); } catch { - resolve(false); + resolve('silent'); } }); }); - req.on('error', () => resolve(false)); + req.on('error', () => resolve('silent')); req.setTimeout(2000, () => { req.destroy(); - resolve(false); + resolve('silent'); }); }); } /** * The registered server for a repository, if it is still that server: alive, and serving that root. - * A stale entry — a dead pid, or a port another server has taken since — is dropped from the - * registry, so nothing else trusts it either. + * An entry that is certainly stale — its process gone, or its port answered for another repository — + * is dropped from the registry so nothing else trusts it. One that is alive but did not answer is a + * server that is busy, not gone: it is left registered and simply not reused this time. */ export async function findServingInstance(repoHash: string, repoRoot: string): Promise { const entry = findInstanceForRepo(repoHash); if (!entry) { return null; } - if (entryIsAlive(entry) && await instanceServes(entry.port, repoRoot)) { - return entry; + if (!entryIsAlive(entry)) { + deregisterInstance(entry.pid); + return null; } - deregisterInstance(entry.pid); - return null; + const answer = await instanceServes(entry.port, repoRoot); + if (answer === 'other') { + deregisterInstance(entry.pid); + } + return answer === 'serves' ? entry : null; } export function killInstance(entry: RegistryEntry): void { diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index 23682805..5448216f 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -181,6 +181,20 @@ describe('the real ensureServer', () => { } }, 30_000); + it('fails as soon as the server it started has exited', async () => { + const prev = process.env.DIFFITY_DATA_DIR; + process.env.DIFFITY_DATA_DIR = join(root, 'dead-data'); + const dying = join(root, 'dying.mjs'); + writeFileSync(dying, 'process.exit(2);\n'); + const started = Date.now(); + try { + await expect(ensureServer(process.execPath, dying, join(root, 'wt'), 'work', undefined, 20_000)).rejects.toThrow(/exit code 2/); + expect(Date.now() - started).toBeLessThan(10_000); + } finally { + if (prev === undefined) delete process.env.DIFFITY_DATA_DIR; else process.env.DIFFITY_DATA_DIR = prev; + } + }, 15_000); + it('throws when nothing registers before the deadline', async () => { const prev = process.env.DIFFITY_DATA_DIR; process.env.DIFFITY_DATA_DIR = join(root, 'empty-data'); diff --git a/packages/cli/tests/registry.test.ts b/packages/cli/tests/registry.test.ts index ee5fc594..ec6d1dc9 100644 --- a/packages/cli/tests/registry.test.ts +++ b/packages/cli/tests/registry.test.ts @@ -152,6 +152,23 @@ describe('findServingInstance', () => { } }); + it('keeps an alive entry whose server did not answer, and does not reuse it', async () => { + const { registerInstance, findServingInstance, readRegistry, deregisterInstance } = await import('../src/registry.js'); + const { createServer } = await import('node:http'); + // A port that was listening and is not any more: the server is busy or bound elsewhere, not gone. + const closed = createServer(() => {}); + await new Promise(resolve => closed.listen(0, '127.0.0.1', () => resolve())); + const { port } = closed.address() as { port: number }; + await new Promise(resolve => closed.close(() => resolve())); + registerInstance(entry({ port, repoHash: 'serving4' })); + try { + expect(await findServingInstance('serving4', '/tmp/repo')).toBeNull(); + expect(readRegistry().find(e => e.repoHash === 'serving4')).toBeDefined(); + } finally { + deregisterInstance(process.pid); + } + }); + it('drops an entry whose process is gone', async () => { const { registerInstance, findServingInstance, readRegistry } = await import('../src/registry.js'); registerInstance(entry({ pid: deadPid(), port: 1, repoHash: 'serving3' }));