From 05cb3ad4f4d1fe0bb325d265c2a86bf5a49fb250 Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:53:55 -0400 Subject: [PATCH] fix(plan-canvas): harden startup and PDF admission --- scripts/lib/plan-canvas/server.js | 16 +++++---- scripts/plan-canvas.js | 27 +++++++++++---- tests/integration/plan-canvas-e2e.test.js | 42 +++++++++++++++++++++++ tests/scripts/plan-canvas.test.js | 21 ++++++++++++ 4 files changed, 93 insertions(+), 13 deletions(-) diff --git a/scripts/lib/plan-canvas/server.js b/scripts/lib/plan-canvas/server.js index 7963980c7..4931aa61f 100644 --- a/scripts/lib/plan-canvas/server.js +++ b/scripts/lib/plan-canvas/server.js @@ -411,6 +411,14 @@ function createPlanCanvasServer({ if (pdfMatch && (req.method === 'GET' || req.method === 'POST')) { const session = store.get(pdfMatch[1]); if (!session) return sendJson(res, 404, { error: 'unknown session' }); + let requestedSnapshot = null; + if (req.method === 'POST') { + const body = await readJsonBody(req, MAX_PDF_SNAPSHOT_BYTES); + if (typeof body.html !== 'string' || !body.html.trim()) { + return sendJson(res, 400, { error: 'html snapshot is required' }); + } + requestedSnapshot = { key: session.key, html: body.html }; + } if (pdfExportActive) { res.setHeader('retry-after', '1'); return sendJson(res, 429, { @@ -420,13 +428,7 @@ function createPlanCanvasServer({ } pdfExportActive = true; try { - if (req.method === 'POST') { - const body = await readJsonBody(req, MAX_PDF_SNAPSHOT_BYTES); - if (typeof body.html !== 'string' || !body.html.trim()) { - return sendJson(res, 400, { error: 'html snapshot is required' }); - } - pdfSnapshot = { key: session.key, html: body.html }; - } + pdfSnapshot = requestedSnapshot; const pdf = await pdfExporter({ url: `http://${req.headers.host}/artifact/${session.key}/?pdf=1`, artifactFile: session.file diff --git a/scripts/plan-canvas.js b/scripts/plan-canvas.js index adc306716..c059432c3 100755 --- a/scripts/plan-canvas.js +++ b/scripts/plan-canvas.js @@ -39,6 +39,7 @@ const { } = require('./lib/plan-canvas/server'); const VERSION = require('../package.json').version; +const DEFAULT_START_LOCK_LEASE_MS = 5 * 1000; const SAFE_REQUEST_PATHS = new Set([ '/', @@ -200,7 +201,7 @@ function readServerStartTicket(file) { } } -function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = 60 * 1000) { +function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = DEFAULT_START_LOCK_LEASE_MS) { const prefix = `ecc-plan-canvas-${validatePort(port)}-`; const entries = []; for (const name of fs.readdirSync(lockDir)) { @@ -219,10 +220,13 @@ function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = 60 * 100 } continue; } - const stale = value.token !== ownToken && !processIsAlive(value.pid); + const stale = value.token !== ownToken && ( + !processIsAlive(value.pid) || Date.now() - value.mtimeMs > staleAfterMs + ); 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. + // A bounded, renewable lease prevents PID reuse from making an exited + // owner's ticket look live forever. Ticket names contain a never-reused + // owner token, so this cannot delete a later caller's ticket path. try { fs.rmSync(file, { force: true }); } catch { /* already removed */ } continue; } @@ -233,6 +237,7 @@ function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = 60 * 100 async function withServerStartLock(port, task, { timeoutMs = 15 * 1000, + leaseMs = DEFAULT_START_LOCK_LEASE_MS, lockDir = path.join(os.homedir(), '.claude', 'plan-canvas', 'locks') } = {}) { const lockPort = validatePort(port); @@ -241,18 +246,27 @@ async function withServerStartLock(port, task, { const choosingFile = path.join(lockDir, `${prefix}.choosing`); const ticketFile = path.join(lockDir, `${prefix}.ticket`); const startedAt = Date.now(); + const lockLeaseMs = Number.isFinite(leaseMs) && leaseMs > 0 + ? leaseMs + : DEFAULT_START_LOCK_LEASE_MS; + let leaseTimer = null; fs.mkdirSync(lockDir, { recursive: true, mode: 0o700 }); fs.writeFileSync(choosingFile, JSON.stringify({ pid: process.pid, token }), { flag: 'wx', mode: 0o600 }); try { - const existing = listServerStartTickets(lockDir, lockPort, token) + const existing = listServerStartTickets(lockDir, lockPort, token, lockLeaseMs) .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 }); + leaseTimer = setInterval(() => { + const now = new Date(); + try { fs.utimesSync(ticketFile, now, now); } catch { /* cleanup or lease loss */ } + }, Math.max(25, Math.floor(lockLeaseMs / 3))); + if (leaseTimer.unref) leaseTimer.unref(); while (true) { - const entries = listServerStartTickets(lockDir, lockPort, token); + const entries = listServerStartTickets(lockDir, lockPort, token, lockLeaseMs); const anotherOwnerIsChoosing = entries.some(entry => entry.choosing && entry.token !== token); const tickets = entries .filter(entry => !entry.choosing && Number.isInteger(entry.number)) @@ -265,6 +279,7 @@ async function withServerStartLock(port, task, { } return await task(); } finally { + clearInterval(leaseTimer); fs.rmSync(choosingFile, { force: true }); fs.rmSync(ticketFile, { force: true }); } diff --git a/tests/integration/plan-canvas-e2e.test.js b/tests/integration/plan-canvas-e2e.test.js index 978f8c5ae..9348f9fe2 100644 --- a/tests/integration/plan-canvas-e2e.test.js +++ b/tests/integration/plan-canvas-e2e.test.js @@ -159,6 +159,28 @@ async function main() { assert.strictEqual(maximumActive, 1); }); + await test('port-scoped startup lock renews its lease while the owner is active', async () => { + const lockPort = port + 6; + const lockDir = path.join(tmp, 'startup-locks'); + let releaseFirst; + let markFirstEntered; + let secondEntered = false; + const firstEntered = new Promise(resolve => { markFirstEntered = resolve; }); + const first = withServerStartLock(lockPort, async () => { + markFirstEntered(); + await new Promise(resolve => { releaseFirst = resolve; }); + }, { lockDir, timeoutMs: 1000, leaseMs: 100 }); + await firstEntered; + const second = withServerStartLock(lockPort, async () => { + secondEntered = true; + }, { lockDir, timeoutMs: 1000, leaseMs: 100 }); + await new Promise(resolve => setTimeout(resolve, 250)); + assert.strictEqual(secondEntered, false); + releaseFirst(); + await Promise.all([first, second]); + assert.strictEqual(secondEntered, true); + }); + await test('port-scoped startup lock recovers dead tickets and failed owners', async () => { const lockPort = port + 3; const lockDir = path.join(tmp, 'startup-locks'); @@ -177,6 +199,26 @@ async function main() { assert.ok(!fs.readdirSync(lockDir).some(name => name.startsWith(`ecc-plan-canvas-${lockPort}-`))); }); + await test('port-scoped startup lock expires a stale ticket after PID reuse', async () => { + const lockPort = port + 5; + const lockDir = path.join(tmp, 'startup-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + const reusedPidTicket = path.join(lockDir, `ecc-plan-canvas-${lockPort}-reused-pid.ticket`); + fs.writeFileSync( + reusedPidTicket, + JSON.stringify({ pid: process.pid, token: 'reused-pid', number: 1 }) + ); + assert.strictEqual( + await withServerStartLock(lockPort, async () => 'recovered', { + lockDir, + timeoutMs: 1000, + leaseMs: 100 + }), + 'recovered' + ); + assert.ok(!fs.existsSync(reusedPidTicket)); + }); + await test('unrelated UDP traffic on the Canvas port does not block startup', async () => { const servicePort = port + 4; const unrelatedSocket = dgram.createSocket('udp4'); diff --git a/tests/scripts/plan-canvas.test.js b/tests/scripts/plan-canvas.test.js index ca3635994..29f746865 100644 --- a/tests/scripts/plan-canvas.test.js +++ b/tests/scripts/plan-canvas.test.js @@ -286,6 +286,27 @@ async function main() { assert.strictEqual((await first).statusCode, 200); })) passed++; else failed++; + if (await test('an incomplete PDF snapshot body does not consume renderer admission', async () => { + const stalled = http.request({ + host: '127.0.0.1', + port, + method: 'POST', + path: `/api/session/${key}/pdf`, + agent: false, + headers: { 'content-type': 'application/json', 'content-length': 1024 } + }); + stalled.on('error', () => {}); + stalled.write('{"html":"partial'); + try { + await new Promise(resolve => setTimeout(resolve, 50)); + const competing = await request(port, 'GET', `/api/session/${key}/pdf`); + assert.strictEqual(competing.statusCode, 200); + assert.strictEqual(competing.headers['content-type'], 'application/pdf'); + } finally { + stalled.destroy(); + } + })) passed++; else failed++; + if (await test('PDF failures log diagnostics without disclosing local paths', async () => { try { const rendererError = new Error('Chromium failed at /Users/private/browser-profile');