diff --git a/docs/design/plan-canvas.md b/docs/design/plan-canvas.md index 0de616849..9fc5b9589 100644 --- a/docs/design/plan-canvas.md +++ b/docs/design/plan-canvas.md @@ -82,7 +82,7 @@ after 30 min, `ECC_PLAN_CANVAS_IDLE_MS`). Feedback is deliver-and-drain: queued handed to exactly one `await` call and persisted to disk until then, so nothing is lost if the poll is interrupted. -- `GET /health` — `{ok, app, version, protocolVersion, runtimeId}`; the CLI reuses a detached server only when its package, protocol, and Canvas-module fingerprint match, preventing an older same-version worktree from serving stale browser code. An OS-managed, port-scoped startup lock on a separate derived UDP endpoint serializes compatibility checks and replacement, so concurrent opens reuse the winning server without colliding with UDP traffic on the Canvas service port. The lock is released automatically when its process exits. +- `GET /health` — `{ok, app, version, protocolVersion, runtimeId}`; the CLI reuses a detached server only when its package, protocol, and Canvas-module fingerprint match, preventing an older same-version worktree from serving stale browser code. A private per-user, port-scoped ticket lock serializes compatibility checks and replacement without consuming a network endpoint, so concurrent opens reuse the winning server. Unique owner tickets make stale-process recovery safe without deleting a later caller's lock. - `GET /` — session list (ECC chrome) - `POST /api/sessions` `{file, reopen?}` — open/resume; `409 user-ended` unless `reopen` - `GET /canvas/` — editor chrome; `GET /artifact//` — rendered artifact diff --git a/docs/testing/plan-canvas-pdf-export.tdd.md b/docs/testing/plan-canvas-pdf-export.tdd.md index 6998a3388..38e849628 100644 --- a/docs/testing/plan-canvas-pdf-export.tdd.md +++ b/docs/testing/plan-canvas-pdf-export.tdd.md @@ -25,8 +25,8 @@ artifact as a real PDF file without sending the plan to an external converter. - RED: the focused server suite produced 30 passes and 2 failures because the Canvas had no Download PDF control or PDF endpoint. - GREEN: renderer unit tests pass 7/7, Plan Canvas server tests pass 35/35, - and the end-to-end review workflow passes 14/14. -- FULL SUITE: the final review-hardened implementation passes all 4,011 + and the end-to-end review workflow passes 13/13. +- FULL SUITE: the final review-hardened implementation passes all 4,010 discovered tests; hosted security reruns are recorded on PR #2894. - COVERAGE: `npm run coverage` passes 4,003/4,003 with 88.97% statements, 80.58% branches, 94.22% functions, and 88.97% lines. The Plan Canvas diff --git a/scripts/plan-canvas.js b/scripts/plan-canvas.js index af7c26287..adc306716 100755 --- a/scripts/plan-canvas.js +++ b/scripts/plan-canvas.js @@ -18,8 +18,8 @@ */ const fs = require('fs'); -const dgram = require('dgram'); const http = require('http'); +const os = require('os'); const path = require('path'); const { spawn } = require('child_process'); @@ -172,68 +172,102 @@ function sleep(ms) { return new Promise(resolve => setTimeout(resolve, ms)); } -function serverStartLockPort(port) { - const servicePort = validatePort(port); - // Keep the kernel-managed mutex independent of the TCP service endpoint. - // The rotation is one-to-one for ordinary non-privileged service ports, so - // separate Canvas ports do not contend with one another. - if (servicePort < 1024) return 49152 + servicePort; - const nonPrivilegedPortCount = 65535 - 1024 + 1; - return 1024 + ((servicePort - 1024 + Math.floor(nonPrivilegedPortCount / 2)) % nonPrivilegedPortCount); +function processIsAlive(pid) { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { + process.kill(pid, 0); + return true; + } catch (error) { + return error.code === 'EPERM'; + } +} + +function readServerStartTicket(file) { + let fd; + try { + fd = fs.openSync(file, 'r'); + const stat = fs.fstatSync(fd); + try { + const value = JSON.parse(fs.readFileSync(fd, 'utf8')); + return { ...value, mtimeMs: stat.mtimeMs, malformed: false }; + } catch { + return { mtimeMs: stat.mtimeMs, malformed: true }; + } + } finally { + if (fd !== undefined) { + try { fs.closeSync(fd); } catch { /* best-effort ticket inspection */ } + } + } +} + +function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = 60 * 1000) { + const prefix = `ecc-plan-canvas-${validatePort(port)}-`; + const entries = []; + for (const name of fs.readdirSync(lockDir)) { + if (!name.startsWith(prefix) || (!name.endsWith('.choosing') && !name.endsWith('.ticket'))) continue; + const file = path.join(lockDir, name); + let value = null; + try { + value = readServerStartTicket(file); + } catch { + // The owner may already have removed its unique ticket. + continue; + } + if (value.malformed) { + if (Date.now() - value.mtimeMs > staleAfterMs) { + try { fs.rmSync(file, { force: true }); } catch { /* already removed */ } + } + continue; + } + const stale = value.token !== ownToken && !processIsAlive(value.pid); + if (stale) { + // Ticket names contain a never-reused random owner token, so removing a + // dead owner's exact path cannot delete a later caller's live ticket. + try { fs.rmSync(file, { force: true }); } catch { /* already removed */ } + continue; + } + entries.push({ ...value, file, choosing: name.endsWith('.choosing') }); + } + return entries; } async function withServerStartLock(port, task, { timeoutMs = 15 * 1000, - dgramImpl = dgram + lockDir = path.join(os.homedir(), '.claude', 'plan-canvas', 'locks') } = {}) { - const lockPort = serverStartLockPort(port); + const lockPort = validatePort(port); + const token = `${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`; + const prefix = `ecc-plan-canvas-${lockPort}-${token}`; + const choosingFile = path.join(lockDir, `${prefix}.choosing`); + const ticketFile = path.join(lockDir, `${prefix}.ticket`); const startedAt = Date.now(); - let socket = null; - let socketError = null; + fs.mkdirSync(lockDir, { recursive: true, mode: 0o700 }); + fs.writeFileSync(choosingFile, JSON.stringify({ pid: process.pid, token }), { flag: 'wx', mode: 0o600 }); - while (true) { - try { - socket = await new Promise((resolve, reject) => { - const candidate = dgramImpl.createSocket('udp4'); - const onError = error => { - try { candidate.close(); } catch { /* failed bind has no open handle */ } - reject(error); - }; - candidate.once('error', onError); - candidate.bind(lockPort, DEFAULT_HOST, () => { - candidate.removeListener('error', onError); - candidate.on('error', error => { socketError = error; }); - candidate.unref(); - resolve(candidate); - }); - }); - break; - } catch (error) { - if (error.code !== 'EADDRINUSE') throw error; + try { + const existing = listServerStartTickets(lockDir, lockPort, token) + .filter(entry => !entry.choosing && Number.isInteger(entry.number)); + const number = existing.reduce((maximum, entry) => Math.max(maximum, entry.number), 0) + 1; + fs.writeFileSync(ticketFile, JSON.stringify({ pid: process.pid, token, number }), { flag: 'wx', mode: 0o600 }); + fs.rmSync(choosingFile, { force: true }); + + while (true) { + const entries = listServerStartTickets(lockDir, lockPort, token); + const anotherOwnerIsChoosing = entries.some(entry => entry.choosing && entry.token !== token); + const tickets = entries + .filter(entry => !entry.choosing && Number.isInteger(entry.number)) + .sort((left, right) => left.number - right.number || left.token.localeCompare(right.token)); + if (!anotherOwnerIsChoosing && tickets[0] && tickets[0].token === token) break; if (Date.now() - startedAt >= timeoutMs) { throw new Error(`timed out waiting for Plan Canvas startup lock on port ${port}`); } await sleep(50); } - } - - let result; - try { - result = await task(); + return await task(); } finally { - if (socket) { - await new Promise(resolve => { - try { socket.close(resolve); } catch { resolve(); } - }); - } + fs.rmSync(choosingFile, { force: true }); + fs.rmSync(ticketFile, { force: true }); } - if (socketError) { - const error = new Error(`Plan Canvas startup lock failed on port ${port}: ${socketError.message}`); - error.code = 'PLAN_CANVAS_START_LOCK_FAILED'; - error.cause = socketError; - throw error; - } - return result; } function serverIsCompatible(health) { diff --git a/tests/integration/plan-canvas-e2e.test.js b/tests/integration/plan-canvas-e2e.test.js index 81d7d2bcc..978f8c5ae 100644 --- a/tests/integration/plan-canvas-e2e.test.js +++ b/tests/integration/plan-canvas-e2e.test.js @@ -18,7 +18,6 @@ const assert = require('assert'); const dgram = require('dgram'); -const { EventEmitter } = require('events'); const fs = require('fs'); const http = require('http'); const os = require('os'); @@ -146,6 +145,7 @@ async function main() { try { await test('port-scoped startup lock serializes server replacement callers', async () => { + const lockDir = path.join(tmp, 'startup-locks'); let active = 0; let maximumActive = 0; const runLocked = label => withServerStartLock(port + 2, async () => { @@ -154,21 +154,27 @@ async function main() { await new Promise(resolve => setTimeout(resolve, 40)); active -= 1; return label; - }, { timeoutMs: 2000 }); + }, { lockDir, timeoutMs: 2000 }); assert.deepStrictEqual(await Promise.all([runLocked('first'), runLocked('second')]), ['first', 'second']); assert.strictEqual(maximumActive, 1); }); - await test('port-scoped startup lock releases after a failed owner', async () => { + await test('port-scoped startup lock recovers dead tickets and failed owners', async () => { const lockPort = port + 3; + const lockDir = path.join(tmp, 'startup-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + const deadTicket = path.join(lockDir, `ecc-plan-canvas-${lockPort}-dead-owner.ticket`); + fs.writeFileSync(deadTicket, JSON.stringify({ pid: 2147483647, token: 'dead-owner', number: 1 })); await assert.rejects( - withServerStartLock(lockPort, async () => { throw new Error('owner failed'); }, { timeoutMs: 2000 }), + withServerStartLock(lockPort, async () => { throw new Error('owner failed'); }, { lockDir, timeoutMs: 2000 }), /owner failed/ ); assert.strictEqual( - await withServerStartLock(lockPort, async () => 'recovered', { timeoutMs: 2000 }), + await withServerStartLock(lockPort, async () => 'recovered', { lockDir, timeoutMs: 2000 }), 'recovered' ); + assert.ok(!fs.existsSync(deadTicket)); + assert.ok(!fs.readdirSync(lockDir).some(name => name.startsWith(`ecc-plan-canvas-${lockPort}-`))); }); await test('unrelated UDP traffic on the Canvas port does not block startup', async () => { @@ -188,21 +194,6 @@ async function main() { } }); - await test('port-scoped startup lock propagates socket failures', async () => { - class FailingLockSocket extends EventEmitter { - bind(_port, _host, callback) { setImmediate(callback); } - unref() {} - close(callback) { if (callback) setImmediate(callback); } - } - const socket = new FailingLockSocket(); - await assert.rejects( - withServerStartLock(port + 5, async () => { - socket.emit('error', new Error('simulated UDP failure')); - }, { dgramImpl: { createSocket: () => socket }, timeoutMs: 2000 }), - error => error.code === 'PLAN_CANVAS_START_LOCK_FAILED' && error.message.includes('simulated UDP failure') - ); - }); - await test('concurrent opens serialize replacement of a same-version legacy server', async () => { const legacyPort = port + 1; const legacyStateDir = path.join(tmp, 'legacy-state');