diff --git a/scripts/lib/plan-canvas/server.js b/scripts/lib/plan-canvas/server.js index 4931aa61f..8ade6527d 100644 --- a/scripts/lib/plan-canvas/server.js +++ b/scripts/lib/plan-canvas/server.js @@ -411,6 +411,16 @@ 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' }); + const sendBusy = () => { + res.setHeader('retry-after', '1'); + if (req.method === 'POST') res.setHeader('connection', 'close'); + return sendJson(res, 429, { + error: 'another PDF export is already in progress', + code: 'PDF_EXPORT_BUSY' + }); + }; + // Reject overload before accepting a potentially slow snapshot body. + if (pdfExportActive) return sendBusy(); let requestedSnapshot = null; if (req.method === 'POST') { const body = await readJsonBody(req, MAX_PDF_SNAPSHOT_BYTES); @@ -419,13 +429,8 @@ function createPlanCanvasServer({ } requestedSnapshot = { key: session.key, html: body.html }; } - if (pdfExportActive) { - res.setHeader('retry-after', '1'); - return sendJson(res, 429, { - error: 'another PDF export is already in progress', - code: 'PDF_EXPORT_BUSY' - }); - } + // A renderer may have started while this request body was arriving. + if (pdfExportActive) return sendBusy(); pdfExportActive = true; try { pdfSnapshot = requestedSnapshot; diff --git a/scripts/plan-canvas.js b/scripts/plan-canvas.js index c059432c3..f55ddf511 100755 --- a/scripts/plan-canvas.js +++ b/scripts/plan-canvas.js @@ -21,7 +21,7 @@ const fs = require('fs'); const http = require('http'); const os = require('os'); const path = require('path'); -const { spawn } = require('child_process'); +const { execFileSync, spawn } = require('child_process'); const { canonicalizeArtifactPath, @@ -39,7 +39,6 @@ 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([ '/', @@ -183,6 +182,37 @@ function processIsAlive(pid) { } } +function readProcessIdentity(pid) { + if (!Number.isInteger(pid) || pid <= 0) return null; + try { + if (process.platform === 'linux') { + const stat = fs.readFileSync(`/proc/${pid}/stat`, 'utf8'); + const commandEnd = stat.lastIndexOf(')'); + if (commandEnd === -1) return null; + const fieldsAfterCommand = stat.slice(commandEnd + 1).trim().split(/\s+/); + const startTicks = fieldsAfterCommand[19]; + return startTicks ? `linux:${startTicks}` : null; + } + if (process.platform === 'win32') { + const command = `(Get-Process -Id ${pid} -ErrorAction Stop).StartTime.ToUniversalTime().Ticks`; + const startTicks = execFileSync( + 'powershell.exe', + ['-NoProfile', '-NonInteractive', '-Command', command], + { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 3000, windowsHide: true } + ).trim(); + return startTicks ? `win32:${startTicks}` : null; + } + const startedAt = execFileSync( + 'ps', + ['-p', String(pid), '-o', 'lstart='], + { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 1000 } + ).trim(); + return startedAt ? `${process.platform}:${startedAt}` : null; + } catch { + return null; + } +} + function readServerStartTicket(file) { let fd; try { @@ -201,9 +231,10 @@ function readServerStartTicket(file) { } } -function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = DEFAULT_START_LOCK_LEASE_MS) { +function listServerStartTickets(lockDir, port, ownToken, malformedStaleAfterMs = 60 * 1000) { const prefix = `ecc-plan-canvas-${validatePort(port)}-`; const entries = []; + const identities = new Map(); for (const name of fs.readdirSync(lockDir)) { if (!name.startsWith(prefix) || (!name.endsWith('.choosing') && !name.endsWith('.ticket'))) continue; const file = path.join(lockDir, name); @@ -215,18 +246,25 @@ function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = DEFAULT_ continue; } if (value.malformed) { - if (Date.now() - value.mtimeMs > staleAfterMs) { + if (Date.now() - value.mtimeMs > malformedStaleAfterMs) { try { fs.rmSync(file, { force: true }); } catch { /* already removed */ } } continue; } - const stale = value.token !== ownToken && ( - !processIsAlive(value.pid) || Date.now() - value.mtimeMs > staleAfterMs - ); + let stale = false; + if (value.token !== ownToken) { + if (!processIsAlive(value.pid)) { + stale = true; + } else if (typeof value.processIdentity === 'string') { + if (!identities.has(value.pid)) identities.set(value.pid, readProcessIdentity(value.pid)); + const currentIdentity = identities.get(value.pid); + stale = currentIdentity !== null && currentIdentity !== value.processIdentity; + } + } if (stale) { - // 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. + // Process start identity distinguishes an exited owner from an unrelated + // process that later reused its PID. A live owner remains authoritative + // even if its event loop is paused for an arbitrary amount of time. try { fs.rmSync(file, { force: true }); } catch { /* already removed */ } continue; } @@ -237,36 +275,36 @@ function listServerStartTickets(lockDir, port, ownToken, staleAfterMs = DEFAULT_ 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); + const processIdentity = readProcessIdentity(process.pid); + if (!processIdentity) throw new Error('could not determine Plan Canvas startup lock process identity'); 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(); - 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 }); + fs.writeFileSync( + choosingFile, + JSON.stringify({ pid: process.pid, processIdentity, token }), + { flag: 'wx', mode: 0o600 } + ); try { - const existing = listServerStartTickets(lockDir, lockPort, token, lockLeaseMs) + 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.writeFileSync( + ticketFile, + JSON.stringify({ pid: process.pid, processIdentity, 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, lockLeaseMs); + 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)) @@ -279,7 +317,6 @@ 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 9348f9fe2..75667ac15 100644 --- a/tests/integration/plan-canvas-e2e.test.js +++ b/tests/integration/plan-canvas-e2e.test.js @@ -159,7 +159,7 @@ async function main() { assert.strictEqual(maximumActive, 1); }); - await test('port-scoped startup lock renews its lease while the owner is active', async () => { + await test('port-scoped startup lock preserves an old ticket from the same live process', async () => { const lockPort = port + 6; const lockDir = path.join(tmp, 'startup-locks'); let releaseFirst; @@ -169,16 +169,25 @@ async function main() { const first = withServerStartLock(lockPort, async () => { markFirstEntered(); await new Promise(resolve => { releaseFirst = resolve; }); - }, { lockDir, timeoutMs: 1000, leaseMs: 100 }); + }, { lockDir, timeoutMs: 1000 }); 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); + const ticketName = fs.readdirSync(lockDir).find(name => name.endsWith('.ticket')); + assert.ok(ticketName); + const old = new Date(Date.now() - 60 * 1000); + fs.utimesSync(path.join(lockDir, ticketName), old, old); + try { + await assert.rejects( + withServerStartLock(lockPort, async () => { secondEntered = true; }, { + lockDir, + timeoutMs: 200 + }), + /timed out waiting for Plan Canvas startup lock/ + ); + assert.strictEqual(secondEntered, false); + } finally { + releaseFirst(); + await first; + } }); await test('port-scoped startup lock recovers dead tickets and failed owners', async () => { @@ -199,20 +208,24 @@ 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 () => { + await test('port-scoped startup lock recovers 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 }) + JSON.stringify({ + pid: process.pid, + processIdentity: 'an-exited-process-instance', + token: 'reused-pid', + number: 1 + }) ); assert.strictEqual( await withServerStartLock(lockPort, async () => 'recovered', { lockDir, - timeoutMs: 1000, - leaseMs: 100 + timeoutMs: 1000 }), 'recovered' ); diff --git a/tests/scripts/plan-canvas.test.js b/tests/scripts/plan-canvas.test.js index 29f746865..4429ba4d3 100644 --- a/tests/scripts/plan-canvas.test.js +++ b/tests/scripts/plan-canvas.test.js @@ -307,6 +307,44 @@ async function main() { } })) passed++; else failed++; + if (await test('an active PDF export rejects incomplete snapshot uploads before reading them', async () => { + holdPdfExport = true; + const first = request(port, 'GET', `/api/session/${key}/pdf`); + let stalled = null; + try { + await waitFor(() => typeof releasePdfExport === 'function'); + const competing = new Promise((resolve, reject) => { + 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 } + }, res => { + let data = ''; + res.on('data', chunk => { data += chunk; }); + res.on('end', () => resolve({ statusCode: res.statusCode, body: data })); + }); + stalled.on('error', reject); + stalled.write('{"html":"partial'); + }); + const timeout = new Promise((_, reject) => { + setTimeout(() => reject(new Error('incomplete PDF upload was not rejected')), 500); + }); + const overloaded = await Promise.race([competing, timeout]); + assert.strictEqual(overloaded.statusCode, 429); + assert.strictEqual(jsonBody(overloaded).code, 'PDF_EXPORT_BUSY'); + } finally { + if (stalled) stalled.destroy(); + const release = releasePdfExport; + holdPdfExport = false; + releasePdfExport = null; + if (release) release(); + } + assert.strictEqual((await first).statusCode, 200); + })) 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');