diff --git a/mcp/src/config.js b/mcp/src/config.js index 9f8dd03e3..250b1b7a5 100644 --- a/mcp/src/config.js +++ b/mcp/src/config.js @@ -9,7 +9,7 @@ // lists that one (mcp.js). import { randomBytes } from 'node:crypto' -import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs' import { homedir } from 'node:os' import { join } from 'node:path' @@ -82,14 +82,23 @@ export function loadLastProject(dir = configDir()) { } } -/** Keep the last project, the folders as they were. */ +/** Keep the last project, the folders as they were. A file there that + * cannot be read is left as it is, the project not kept: written over, the + * folders it held would be lost. */ export function saveLastProject(project, dir = configDir()) { - writeKept(dir, { folders: loadFolders(dir) ?? [], project }) + const folders = loadFolders(dir) + if (!folders && existsSync(join(dir, 'folders.json'))) return + writeKept(dir, { folders: folders ?? [], project }) } +/** Written whole and renamed into place, as update.json is: another copy + * reading it meanwhile reads the old file or the new, never half of one. */ const writeKept = (dir, { folders, project }) => { mkdirSync(dir, { recursive: true }) - writeFileSync(join(dir, 'folders.json'), JSON.stringify({ saved: new Date().toISOString(), folders, ...(project ? { project } : {}) }) + '\n') + const file = join(dir, 'folders.json') + const part = `${file}.${process.pid}.part` + writeFileSync(part, JSON.stringify({ saved: new Date().toISOString(), folders, ...(project ? { project } : {}) }) + '\n') + renameSync(part, file) } /** The stored config, with a token made and saved if there was none. */ diff --git a/mcp/src/link.js b/mcp/src/link.js index a2f80645f..ecde3dd41 100644 --- a/mcp/src/link.js +++ b/mcp/src/link.js @@ -99,12 +99,15 @@ export class Asker extends EventEmitter { /** Ask the page something; resolves with its result, rejects with its * `{ code, message }`. `binaryPath` names bytes in the request that go as - * a binary frame; `onProgress` hears the page's progress lines. */ - request(message, binaryPath, onProgress) { + * a binary frame; `onProgress` hears the page's progress lines. With + * `timeoutMs`, an answer not come by then is given up on — rejected with + * code timeout and no longer waited for: a frozen tab still answers the + * heartbeat, so its link stays up while nothing it is asked comes back. */ + request(message, binaryPath, onProgress, timeoutMs) { const ws = this.socket if (!ws || ws.readyState !== ws.OPEN) return Promise.reject({ code: 'unavailable', message: 'No ScanRuler page is connected.' }) const id = this.nextId++ - return new Promise((resolve, reject) => { + const asked = new Promise((resolve, reject) => { this.pending.set(id, { resolve, reject, onProgress }) try { for (const frame of encode({ id, ...message }, binaryPath)) ws.send(frame) @@ -113,14 +116,24 @@ export class Asker extends EventEmitter { reject({ code: 'internal', message: e.message }) } }) + if (timeoutMs === undefined) return asked + let timer + const late = new Promise((_, reject) => { + timer = setTimeout(() => { + this.pending.delete(id) + reject({ code: 'timeout', message: `The ScanRuler page did not answer within ${Math.round(timeoutMs / 100) / 10} s; its tab may be frozen, or busy with something long.` }) + }, Math.max(0, timeoutMs)) + }) + return Promise.race([asked, late]).finally(() => clearTimeout(timer)) } /** Run a command on the page. `binaryKey` is the input field holding a * file's bytes, if one does; `job` the job the run is part of, which the * page keeps its outcome under (session.recent); `deferred`, that more - * changes follow straight after it, which build once after the last. */ - run(name, input, binaryKey, onProgress, job, deferred = false) { - return this.request({ method: 'run', name, input, ...(job ? { job } : {}), ...(deferred ? { deferred: true } : {}) }, binaryKey ? `input.${binaryKey}` : undefined, onProgress) + * changes follow straight after it, which build once after the last; + * `timeoutMs`, how long its answer is waited for, as request takes it. */ + run(name, input, binaryKey, onProgress, job, deferred = false, timeoutMs) { + return this.request({ method: 'run', name, input, ...(job ? { job } : {}), ...(deferred ? { deferred: true } : {}) }, binaryKey ? `input.${binaryKey}` : undefined, onProgress, timeoutMs) } /** Resolves true once a page is connected, false after `timeoutMs`. */ @@ -263,6 +276,10 @@ export class PageLink extends Asker { if (this.socket && this.socket.readyState === this.socket.OPEN) { return refuse(CLOSE.taken, 'Another ScanRuler tab is connected to this agent already — close the connection there first.') } + // A page still closing is replaced; its close comes after and is then + // not this link's, so what it was asked is turned down here, or it would + // wait for an answer that never comes. + if (this.socket) this.dropPending('The ScanRuler page closed the connection before it answered.') this.socket = ws const { token: _token, ...page } = hello this.page = { ...page, origin: origin ?? null, commands: Array.isArray(hello.commands) ? hello.commands : [] } diff --git a/mcp/src/main.js b/mcp/src/main.js index fd7bf82db..dd7511f43 100644 --- a/mcp/src/main.js +++ b/mcp/src/main.js @@ -176,7 +176,7 @@ export async function main(argv = process.argv.slice(2)) { const cwd = process.cwd() const outDir = values.out ? (values.out.match(/^([A-Za-z]:)?[\\/]/) ? values.out : `${cwd}/${values.out}`) : cwd const traceDir = values.trace ?? process.env.SCANRULER_MCP_TRACE - const trace = traceDir ? createTrace(resolve(cwd, traceDir), { args: values['trace-args'] === true }) : null + const trace = traceDir ? createTrace(resolve(cwd, traceDir), { args: values['trace-args'] === true, secrets: [token] }) : null if (trace) log(`Tracing every call to ${trace.file}${trace.args ? ', arguments too' : ''}.`) const engineFile = values.engine ?? process.env.SCANRULER_ENGINE ?? (values.headless ? CHECKOUT_ENGINE : null) if (engineFile) return headless(resolve(cwd, engineFile), { cwd, outDir, tools, trace, answerWithinMs, updates }) diff --git a/mcp/src/mcp.js b/mcp/src/mcp.js index e723e7858..ceb196860 100644 --- a/mcp/src/mcp.js +++ b/mcp/src/mcp.js @@ -353,10 +353,17 @@ const SEEN_WITHIN_MS = 55_000 * the minute a client or a bridge may cut it at. */ export const ANSWER_WITHIN_MS = 45_000 +/** The longest a Node timer waits, ms; one set for longer fires at once. */ +const MAX_TIMER_MS = 2 ** 31 - 1 + /** How long a batch waits for the page to be idle — before it retries a * call refused busy, and before it rolls back. */ const IDLE_WAIT_MS = 60_000 +/** How long a wait at its end still gives the page to answer the one read + * it asks, ms. */ +const IDLE_ASK_MS = 100 + /** How long a listing of the tools waits for a page that connected before * and is not back yet, ms. */ const LIST_WAIT_MS = 3_000 @@ -481,9 +488,10 @@ export function createMcpServer({ listWaitMs = LIST_WAIT_MS, answerCap = ANSWER_CAP, }) { - // The most any call is held, ms — Infinity with no limit. - const holdMs = answerWithinMs > 0 ? answerWithinMs : Infinity - const holdS = answerWithinMs > 0 ? answerWithinMs / 1000 : null + // The most any call is held, ms — Infinity with no limit. A limit past + // what a timer can wait is cut to that, or it would answer at once. + const holdMs = answerWithinMs > 0 ? Math.min(answerWithinMs, MAX_TIMER_MS) : Infinity + const holdS = answerWithinMs > 0 ? holdMs / 1000 : null const headless = link.backend === 'headless' // Full, compact, or the namespaces to list: a command of a namespace // left out is not listed, and not run either. @@ -624,7 +632,15 @@ export function createMcpServer({ let pageBusy = null const pageIdle = async (until) => { for (;;) { - const state = await link.run('session.state', {}).catch(() => null) + // Asked no longer than until — but long enough for a page that is + // there to answer — and a page that does not answer by then is not + // taken as idle. + let silent = false + const state = await link.run('session.state', {}, undefined, undefined, undefined, false, Math.max(IDLE_ASK_MS, until - Date.now())).catch((e) => { + silent = e?.code === 'timeout' + return null + }) + if (silent) return false pageBusy = state && typeof state === 'object' ? { busy: state.busy ?? null, running: state.running ?? null, runningMs: state.runningMs ?? null } : null if (!state || typeof state !== 'object' || (state.busy == null && state.running == null)) return true if (Date.now() >= until) return false @@ -640,7 +656,7 @@ export function createMcpServer({ for (;;) { let outcomes try { - outcomes = (await link.run('session.recent', { job, limit: 100 }))?.outcomes + outcomes = (await link.run('session.recent', { job, limit: 100 }, undefined, undefined, undefined, false, Math.max(IDLE_ASK_MS, until - Date.now())))?.outcomes } catch { return null } @@ -697,12 +713,7 @@ export function createMcpServer({ * not answer soon. */ const sessionRead = async () => { if (!link.connected) return null - let timer - const state = await Promise.race([ - link.run('session.state', {}).catch(() => null), - new Promise((r) => (timer = setTimeout(() => r(null), SESSION_WITHIN_MS))), - ]) - clearTimeout(timer) + const state = await link.run('session.state', {}, undefined, undefined, undefined, false, SESSION_WITHIN_MS).catch(() => null) return state && typeof state === 'object' ? state : null } /** Whether a session holds work: a scan, or anything counted. */ @@ -1017,7 +1028,11 @@ export function createMcpServer({ call: async (args, ctx) => { if (ctx) ctx.fields.wait = args?.for ?? (args?.job !== undefined ? 'job' : 'page') // Never held past the answer limit: a cut call says nothing. - const until = Date.now() + Math.min(1000 * (args?.timeoutSeconds ?? 120), holdMs) + // The schema is not checked for it: a timeout that is not a number + // of seconds above 0 waits the default, and none waits past 600 s. + const asked = Number(args?.timeoutSeconds ?? 120) + const seconds = Number.isFinite(asked) && asked > 0 ? Math.min(asked, 600) : 120 + const until = Date.now() + Math.min(1000 * seconds, holdMs) if (args?.for === 'job' || args?.job !== undefined) { const id = String(args?.job ?? '') const j = jobs.get(id) @@ -1381,7 +1396,7 @@ export function createMcpServer({ * `deferred`: more changes follow straight after it (a batch with * build: "end"), and the page may leave their rebuild to the last. * A `{ "$file": path }` anywhere in the arguments is that file's JSON. */ - const runPageTool = async (name, given, onProgress, { timings = false, ms = false, batched = false, job, deferred = false } = {}) => { + const runPageTool = async (name, given, onProgress, { timings = false, ms = false, batched = false, job, deferred = false, within } = {}) => { const command = commands().find((c) => toolName(c) === name) const needsPage = !command && headless ? link.pageCommand(name) : null if (needsPage) return failure({ code: 'unavailable', message: `${name} needs the ScanRuler page — a scan, the viewport or the browser — and this server runs ScanRuler headless, with none. Start scanruler-mcp in page mode (without --engine or --headless) and pair it with a ScanRuler tab for it.` }) @@ -1402,7 +1417,7 @@ export function createMcpServer({ const savedTo = replacesSession(command) && args?.saveTo !== undefined ? await saveSession(String(args.saveTo), job, base) : undefined if (savedTo !== undefined) input.discard = true const started = Date.now() - let result = await link.run(command.name, input, binaryKey, onProgress, job, deferred) + let result = await link.run(command.name, input, binaryKey, onProgress, job, deferred, within) // The folder a scan or a project was opened from, for scanruler_write. if (binaryKey && replacesSession(command) && file) { rememberFolder(file) @@ -1670,7 +1685,9 @@ export function createMcpServer({ // past it — then the whole goes to a file, and the answer keeps how // the batch ended. A call that may answer longer (a guide) lets the // batch it is in answer as long. - const cap = Math.max(...calls.map((c) => capOf(commandOf(String(c?.tool ?? ''))))) + // Not Math.max(...): a batch read from a file may hold more calls than + // a spread can pass. + const cap = calls.reduce((most, c) => Math.max(most, capOf(commandOf(String(c?.tool ?? '')))), 0) return text(await fitted(answer, 'scanruler_batch', exact, ['failed', 'undone'], { cap }), exact) } @@ -1773,7 +1790,9 @@ export function createMcpServer({ // before it is done. const command = commands().find((c) => toolName(c) === name) if (!command || (command.readOnly && !command.long)) { - const answer = runPageTool(name, args, onProgress) + // Not a job, so held no longer than the answer limit by itself: a + // frozen tab keeps its link up while nothing it is asked comes back. + const answer = runPageTool(name, args, onProgress, { within: holdMs === Infinity ? undefined : holdMs }) callsInFlight.add(answer) return answer.finally(() => callsInFlight.delete(answer)) } diff --git a/mcp/src/relay.js b/mcp/src/relay.js index 2eab76319..98acee82b 100644 --- a/mcp/src/relay.js +++ b/mcp/src/relay.js @@ -151,10 +151,10 @@ export class SharedLink extends EventEmitter { return this.role === 'relay' ? (this.peer?.holderVersion ?? null) : undefined } - run(name, input, binaryKey, onProgress, job, deferred = false) { + run(name, input, binaryKey, onProgress, job, deferred = false, timeoutMs) { const link = this.current if (!link) return Promise.reject({ code: 'unavailable', message: 'No ScanRuler page is connected.' }) - return link.run(name, input, binaryKey, onProgress, job, deferred) + return link.run(name, input, binaryKey, onProgress, job, deferred, timeoutMs) } waitForPage(timeoutMs) { diff --git a/mcp/src/trace.js b/mcp/src/trace.js index 11db2f1ce..05a2c240c 100644 --- a/mcp/src/trace.js +++ b/mcp/src/trace.js @@ -45,8 +45,10 @@ export function replyHead(reply) { * list handed out), "call" (a tool call answered), "entry" (one call of a * batch), "job" (a change started with wait: false, finished) and * "visibility" (the tab hidden or shown). Times are ms since the epoch - * (`t`, the start) and durations ms. */ -export function createTrace(dir, { args = false, now = () => Date.now(), pid = process.pid } = {}) { + * (`t`, the start) and durations ms. `secrets` are written as + * "[redacted]" wherever they appear — the pairing token, which the help on + * connecting gives in full: a trace is made to be shared. */ +export function createTrace(dir, { args = false, now = () => Date.now(), pid = process.pid, secrets = [] } = {}) { if (!dir) return null mkdirSync(dir, { recursive: true }) const stamp = new Date(now()).toISOString().replace(/[:.]/g, '-').replace(/Z$/, '') @@ -55,7 +57,9 @@ export function createTrace(dir, { args = false, now = () => Date.now(), pid = p const write = (line) => { if (broken) return try { - appendFileSync(file, `${JSON.stringify(line)}\n`) + let json = JSON.stringify(line) + for (const secret of secrets) if (secret) json = json.split(secret).join('[redacted]') + appendFileSync(file, `${json}\n`) } catch (e) { // A trace that cannot be written is not a reason to fail the call. broken = true diff --git a/mcp/tests/files.test.js b/mcp/tests/files.test.js index f2141d7ae..6903bab50 100644 --- a/mcp/tests/files.test.js +++ b/mcp/tests/files.test.js @@ -274,3 +274,13 @@ test('relative paths from one folder: the scan’s once one is opened, else wher assert.match(tools.find((t) => t.name === 'scanruler_batch').description, /\$file.*relative path is taken from the folder of the scan/) await c.close() }) + +test('a batch file of more calls than a spread can pass still answers', async () => { + const c = await copy() + await writeFile(join(dir, 'many.json'), JSON.stringify(Array.from({ length: 130_000 }, () => ({ tool: 'demo_echo' })))) + const r = await c.call('scanruler_batch', { file: join(dir, 'many.json'), wait: false }) + const done = await c.call('scanruler_wait', { for: 'job', job: r.json.job, timeoutSeconds: 600 }) + assert.equal(done.error, false, done.text) + assert.equal(done.json.state, 'done', done.text) + await c.close() +}) diff --git a/mcp/tests/inputs.test.js b/mcp/tests/inputs.test.js index 19fb26015..4093ed783 100644 --- a/mcp/tests/inputs.test.js +++ b/mcp/tests/inputs.test.js @@ -207,3 +207,15 @@ test('a page older than its app: a stale_page answer, or the page’s own word, assert.equal(said.page.stale, true) assert.equal(said.page.staleNote, 'this tab is older than the app it came from') }) + +test('the last project kept beside folders that cannot be read leaves them, not an empty list', async () => { + const kept = await mkdtemp(join(tmpdir(), 'scanruler-mcp-folders-')) + // Half of what another copy was writing, as a read in the middle of it saw. + await writeFile(join(kept, 'folders.json'), '{"saved":"2026-10-24T14:41:00.000Z","folders":["C:\\\\scans\\\\a",') + saveLastProject('C:\\scans\\a\\p.scanruler', kept) + assert.equal(loadFolders(kept), null, 'not written over with no folders') + saveFolders(['C:\\scans\\a'], kept) + saveLastProject('C:\\scans\\a\\p.scanruler', kept) + assert.deepEqual(loadFolders(kept), ['C:\\scans\\a']) + assert.equal(loadLastProject(kept), 'C:\\scans\\a\\p.scanruler') +}) diff --git a/mcp/tests/limit.test.js b/mcp/tests/limit.test.js index 7393a08be..b2a9ee2e9 100644 --- a/mcp/tests/limit.test.js +++ b/mcp/tests/limit.test.js @@ -75,7 +75,7 @@ after(async () => { }) /** A copy of the server on its own port, the fake page connected to it. */ -async function copy({ answerWithinMs = LIMIT_MS } = {}) { +async function copy({ answerWithinMs = LIMIT_MS, answers = answer } = {}) { const link = new PageLink({ token: TOKEN }) const http = httpServer({}) link.attach(http) @@ -84,7 +84,7 @@ async function copy({ answerWithinMs = LIMIT_MS } = {}) { const [a, b] = InMemoryTransport.createLinkedPair() const client = new Client({ name: 'test', version: '0.0.0' }) await Promise.all([server.connect(a), client.connect(b)]) - const { page } = await fakePage(http.address().port, { token: TOKEN, commands: COMMANDS, answer }) + const { page } = await fakePage(http.address().port, { token: TOKEN, commands: COMMANDS, answer: answers }) let closed = false const close = async () => { if (closed) return @@ -100,7 +100,7 @@ async function copy({ answerWithinMs = LIMIT_MS } = {}) { const text = r.content[0].text return { error: r.isError === true, text, json: r.isError ? null : JSON.parse(text), images: r.content.filter((c) => c.type === 'image') } } - return { client, call, page, close } + return { client, call, page, close, link } } /** scanruler_wait for a job, called again while it says running. */ @@ -283,3 +283,32 @@ test('--answer-within 0 holds a call to its end', async () => { assert.equal((await one.call('scanruler_status')).json.server.answerWithin, null) await one.close() }) + +test('an answer limit or a wait too long for a timer still waits, as does a wait with a timeout that is not a number', async () => { + const one = await copy({ answerWithinMs: 1e10 }) + assert.deepEqual((await one.call('demo_update', { n: 4, ms: 150 })).json, { n: 4 }, 'not queued at once') + await one.close() + const two = await copy({ answerWithinMs: 0 }) + for (const timeoutSeconds of [1e10, 'soon', -1]) { + const { job } = (await two.call('demo_update', { n: 5, ms: 150, wait: false })).json + const waited = await two.call('scanruler_wait', { for: 'job', job, timeoutSeconds }) + assert.equal(waited.json.state, 'done', `timeoutSeconds ${timeoutSeconds}`) + } + await two.close() +}) + +test('a page that never answers: a read, a wait for idle and a wait for a job of another copy answer within the limit, and nothing is left waiting', async () => { + const one = await copy({ answerWithinMs: 300, answers: () => new Promise(() => {}) }) + for (const [name, args] of [ + ['session_state', {}], + ['scanruler_wait', { for: 'idle', timeoutSeconds: 1 }], + ['scanruler_wait', { for: 'job', job: 'j9-zzzz', timeoutSeconds: 1 }], + ]) { + const at = Date.now() + const r = await Promise.race([one.call(name, args), new Promise((resolve) => setTimeout(() => resolve('still waiting'), 2000))]) + assert.notEqual(r, 'still waiting', `${name} ${JSON.stringify(args)}`) + assert.ok(Date.now() - at < 1000, `${name} answered after ${Date.now() - at} ms`) + } + assert.equal(one.link.pending.size, 0) + await one.close() +}) diff --git a/mcp/tests/link.test.js b/mcp/tests/link.test.js index 0ee50e736..f125bf79e 100644 --- a/mcp/tests/link.test.js +++ b/mcp/tests/link.test.js @@ -195,3 +195,23 @@ test('waiting for a page ends when one connects, or when the time is up', async page.page.close() await new Promise((resolve) => link.once('disconnected', resolve)) }) + +test('a page taken over while its connection is still closing has its calls turned down', async () => { + const old = await fakePage(port, { token: TOKEN, answer: () => new Promise(() => {}) }) + const call = link.run('session.state', {}).then( + () => 'answered', + (e) => e.code, + ) + await new Promise((resolve) => setTimeout(resolve, 100)) + // It says goodbye but stops reading, so this side stays closing. + old.page.ws.close(1001, 'going away') + old.page.ws._socket.pause() + await new Promise((resolve) => setTimeout(resolve, 100)) + const next = await fakePage(port, { token: TOKEN }) + assert.ok(next.welcome, 'the new page is taken in') + assert.equal(await Promise.race([call, new Promise((resolve) => setTimeout(() => resolve('still waiting'), 1000))]), 'unavailable') + assert.equal(link.pending.size, 0) + old.page.ws._socket.destroy() + next.page.close() + await new Promise((resolve) => link.once('disconnected', resolve)) +}) diff --git a/mcp/tests/trace.test.js b/mcp/tests/trace.test.js index 9bf001488..60f866bd6 100644 --- a/mcp/tests/trace.test.js +++ b/mcp/tests/trace.test.js @@ -188,6 +188,15 @@ test('compact: a batch of one and scanruler_tools are traced, a queued batch too } }) +test('the pairing token is not written to a trace, though the help on connecting gives it', async () => { + const dir = await mkdtemp(join(tmpdir(), 'scanruler-trace-')) + const trace = createTrace(dir, { args: true, secrets: [TOKEN] }) + trace.write({ ev: 'call', tool: 'demo_read', args: { q: TOKEN }, head: `unavailable: No ScanRuler page is connected. Enter port 7317 and token ${TOKEN}.` }) + const written = await readFile(trace.file, 'utf8') + assert.equal(written.includes(TOKEN), false) + assert.match(JSON.parse(written).head, /and token \[redacted\]\.$/) +}) + test('no trace without a directory', () => { assert.equal(createTrace(''), null) assert.equal(createTrace(undefined), null)