diff --git a/package-lock.json b/package-lock.json index 53ffeff..71a471a 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 7f07def..859fb56 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 8a12172..6276ecc 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 1d5ec84..97dfae0 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 f4ea43c..ded0a72 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,23 +55,30 @@ 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; } 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); - const entryRow = findInstanceForRepo(hash); - if (entryRow && await checkInstanceHealth(entryRow.port)) { - return entryRow.port; + 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); + if (own && await checkInstanceHealth(own.port)) { + return own.port; } } try { if (child.pid) process.kill(child.pid, 'SIGTERM'); } catch { /* already gone */ } @@ -89,9 +96,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 e67d951..999ef08 100644 --- a/packages/cli/src/registry.ts +++ b/packages/cli/src/registry.ts @@ -252,6 +252,57 @@ export function checkInstanceHealth(port: number): Promise { }); } +/** + * 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<'serves' | 'other' | 'silent'> { + 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 { + const root = (JSON.parse(body) as { root?: unknown }).root; + resolve(res.statusCode !== 200 || typeof root !== 'string' ? 'silent' : root === repoRoot ? 'serves' : 'other'); + } catch { + resolve('silent'); + } + }); + }); + req.on('error', () => resolve('silent')); + req.setTimeout(2000, () => { + req.destroy(); + resolve('silent'); + }); + }); +} + +/** + * The registered server for a repository, if it is still that server: alive, and serving that root. + * 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)) { + 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 { // 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 5b5ce51..a093cfa 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 96a258b..ec44b77 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 b4dc7a8..5448216 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,20 +150,51 @@ 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; } }, 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 2f6ec77..ec6d1dc 100644 --- a/packages/cli/tests/registry.test.ts +++ b/packages/cli/tests/registry.test.ts @@ -115,4 +115,65 @@ 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('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' })); + 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 0daebbc..b8fe39c 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 13b2b56..dd1b285 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 ce76889..fdc9ac1 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 62b5675..9cd5d6a 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": {