From f28ac02782aa0432e6144aeeb3737f1b989154da Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Thu, 24 Sep 2026 15:22:17 +0300 Subject: [PATCH 1/9] test(startup): starting the server must not load the Excel, PDF and DOCX packages (#715) The reporter's client timed out waiting for `initialize`: 25-90 s on Windows 11, one core busy for ~25 s. Every launch loads exceljs, pdf-lib, md-to-pdf with puppeteer, unpdf, @opendocsg/pdf2md and pizzip before it answers, whether or not the session ever opens such a file: the startup modules import them at the top of the file. Measured here, 1,183 of the 1,556 modules resolved before the answer come in through them, on Windows and macOS alike (~480 of ~610 ms warm on Windows). test-startup-imports.js starts dist/index.js over MCP stdio with a preload that records every resolved module (module.registerHooks, import and require). On this commit it fails its first two cases: the packages are loaded before `initialize` is answered, and after tools/list and the Chrome warm-up. Its third case (xlsx, docx and pdf files work through the server and load their packages then) passes: it is coverage for the fix. test/repro/test-startup-heavy-imports.js reports the module counts and startup times per start, and exits 1 here. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/fixtures/record-modules-preload.mjs | 15 +++ test/helpers/server-modules.js | 109 ++++++++++++++++++++++ test/repro/test-startup-heavy-imports.js | 85 +++++++++++++++++ test/test-startup-imports.js | 112 +++++++++++++++++++++++ 4 files changed, 321 insertions(+) create mode 100644 test/fixtures/record-modules-preload.mjs create mode 100644 test/helpers/server-modules.js create mode 100644 test/repro/test-startup-heavy-imports.js create mode 100644 test/test-startup-imports.js diff --git a/test/fixtures/record-modules-preload.mjs b/test/fixtures/record-modules-preload.mjs new file mode 100644 index 000000000..9cf380569 --- /dev/null +++ b/test/fixtures/record-modules-preload.mjs @@ -0,0 +1,15 @@ +// Preloaded into the real server (node --import) by test/helpers/server-modules.js: +// writes every module the server resolves, through import or require, to +// DC_TEST_MODULE_LOG as it happens, one " " line each. +import fs from 'node:fs'; +import { registerHooks } from 'node:module'; + +const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); + +registerHooks({ + resolve(specifier, context, nextResolve) { + const resolved = nextResolve(specifier, context); + fs.writeSync(log, `${Date.now()} ${resolved.url} ${context.parentURL ?? ''}\n`); + return resolved; + }, +}); diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js new file mode 100644 index 000000000..e704a6feb --- /dev/null +++ b/test/helpers/server-modules.js @@ -0,0 +1,109 @@ +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { fileURLToPath, pathToFileURL } from 'url'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { isTestHome } from './test-env.js'; + +const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); +const SERVER = path.join(PROJECT_ROOT, 'dist/index.js'); +const PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-preload.mjs')).href; + +/** Packages only reading, writing or rendering Excel, PDF and DOCX files need */ +export const HEAVY_PACKAGES = ['exceljs', 'pdf-lib', 'md-to-pdf', 'puppeteer', 'unpdf', '@opendocsg/pdf2md', 'pizzip']; + +/** The npm package a module URL belongs to ('@scope/name' or 'name'), or undefined for Node's and the server's own modules */ +export function packageOf(url) { + const marker = '/node_modules/'; + const at = url.lastIndexOf(marker); + if (at === -1) return undefined; + const [first, second] = url.slice(at + marker.length).split('/'); + return first.startsWith('@') ? `${first}/${second}` : first; +} + +function chromeExecutable(buildDir) { + if (process.platform === 'win32') return path.join(buildDir, 'chrome-win64', 'chrome.exe'); + if (process.platform === 'darwin') { + return path.join(buildDir, 'chrome-mac-arm64', 'Google Chrome for Testing.app', 'Contents', 'MacOS', 'Google Chrome for Testing'); + } + return path.join(buildDir, 'chrome-linux64', 'chrome'); +} + +/** + * Puts two Chrome for Testing builds (empty stand-ins) in Desktop Commander's + * Puppeteer cache. The server's Chrome warm-up picks the newer one and removes + * the older one, which shows the warm-up has run, and never downloads Chrome. + * Returns the older build's folder. + */ +function seedChromeCache() { + const chromeDir = path.join(os.homedir(), '.claude-server-commander', 'puppeteer-cache', 'chrome'); + const [stale, current] = ['100.0.0.0', '101.0.0.0'].map((version) => path.join(chromeDir, `test-${version}`)); + for (const buildDir of [stale, current]) { + const executable = chromeExecutable(buildDir); + fs.mkdirSync(path.dirname(executable), { recursive: true }); + fs.writeFileSync(executable, ''); + } + return stale; +} + +/** + * Starts the real server (dist/index.js) over MCP stdio, the way a client + * does, with a preload that records every module it resolves (import and + * require). Resolves once `initialize` is answered. + * + * Returns: + * - client: the connected MCP client + * - startedAt / initializedAt: Date.now() at spawn and once `initialize` was answered + * - modules(): every module resolved so far, first resolution each: [{ at, url, parent }] + * - waitForChromeWarmUp(timeoutMs): resolves true once the server's Chrome warm-up + * (run after the handshake) has finished, false if it hasn't within timeoutMs + * - close() + * + * It writes to Desktop Commander's folder in the home, so it runs only in a test home. + */ +export async function startServerRecordingModules() { + if (!isTestHome()) { + throw new Error('startServerRecordingModules writes to the home: run it through the test or repro runner'); + } + const staleChromeBuild = seedChromeCache(); + const logFile = path.join(os.homedir(), `modules-${process.pid}-${Date.now()}.log`); + fs.writeFileSync(logFile, ''); + + const transport = new StdioClientTransport({ + command: process.execPath, + args: ['--import', PRELOAD, SERVER, '--no-onboarding'], + cwd: PROJECT_ROOT, + env: { ...process.env, DC_TEST_MODULE_LOG: logFile }, + stderr: 'pipe', + }); + const client = new Client({ name: 'startup-modules-test', version: '1.0.0' }, { capabilities: {} }); + const startedAt = Date.now(); + await client.connect(transport, { timeout: 120_000 }); + const initializedAt = Date.now(); + + const modules = () => { + const seen = new Map(); + for (const line of fs.readFileSync(logFile, 'utf8').split('\n')) { + const [at, url, parent] = line.split(' '); + if (url && !seen.has(url)) seen.set(url, { at: Number(at), url, parent }); + } + return [...seen.values()]; + }; + + const waitForChromeWarmUp = async (timeoutMs) => { + const deadline = Date.now() + timeoutMs; + while (fs.existsSync(staleChromeBuild)) { + if (Date.now() > deadline) return false; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return true; + }; + + const close = async () => { + await client.close().catch(() => {}); + fs.rmSync(logFile, { force: true }); + }; + + return { client, startedAt, initializedAt, modules, waitForChromeWarmUp, close }; +} diff --git a/test/repro/test-startup-heavy-imports.js b/test/repro/test-startup-heavy-imports.js new file mode 100644 index 000000000..0daeb8256 --- /dev/null +++ b/test/repro/test-startup-heavy-imports.js @@ -0,0 +1,85 @@ +// Repro (#715): Desktop Commander takes 25-90 s to answer the MCP `initialize` +// request on the reporter's Windows 11 machine (Node 24.14, 0.2.50 local +// clone), one core busy for ~25 s, and the client gives up; loading the +// Excel, PDF and DOCX packages on first use brought it to 8-10 s there. +// +// Every launch loads the packages that only reading, writing and rendering +// Excel, PDF and DOCX files need (exceljs, pdf-lib, md-to-pdf with puppeteer, +// unpdf, @opendocsg/pdf2md, pizzip), before it answers `initialize`, whether +// or not the session ever opens such a file: the server's startup modules +// import them at the top of the file. +// +// Here the built server is started the way a client starts it (MCP over +// stdio), with a preload that records every module it resolves (import and +// require). Each run reports how long `initialize` took (a measurement, not a +// verdict), how many modules were loaded before the answer and how many of +// them came in through those packages, then lists the ones loaded once +// tools/list has been answered and the Chrome warm-up after the handshake has +// run. +// +// Run: node test/repro/run-repro.js test-startup-heavy-imports.js +// (REPRO_RUNS=5 starts by default) +// Exit code: 1 if any of those packages is loaded by then. +import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from '../helpers/server-modules.js'; +import { exitProcess } from '../../dist/utils/exit-process.js'; + +const RUNS = Number(process.env.REPRO_RUNS || 5); +const WARM_UP_TIMEOUT_MS = 60_000; + +/** For each module, the heavy package it was first loaded through, if any */ +function heavyPackageBehind(modules) { + const byUrl = new Map(modules.map((module) => [module.url, module])); + const behind = new Map(); + const find = (module, depth = 0) => { + if (behind.has(module.url)) return behind.get(module.url); + const pkg = packageOf(module.url); + let result; + if (pkg && HEAVY_PACKAGES.includes(pkg)) result = pkg; + else if (pkg && module.parent && byUrl.has(module.parent) && depth < 200) result = find(byUrl.get(module.parent), depth + 1); + behind.set(module.url, result); + return result; + }; + for (const module of modules) find(module); + return behind; +} + +function describe(modules) { + const own = modules.filter((module) => !module.url.startsWith('node:')); + const behind = heavyPackageBehind(own); + const counts = {}; + for (const module of own) { + const pkg = behind.get(module.url); + if (pkg) counts[pkg] = (counts[pkg] ?? 0) + 1; + } + const heavy = Object.values(counts).reduce((sum, count) => sum + count, 0); + const detail = Object.entries(counts).map(([pkg, count]) => `${pkg} ${count}`).join(', '); + return `${own.length} modules, ${heavy ? `${heavy} of them through ${detail}` : 'none of them through those packages'}`; +} + +const heavyLoaded = (modules) => HEAVY_PACKAGES.filter((pkg) => modules.some((module) => packageOf(module.url) === pkg)); + +let runsLoadingHeavy = 0; +const startupMs = []; +for (let run = 1; run <= RUNS; run++) { + const server = await startServerRecordingModules(); + try { + const beforeAnswer = server.modules().filter((module) => module.at <= server.initializedAt); + startupMs.push(server.initializedAt - server.startedAt); + console.log(`start ${run}: initialize answered after ${server.initializedAt - server.startedAt} ms; loaded before it: ${describe(beforeAnswer)}`); + + await server.client.listTools(); + const warmedUp = await server.waitForChromeWarmUp(WARM_UP_TIMEOUT_MS); + const loaded = heavyLoaded(server.modules()); + if (loaded.length > 0) runsLoadingHeavy++; + console.log(` after tools/list and the Chrome warm-up${warmedUp ? '' : ' (still running)'}: ${loaded.length ? `loaded ${loaded.join(', ')}` : 'none of those packages loaded'}`); + } finally { + await server.close(); + } +} + +startupMs.sort((a, b) => a - b); +const median = startupMs[Math.floor(startupMs.length / 2)]; +console.log(runsLoadingHeavy > 0 + ? `REPRODUCED: ${runsLoadingHeavy} of ${RUNS} starts loaded Excel/PDF/DOCX packages without opening any such file (initialize answered after ${median} ms median)` + : `NOT REPRODUCED: ${RUNS} starts, none loaded Excel/PDF/DOCX packages (initialize answered after ${median} ms median)`); +exitProcess(runsLoadingHeavy > 0 ? 1 : 0); diff --git a/test/test-startup-imports.js b/test/test-startup-imports.js new file mode 100644 index 000000000..8a9b7e53a --- /dev/null +++ b/test/test-startup-imports.js @@ -0,0 +1,112 @@ +/** + * Starting the server must not load the packages only Excel, PDF and DOCX + * files need (#715). Every launch loaded them before answering `initialize`, + * whether or not the session ever opened such a file: 1,183 of the 1,556 + * modules loaded before the answer, which one reporter saw take 25-90 s and + * time the client out. Each package still loads the first time a file needs it. + * + * Starts the real server (dist/index.js) over MCP stdio, the way a client + * does, recording every module it resolves (import and require). + */ +import assert from 'assert'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { fileURLToPath } from 'url'; +import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from './helpers/server-modules.js'; +import { isTestHome } from './helpers/test-env.js'; +import { runIfMain, skip } from './helpers/run-if-main.js'; + +const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const SAMPLE_PDF = path.join(PROJECT_ROOT, 'test/samples/01_sample_simple.pdf'); +const WARM_UP_TIMEOUT_MS = 60_000; + +const heavyLoaded = (modules) => HEAVY_PACKAGES.filter((pkg) => modules.some((module) => packageOf(module.url) === pkg)); + +function text(result) { + return (result.content ?? []).map((item) => item.text ?? '').join('\n'); +} + +async function callTool(client, name, args) { + const result = await client.callTool({ name, arguments: args }); + assert.notStrictEqual(result.isError, true, `${name} ${JSON.stringify(args)} failed: ${text(result)}`); + return text(result); +} + +/** Nothing for Excel, PDF or DOCX files is loaded before the server answers `initialize` */ +async function testNothingLoadedBeforeInitialize(server) { + const beforeAnswer = server.modules().filter((module) => module.at <= server.initializedAt && !module.url.startsWith('node:')); + const loaded = heavyLoaded(beforeAnswer); + assert.deepStrictEqual(loaded, [], + `with no Excel, PDF or DOCX file opened, the server loaded ${loaded.join(', ')} before answering initialize ` + + `(${beforeAnswer.length} modules resolved before the answer)`); + console.log(`✓ Nothing for Excel, PDF or DOCX loaded before initialize was answered (${beforeAnswer.length} modules)`); +} + +/** Nor once the tools are listed and the Chrome warm-up after the handshake has run */ +async function testNothingLoadedAfterWarmUp(server) { + await server.client.listTools(); + assert(await server.waitForChromeWarmUp(WARM_UP_TIMEOUT_MS), + `the Chrome warm-up after the handshake did not finish within ${WARM_UP_TIMEOUT_MS} ms`); + const loaded = heavyLoaded(server.modules()); + assert.deepStrictEqual(loaded, [], + `with no Excel, PDF or DOCX file opened, the server loaded ${loaded.join(', ')} by the time tools/list ` + + 'was answered and the Chrome warm-up had run'); + console.log('✓ Nothing for Excel, PDF or DOCX loaded after tools/list and the Chrome warm-up'); +} + +/** Each package loads when a file first needs it, and the file works */ +async function testLoadedOnFirstUse(server, dir) { + const { client } = server; + const xlsx = path.join(dir, 'budget.xlsx'); + await callTool(client, 'write_file', { path: xlsx, content: JSON.stringify([['Item', 'Note'], ['Alice', 'ZebraQuartz budget']]) }); + assert(/ZebraQuartz budget/.test(await callTool(client, 'read_file', { path: xlsx })), + 'reading back a spreadsheet written through the server should show its cells'); + + const docx = path.join(dir, 'memo.docx'); + await callTool(client, 'write_file', { path: docx, content: 'Memo title\nThe ZebraQuartz review is due' }); + await callTool(client, 'edit_block', { file_path: docx, old_string: 'ZebraQuartz', new_string: 'ZebraQuokka' }); + assert(/ZebraQuokka review/.test(await callTool(client, 'read_file', { path: docx })), + 'reading back a DOCX written and edited through the server should show the edited text'); + + assert((await callTool(client, 'read_file', { path: SAMPLE_PDF })).trim().length > 0, + 'reading a PDF through the server should return its text'); + + const loaded = heavyLoaded(server.modules()); + for (const pkg of ['exceljs', 'pizzip', '@opendocsg/pdf2md']) { + assert(loaded.includes(pkg), `${pkg} should be loaded once a file needed it; loaded: ${loaded.join(', ') || 'none'}`); + } + console.log('✓ Excel, DOCX and PDF files work on first use, loading their packages then'); +} + +export default async function runTests() { + if (!isTestHome()) { + skip('test-startup-imports.js writes to the home: run it through node test/run-all-tests.js'); + return true; + } + const dir = fs.mkdtempSync(path.join(os.homedir(), 'startup-imports-')); + const failures = []; + let server; + try { + server = await startServerRecordingModules(); + for (const test of [testNothingLoadedBeforeInitialize, testNothingLoadedAfterWarmUp, testLoadedOnFirstUse]) { + try { + await test(server, dir); + } catch (error) { + failures.push(error); + console.error(`❌ ${test.name}: ${error.message}`); + } + } + } finally { + await server?.close(); + fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); + } + if (failures.length > 0) { + console.error(`❌ ${failures.length} startup import test(s) failed`); + return false; + } + console.log('✅ Startup import tests passed'); + return true; +} + +runIfMain(import.meta.url, runTests); From 38fa916d724d9264c679999b82be1da498a11424 Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Thu, 24 Sep 2026 15:38:54 +0300 Subject: [PATCH 2/9] fix(startup): load the Excel, PDF and DOCX packages on first use (#715) The reporter's client timed out waiting for `initialize` (25-90 s on Windows 11): every launch loaded exceljs, pdf-lib, md-to-pdf with puppeteer, unpdf, @opendocsg/pdf2md and pizzip before answering, whether or not the session ever opened such a file. The server's startup modules (the file handlers, the PDF tools, search-manager, and markdown.ts, which the Chrome warm-up after the handshake runs from) imported them at the top of the file. Each package is now loaded where it is used, when a file first needs it: exceljs in the Excel handler's methods, pizzip when the DOCX handler or a DOCX search opens a zip, pdf-lib when a PDF is edited, @opendocsg/pdf2md and unpdf when a PDF is read, and md-to-pdf with its puppeteer, serve-handler and gray-matter when markdown is rendered or its options resolved. Routing, the tools and the Chrome warm-up are unchanged. No loader of ours remembers a result, only Node's module caches do: a missing package is looked up again on the next use. A missing or broken package no longer stops the server from starting; the operation that needs it fails with the load error. Before `initialize` is answered the server now resolves 373 modules instead of 1,556 (~290 ms instead of ~780 ms median on Windows, warm). test-startup-imports.js passes; test/repro/test-startup-heavy-imports.js exits 0 (NOT REPRODUCED). Co-Authored-By: Claude Opus 5.5 (1M context) --- src/search-manager.ts | 4 ++- src/tools/pdf/extract-images.ts | 5 +-- src/tools/pdf/lib/pdf2md.ts | 10 +++--- src/tools/pdf/manipulations.ts | 4 ++- src/tools/pdf/markdown.ts | 55 ++++++++++++++++++++++----------- src/utils/files/docx.ts | 21 ++++++++++--- src/utils/files/excel.ts | 22 +++++++++---- 7 files changed, 85 insertions(+), 36 deletions(-) diff --git a/src/search-manager.ts b/src/search-manager.ts index 8b111cf36..85b51d4a4 100644 --- a/src/search-manager.ts +++ b/src/search-manager.ts @@ -7,7 +7,6 @@ import { capture } from './utils/capture.js'; import { logger } from './utils/logger.js'; import { getRipgrepPath } from './utils/ripgrep-resolver.js'; import { isExcelFile } from './utils/files/index.js'; -import PizZip from 'pizzip'; export interface SearchResult { context?: boolean; // A line around a match (contextLines), not a match @@ -773,6 +772,9 @@ function characterClassEnd(glob: string, start: number): number { docxFiles = this.filterOfficeFiles(docxFiles, filePattern, rootPath); } + // Dynamically import PizZip to open the DOCX files + const { default: PizZip } = await import('pizzip'); + for (const filePath of docxFiles) { if (sink.isStopped()) break; diff --git a/src/tools/pdf/extract-images.ts b/src/tools/pdf/extract-images.ts index 472585b8e..c592f8935 100644 --- a/src/tools/pdf/extract-images.ts +++ b/src/tools/pdf/extract-images.ts @@ -1,5 +1,3 @@ -import { getDocumentProxy, extractImages } from 'unpdf'; - export interface ImageInfo { /** Object ID within PDF */ objId: number; @@ -41,6 +39,9 @@ export async function extractImagesFromPdf( pageNumbers?: number[], compressionOptions: ImageCompressionOptions = {} ): Promise> { + // unpdf is loaded here, on first use, not with this module: the server loads + // the PDF tools at startup, and most sessions never read a PDF (#715) + const { getDocumentProxy, extractImages } = await import('unpdf'); const pdfDocument = await getDocumentProxy(pdfBuffer); const pagesToProcess = pageNumbers || Array.from({ length: pdfDocument.numPages }, (_, i) => i + 1); diff --git a/src/tools/pdf/lib/pdf2md.ts b/src/tools/pdf/lib/pdf2md.ts index 0b5b3b176..24c88a8f6 100644 --- a/src/tools/pdf/lib/pdf2md.ts +++ b/src/tools/pdf/lib/pdf2md.ts @@ -4,10 +4,8 @@ import { generatePageNumbers } from '../utils.js'; import { extractImagesFromPdf, ImageInfo } from '../extract-images.js'; const require = createRequire(import.meta.url); -const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); -const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); - -type ParseResult = ReturnType; +/** What @opendocsg/pdf2md's parse() returns: its modules are loaded untyped, with require() */ +type ParseResult = any; /** @@ -69,6 +67,10 @@ export type PageRange = { * @returns A Promise that resolves to a PdfParseResult object containing the parsed data. */ export async function pdf2md(pdfBuffer: Uint8Array, pageNumbers: number[] | PageRange = []): Promise { + // @opendocsg/pdf2md is loaded here, on first use, not with this module: the + // server loads the PDF tools at startup, and most sessions never read a PDF (#715) + const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); + const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); const result = await parse(pdfBuffer); const { fonts, pages, pdfDocument } = result; diff --git a/src/tools/pdf/manipulations.ts b/src/tools/pdf/manipulations.ts index ad8209b36..a981dc336 100644 --- a/src/tools/pdf/manipulations.ts +++ b/src/tools/pdf/manipulations.ts @@ -8,7 +8,6 @@ import { z } from 'zod'; // Use createRequire to load pdf-lib as CJS (works around Node 25 ESM resolution issues) const require = createRequire(import.meta.url); -const { PDFDocument } = require('pdf-lib') as { PDFDocument: typeof PDFDocumentType }; // Infer TypeScript types from Zod schemas for consistency type PdfInsertOperation = z.infer; @@ -18,6 +17,9 @@ type PdfOperations = z.infer; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation }; async function loadPdfDocumentFromBuffer(filePathOrBuffer: string | Buffer | Uint8Array): Promise { + // pdf-lib is loaded here, on first use, not with this module: the server loads + // the PDF tools at startup, and most sessions never edit a PDF (#715) + const { PDFDocument } = require('pdf-lib') as { PDFDocument: typeof PDFDocumentType }; const buffer = typeof filePathOrBuffer === 'string' ? await fs.readFile(filePathOrBuffer) : filePathOrBuffer; const pdfBytes = new Uint8Array(buffer); return await PDFDocument.load(pdfBytes); diff --git a/src/tools/pdf/markdown.ts b/src/tools/pdf/markdown.ts index aa490ffab..746f3c547 100644 --- a/src/tools/pdf/markdown.ts +++ b/src/tools/pdf/markdown.ts @@ -8,8 +8,7 @@ import { createRequire } from 'module'; import { tmpdir, userInfo } from 'os'; import { basename, dirname, isAbsolute, join, relative, resolve, sep } from 'path'; import type { Browser, LaunchOptions, PuppeteerNode } from 'puppeteer'; -import { convertMdToPdf } from 'md-to-pdf/dist/lib/md-to-pdf.js'; -import { defaultConfig, type Config as MdToPdfConfig } from 'md-to-pdf/dist/lib/config.js'; +import type { Config as MdToPdfConfig } from 'md-to-pdf/dist/lib/config.js'; import type { PageRange } from './lib/pdf2md.js'; import { PdfParseResult, pdf2md } from './lib/pdf2md.js'; import { CONFIG_FILE } from '../../config.js'; @@ -34,13 +33,25 @@ const CHROME_EXIT_WAIT_MS = 10_000; /** Cookie a render's Chrome sends to its web server; nothing else gets in */ const RENDER_COOKIE_NAME = 'desktop-commander-pdf-render'; -// md-to-pdf's own Puppeteer, file server and front matter parser, loaded the -// way md-to-pdf loads them (they are its dependencies, not Desktop Commander's) -const requireFromMdToPdf = createRequire(createRequire(import.meta.url).resolve('md-to-pdf')); -const puppeteer: PuppeteerNode = requireFromMdToPdf('puppeteer'); -const serveHandler: (request: IncomingMessage, response: ServerResponse, config: { public: string; directoryListing: boolean; cleanUrls: boolean }) => Promise = - requireFromMdToPdf('serve-handler'); -const grayMatter: (input: string, options: unknown) => { content: string; data: unknown } = requireFromMdToPdf('gray-matter'); +const require = createRequire(import.meta.url); + +/** + * md-to-pdf, with its own Puppeteer, file server and front matter parser + * loaded the way md-to-pdf loads them (they are its dependencies, not Desktop + * Commander's). Loaded here, on first use, not with this module: the server + * loads this module at startup for the Chrome warm-up, and most sessions + * never write a PDF (#715). + */ +function loadMdToPdf() { + const requireFromMdToPdf = createRequire(require.resolve('md-to-pdf')); + const { convertMdToPdf }: typeof import('md-to-pdf/dist/lib/md-to-pdf.js') = require('md-to-pdf/dist/lib/md-to-pdf.js'); + const { defaultConfig }: typeof import('md-to-pdf/dist/lib/config.js') = require('md-to-pdf/dist/lib/config.js'); + const puppeteer: PuppeteerNode = requireFromMdToPdf('puppeteer'); + const serveHandler: (request: IncomingMessage, response: ServerResponse, config: { public: string; directoryListing: boolean; cleanUrls: boolean }) => Promise = + requireFromMdToPdf('serve-handler'); + const grayMatter: (input: string, options: unknown) => { content: string; data: unknown } = requireFromMdToPdf('gray-matter'); + return { convertMdToPdf, defaultConfig, puppeteer, serveHandler, grayMatter }; +} const isPlainObject = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -100,24 +111,24 @@ interface ResolvedRender { ignoredOptions: IgnoredRenderOption[]; } -/** md-to-pdf's switch-off of gray-matter's JavaScript engine, which evaluates the header's code */ -const DISABLED_JS_ENGINE = (defaultConfig.gray_matter_options as { engines: Record }).engines.javascript; - /** * The caller's gray_matter_options over md-to-pdf's defaults, with the * JavaScript engine off whatever the caller says: a caller's settings (even * `{}` or null) replaced the defaults and switched it back on, so a `---js` * header, or any header with `language: 'javascript'`, ran in the server. * gray-matter finds an engine by name, then by alias (js -> javascript), so - * both names are switched off. + * both names are switched off. `defaultConfig` is md-to-pdf's, loaded on first use + * (loadMdToPdf()). */ -function safeGrayMatterOptions(callerOptions: unknown): Record { +function safeGrayMatterOptions(callerOptions: unknown, defaultConfig: ReturnType['defaultConfig']): Record { + // md-to-pdf's switch-off of gray-matter's JavaScript engine, which evaluates the header's code + const disabledJsEngine = (defaultConfig.gray_matter_options as { engines: Record }).engines.javascript; const caller = isPlainObject(callerOptions) ? callerOptions : {}; const callerEngines = isPlainObject(caller.engines) ? caller.engines : {}; return { ...defaultConfig.gray_matter_options, ...caller, - engines: { ...callerEngines, js: DISABLED_JS_ENGINE, javascript: DISABLED_JS_ENGINE }, + engines: { ...callerEngines, js: disabledJsEngine, javascript: disabledJsEngine }, }; } @@ -130,10 +141,11 @@ function safeGrayMatterOptions(callerOptions: unknown): Record * md-to-pdf merging the front matter a second time. */ export function resolveRender(markdown: string, options: unknown = {}): ResolvedRender { + const { defaultConfig, grayMatter } = loadMdToPdf(); const fromOptions = isPlainObject(options) ? options : {}; // Parse the front matter the way md-to-pdf would, with the caller's // gray_matter_options, but never with gray-matter's JavaScript engine - const grayMatterOptions = safeGrayMatterOptions(fromOptions.gray_matter_options); + const grayMatterOptions = safeGrayMatterOptions(fromOptions.gray_matter_options, defaultConfig); const { content, data } = grayMatter(markdown, grayMatterOptions); const frontMatter = isPlainObject(data) ? data : {}; @@ -562,7 +574,12 @@ function nameGivenPaths(error: unknown, givenPaths: Map): void { * the check resolved, not the URL's path looked up again, so a link on the way * that is changed after the check doesn't lead elsewhere. */ -async function serveAllowedFile(request: IncomingMessage, response: ServerResponse, basedir: string): Promise { +async function serveAllowedFile( + request: IncomingMessage, + response: ServerResponse, + basedir: string, + serveHandler: ReturnType['serveHandler'] +): Promise { // Imported when used: tools/filesystem.js loads this module const { validatePath } = await import('../filesystem.js'); let file: string; @@ -595,6 +612,7 @@ async function serveAllowedFile(request: IncomingMessage, response: ServerRespon * folder listings, only from inside the allowed folders (serveAllowedFile). */ async function startRenderServer(basedir: string): Promise { + const { serveHandler } = loadMdToPdf(); const cookie = { name: RENDER_COOKIE_NAME, value: randomBytes(32).toString('hex') }; const expected = `${cookie.name}=${cookie.value}`; const server = createServer((request, response) => { @@ -602,7 +620,7 @@ async function startRenderServer(basedir: string): Promise { response.writeHead(403, { 'Content-Type': 'text/plain' }).end('Forbidden'); return; } - serveAllowedFile(request, response, basedir).catch((error) => { + serveAllowedFile(request, response, basedir, serveHandler).catch((error) => { console.error('The PDF render server could not serve a file:', error); if (!response.headersSent) response.writeHead(500); response.end(); @@ -767,6 +785,7 @@ export async function parseMarkdownToPdf(markdown: string, options: any = {}): P // The render files as the caller gave them, by the checked paths the render reads let givenPaths = new Map(); try { + const { convertMdToPdf, defaultConfig, puppeteer } = loadMdToPdf(); // The folder the markdown's files are served from must be inside the allowed folders const { validatePath } = await import('../filesystem.js'); const basedir: string = options.basedir ? await validatePath(options.basedir) : process.cwd(); diff --git a/src/utils/files/docx.ts b/src/utils/files/docx.ts index 11b029402..c7ac7697e 100644 --- a/src/utils/files/docx.ts +++ b/src/utils/files/docx.ts @@ -17,7 +17,8 @@ */ import fs from 'fs/promises'; -import PizZip from 'pizzip'; +import { createRequire } from 'module'; +import type PizZip from 'pizzip'; import { FileHandler, FileResult, FileInfo, ReadOptions, EditResult } from './base.js'; // ════════════════════════════════════════════════════════════════ @@ -71,8 +72,20 @@ interface DocxZipContents { xmlParts: Map; } +const require = createRequire(import.meta.url); + +/** + * Opens a zip from its bytes, or a new, empty one. pizzip is loaded here, on + * first use, not with this module: the server loads the file handlers at + * startup, and most sessions never open a DOCX file (#715). + */ +function openZip(data?: Buffer): PizZip { + const PizZipClass: typeof PizZip = require('pizzip'); + return data === undefined ? new PizZipClass() : new PizZipClass(data); +} + function loadDocxZip(buf: Buffer): DocxZipContents { - const zip = new PizZip(buf); + const zip = openZip(buf); const docFile = zip.file('word/document.xml'); if (!docFile) throw new Error('Invalid DOCX: missing word/document.xml'); @@ -448,7 +461,7 @@ function escapeXml(text: string): string { } function createMinimalDocxZip(documentXml: string): PizZip { - const zip = new PizZip(); + const zip = openZip(); zip.file('[Content_Types].xml', `` + @@ -651,7 +664,7 @@ export class DocxFileHandler implements FileHandler { // Load and pretty-print const buf = await fs.readFile(path); - const zip = new PizZip(buf); + const zip = openZip(buf); const docFile = zip.file('word/document.xml'); if (!docFile) throw new Error('Invalid DOCX: missing word/document.xml'); diff --git a/src/utils/files/excel.ts b/src/utils/files/excel.ts index 4f5284588..7f6b95754 100644 --- a/src/utils/files/excel.ts +++ b/src/utils/files/excel.ts @@ -3,7 +3,7 @@ * Handles reading, writing, and editing Excel files (.xlsx, .xls, .xlsm) */ -import ExcelJS from 'exceljs'; +import type ExcelJS from 'exceljs'; import fs from 'fs/promises'; import { FileHandler, @@ -14,6 +14,16 @@ import { ExcelSheet } from './base.js'; +/** + * A new exceljs Workbook. exceljs is loaded here, on first use, not with this + * module: the server loads the file handlers at startup, and most sessions + * never open a spreadsheet (#715). + */ +async function newWorkbook(): Promise { + const { default: ExcelJS } = await import('exceljs'); + return new ExcelJS.Workbook(); +} + // File size limit: 10MB const FILE_SIZE_LIMIT = 10 * 1024 * 1024; @@ -40,7 +50,7 @@ export class ExcelFileHandler implements FileHandler { async read(path: string, options?: ReadOptions): Promise { await this.checkFileSize(path); - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); const metadata = await this.extractMetadata(workbook, path); @@ -104,7 +114,7 @@ ${JSON.stringify(data)}`; // Handle append mode by finding last row and writing after it if (mode === 'append') { try { - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); if (Array.isArray(parsedContent)) { @@ -141,7 +151,7 @@ ${JSON.stringify(data)}`; } // Rewrite mode (or append to non-existent file): create new workbook - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); if (Array.isArray(parsedContent)) { // Single sheet from 2D array @@ -180,7 +190,7 @@ ${JSON.stringify(data)}`; // Parse range: "Sheet1!A1:C10" or "Sheet1" const [sheetName, cellRange] = this.parseRange(range); - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); // Get or create sheet @@ -259,7 +269,7 @@ ${JSON.stringify(data)}`; const stats = await fs.stat(path); try { - const workbook = new ExcelJS.Workbook(); + const workbook = await newWorkbook(); await workbook.xlsx.readFile(path); const metadata = await this.extractMetadata(workbook, path); From ff1d5e47277f9728d88f600bd18aa9cf454f1ae1 Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Thu, 24 Sep 2026 18:22:02 +0300 Subject: [PATCH 3/9] test(startup): the module-recording helper closes its client through closeClient() (review) Co-Authored-By: Claude Opus 5.5 (1M context) --- test/helpers/server-modules.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js index e704a6feb..24d2119c8 100644 --- a/test/helpers/server-modules.js +++ b/test/helpers/server-modules.js @@ -5,6 +5,7 @@ import { fileURLToPath, pathToFileURL } from 'url'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { isTestHome } from './test-env.js'; +import { closeClient } from './close-client.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); const SERVER = path.join(PROJECT_ROOT, 'dist/index.js'); @@ -101,7 +102,7 @@ export async function startServerRecordingModules() { }; const close = async () => { - await client.close().catch(() => {}); + await closeClient(client); fs.rmSync(logFile, { force: true }); }; From 5db439ee5df90909a6cbb69dfaf28cba1d9c406e Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Fri, 25 Sep 2026 08:43:39 +0300 Subject: [PATCH 4/9] test(startup): record the server's modules on Node before 22.15 too test-startup-imports.js and the repro test-startup-heavy-imports.js failed at client.connect ("Connection closed") on every Node before 22.15.0: checked on 18.20.8, 20.19.5, 20.20.2 and 22.14.0. Their preload imported registerHooks from node:module, which Node has only from 22.15.0 / 23.5.0. On older Node the preload failed to link ("does not provide an export named 'registerHooks'") and the server never started. The preload now uses registerHooks() where it exists. Before 22.15 it records imports through module.register() (record-modules-hooks.mjs) and requires through Module._resolveFilename, in the same log format. At the commit before, the test fails at connect on 20.20.2 and 22.14.0. Here it passes on 18.20.8, 20.20.2, 22.14.0, 22.15.0 and 24.18.0, with the same 374 modules recorded before initialize on each. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/fixtures/record-modules-hooks.mjs | 12 +++++++++ test/fixtures/record-modules-preload.mjs | 33 ++++++++++++++++++------ 2 files changed, 37 insertions(+), 8 deletions(-) create mode 100644 test/fixtures/record-modules-hooks.mjs diff --git a/test/fixtures/record-modules-hooks.mjs b/test/fixtures/record-modules-hooks.mjs new file mode 100644 index 000000000..1f801f370 --- /dev/null +++ b/test/fixtures/record-modules-hooks.mjs @@ -0,0 +1,12 @@ +// Registered by record-modules-preload.mjs with module.register() on Node +// before 22.15. Runs on Node's loader thread and writes every module an import +// resolves to DC_TEST_MODULE_LOG, in the preload's format. +import fs from 'node:fs'; + +const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); + +export async function resolve(specifier, context, nextResolve) { + const resolved = await nextResolve(specifier, context); + fs.writeSync(log, `${Date.now()} ${resolved.url} ${context.parentURL ?? ''}\n`); + return resolved; +} diff --git a/test/fixtures/record-modules-preload.mjs b/test/fixtures/record-modules-preload.mjs index 9cf380569..fe634b72a 100644 --- a/test/fixtures/record-modules-preload.mjs +++ b/test/fixtures/record-modules-preload.mjs @@ -1,15 +1,32 @@ // Preloaded into the real server (node --import) by test/helpers/server-modules.js: // writes every module the server resolves, through import or require, to // DC_TEST_MODULE_LOG as it happens, one " " line each. +// module.registerHooks() sees both, but Node has it only from 22.15. Before +// that, imports are recorded by record-modules-hooks.mjs through +// module.register(), and requires through Module._resolveFilename (which a +// require('node:...') of a built-in skips; the tests leave built-ins out). import fs from 'node:fs'; -import { registerHooks } from 'node:module'; +import Module from 'node:module'; +import { pathToFileURL } from 'node:url'; const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); +const record = (url, parentURL) => fs.writeSync(log, `${Date.now()} ${url} ${parentURL ?? ''}\n`); -registerHooks({ - resolve(specifier, context, nextResolve) { - const resolved = nextResolve(specifier, context); - fs.writeSync(log, `${Date.now()} ${resolved.url} ${context.parentURL ?? ''}\n`); - return resolved; - }, -}); +if (Module.registerHooks) { + Module.registerHooks({ + resolve(specifier, context, nextResolve) { + const resolved = nextResolve(specifier, context); + record(resolved.url, context.parentURL); + return resolved; + }, + }); +} else { + Module.register('./record-modules-hooks.mjs', import.meta.url); + const resolveFilename = Module._resolveFilename; + Module._resolveFilename = function (request, parent, ...rest) { + const filename = resolveFilename.call(this, request, parent, ...rest); + const url = Module.isBuiltin(filename) ? `node:${filename.replace(/^node:/, '')}` : pathToFileURL(filename).href; + record(url, parent?.filename && pathToFileURL(parent.filename).href); + return filename; + }; +} From 59eb0971be11a5b92b7c6446cb991939483a5743 Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Fri, 25 Sep 2026 10:59:47 +0300 Subject: [PATCH 5/9] test(startup): record the server's modules through hookArgs(), on Node 18.18-20.5 too test-startup-imports.js and the repro test-startup-heavy-imports.js failed at client.connect ("Connection closed") on Node that has --import but not module.register(): 18.18.2, 19.9.0, 20.0.0 and 20.5.1 checked. The preload's module.register() call threw "Module.register is not a function" and the server never started. The recorder now installs its hooks module (imports) with hookArgs() from test/helpers/module-hooks.js, which picks module.register() or --experimental-loader for the running Node, the same helper the search tests use. The preload only records requires (Module._resolveFilename). There is no Node version check left in the recorder; module.registerHooks() isn't used. At the commit before, the test fails at connect on those four versions. Here it passes on 18.18.2, 20.5.1, 20.6.0, 22.15.0 and 24.18.0, and the repro gives NOT REPRODUCED on 20.5.1 and 24.18.0. On the product before the #715 fix, Node 24.18.0 and 20.5.1 both catch all 7 packages: 1,183 of 1,558 modules, the same per-package counts as before (the hooks now also record the preload itself). Co-Authored-By: Claude Opus 5.5 (1M context) --- test/fixtures/record-modules-hooks.mjs | 6 ++-- test/fixtures/record-modules-preload.mjs | 38 ++++++++---------------- test/helpers/server-modules.js | 5 +++- 3 files changed, 20 insertions(+), 29 deletions(-) diff --git a/test/fixtures/record-modules-hooks.mjs b/test/fixtures/record-modules-hooks.mjs index 1f801f370..85222b552 100644 --- a/test/fixtures/record-modules-hooks.mjs +++ b/test/fixtures/record-modules-hooks.mjs @@ -1,6 +1,6 @@ -// Registered by record-modules-preload.mjs with module.register() on Node -// before 22.15. Runs on Node's loader thread and writes every module an import -// resolves to DC_TEST_MODULE_LOG, in the preload's format. +// Installed in the real server by test/helpers/server-modules.js with hookArgs() +// (helpers/module-hooks.js): writes every module an import resolves to +// DC_TEST_MODULE_LOG, in record-modules-preload.mjs's format. import fs from 'node:fs'; const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); diff --git a/test/fixtures/record-modules-preload.mjs b/test/fixtures/record-modules-preload.mjs index fe634b72a..965482865 100644 --- a/test/fixtures/record-modules-preload.mjs +++ b/test/fixtures/record-modules-preload.mjs @@ -1,10 +1,9 @@ -// Preloaded into the real server (node --import) by test/helpers/server-modules.js: -// writes every module the server resolves, through import or require, to -// DC_TEST_MODULE_LOG as it happens, one " " line each. -// module.registerHooks() sees both, but Node has it only from 22.15. Before -// that, imports are recorded by record-modules-hooks.mjs through -// module.register(), and requires through Module._resolveFilename (which a -// require('node:...') of a built-in skips; the tests leave built-ins out). +// Preloaded into the real server (node --import) by test/helpers/server-modules.js, +// which records the server's imports with record-modules-hooks.mjs: writes every +// module the server resolves through require to DC_TEST_MODULE_LOG as it happens, +// one " " line each, the hooks' format. (A +// require('node:...') of a built-in skips Module._resolveFilename; the tests +// leave built-ins out.) import fs from 'node:fs'; import Module from 'node:module'; import { pathToFileURL } from 'node:url'; @@ -12,21 +11,10 @@ import { pathToFileURL } from 'node:url'; const log = fs.openSync(process.env.DC_TEST_MODULE_LOG, 'a'); const record = (url, parentURL) => fs.writeSync(log, `${Date.now()} ${url} ${parentURL ?? ''}\n`); -if (Module.registerHooks) { - Module.registerHooks({ - resolve(specifier, context, nextResolve) { - const resolved = nextResolve(specifier, context); - record(resolved.url, context.parentURL); - return resolved; - }, - }); -} else { - Module.register('./record-modules-hooks.mjs', import.meta.url); - const resolveFilename = Module._resolveFilename; - Module._resolveFilename = function (request, parent, ...rest) { - const filename = resolveFilename.call(this, request, parent, ...rest); - const url = Module.isBuiltin(filename) ? `node:${filename.replace(/^node:/, '')}` : pathToFileURL(filename).href; - record(url, parent?.filename && pathToFileURL(parent.filename).href); - return filename; - }; -} +const resolveFilename = Module._resolveFilename; +Module._resolveFilename = function (request, parent, ...rest) { + const filename = resolveFilename.call(this, request, parent, ...rest); + const url = Module.isBuiltin(filename) ? `node:${filename.replace(/^node:/, '')}` : pathToFileURL(filename).href; + record(url, parent?.filename && pathToFileURL(parent.filename).href); + return filename; +}; diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js index 24d2119c8..be6310ae9 100644 --- a/test/helpers/server-modules.js +++ b/test/helpers/server-modules.js @@ -6,10 +6,13 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { isTestHome } from './test-env.js'; import { closeClient } from './close-client.js'; +import { hookArgs } from './module-hooks.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); const SERVER = path.join(PROJECT_ROOT, 'dist/index.js'); const PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-preload.mjs')).href; +/** Records the server's imports; the preload records its requires */ +const HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-hooks.mjs')).href; /** Packages only reading, writing or rendering Excel, PDF and DOCX files need */ export const HEAVY_PACKAGES = ['exceljs', 'pdf-lib', 'md-to-pdf', 'puppeteer', 'unpdf', '@opendocsg/pdf2md', 'pizzip']; @@ -73,7 +76,7 @@ export async function startServerRecordingModules() { const transport = new StdioClientTransport({ command: process.execPath, - args: ['--import', PRELOAD, SERVER, '--no-onboarding'], + args: [...hookArgs(HOOKS), '--import', PRELOAD, SERVER, '--no-onboarding'], cwd: PROJECT_ROOT, env: { ...process.env, DC_TEST_MODULE_LOG: logFile }, stderr: 'pipe', From 10b3c93c6d3ed7d1741fab881b41a759499152cf Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Thu, 1 Oct 2026 16:40:52 +0300 Subject: [PATCH 6/9] test(startup): heavy packages load in the background right after initialize; a call meanwhile answers at once Dmitry's review of #777: a tool call has a few seconds, so loading a package inside the call (up to 30 s on a cold machine) fails it; load them at startup, in the background, and keep initialize under 5 s. test-startup-imports.js now checks, on the real server: - nothing for Excel, PDF or DOCX is loaded before the initialize answer arrives (the cut-off is now that moment, recorded as the answer arrives, not after notifications/initialized) - shortly after it, with no tool call, all of them are loaded - with exceljs held back (test/fixtures/package-load-hooks.mjs, installed with hookArgs()), read_file of held.xlsx answers within 5 s "Can't read held.xlsx yet: Desktop Commander is still loading its Excel support ...", and read_multiple_files says so for each such file, by its name - the same call works once it has loaded - with exceljs failing to load once, read_file says it can't read held.xlsx as Excel support couldn't be loaded, with the reason, and a later call loads it - with md-to-pdf failing to load once (test/fixtures/package-load-preload.mjs, for a require()), write_pdf with markdown that starts with a link says it can't write links.pdf: markdown, as the tool reads it, not page edits The startup repro reports initialize times and fails only on packages loaded before the answer (the "loaded after the warm-up" failure is gone). The server tests that use PDF and DOCX files right after connecting (test-client-results.js, test-docx-header-footer-edit.js, the repro test-pdf-launch-failure-server.js, the integration edit-block-performance.js) call through test/helpers/heavy-packages.js callToolOnceLoaded(), which tries again while the answer says the support is still loading. At this commit the second, third, fifth and sixth checks fail: nothing loads until a call needs it, the call waits for it, and a failure is the raw error. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/fixtures/package-load-hooks.mjs | 23 +++ test/fixtures/package-load-preload.mjs | 18 ++ test/helpers/heavy-packages.js | 22 +++ test/helpers/server-modules.js | 76 ++++++-- test/integration/edit-block-performance.js | 3 +- test/repro/test-pdf-launch-failure-server.js | 3 +- test/repro/test-startup-heavy-imports.js | 33 ++-- test/test-client-results.js | 3 +- test/test-docx-header-footer-edit.js | 3 +- test/test-startup-imports.js | 193 +++++++++++++++---- 10 files changed, 300 insertions(+), 77 deletions(-) create mode 100644 test/fixtures/package-load-hooks.mjs create mode 100644 test/fixtures/package-load-preload.mjs create mode 100644 test/helpers/heavy-packages.js diff --git a/test/fixtures/package-load-hooks.mjs b/test/fixtures/package-load-hooks.mjs new file mode 100644 index 000000000..6e167d35c --- /dev/null +++ b/test/fixtures/package-load-hooks.mjs @@ -0,0 +1,23 @@ +// Installed in the real server by test/helpers/server-modules.js with hookArgs() +// when a test holds a package back or makes it fail once: +// - DC_TEST_HOLD_PACKAGE: an import of it doesn't resolve until the file +// DC_TEST_HOLD_RELEASE exists, so that package stays "still loading" +// - DC_TEST_FAIL_PACKAGE_ONCE: the first import of it fails (the file +// DC_TEST_FAILED_MARKER records that it did); later ones load it +import fs from 'node:fs'; + +const HELD = process.env.DC_TEST_HOLD_PACKAGE; +const RELEASE = process.env.DC_TEST_HOLD_RELEASE; +const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; +const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; + +export async function resolve(specifier, context, nextResolve) { + if (specifier === HELD) { + while (!fs.existsSync(RELEASE)) await new Promise((done) => setTimeout(done, 25)); + } + if (specifier === FAIL_ONCE && !fs.existsSync(FAILED_MARKER)) { + fs.writeFileSync(FAILED_MARKER, ''); + throw new Error(`${specifier} failed to load (test)`); + } + return nextResolve(specifier, context); +} diff --git a/test/fixtures/package-load-preload.mjs b/test/fixtures/package-load-preload.mjs new file mode 100644 index 000000000..e2a7dea82 --- /dev/null +++ b/test/fixtures/package-load-preload.mjs @@ -0,0 +1,18 @@ +// Preloaded into the real server (node --import) by test/helpers/server-modules.js +// when a test makes a package fail to load once: the first require() of +// DC_TEST_FAIL_PACKAGE_ONCE (or of a file in it) throws, as package-load-hooks.mjs +// does for an import; DC_TEST_FAILED_MARKER records that it did. +import fs from 'node:fs'; +import Module from 'node:module'; + +const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; +const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; + +const resolveFilename = Module._resolveFilename; +Module._resolveFilename = function (request, ...rest) { + if ((request === FAIL_ONCE || request.startsWith(`${FAIL_ONCE}/`)) && !fs.existsSync(FAILED_MARKER)) { + fs.writeFileSync(FAILED_MARKER, ''); + throw new Error(`${FAIL_ONCE} failed to load (test)`); + } + return resolveFilename.call(this, request, ...rest); +}; diff --git a/test/helpers/heavy-packages.js b/test/helpers/heavy-packages.js new file mode 100644 index 000000000..031aa5c3c --- /dev/null +++ b/test/helpers/heavy-packages.js @@ -0,0 +1,22 @@ +/** + * Tool calls for Excel, DOCX and PDF files through the real server, the way a + * client gets past the server's background load: right after initialize, while + * the support for a file type is still loading, the server answers "Can't read + * report.xlsx yet: Desktop Commander is still loading its Excel support …". + * callToolOnceLoaded() tries again until it doesn't (read_multiple_files says + * so per file). + */ + +export const STILL_LOADING = /Desktop Commander is still loading its [A-Za-z ]+ support/; + +const textOf = (result) => (result.content ?? []).map((item) => item.text ?? '').join('\n'); + +/** client.callTool(params), again every 100 ms while the answer says the support is still loading, up to timeoutMs */ +export async function callToolOnceLoaded(client, params, { timeoutMs = 60_000, requestOptions } = {}) { + const deadline = Date.now() + timeoutMs; + for (;;) { + const result = await client.callTool(params, undefined, requestOptions); + if (!STILL_LOADING.test(textOf(result)) || Date.now() > deadline) return result; + await new Promise((resolve) => setTimeout(resolve, 100)); + } +} diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js index be6310ae9..569056d50 100644 --- a/test/helpers/server-modules.js +++ b/test/helpers/server-modules.js @@ -4,6 +4,7 @@ import path from 'path'; import { fileURLToPath, pathToFileURL } from 'url'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { LoggingMessageNotificationSchema } from '@modelcontextprotocol/sdk/types.js'; import { isTestHome } from './test-env.js'; import { closeClient } from './close-client.js'; import { hookArgs } from './module-hooks.js'; @@ -13,6 +14,10 @@ const SERVER = path.join(PROJECT_ROOT, 'dist/index.js'); const PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-preload.mjs')).href; /** Records the server's imports; the preload records its requires */ const HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-hooks.mjs')).href; +/** Holds one package back, or makes it fail once (startServerRecordingModules({ holdPackage, failPackageOnce })) */ +const PACKAGE_LOAD_HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-hooks.mjs')).href; +/** Makes a package's first require() fail (startServerRecordingModules({ failPackageOnce })) */ +const PACKAGE_LOAD_PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-preload.mjs')).href; /** Packages only reading, writing or rendering Excel, PDF and DOCX files need */ export const HEAVY_PACKAGES = ['exceljs', 'pdf-lib', 'md-to-pdf', 'puppeteer', 'unpdf', '@opendocsg/pdf2md', 'pizzip']; @@ -37,8 +42,7 @@ function chromeExecutable(buildDir) { /** * Puts two Chrome for Testing builds (empty stand-ins) in Desktop Commander's * Puppeteer cache. The server's Chrome warm-up picks the newer one and removes - * the older one, which shows the warm-up has run, and never downloads Chrome. - * Returns the older build's folder. + * the older one, and never downloads Chrome. */ function seedChromeCache() { const chromeDir = path.join(os.homedir(), '.claude-server-commander', 'puppeteer-cache', 'chrome'); @@ -48,7 +52,6 @@ function seedChromeCache() { fs.mkdirSync(path.dirname(executable), { recursive: true }); fs.writeFileSync(executable, ''); } - return stale; } /** @@ -56,35 +59,72 @@ function seedChromeCache() { * does, with a preload that records every module it resolves (import and * require). Resolves once `initialize` is answered. * + * With `holdPackage` (an npm package name), the server's imports of that + * package don't resolve until release() is called: it stays "still loading". + * With `failPackageOnce`, the server's first import or require() of that package fails. + * While a package is held, keep other module loads out of the test: the + * held import can hold up the server's other imports and requires. + * * Returns: * - client: the connected MCP client - * - startedAt / initializedAt: Date.now() at spawn and once `initialize` was answered + * - startedAt: Date.now() at spawn + * - initializedAt: Date.now() when the `initialize` answer arrived, before the + * client sent `notifications/initialized` (what the server does after that + * comes later) * - modules(): every module resolved so far, first resolution each: [{ at, url, parent }] - * - waitForChromeWarmUp(timeoutMs): resolves true once the server's Chrome warm-up - * (run after the handshake) has finished, false if it hasn't within timeoutMs + * - logs(): what the server has logged so far (MCP log notifications), one string each + * - release(): lets a held package load * - close() * * It writes to Desktop Commander's folder in the home, so it runs only in a test home. */ -export async function startServerRecordingModules() { +export async function startServerRecordingModules({ holdPackage, failPackageOnce } = {}) { if (!isTestHome()) { throw new Error('startServerRecordingModules writes to the home: run it through the test or repro runner'); } - const staleChromeBuild = seedChromeCache(); + // The Chrome warm-up after the handshake finds a (stand-in) Chrome and never downloads one + seedChromeCache(); const logFile = path.join(os.homedir(), `modules-${process.pid}-${Date.now()}.log`); + const releaseFile = path.join(os.homedir(), `release-${process.pid}-${Date.now()}`); + const failedMarker = path.join(os.homedir(), `failed-${process.pid}-${Date.now()}`); fs.writeFileSync(logFile, ''); const transport = new StdioClientTransport({ command: process.execPath, - args: [...hookArgs(HOOKS), '--import', PRELOAD, SERVER, '--no-onboarding'], + args: [ + ...(holdPackage || failPackageOnce ? hookArgs(PACKAGE_LOAD_HOOKS) : []), + ...(failPackageOnce ? ['--import', PACKAGE_LOAD_PRELOAD] : []), + ...hookArgs(HOOKS), '--import', PRELOAD, SERVER, '--no-onboarding', + ], cwd: PROJECT_ROOT, - env: { ...process.env, DC_TEST_MODULE_LOG: logFile }, + env: { + ...process.env, + DC_TEST_MODULE_LOG: logFile, + ...(holdPackage ? { DC_TEST_HOLD_PACKAGE: holdPackage, DC_TEST_HOLD_RELEASE: releaseFile } : {}), + ...(failPackageOnce ? { DC_TEST_FAIL_PACKAGE_ONCE: failPackageOnce, DC_TEST_FAILED_MARKER: failedMarker } : {}), + }, stderr: 'pipe', }); + // The moment the initialize answer arrives, seen before the client acts on it + let initializedAt; + let onMessage; + Object.defineProperty(transport, 'onmessage', { + configurable: true, + get: () => onMessage, + set: (handler) => { + onMessage = handler && ((message, extra) => { + if (initializedAt === undefined && message?.result?.serverInfo) initializedAt = Date.now(); + return handler(message, extra); + }); + }, + }); const client = new Client({ name: 'startup-modules-test', version: '1.0.0' }, { capabilities: {} }); + const logLines = []; + client.setNotificationHandler(LoggingMessageNotificationSchema, (notification) => { + logLines.push(String(notification.params.data)); + }); const startedAt = Date.now(); await client.connect(transport, { timeout: 120_000 }); - const initializedAt = Date.now(); const modules = () => { const seen = new Map(); @@ -95,19 +135,15 @@ export async function startServerRecordingModules() { return [...seen.values()]; }; - const waitForChromeWarmUp = async (timeoutMs) => { - const deadline = Date.now() + timeoutMs; - while (fs.existsSync(staleChromeBuild)) { - if (Date.now() > deadline) return false; - await new Promise((resolve) => setTimeout(resolve, 50)); - } - return true; - }; + const release = () => fs.writeFileSync(releaseFile, ''); const close = async () => { + release(); await closeClient(client); fs.rmSync(logFile, { force: true }); + fs.rmSync(releaseFile, { force: true }); + fs.rmSync(failedMarker, { force: true }); }; - return { client, startedAt, initializedAt, modules, waitForChromeWarmUp, close }; + return { client, startedAt, initializedAt, modules, logs: () => [...logLines], release, close }; } diff --git a/test/integration/edit-block-performance.js b/test/integration/edit-block-performance.js index adde4a2c7..a9343d4ab 100644 --- a/test/integration/edit-block-performance.js +++ b/test/integration/edit-block-performance.js @@ -16,6 +16,7 @@ import { exitProcess } from '../../dist/utils/exit-process.js'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; +import { callToolOnceLoaded } from '../helpers/heavy-packages.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); @@ -59,7 +60,7 @@ function assertToolSuccess(result, message) { } async function callTool(client, name, args) { - return client.callTool({ name, arguments: args }, undefined, { timeout: 120000 }); + return callToolOnceLoaded(client, { name, arguments: args }, { requestOptions: { timeout: 120000 } }); } async function sleep(ms) { diff --git a/test/repro/test-pdf-launch-failure-server.js b/test/repro/test-pdf-launch-failure-server.js index 41d80d063..6323e67d7 100644 --- a/test/repro/test-pdf-launch-failure-server.js +++ b/test/repro/test-pdf-launch-failure-server.js @@ -22,6 +22,7 @@ import os from 'os'; import path from 'path'; import { fileURLToPath } from 'url'; import { closeClient } from '../helpers/close-client.js'; +import { callToolOnceLoaded } from '../helpers/heavy-packages.js'; import { exitProcess } from '../../dist/utils/exit-process.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); @@ -67,7 +68,7 @@ const remainingProfiles = () => [...profiles].filter((name) => fs.existsSync(pat let failed = false; try { - const result = await client.callTool({ + const result = await callToolOnceLoaded(client, { name: 'write_pdf', arguments: { path: path.join(tempDir, 'never-written.pdf'), diff --git a/test/repro/test-startup-heavy-imports.js b/test/repro/test-startup-heavy-imports.js index 0daeb8256..809bbcde1 100644 --- a/test/repro/test-startup-heavy-imports.js +++ b/test/repro/test-startup-heavy-imports.js @@ -11,20 +11,19 @@ // // Here the built server is started the way a client starts it (MCP over // stdio), with a preload that records every module it resolves (import and -// require). Each run reports how long `initialize` took (a measurement, not a -// verdict), how many modules were loaded before the answer and how many of -// them came in through those packages, then lists the ones loaded once -// tools/list has been answered and the Chrome warm-up after the handshake has -// run. +// require). Each run reports how long `initialize` took, counted to the +// moment its answer arrived (a measurement, not a verdict), how many modules +// were loaded before the answer and how many of them came in through those +// packages. The server loads them right after `initialize`, in the background; +// that is test-startup-imports.js's to check. // // Run: node test/repro/run-repro.js test-startup-heavy-imports.js // (REPRO_RUNS=5 starts by default) -// Exit code: 1 if any of those packages is loaded by then. +// Exit code: 1 if any of those packages is loaded before the answer. import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from '../helpers/server-modules.js'; import { exitProcess } from '../../dist/utils/exit-process.js'; const RUNS = Number(process.env.REPRO_RUNS || 5); -const WARM_UP_TIMEOUT_MS = 60_000; /** For each module, the heavy package it was first loaded through, if any */ function heavyPackageBehind(modules) { @@ -63,23 +62,21 @@ const startupMs = []; for (let run = 1; run <= RUNS; run++) { const server = await startServerRecordingModules(); try { - const beforeAnswer = server.modules().filter((module) => module.at <= server.initializedAt); + // Before the answer arrived: the server's background load starts only after it + const beforeAnswer = server.modules().filter((module) => module.at < server.initializedAt); + const loaded = heavyLoaded(beforeAnswer); + if (loaded.length > 0) runsLoadingHeavy++; startupMs.push(server.initializedAt - server.startedAt); console.log(`start ${run}: initialize answered after ${server.initializedAt - server.startedAt} ms; loaded before it: ${describe(beforeAnswer)}`); - - await server.client.listTools(); - const warmedUp = await server.waitForChromeWarmUp(WARM_UP_TIMEOUT_MS); - const loaded = heavyLoaded(server.modules()); - if (loaded.length > 0) runsLoadingHeavy++; - console.log(` after tools/list and the Chrome warm-up${warmedUp ? '' : ' (still running)'}: ${loaded.length ? `loaded ${loaded.join(', ')}` : 'none of those packages loaded'}`); } finally { await server.close(); } } -startupMs.sort((a, b) => a - b); -const median = startupMs[Math.floor(startupMs.length / 2)]; +const sorted = [...startupMs].sort((a, b) => a - b); +const median = sorted[Math.floor(sorted.length / 2)]; +const times = `initialize answered after ${startupMs.join(', ')} ms (median ${median} ms)`; console.log(runsLoadingHeavy > 0 - ? `REPRODUCED: ${runsLoadingHeavy} of ${RUNS} starts loaded Excel/PDF/DOCX packages without opening any such file (initialize answered after ${median} ms median)` - : `NOT REPRODUCED: ${RUNS} starts, none loaded Excel/PDF/DOCX packages (initialize answered after ${median} ms median)`); + ? `REPRODUCED: ${runsLoadingHeavy} of ${RUNS} starts loaded Excel/PDF/DOCX packages before answering initialize; ${times}` + : `NOT REPRODUCED: ${RUNS} starts, none loaded Excel/PDF/DOCX packages before answering initialize; ${times}`); exitProcess(runsLoadingHeavy > 0 ? 1 : 0); diff --git a/test/test-client-results.js b/test/test-client-results.js index 953b0f2d4..c57e7c050 100644 --- a/test/test-client-results.js +++ b/test/test-client-results.js @@ -15,6 +15,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { runIfMain, skip } from './helpers/run-if-main.js'; import { closeClient } from './helpers/close-client.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -23,7 +24,7 @@ const textOf = (result) => result.content?.find((block) => block.type === 'text' /** write_pdf with an option Desktop Commander ignores: the answer is the plain success line */ async function writePdfIgnoringAnOption(client, dir) { const target = path.join(dir, 'ignored-option.pdf'); - const result = await client.callTool({ + const result = await callToolOnceLoaded(client, { name: 'write_pdf', arguments: { path: target, content: '# Client results\n', options: { devtools: true } }, }); diff --git a/test/test-docx-header-footer-edit.js b/test/test-docx-header-footer-edit.js index 78892b17d..ae14ce3d1 100644 --- a/test/test-docx-header-footer-edit.js +++ b/test/test-docx-header-footer-edit.js @@ -19,6 +19,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { runIfMain } from './helpers/run-if-main.js'; import { closeClient } from './helpers/close-client.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const PizZip = createRequire(import.meta.url)('pizzip'); @@ -77,7 +78,7 @@ export default async function runTests() { try { await client.connect(transport, { timeout: 30_000 }); const file = path.join(dir, 'sections.docx'); - await client.callTool({ name: 'write_file', arguments: { path: file, content: '# Title\n\nBody text' } }); + await callToolOnceLoaded(client, { name: 'write_file', arguments: { path: file, content: '# Title\n\nBody text' } }); const texts = addSections(file); const outline = textOf(await client.callTool({ name: 'read_file', arguments: { path: file } })); diff --git a/test/test-startup-imports.js b/test/test-startup-imports.js index 8a9b7e53a..6ad17deae 100644 --- a/test/test-startup-imports.js +++ b/test/test-startup-imports.js @@ -1,26 +1,37 @@ /** * Starting the server must not load the packages only Excel, PDF and DOCX - * files need (#715). Every launch loaded them before answering `initialize`, - * whether or not the session ever opened such a file: 1,183 of the 1,556 - * modules loaded before the answer, which one reporter saw take 25-90 s and - * time the client out. Each package still loads the first time a file needs it. + * files need before it answers `initialize` (#715). Every launch loaded them + * first, whether or not the session ever opened such a file: 1,183 of the + * 1,556 modules loaded before the answer, which one reporter saw take 25-90 s + * and time the client out. Nor may a tool call wait for one to load: a client + * gives a call a few seconds (review on #777). So right after `initialize` the + * server loads them in the background, one at a time; a call that needs one + * still loading answers at once that it is still loading, and works once it + * is loaded. A load that fails is not kept: the call says so, and a later call + * loads it again. * * Starts the real server (dist/index.js) over MCP stdio, the way a client * does, recording every module it resolves (import and require). */ import assert from 'assert'; +import ExcelJS from 'exceljs'; import fs from 'fs'; import os from 'os'; import path from 'path'; import { fileURLToPath } from 'url'; import { HEAVY_PACKAGES, packageOf, startServerRecordingModules } from './helpers/server-modules.js'; +import { callToolOnceLoaded } from './helpers/heavy-packages.js'; import { isTestHome } from './helpers/test-env.js'; import { runIfMain, skip } from './helpers/run-if-main.js'; const PROJECT_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const SAMPLE_PDF = path.join(PROJECT_ROOT, 'test/samples/01_sample_simple.pdf'); -const WARM_UP_TIMEOUT_MS = 60_000; +/** The background load takes a few seconds at most; far above that */ +const LOAD_DEADLINE_MS = 30_000; +/** An answer "at once": a refused call takes milliseconds; far above that */ +const AT_ONCE_MS = 5_000; +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); const heavyLoaded = (modules) => HEAVY_PACKAGES.filter((pkg) => modules.some((module) => packageOf(module.url) === pkg)); function text(result) { @@ -28,35 +39,40 @@ function text(result) { } async function callTool(client, name, args) { - const result = await client.callTool({ name, arguments: args }); + const result = await callToolOnceLoaded(client, { name, arguments: args }); assert.notStrictEqual(result.isError, true, `${name} ${JSON.stringify(args)} failed: ${text(result)}`); return text(result); } -/** Nothing for Excel, PDF or DOCX files is loaded before the server answers `initialize` */ +/** Nothing for Excel, PDF or DOCX is loaded before the server answers `initialize` */ async function testNothingLoadedBeforeInitialize(server) { - const beforeAnswer = server.modules().filter((module) => module.at <= server.initializedAt && !module.url.startsWith('node:')); + // Before the answer arrived: the server starts its background load only after it + const beforeAnswer = server.modules().filter((module) => module.at < server.initializedAt && !module.url.startsWith('node:')); const loaded = heavyLoaded(beforeAnswer); assert.deepStrictEqual(loaded, [], - `with no Excel, PDF or DOCX file opened, the server loaded ${loaded.join(', ')} before answering initialize ` + + `the server loaded ${loaded.join(', ')} before answering initialize ` + `(${beforeAnswer.length} modules resolved before the answer)`); console.log(`✓ Nothing for Excel, PDF or DOCX loaded before initialize was answered (${beforeAnswer.length} modules)`); } -/** Nor once the tools are listed and the Chrome warm-up after the handshake has run */ -async function testNothingLoadedAfterWarmUp(server) { - await server.client.listTools(); - assert(await server.waitForChromeWarmUp(WARM_UP_TIMEOUT_MS), - `the Chrome warm-up after the handshake did not finish within ${WARM_UP_TIMEOUT_MS} ms`); - const loaded = heavyLoaded(server.modules()); - assert.deepStrictEqual(loaded, [], - `with no Excel, PDF or DOCX file opened, the server loaded ${loaded.join(', ')} by the time tools/list ` + - 'was answered and the Chrome warm-up had run'); - console.log('✓ Nothing for Excel, PDF or DOCX loaded after tools/list and the Chrome warm-up'); +/** Shortly after it, all of them are loaded, without any tool call */ +async function testLoadedSoonAfterInitialize(server) { + const deadline = Date.now() + LOAD_DEADLINE_MS; + let missing = HEAVY_PACKAGES; + while (missing.length > 0 && Date.now() < deadline) { + await sleep(100); + const loaded = heavyLoaded(server.modules()); + missing = HEAVY_PACKAGES.filter((pkg) => !loaded.includes(pkg)); + } + assert.deepStrictEqual(missing, [], + `${LOAD_DEADLINE_MS / 1000} s after initialize, with no tool call, the server had not loaded ${missing.join(', ')}: ` + + 'the first call that needs them loads them inside the call, which a client gives only a few seconds'); + const lastAt = Math.max(...server.modules().filter((module) => HEAVY_PACKAGES.includes(packageOf(module.url))).map((module) => module.at)); + console.log(`✓ All of them loaded in the background, the last ${lastAt - server.initializedAt} ms after initialize was answered`); } -/** Each package loads when a file first needs it, and the file works */ -async function testLoadedOnFirstUse(server, dir) { +/** Excel, DOCX and PDF files work once loaded */ +async function testFilesWorkOnceLoaded(server, dir) { const { client } = server; const xlsx = path.join(dir, 'budget.xlsx'); await callTool(client, 'write_file', { path: xlsx, content: JSON.stringify([['Item', 'Note'], ['Alice', 'ZebraQuartz budget']]) }); @@ -71,12 +87,79 @@ async function testLoadedOnFirstUse(server, dir) { assert((await callTool(client, 'read_file', { path: SAMPLE_PDF })).trim().length > 0, 'reading a PDF through the server should return its text'); + console.log('✓ Excel, DOCX and PDF files work once their support is loaded'); +} - const loaded = heavyLoaded(server.modules()); - for (const pkg of ['exceljs', 'pizzip', '@opendocsg/pdf2md']) { - assert(loaded.includes(pkg), `${pkg} should be loaded once a file needed it; loaded: ${loaded.join(', ') || 'none'}`); +/** A call that needs a package still loading answers at once that it is still loading */ +async function testCallWhileLoadingAnswersAtOnce(server, file) { + const started = Date.now(); + const call = server.client.callTool({ name: 'read_file', arguments: { path: file } }, undefined, { timeout: 120_000 }); + const answer = await Promise.race([call, sleep(AT_ONCE_MS).then(() => null)]); + try { + assert(answer, + `with Excel support still loading, read_file of an .xlsx gave no answer within ${AT_ONCE_MS / 1000} s: ` + + 'it waited for exceljs to load inside the call, and a client gives a call only a few seconds'); + assert(answer.isError === true && /Can't read held\.xlsx yet: Desktop Commander is still loading its Excel support/.test(text(answer)), + `with Excel support still loading, read_file of held.xlsx should answer at once that it can't read held.xlsx yet, as Excel support is still loading; it answered: ${text(answer)}`); + console.log(`✓ A call while Excel support is loading answers in ${Date.now() - started} ms: ${text(answer)}`); + } finally { + server.release(); + await call.catch(() => {}); + } +} + +/** read_multiple_files says so for each file that needs it, by its name */ +async function testReadMultipleFilesWhileLoading(server, file, second) { + const answer = text(await server.client.callTool({ name: 'read_multiple_files', arguments: { paths: [file, second] } })); + for (const name of [path.basename(file), path.basename(second)]) { + const escaped = name.replace(/\./g, '\\.'); + assert(new RegExp(`${escaped}: Error - Can't read ${escaped} yet: Desktop Commander is still loading its Excel support`).test(answer), + `with Excel support still loading, read_multiple_files should say for ${name} that it can't read ${name} yet; it answered: ${answer}`); + } + console.log('✓ read_multiple_files says for each file, by name, that it can\'t be read yet'); +} + +/** write_pdf with markdown that starts with a link is writing a PDF, not editing one, as the tool reads it */ +async function testWritePdfMarkdownStartingWithLink(server, dir) { + // PDF writing (md-to-pdf) failed to load once; PDF editing (pdf-lib) is loaded + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading md-to-pdf failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'write_pdf', arguments: { path: path.join(dir, 'links.pdf'), content: '[Home](https://example.com)\n\n# Links' } }); + assert(answer.isError === true && /Can't write links\.pdf: Desktop Commander couldn't load its PDF writing support/.test(text(answer)), + `write_pdf with markdown starting with a link, while PDF writing can't be loaded, should say it can't write links.pdf ` + + `(markdown, as the tool reads it, not page edits); it answered: ${text(answer)}`); + console.log(`✓ write_pdf with markdown starting with a link is writing a PDF: ${text(answer)}`); +} + +/** The same call works once the package is loaded */ +async function testSameCallWorksOnceLoaded(server, file) { + assert(/ZebraQuartz held/.test(await callTool(server.client, 'read_file', { path: file })), + 'once Excel support has loaded, read_file of the .xlsx should show its cells'); + console.log('✓ The same call works once Excel support has loaded'); +} + +/** A failed load isn't kept: the call says so, with the reason, and a later call loads it again */ +async function testFailedLoadIsNotKept(server, file) { + // Calls only once the background load has tried exceljs and logged the failure + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading exceljs failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'read_file', arguments: { path: file } }); + assert(answer.isError === true && /Can't read held\.xlsx: Desktop Commander couldn't load its Excel support \(.*exceljs failed to load \(test\)/.test(text(answer)), + `when exceljs failed to load, read_file of held.xlsx should say it can't read held.xlsx as Excel support couldn't be loaded, and why; it answered: ${text(answer)}`); + assert(/ZebraQuartz held/.test(await callTool(server.client, 'read_file', { path: file })), + 'after a failed load, a later read_file of the .xlsx should load Excel support again and show its cells'); + console.log(`✓ A failed load isn't kept: "${text(answer)}", and a later call loads it`); +} + +async function runCases(failures, cases) { + for (const [check, ...args] of cases) { + try { + await check(...args); + } catch (error) { + failures.push(error); + console.error(`❌ ${check.name}: ${error.message}`); + } } - console.log('✓ Excel, DOCX and PDF files work on first use, loading their packages then'); } export default async function runTests() { @@ -86,19 +169,59 @@ export default async function runTests() { } const dir = fs.mkdtempSync(path.join(os.homedir(), 'startup-imports-')); const failures = []; - let server; try { - server = await startServerRecordingModules(); - for (const test of [testNothingLoadedBeforeInitialize, testNothingLoadedAfterWarmUp, testLoadedOnFirstUse]) { - try { - await test(server, dir); - } catch (error) { - failures.push(error); - console.error(`❌ ${test.name}: ${error.message}`); - } + const server = await startServerRecordingModules(); + try { + await runCases(failures, [ + [testNothingLoadedBeforeInitialize, server], + [testLoadedSoonAfterInitialize, server], + [testFilesWorkOnceLoaded, server, dir], + ]); + } finally { + await server.close(); + } + + // Excel support held back: exceljs doesn't load until the server is released + const held = path.join(dir, 'held.xlsx'); + const heldToo = path.join(dir, 'held-too.xlsx'); + const workbook = new ExcelJS.Workbook(); + workbook.addWorksheet('Sheet1').addRow(['Item', 'ZebraQuartz held']); + await workbook.xlsx.writeFile(held); + fs.copyFileSync(held, heldToo); + const holding = await startServerRecordingModules({ holdPackage: 'exceljs' }); + try { + await runCases(failures, [ + [testCallWhileLoadingAnswersAtOnce, holding, held], + [testSameCallWorksOnceLoaded, holding, held], + ]); + } finally { + await holding.close(); + } + // A server of its own: after a call that succeeds, the server loads modules + // of its own, which a held import would hold up (see startServerRecordingModules) + const holdingToo = await startServerRecordingModules({ holdPackage: 'exceljs' }); + try { + await runCases(failures, [[testReadMultipleFilesWhileLoading, holdingToo, held, heldToo]]); + } finally { + await holdingToo.close(); + } + + // Excel support failing to load once + const failing = await startServerRecordingModules({ failPackageOnce: 'exceljs' }); + try { + await runCases(failures, [[testFailedLoadIsNotKept, failing, held]]); + } finally { + await failing.close(); + } + + // PDF writing failing to load once + const failingPdf = await startServerRecordingModules({ failPackageOnce: 'md-to-pdf' }); + try { + await runCases(failures, [[testWritePdfMarkdownStartingWithLink, failingPdf, dir]]); + } finally { + await failingPdf.close(); } } finally { - await server?.close(); fs.rmSync(dir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); } if (failures.length > 0) { From 938f1df91c49860f82e87e89070de1314965d359 Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Thu, 1 Oct 2026 16:40:52 +0300 Subject: [PATCH 7/9] fix(startup): load the Excel, DOCX and PDF packages in the background right after initialize Since #777's first fix, initialize loaded none of the Excel, DOCX and PDF packages, but the first call that needed one loaded it inside the call. On the reporter's cold Windows machine that took up to 9 s for exceljs, longer than the few seconds a client gives a tool call (Dmitry's review). src/utils/heavy-packages.ts loads them right after initialize (from server.oninitialized, next to the Chrome check), one package at a time with a turn of the event loop between them, so each pause of the main thread is one package. It reuses each module's own loader, now exported: loadExcelJS, loadPizZip, loadPdf2md, loadUnpdf, loadMdToPdf, loadPdfLib; search-manager.ts uses the first two instead of its own imports. Until a file type's packages are loaded, read_file, write_file, edit_block and write_pdf on that type answer at once with an error naming the file and what the call does ("Can't read report.xlsx yet: Desktop Commander is still loading its Excel support (it starts right after launch). Try again in a few seconds."), and read_multiple_files says so for that file. write_pdf is markdown or page edits as the tool reads it: its arguments are parsed with WritePdfArgsSchema. The check sits in handleCallToolRequest (server.ts), so code called without the server loads on demand as before. A load that fails is not kept: the call says it couldn't be loaded, with the reason, and starts it again. get_file_info keeps its basic-info fallback; a content search waits for the load. test-search-office-completion.js's failed-Office-search case makes exceljs fail where the search now loads it (utils/files/excel.js) instead of in search-manager.js. The tests of the commit before pass here, on Windows and macOS. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/handlers/filesystem-handlers.ts | 4 +- src/index.ts | 5 ++ src/search-manager.ts | 13 ++-- src/server.ts | 63 ++++++++++++++- src/tools/filesystem.ts | 11 ++- src/tools/pdf/extract-images.ts | 13 +++- src/tools/pdf/lib/pdf2md.ts | 16 +++- src/tools/pdf/manipulations.ts | 13 +++- src/tools/pdf/markdown.ts | 8 +- src/utils/files/docx.ts | 13 +++- src/utils/files/excel.ts | 16 ++-- src/utils/heavy-packages.ts | 106 ++++++++++++++++++++++++++ test/test-search-office-completion.js | 5 +- 13 files changed, 251 insertions(+), 35 deletions(-) create mode 100644 src/utils/heavy-packages.ts diff --git a/src/handlers/filesystem-handlers.ts b/src/handlers/filesystem-handlers.ts index fadd05e79..a8f7c7ad9 100644 --- a/src/handlers/filesystem-handlers.ts +++ b/src/handlers/filesystem-handlers.ts @@ -243,9 +243,9 @@ export async function handleReadFile(args: unknown): Promise { /** * Handle read_multiple_files command */ -export async function handleReadMultipleFiles(args: unknown): Promise { +export async function handleReadMultipleFiles(args: unknown, notReady?: (filePath: string) => string | undefined): Promise { const parsed = ReadMultipleFilesArgsSchema.parse(args); - const fileResults = await readMultipleFiles(parsed.paths); + const fileResults = await readMultipleFiles(parsed.paths, notReady); // Create a text summary of all files const textSummary = fileResults.map(result => { diff --git a/src/index.ts b/src/index.ts index 60d22f57b..55acecedc 100644 --- a/src/index.ts +++ b/src/index.ts @@ -15,6 +15,7 @@ import { logToStderr, logger } from './utils/logger.js'; import { exitProcess } from './utils/exit-process.js'; import { runRemote } from './npm-scripts/remote.js'; import { ensureChromeAvailable } from './tools/pdf/markdown.js'; +import { startBackgroundLoad } from './utils/heavy-packages.js'; // Store messages to defer until after initialization const deferredMessages: Array<{ level: string, message: string }> = []; @@ -136,6 +137,10 @@ async function runServer() { // Preemptively check/download Chrome for PDF generation (runs in background) ensureChromeAvailable(); + + // Load the Excel, DOCX and PDF packages, one at a time, now that initialize + // has been answered; until then the calls that need them answer at once + void startBackgroundLoad(); }; await server.connect(transport); diff --git a/src/search-manager.ts b/src/search-manager.ts index 85b51d4a4..98ffc3d60 100644 --- a/src/search-manager.ts +++ b/src/search-manager.ts @@ -7,6 +7,8 @@ import { capture } from './utils/capture.js'; import { logger } from './utils/logger.js'; import { getRipgrepPath } from './utils/ripgrep-resolver.js'; import { isExcelFile } from './utils/files/index.js'; +import { loadExcelJS } from './utils/files/excel.js'; +import { loadPizZip } from './utils/files/docx.js'; export interface SearchResult { context?: boolean; // A line around a match (contextLines), not a match @@ -575,14 +577,15 @@ function characterClassEnd(glob: string, start: number): number { excelFiles = this.filterOfficeFiles(excelFiles, filePattern, rootPath); } - // Dynamically import ExcelJS to search all sheets - const ExcelJS = await import('exceljs'); + // ExcelJS, to search all sheets; while the server is still loading it in + // the background, this waits for it (the search runs in the background too) + const ExcelJS = await loadExcelJS(); for (const filePath of excelFiles) { if (sink.isStopped()) break; try { - const workbook = new ExcelJS.default.Workbook(); + const workbook = new ExcelJS.Workbook(); await workbook.xlsx.readFile(filePath); // Search ALL sheets in the workbook (row-wise for speed and cross-column matching) @@ -772,8 +775,8 @@ function characterClassEnd(glob: string, start: number): number { docxFiles = this.filterOfficeFiles(docxFiles, filePattern, rootPath); } - // Dynamically import PizZip to open the DOCX files - const { default: PizZip } = await import('pizzip'); + // PizZip, to open the DOCX files + const PizZip = loadPizZip(); for (const filePath of docxFiles) { if (sink.isStopped()) break; diff --git a/src/server.ts b/src/server.ts index bb035af02..b354d80a8 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1202,6 +1202,61 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { import * as handlers from './handlers/index.js'; import { ServerResult } from './types.js'; import { withoutInternalFacts } from './utils/internal-facts.js'; +import { createErrorResponse } from './error-handlers.js'; +import { isExcelFile } from './utils/files/index.js'; +import { isReady, notReadyMessage, type FileAction, type HeavySupport } from './utils/heavy-packages.js'; + +/** What a file needs from utils/heavy-packages.ts to be read, written or edited */ +function heavySupportFor(filePath: string, action: FileAction): HeavySupport[] { + const lower = filePath.toLowerCase(); + if (isExcelFile(lower)) return ['excel']; + if (lower.endsWith('.docx')) return ['docx']; + if (lower.endsWith('.pdf')) { + // Editing a PDF can insert markdown pages, which are rendered + return action === 'read' ? ['pdfRead'] : action === 'write' ? ['pdfWrite'] : ['pdfEdit', 'pdfWrite']; + } + return []; +} + +/** + * If `action` on `filePath` needs support not loaded yet (by default, what its + * file type needs), the error the call answers with at once, naming the file + */ +function notReady(filePath: unknown, action: FileAction, supports?: HeavySupport[]): string | undefined { + if (typeof filePath !== 'string') return undefined; + const pending = (supports ?? heavySupportFor(filePath, action)).find((support) => !isReady(support)); + return pending && notReadyMessage(pending, filePath, action); +} + +/** + * A tool call on an Excel, DOCX or PDF file whose support is still loading + * in the background (utils/heavy-packages.ts) answers at once with this error + * instead of loading it inside the call; undefined for every other call. + * read_multiple_files answers per file (readMultipleFiles), get_file_info + * falls back to the basic info, and a search waits for the load. + */ +function heavySupportNotReady(name: string, args: unknown): string | undefined { + const params = (args && typeof args === 'object' ? args : {}) as Record; + switch (name) { + case 'read_file': + return notReady(params.path, 'read'); + case 'write_file': + return notReady(params.path, 'write'); + case 'edit_block': + return notReady(params.file_path, 'edit'); + case 'write_pdf': { + // Markdown (a new PDF) or page edits, read exactly as the tool reads + // them; arguments the tool refuses get its own error + const parsed = WritePdfArgsSchema.safeParse(args); + if (!parsed.success) return undefined; + return typeof parsed.data.content === 'string' + ? notReady(parsed.data.path, 'write', ['pdfWrite']) + : notReady(parsed.data.path, 'edit', ['pdfEdit', 'pdfWrite']); + } + default: + return undefined; + } +} server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest): Promise => { const args = request.params.arguments; @@ -1292,7 +1347,11 @@ async function handleCallToolRequest(request: CallToolRequest): Promise notReady(filePath, 'read')); break; case "write_file": diff --git a/src/tools/filesystem.ts b/src/tools/filesystem.ts index f84a796dd..0599d5ebd 100644 --- a/src/tools/filesystem.ts +++ b/src/tools/filesystem.ts @@ -750,9 +750,18 @@ export interface MultiFileResult { payload?: FileResultPayloads; } -export async function readMultipleFiles(paths: string[]): Promise { +/** + * Reads each file; a failure is that file's `error`. `notReady` (the server's) + * gives the error for a file whose support is still loading, which is then + * not read (utils/heavy-packages.ts). + */ +export async function readMultipleFiles(paths: string[], notReady?: (filePath: string) => string | undefined): Promise { return Promise.all( paths.map(async (filePath: string) => { + const notReadyError = notReady?.(filePath); + if (notReadyError) { + return { path: filePath, error: notReadyError }; + } try { const validPath = await validatePath(filePath); const fileResult = await readFile(validPath); diff --git a/src/tools/pdf/extract-images.ts b/src/tools/pdf/extract-images.ts index c592f8935..79e5f5cd6 100644 --- a/src/tools/pdf/extract-images.ts +++ b/src/tools/pdf/extract-images.ts @@ -27,6 +27,15 @@ export interface ImageCompressionOptions { maxDimension?: number; } +/** + * unpdf, loaded when first used, not with this module: the server loads the + * PDF tools at startup, before it answers initialize (#715). The server loads + * it right after initialize (utils/heavy-packages.ts). + */ +export function loadUnpdf(): Promise { + return import('unpdf'); +} + /** * Optimized image extraction from PDF using unpdf's built-in extractImages method * @param pdfBuffer PDF file as Uint8Array @@ -39,9 +48,7 @@ export async function extractImagesFromPdf( pageNumbers?: number[], compressionOptions: ImageCompressionOptions = {} ): Promise> { - // unpdf is loaded here, on first use, not with this module: the server loads - // the PDF tools at startup, and most sessions never read a PDF (#715) - const { getDocumentProxy, extractImages } = await import('unpdf'); + const { getDocumentProxy, extractImages } = await loadUnpdf(); const pdfDocument = await getDocumentProxy(pdfBuffer); const pagesToProcess = pageNumbers || Array.from({ length: pdfDocument.numPages }, (_, i) => i + 1); diff --git a/src/tools/pdf/lib/pdf2md.ts b/src/tools/pdf/lib/pdf2md.ts index 24c88a8f6..845e5d8ab 100644 --- a/src/tools/pdf/lib/pdf2md.ts +++ b/src/tools/pdf/lib/pdf2md.ts @@ -7,6 +7,17 @@ const require = createRequire(import.meta.url); /** What @opendocsg/pdf2md's parse() returns: its modules are loaded untyped, with require() */ type ParseResult = any; +/** + * @opendocsg/pdf2md, loaded when first used, not with this module: the server + * loads the PDF tools at startup, before it answers initialize (#715). The + * server loads it right after initialize (utils/heavy-packages.ts). + */ +export function loadPdf2md(): { parse: (pdfBuffer: Uint8Array) => Promise; makeTransformations: any; transform: any } { + const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); + const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); + return { parse, makeTransformations, transform }; +} + /** * PDF metadata structure @@ -67,10 +78,7 @@ export type PageRange = { * @returns A Promise that resolves to a PdfParseResult object containing the parsed data. */ export async function pdf2md(pdfBuffer: Uint8Array, pageNumbers: number[] | PageRange = []): Promise { - // @opendocsg/pdf2md is loaded here, on first use, not with this module: the - // server loads the PDF tools at startup, and most sessions never read a PDF (#715) - const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); - const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); + const { parse, makeTransformations, transform } = loadPdf2md(); const result = await parse(pdfBuffer); const { fonts, pages, pdfDocument } = result; diff --git a/src/tools/pdf/manipulations.ts b/src/tools/pdf/manipulations.ts index a981dc336..d02cd89e9 100644 --- a/src/tools/pdf/manipulations.ts +++ b/src/tools/pdf/manipulations.ts @@ -16,10 +16,17 @@ type PdfOperations = z.infer; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation }; +/** + * pdf-lib, loaded when first used, not with this module: the server loads the + * PDF tools at startup, before it answers initialize (#715). The server loads + * it right after initialize (utils/heavy-packages.ts). + */ +export function loadPdfLib(): { PDFDocument: typeof PDFDocumentType } { + return require('pdf-lib'); +} + async function loadPdfDocumentFromBuffer(filePathOrBuffer: string | Buffer | Uint8Array): Promise { - // pdf-lib is loaded here, on first use, not with this module: the server loads - // the PDF tools at startup, and most sessions never edit a PDF (#715) - const { PDFDocument } = require('pdf-lib') as { PDFDocument: typeof PDFDocumentType }; + const { PDFDocument } = loadPdfLib(); const buffer = typeof filePathOrBuffer === 'string' ? await fs.readFile(filePathOrBuffer) : filePathOrBuffer; const pdfBytes = new Uint8Array(buffer); return await PDFDocument.load(pdfBytes); diff --git a/src/tools/pdf/markdown.ts b/src/tools/pdf/markdown.ts index 746f3c547..063e7bb6f 100644 --- a/src/tools/pdf/markdown.ts +++ b/src/tools/pdf/markdown.ts @@ -38,11 +38,11 @@ const require = createRequire(import.meta.url); /** * md-to-pdf, with its own Puppeteer, file server and front matter parser * loaded the way md-to-pdf loads them (they are its dependencies, not Desktop - * Commander's). Loaded here, on first use, not with this module: the server - * loads this module at startup for the Chrome warm-up, and most sessions - * never write a PDF (#715). + * Commander's). Loaded when first used, not with this module: the server + * loads this module at startup, before it answers initialize (#715). The + * server loads it right after initialize (utils/heavy-packages.ts). */ -function loadMdToPdf() { +export function loadMdToPdf() { const requireFromMdToPdf = createRequire(require.resolve('md-to-pdf')); const { convertMdToPdf }: typeof import('md-to-pdf/dist/lib/md-to-pdf.js') = require('md-to-pdf/dist/lib/md-to-pdf.js'); const { defaultConfig }: typeof import('md-to-pdf/dist/lib/config.js') = require('md-to-pdf/dist/lib/config.js'); diff --git a/src/utils/files/docx.ts b/src/utils/files/docx.ts index c7ac7697e..37cb9682c 100644 --- a/src/utils/files/docx.ts +++ b/src/utils/files/docx.ts @@ -75,12 +75,17 @@ interface DocxZipContents { const require = createRequire(import.meta.url); /** - * Opens a zip from its bytes, or a new, empty one. pizzip is loaded here, on - * first use, not with this module: the server loads the file handlers at - * startup, and most sessions never open a DOCX file (#715). + * pizzip, loaded when first used, not with this module: the server loads the + * file handlers at startup, before it answers initialize (#715). The server + * loads it right after initialize (utils/heavy-packages.ts). */ +export function loadPizZip(): typeof PizZip { + return require('pizzip'); +} + +/** Opens a zip from its bytes, or a new, empty one */ function openZip(data?: Buffer): PizZip { - const PizZipClass: typeof PizZip = require('pizzip'); + const PizZipClass = loadPizZip(); return data === undefined ? new PizZipClass() : new PizZipClass(data); } diff --git a/src/utils/files/excel.ts b/src/utils/files/excel.ts index 7f6b95754..e2efc4a42 100644 --- a/src/utils/files/excel.ts +++ b/src/utils/files/excel.ts @@ -15,13 +15,19 @@ import { } from './base.js'; /** - * A new exceljs Workbook. exceljs is loaded here, on first use, not with this - * module: the server loads the file handlers at startup, and most sessions - * never open a spreadsheet (#715). + * exceljs, loaded when first used, not with this module: the server loads the + * file handlers at startup, before it answers initialize (#715). The server + * loads it right after initialize (utils/heavy-packages.ts). */ +export async function loadExcelJS(): Promise { + const { default: ExcelJSModule } = await import('exceljs'); + return ExcelJSModule; +} + +/** A new exceljs Workbook */ async function newWorkbook(): Promise { - const { default: ExcelJS } = await import('exceljs'); - return new ExcelJS.Workbook(); + const ExcelJSModule = await loadExcelJS(); + return new ExcelJSModule.Workbook(); } // File size limit: 10MB diff --git a/src/utils/heavy-packages.ts b/src/utils/heavy-packages.ts new file mode 100644 index 000000000..598a5fc15 --- /dev/null +++ b/src/utils/heavy-packages.ts @@ -0,0 +1,106 @@ +/** + * The packages only Excel, DOCX and PDF files need, loaded in the background + * right after initialize, one at a time (#715, review on #777). + * + * Loading them before answering initialize took 25-90 s on one reporter's + * machine and timed the client out, and loading one inside the tool call that + * first needs it can take longer than the few seconds a client gives a call. + * So initialize loads none of them. Right after it, startBackgroundLoad() + * loads them one by one, a turn of the event loop between them, so each pause + * of the main thread is one package. Until a file type's packages are loaded, + * the tool calls that need them answer at once (notReadyMessage(), checked in + * server.ts). A load that fails isn't kept: the next check starts it again. + * + * Each package is loaded with the loader its own module uses on demand, so + * code called without the server (tests, other callers) loads it as before. + */ +import path from 'path'; +import { loadExcelJS } from './files/excel.js'; +import { loadPizZip } from './files/docx.js'; +import { loadPdf2md } from '../tools/pdf/lib/pdf2md.js'; +import { loadUnpdf } from '../tools/pdf/extract-images.js'; +import { loadMdToPdf } from '../tools/pdf/markdown.js'; +import { loadPdfLib } from '../tools/pdf/manipulations.js'; +import { logger } from './logger.js'; + +/** Each package and its module's loader, in the order the background load takes them (smaller first) */ +const PACKAGES = { + pizzip: loadPizZip, + 'pdf-lib': loadPdfLib, + '@opendocsg/pdf2md': loadPdf2md, + unpdf: loadUnpdf, + exceljs: loadExcelJS, + 'md-to-pdf': loadMdToPdf, +} satisfies Record unknown>; +type HeavyPackage = keyof typeof PACKAGES; + +/** What a file type needs, and its name in the answers */ +const SUPPORT = { + excel: { name: 'Excel support', packages: ['exceljs'] }, + docx: { name: 'DOCX support', packages: ['pizzip'] }, + pdfRead: { name: 'PDF reading support', packages: ['@opendocsg/pdf2md', 'unpdf'] }, + pdfWrite: { name: 'PDF writing support', packages: ['md-to-pdf'] }, + pdfEdit: { name: 'PDF editing support', packages: ['pdf-lib'] }, +} as const satisfies Record; +export type HeavySupport = keyof typeof SUPPORT; + +type LoadState = { status: 'loading'; done: Promise } | { status: 'ready' } | { status: 'failed'; error: string }; +const states = new Map(); + +/** + * Loads `pkg` on a later turn of the event loop, never inside the caller's + * (a tool call that starts it answers first). Resolves once loaded or failed. + */ +function load(pkg: HeavyPackage): Promise { + const state = states.get(pkg); + if (state?.status === 'ready') return Promise.resolve(); + if (state?.status === 'loading') return state.done; + const done = new Promise((resolve) => setImmediate(resolve)) + .then(async () => { await PACKAGES[pkg](); }) + .then( + () => { states.set(pkg, { status: 'ready' }); }, + (error) => { + const message = error instanceof Error ? error.message : String(error); + states.set(pkg, { status: 'failed', error: message }); + logger.error(`Loading ${pkg} failed: ${message}`); + }); + states.set(pkg, { status: 'loading', done }); + return done; +} + +/** Loads every package, one at a time; one already loaded or loading isn't loaded again */ +export async function startBackgroundLoad(): Promise { + for (const pkg of Object.keys(PACKAGES) as HeavyPackage[]) { + await load(pkg); + } +} + +/** Whether everything `support` needs is loaded */ +export function isReady(support: HeavySupport): boolean { + return SUPPORT[support].packages.every((pkg) => states.get(pkg)?.status === 'ready'); +} + +/** What a tool call does to the file, as its error says */ +export type FileAction = 'read' | 'write' | 'edit'; + +/** + * The error a tool call that needs `support` to `action` `filePath` answers + * with while it isn't loaded, or undefined once it is. A package not started + * yet (before initialize) or that failed is started here, after the call has + * answered. + */ +export function notReadyMessage(support: HeavySupport, filePath: string, action: FileAction): string | undefined { + const { name, packages } = SUPPORT[support]; + const file = path.basename(filePath); + for (const pkg of packages) { + const state = states.get(pkg); + if (state?.status === 'ready') continue; + if (state?.status === 'failed') { + void load(pkg); + return `Can't ${action} ${file}: Desktop Commander couldn't load its ${name} (${state.error}). It's loading it again; try again in a few seconds.`; + } + if (!state) void load(pkg); + return `Can't ${action} ${file} yet: Desktop Commander is still loading its ${name} (it starts right after launch). Try again in a few seconds.`; + } + return undefined; +} diff --git a/test/test-search-office-completion.js b/test/test-search-office-completion.js index 01b76cb1e..e1a04cab6 100644 --- a/test/test-search-office-completion.js +++ b/test/test-search-office-completion.js @@ -189,7 +189,8 @@ async function testTimeoutStopsOfficeSearches() { * An Office search that fails as a whole (here: ExcelJS can't be loaded) must * not vanish: the search answers that part of it failed, with the other sources' * matches, and the log says which part failed and why. Runs in a child process - * whose search-manager can't import exceljs. + * where exceljs can't be loaded (the search loads it with loadExcelJS(), in + * utils/files/excel.js). */ async function testFailedOfficeSearchIsLogged() { console.log('Testing that a failed Office search is logged...'); @@ -197,7 +198,7 @@ async function testFailedOfficeSearchIsLogged() { const REASON = 'exceljs is unavailable in this test'; const hooks = ` export async function resolve(specifier, context, nextResolve) { - if (specifier === 'exceljs' && context.parentURL?.endsWith('/search-manager.js')) { + if (specifier === 'exceljs' && context.parentURL?.endsWith('/utils/files/excel.js')) { throw new Error(${JSON.stringify(REASON)}); } return nextResolve(specifier, context); From aeaae5e59ba140a9627e7045a852131e91898f1d Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Mon, 5 Oct 2026 13:19:56 +0300 Subject: [PATCH 8/9] test(startup): the still-loading answer: no file name in telemetry, a retry that works, URLs not refused Three bugs in #777's still-loading answers, found in review: - The answer names the file ("Can't read salary-2026.xlsx yet: ..."), and the same text went to telemetry as the call's error (server_request_error), on every retry. - "Try again in a few seconds" can't work for Excel: exceljs is loaded with import(), and Node keeps an import() that failed failed. unpdf, the one package that stays import(), should say to restart Desktop Commander. - read_file of a URL ending in .docx or .xlsx is refused while that support loads, though a URL read fetches the URL and never uses it. test-still-loading-answers.js (the server in this process, telemetry caught as in test-telemetry-paths.js) checks the URL and the file name. test-startup-imports.js checks the retry: a failed exceljs load is followed by a call that works, and a failed unpdf load says to restart. The test hooks now fail a package the way a broken package fails, when its first file runs: CommonJS files in package-load-preload.mjs (Module._extensions), ES modules in package-load-hooks.mjs (a load hook). The still-loading cases hold unpdf back, the package that stays import(): a require() can't be held. At this commit the URL, file-name, Excel-retry and unpdf-restart checks fail. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/fixtures/package-load-hooks.mjs | 24 ++++-- test/fixtures/package-load-preload.mjs | 20 +++-- test/helpers/server-modules.js | 13 ++-- test/test-startup-imports.js | 70 +++++++++++------ test/test-still-loading-answers.js | 103 +++++++++++++++++++++++++ 5 files changed, 188 insertions(+), 42 deletions(-) create mode 100644 test/test-still-loading-answers.js diff --git a/test/fixtures/package-load-hooks.mjs b/test/fixtures/package-load-hooks.mjs index 6e167d35c..dd6a9d57e 100644 --- a/test/fixtures/package-load-hooks.mjs +++ b/test/fixtures/package-load-hooks.mjs @@ -1,23 +1,35 @@ // Installed in the real server by test/helpers/server-modules.js with hookArgs() // when a test holds a package back or makes it fail once: -// - DC_TEST_HOLD_PACKAGE: an import of it doesn't resolve until the file +// - DC_TEST_HOLD_PACKAGE: an import() of it doesn't resolve until the file // DC_TEST_HOLD_RELEASE exists, so that package stays "still loading" -// - DC_TEST_FAIL_PACKAGE_ONCE: the first import of it fails (the file -// DC_TEST_FAILED_MARKER records that it did); later ones load it +// (a require() can't be held: package-load-preload.mjs fails it instead) +// - DC_TEST_FAIL_PACKAGE_ONCE: the first ES module of the server's own copy of +// it (node_modules/ in its working folder) that loads fails, as a +// broken package does (DC_TEST_FAILED_MARKER records that it did); a +// CommonJS one fails in package-load-preload.mjs import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; const HELD = process.env.DC_TEST_HOLD_PACKAGE; const RELEASE = process.env.DC_TEST_HOLD_RELEASE; const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; +// Its real path, as Node names its files (node_modules can be a link) +const PACKAGE_URL = FAIL_ONCE && `${pathToFileURL(fs.realpathSync(path.join(process.cwd(), 'node_modules', FAIL_ONCE))).href}/`; export async function resolve(specifier, context, nextResolve) { if (specifier === HELD) { while (!fs.existsSync(RELEASE)) await new Promise((done) => setTimeout(done, 25)); } - if (specifier === FAIL_ONCE && !fs.existsSync(FAILED_MARKER)) { + return nextResolve(specifier, context); +} + +export async function load(url, context, nextLoad) { + const result = await nextLoad(url, context); + if (FAIL_ONCE && result.format === 'module' && url.startsWith(PACKAGE_URL) && !fs.existsSync(FAILED_MARKER)) { fs.writeFileSync(FAILED_MARKER, ''); - throw new Error(`${specifier} failed to load (test)`); + throw new Error(`${FAIL_ONCE} failed to load (test)`); } - return nextResolve(specifier, context); + return result; } diff --git a/test/fixtures/package-load-preload.mjs b/test/fixtures/package-load-preload.mjs index e2a7dea82..5fa030a85 100644 --- a/test/fixtures/package-load-preload.mjs +++ b/test/fixtures/package-load-preload.mjs @@ -1,18 +1,24 @@ // Preloaded into the real server (node --import) by test/helpers/server-modules.js -// when a test makes a package fail to load once: the first require() of -// DC_TEST_FAIL_PACKAGE_ONCE (or of a file in it) throws, as package-load-hooks.mjs -// does for an import; DC_TEST_FAILED_MARKER records that it did. +// when a test makes a package fail to load once: the first CommonJS file of +// DC_TEST_FAIL_PACKAGE_ONCE that runs throws, as a broken package does, whether +// require() or import() loads it (DC_TEST_FAILED_MARKER records that it did). +// Node keeps such an import() failed; a require() can try again. Only the +// server's own copy (node_modules/ in its working folder) fails, not +// one another package carries (@opendocsg/pdf2md has its own unpdf). import fs from 'node:fs'; import Module from 'node:module'; +import path from 'node:path'; const FAIL_ONCE = process.env.DC_TEST_FAIL_PACKAGE_ONCE; const FAILED_MARKER = process.env.DC_TEST_FAILED_MARKER; +// Its real path, as Node names its files (node_modules can be a link) +const PACKAGE_DIR = fs.realpathSync(path.join(process.cwd(), 'node_modules', FAIL_ONCE)) + path.sep; -const resolveFilename = Module._resolveFilename; -Module._resolveFilename = function (request, ...rest) { - if ((request === FAIL_ONCE || request.startsWith(`${FAIL_ONCE}/`)) && !fs.existsSync(FAILED_MARKER)) { +const compileJs = Module._extensions['.js']; +Module._extensions['.js'] = function (module, filename) { + if (filename.startsWith(PACKAGE_DIR) && !fs.existsSync(FAILED_MARKER)) { fs.writeFileSync(FAILED_MARKER, ''); throw new Error(`${FAIL_ONCE} failed to load (test)`); } - return resolveFilename.call(this, request, ...rest); + return compileJs.call(this, module, filename); }; diff --git a/test/helpers/server-modules.js b/test/helpers/server-modules.js index 569056d50..ef803cee8 100644 --- a/test/helpers/server-modules.js +++ b/test/helpers/server-modules.js @@ -16,7 +16,7 @@ const PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modu const HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/record-modules-hooks.mjs')).href; /** Holds one package back, or makes it fail once (startServerRecordingModules({ holdPackage, failPackageOnce })) */ const PACKAGE_LOAD_HOOKS = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-hooks.mjs')).href; -/** Makes a package's first require() fail (startServerRecordingModules({ failPackageOnce })) */ +/** Makes a package's first CommonJS file fail (startServerRecordingModules({ failPackageOnce })) */ const PACKAGE_LOAD_PRELOAD = pathToFileURL(path.join(PROJECT_ROOT, 'test/fixtures/package-load-preload.mjs')).href; /** Packages only reading, writing or rendering Excel, PDF and DOCX files need */ @@ -59,11 +59,12 @@ function seedChromeCache() { * does, with a preload that records every module it resolves (import and * require). Resolves once `initialize` is answered. * - * With `holdPackage` (an npm package name), the server's imports of that - * package don't resolve until release() is called: it stays "still loading". - * With `failPackageOnce`, the server's first import or require() of that package fails. - * While a package is held, keep other module loads out of the test: the - * held import can hold up the server's other imports and requires. + * With `holdPackage` (a package the server loads with import()), the server's + * imports of it don't resolve until release() is called: it stays "still + * loading". While it is held, keep other module loads out of the test: the + * held import can hold up the server's other imports. + * With `failPackageOnce`, the first file of that package the server runs fails, as + * a broken package does. * * Returns: * - client: the connected MCP client diff --git a/test/test-startup-imports.js b/test/test-startup-imports.js index 6ad17deae..9598f4987 100644 --- a/test/test-startup-imports.js +++ b/test/test-startup-imports.js @@ -97,11 +97,11 @@ async function testCallWhileLoadingAnswersAtOnce(server, file) { const answer = await Promise.race([call, sleep(AT_ONCE_MS).then(() => null)]); try { assert(answer, - `with Excel support still loading, read_file of an .xlsx gave no answer within ${AT_ONCE_MS / 1000} s: ` + - 'it waited for exceljs to load inside the call, and a client gives a call only a few seconds'); - assert(answer.isError === true && /Can't read held\.xlsx yet: Desktop Commander is still loading its Excel support/.test(text(answer)), - `with Excel support still loading, read_file of held.xlsx should answer at once that it can't read held.xlsx yet, as Excel support is still loading; it answered: ${text(answer)}`); - console.log(`✓ A call while Excel support is loading answers in ${Date.now() - started} ms: ${text(answer)}`); + `with PDF reading support still loading, read_file of a PDF gave no answer within ${AT_ONCE_MS / 1000} s: ` + + 'it waited for unpdf to load inside the call, and a client gives a call only a few seconds'); + assert(answer.isError === true && /Can't read held\.pdf yet: Desktop Commander is still loading its PDF reading support/.test(text(answer)), + `with PDF reading support still loading, read_file of held.pdf should answer at once that it can't read held.pdf yet; it answered: ${text(answer)}`); + console.log(`✓ A call while PDF reading support is loading answers in ${Date.now() - started} ms: ${text(answer)}`); } finally { server.release(); await call.catch(() => {}); @@ -113,8 +113,8 @@ async function testReadMultipleFilesWhileLoading(server, file, second) { const answer = text(await server.client.callTool({ name: 'read_multiple_files', arguments: { paths: [file, second] } })); for (const name of [path.basename(file), path.basename(second)]) { const escaped = name.replace(/\./g, '\\.'); - assert(new RegExp(`${escaped}: Error - Can't read ${escaped} yet: Desktop Commander is still loading its Excel support`).test(answer), - `with Excel support still loading, read_multiple_files should say for ${name} that it can't read ${name} yet; it answered: ${answer}`); + assert(new RegExp(`${escaped}: Error - Can't read ${escaped} yet: Desktop Commander is still loading its PDF reading support`).test(answer), + `with PDF reading support still loading, read_multiple_files should say for ${name} that it can't read ${name} yet; it answered: ${answer}`); } console.log('✓ read_multiple_files says for each file, by name, that it can\'t be read yet'); } @@ -133,9 +133,9 @@ async function testWritePdfMarkdownStartingWithLink(server, dir) { /** The same call works once the package is loaded */ async function testSameCallWorksOnceLoaded(server, file) { - assert(/ZebraQuartz held/.test(await callTool(server.client, 'read_file', { path: file })), - 'once Excel support has loaded, read_file of the .xlsx should show its cells'); - console.log('✓ The same call works once Excel support has loaded'); + assert((await callTool(server.client, 'read_file', { path: file })).trim().length > 0, + 'once PDF reading support has loaded, read_file of the PDF should return its text'); + console.log('✓ The same call works once PDF reading support has loaded'); } /** A failed load isn't kept: the call says so, with the reason, and a later call loads it again */ @@ -147,10 +147,23 @@ async function testFailedLoadIsNotKept(server, file) { assert(answer.isError === true && /Can't read held\.xlsx: Desktop Commander couldn't load its Excel support \(.*exceljs failed to load \(test\)/.test(text(answer)), `when exceljs failed to load, read_file of held.xlsx should say it can't read held.xlsx as Excel support couldn't be loaded, and why; it answered: ${text(answer)}`); assert(/ZebraQuartz held/.test(await callTool(server.client, 'read_file', { path: file })), - 'after a failed load, a later read_file of the .xlsx should load Excel support again and show its cells'); + 'after a failed load, a later read_file of the .xlsx should load Excel support again and show its cells ' + + '(Node keeps a failed import() failed, so it has to be loaded with require())'); console.log(`✓ A failed load isn't kept: "${text(answer)}", and a later call loads it`); } +/** A package Node can't load again (an import() that failed) says to restart, not to try again */ +async function testFailedImportSaysRestart(server, file) { + const deadline = Date.now() + LOAD_DEADLINE_MS; + while (!server.logs().some((line) => /Loading unpdf failed/.test(line)) && Date.now() < deadline) await sleep(100); + const answer = await server.client.callTool({ name: 'read_file', arguments: { path: file } }); + assert(answer.isError === true && + /Can't read held\.pdf: Desktop Commander couldn't load its PDF reading support \(.*unpdf failed to load \(test\)\)\. Restart Desktop Commander/.test(text(answer)), + 'when unpdf failed to load, read_file of held.pdf should say to restart Desktop Commander: Node keeps a failed import() failed, ' + + `so trying again can't work; it answered: ${text(answer)}`); + console.log(`✓ A failed import() says to restart: ${text(answer)}`); +} + async function runCases(failures, cases) { for (const [check, ...args] of cases) { try { @@ -181,32 +194,35 @@ export default async function runTests() { await server.close(); } - // Excel support held back: exceljs doesn't load until the server is released - const held = path.join(dir, 'held.xlsx'); - const heldToo = path.join(dir, 'held-too.xlsx'); - const workbook = new ExcelJS.Workbook(); - workbook.addWorksheet('Sheet1').addRow(['Item', 'ZebraQuartz held']); - await workbook.xlsx.writeFile(held); - fs.copyFileSync(held, heldToo); - const holding = await startServerRecordingModules({ holdPackage: 'exceljs' }); + // PDF reading support held back: unpdf (loaded with import()) doesn't load + // until the server is released + const heldPdf = path.join(dir, 'held.pdf'); + const heldPdfToo = path.join(dir, 'held-too.pdf'); + fs.copyFileSync(SAMPLE_PDF, heldPdf); + fs.copyFileSync(SAMPLE_PDF, heldPdfToo); + const holding = await startServerRecordingModules({ holdPackage: 'unpdf' }); try { await runCases(failures, [ - [testCallWhileLoadingAnswersAtOnce, holding, held], - [testSameCallWorksOnceLoaded, holding, held], + [testCallWhileLoadingAnswersAtOnce, holding, heldPdf], + [testSameCallWorksOnceLoaded, holding, heldPdf], ]); } finally { await holding.close(); } // A server of its own: after a call that succeeds, the server loads modules // of its own, which a held import would hold up (see startServerRecordingModules) - const holdingToo = await startServerRecordingModules({ holdPackage: 'exceljs' }); + const holdingToo = await startServerRecordingModules({ holdPackage: 'unpdf' }); try { - await runCases(failures, [[testReadMultipleFilesWhileLoading, holdingToo, held, heldToo]]); + await runCases(failures, [[testReadMultipleFilesWhileLoading, holdingToo, heldPdf, heldPdfToo]]); } finally { await holdingToo.close(); } // Excel support failing to load once + const held = path.join(dir, 'held.xlsx'); + const workbook = new ExcelJS.Workbook(); + workbook.addWorksheet('Sheet1').addRow(['Item', 'ZebraQuartz held']); + await workbook.xlsx.writeFile(held); const failing = await startServerRecordingModules({ failPackageOnce: 'exceljs' }); try { await runCases(failures, [[testFailedLoadIsNotKept, failing, held]]); @@ -214,6 +230,14 @@ export default async function runTests() { await failing.close(); } + // PDF reading support (unpdf, loaded with import()) failing to load once + const failingImport = await startServerRecordingModules({ failPackageOnce: 'unpdf' }); + try { + await runCases(failures, [[testFailedImportSaysRestart, failingImport, heldPdf]]); + } finally { + await failingImport.close(); + } + // PDF writing failing to load once const failingPdf = await startServerRecordingModules({ failPackageOnce: 'md-to-pdf' }); try { diff --git a/test/test-still-loading-answers.js b/test/test-still-loading-answers.js new file mode 100644 index 000000000..5b6a4bbed --- /dev/null +++ b/test/test-still-loading-answers.js @@ -0,0 +1,103 @@ +/** + * What a tool call answers while the Excel, DOCX or PDF support it needs is + * still loading (#777), on the server in this process, where nothing has + * loaded that support yet: + * - the answer names the file, but no telemetry event does: the answer was + * also sent as the call's error, file name included, on every retry + * - read_file of a URL ending in .docx fetches the URL and never uses DOCX + * support, so it isn't refused while that support loads + * + * Telemetry is caught the way test-telemetry-paths.js catches it. + */ +import assert from 'assert'; +import { EventEmitter } from 'events'; +import http from 'http'; +import https from 'https'; +import { syncBuiltinESMExports } from 'module'; +import os from 'os'; +import path from 'path'; +import { runIfMain, skip } from './helpers/run-if-main.js'; +import { isTestHome } from './helpers/test-env.js'; + +// Every telemetry payload this process would send +const sent = []; +https.request = (options, callback) => { + const chunks = []; + const req = new EventEmitter(); + req.write = (data) => { chunks.push(String(data)); return true; }; + req.setTimeout = () => req; + req.destroy = () => {}; + req.end = () => { + sent.push(JSON.parse(chunks.join(''))); + const res = new EventEmitter(); + res.statusCode = 204; + res.resume = () => res; + callback?.(res); + setImmediate(() => res.emit('end')); + }; + return req; +}; +syncBuiltinESMExports(); +delete process.env.DESKTOP_COMMANDER_DISABLE_TELEMETRY; + +const { server } = await import('../dist/server.js'); + +const text = (result) => (result.content ?? []).map((item) => item.text ?? '').join('\n'); +const callTool = (name, args) => + server._requestHandlers.get('tools/call')({ method: 'tools/call', params: { name, arguments: args } }, {}); + +/** Waits until telemetry has sent an event named `name` (or 5 s) */ +async function waitForEvent(name) { + for (let waited = 0; waited < 5000; waited += 50) { + if (sent.some((payload) => payload.events.some((event) => event.name === name))) return; + await new Promise((resolve) => setTimeout(resolve, 50)); + } +} + +async function testUrlIsNotRefused() { + const urlServer = http.createServer((request, response) => { + response.writeHead(200, { 'Content-Type': 'text/plain' }).end('ZebraQuartz from a URL'); + }); + await new Promise((resolve) => urlServer.listen(0, '127.0.0.1', resolve)); + try { + const url = `http://127.0.0.1:${urlServer.address().port}/report.docx`; + const answer = await callTool('read_file', { path: url, isUrl: true }); + assert(/ZebraQuartz from a URL/.test(text(answer)), + `read_file of a URL ending in .docx, while DOCX support is still loading, should fetch the URL (it never uses DOCX support); it answered: ${text(answer)}`); + console.log('✓ read_file of a URL ending in .docx fetches it while DOCX support is still loading'); + } finally { + urlServer.close(); + } +} + +async function testFileNameNotInTelemetry() { + const answer = await callTool('read_file', { path: path.join(os.tmpdir(), 'salary-2026.xlsx') }); + assert(answer.isError === true && /Can't read salary-2026\.xlsx yet/.test(text(answer)), + `setup: with Excel support still loading, read_file of salary-2026.xlsx should say it can't read it yet; it answered: ${text(answer)}`); + await waitForEvent('server_call_tool'); + const leaking = sent.flatMap((payload) => payload.events).filter((event) => JSON.stringify(event).includes('salary-2026')); + assert.deepStrictEqual(leaking, [], + 'the answer that a file can\'t be read yet names the file, and telemetry got that name: ' + JSON.stringify(leaking)); + console.log('✓ The answer names the file; no telemetry event does'); +} + +async function run() { + if (!isTestHome()) { + return skip('still-loading answers: the server in this process writes its config to the home; run through the test runner'); + } + const failures = []; + // The URL first: each case uses support nothing else here has loaded + for (const test of [testUrlIsNotRefused, testFileNameNotInTelemetry]) { + try { + await test(); + } catch (error) { + failures.push(test.name); + console.log(`✗ ${test.name}: ${error.message}`); + } + } + return failures.length === 0; +} + +runIfMain(import.meta.url, run); + +export default run; From 6da036105335f166167505b19760a29076f1afd2 Mon Sep 17 00:00:00 2001 From: Mihails Tumkins Date: Mon, 5 Oct 2026 13:31:58 +0300 Subject: [PATCH 9/9] refactor(startup): each module declares its lazy package; the still-loading check lives with the file handlers src/utils/heavy-packages.ts kept its own table of the packages, their loaders and what each file type needs. Each module now declares its package next to the code that uses it, as a LazyPackage (src/utils/lazy-package.ts): exceljsPackage (excel.ts), pizzipPackage (docx.ts), pdfLibPackage (manipulations.ts), pdf2mdPackage (lib/pdf2md.ts), unpdfPackage (extract-images.ts) and mdToPdfPackage (markdown.ts). Its load() loads it on first use, as before; preload() loads it on a later turn of the event loop, never rejects, shares a running preload and logs "Loading failed". utils/files/factory.ts has preloadFileSupport(), called from index.ts next to the Chrome check (the same six packages, one at a time, in the same order), and stillLoadingError(filePath, action), which finds what a file needs with the handlers' own canHandle(). server.ts checks it in each case it applies to, so the second switch is gone; write_pdf still reads markdown or page edits with WritePdfArgsSchema. Fixes, tested in the commit before: - The still-loading answer names the file, so it no longer goes through createErrorResponse, which sent it to telemetry as server_request_error. - exceljs loads with require(): Node keeps a failed import() failed, so after a failed load "try again" never worked. unpdf is an ES module only and stays import(); if it fails, the answer says to restart Desktop Commander. - read_file of a URL fetches it without the check, except a URL ending in .pdf, which is parsed with pdf2md and unpdf. Also: markdown.ts no longer passes md-to-pdf's loaded modules as parameters, a search with no Office files doesn't load their packages, and the comment on get_file_info says what it does (it waits for the load, or loads it itself). test-search-office-completion.js makes exceljs fail where require() now resolves it. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/handlers/filesystem-handlers.ts | 4 +- src/index.ts | 4 +- src/search-manager.ts | 16 ++-- src/server.ts | 83 +++++--------------- src/tools/filesystem.ts | 14 ++-- src/tools/pdf/extract-images.ts | 14 ++-- src/tools/pdf/index.ts | 7 +- src/tools/pdf/lib/pdf2md.ts | 12 +-- src/tools/pdf/manipulations.ts | 12 +-- src/tools/pdf/markdown.ts | 38 ++++----- src/utils/files/docx.ts | 12 +-- src/utils/files/excel.ts | 19 ++--- src/utils/files/factory.ts | 63 ++++++++++++++- src/utils/files/index.ts | 6 +- src/utils/heavy-packages.ts | 106 -------------------------- src/utils/lazy-package.ts | 41 ++++++++++ test/test-search-office-completion.js | 21 +++-- 17 files changed, 197 insertions(+), 275 deletions(-) delete mode 100644 src/utils/heavy-packages.ts create mode 100644 src/utils/lazy-package.ts diff --git a/src/handlers/filesystem-handlers.ts b/src/handlers/filesystem-handlers.ts index a8f7c7ad9..e79ddaa72 100644 --- a/src/handlers/filesystem-handlers.ts +++ b/src/handlers/filesystem-handlers.ts @@ -243,9 +243,9 @@ export async function handleReadFile(args: unknown): Promise { /** * Handle read_multiple_files command */ -export async function handleReadMultipleFiles(args: unknown, notReady?: (filePath: string) => string | undefined): Promise { +export async function handleReadMultipleFiles(args: unknown, stillLoadingError?: (filePath: string) => string | undefined): Promise { const parsed = ReadMultipleFilesArgsSchema.parse(args); - const fileResults = await readMultipleFiles(parsed.paths, notReady); + const fileResults = await readMultipleFiles(parsed.paths, stillLoadingError); // Create a text summary of all files const textSummary = fileResults.map(result => { diff --git a/src/index.ts b/src/index.ts index 55acecedc..1cd207c4a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -15,7 +15,7 @@ import { logToStderr, logger } from './utils/logger.js'; import { exitProcess } from './utils/exit-process.js'; import { runRemote } from './npm-scripts/remote.js'; import { ensureChromeAvailable } from './tools/pdf/markdown.js'; -import { startBackgroundLoad } from './utils/heavy-packages.js'; +import { preloadFileSupport } from './utils/files/index.js'; // Store messages to defer until after initialization const deferredMessages: Array<{ level: string, message: string }> = []; @@ -140,7 +140,7 @@ async function runServer() { // Load the Excel, DOCX and PDF packages, one at a time, now that initialize // has been answered; until then the calls that need them answer at once - void startBackgroundLoad(); + preloadFileSupport(); }; await server.connect(transport); diff --git a/src/search-manager.ts b/src/search-manager.ts index 98ffc3d60..858f6feba 100644 --- a/src/search-manager.ts +++ b/src/search-manager.ts @@ -7,8 +7,8 @@ import { capture } from './utils/capture.js'; import { logger } from './utils/logger.js'; import { getRipgrepPath } from './utils/ripgrep-resolver.js'; import { isExcelFile } from './utils/files/index.js'; -import { loadExcelJS } from './utils/files/excel.js'; -import { loadPizZip } from './utils/files/docx.js'; +import { exceljsPackage } from './utils/files/excel.js'; +import { pizzipPackage } from './utils/files/docx.js'; export interface SearchResult { context?: boolean; // A line around a match (contextLines), not a match @@ -577,9 +577,10 @@ function characterClassEnd(glob: string, start: number): number { excelFiles = this.filterOfficeFiles(excelFiles, filePattern, rootPath); } - // ExcelJS, to search all sheets; while the server is still loading it in - // the background, this waits for it (the search runs in the background too) - const ExcelJS = await loadExcelJS(); + if (excelFiles.length === 0) return; + // ExcelJS, to search all sheets: loaded here if the server hasn't yet + // (a search runs in the background, so it isn't refused while it loads) + const ExcelJS = exceljsPackage.load(); for (const filePath of excelFiles) { if (sink.isStopped()) break; @@ -775,8 +776,9 @@ function characterClassEnd(glob: string, start: number): number { docxFiles = this.filterOfficeFiles(docxFiles, filePattern, rootPath); } - // PizZip, to open the DOCX files - const PizZip = loadPizZip(); + if (docxFiles.length === 0) return; + // PizZip, to open the DOCX files: loaded here if the server hasn't yet + const PizZip = pizzipPackage.load(); for (const filePath of docxFiles) { if (sink.isStopped()) break; diff --git a/src/server.ts b/src/server.ts index b354d80a8..8e8157dc2 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1202,60 +1202,18 @@ server.setRequestHandler(ListToolsRequestSchema, async () => { import * as handlers from './handlers/index.js'; import { ServerResult } from './types.js'; import { withoutInternalFacts } from './utils/internal-facts.js'; -import { createErrorResponse } from './error-handlers.js'; -import { isExcelFile } from './utils/files/index.js'; -import { isReady, notReadyMessage, type FileAction, type HeavySupport } from './utils/heavy-packages.js'; - -/** What a file needs from utils/heavy-packages.ts to be read, written or edited */ -function heavySupportFor(filePath: string, action: FileAction): HeavySupport[] { - const lower = filePath.toLowerCase(); - if (isExcelFile(lower)) return ['excel']; - if (lower.endsWith('.docx')) return ['docx']; - if (lower.endsWith('.pdf')) { - // Editing a PDF can insert markdown pages, which are rendered - return action === 'read' ? ['pdfRead'] : action === 'write' ? ['pdfWrite'] : ['pdfEdit', 'pdfWrite']; - } - return []; -} +import { stillLoadingError, type FileAction } from './utils/files/index.js'; /** - * If `action` on `filePath` needs support not loaded yet (by default, what its - * file type needs), the error the call answers with at once, naming the file + * A tool call on an Excel, DOCX or PDF file whose package is still loading + * after initialize answers at once with this error (utils/files/factory.ts) + * instead of loading it inside the call. Not sent to telemetry, unlike other + * errors: it names the file. read_multiple_files answers per file; + * get_file_info waits for the load, or loads it itself, and so does a search. */ -function notReady(filePath: unknown, action: FileAction, supports?: HeavySupport[]): string | undefined { - if (typeof filePath !== 'string') return undefined; - const pending = (supports ?? heavySupportFor(filePath, action)).find((support) => !isReady(support)); - return pending && notReadyMessage(pending, filePath, action); -} - -/** - * A tool call on an Excel, DOCX or PDF file whose support is still loading - * in the background (utils/heavy-packages.ts) answers at once with this error - * instead of loading it inside the call; undefined for every other call. - * read_multiple_files answers per file (readMultipleFiles), get_file_info - * falls back to the basic info, and a search waits for the load. - */ -function heavySupportNotReady(name: string, args: unknown): string | undefined { - const params = (args && typeof args === 'object' ? args : {}) as Record; - switch (name) { - case 'read_file': - return notReady(params.path, 'read'); - case 'write_file': - return notReady(params.path, 'write'); - case 'edit_block': - return notReady(params.file_path, 'edit'); - case 'write_pdf': { - // Markdown (a new PDF) or page edits, read exactly as the tool reads - // them; arguments the tool refuses get its own error - const parsed = WritePdfArgsSchema.safeParse(args); - if (!parsed.success) return undefined; - return typeof parsed.data.content === 'string' - ? notReady(parsed.data.path, 'write', ['pdfWrite']) - : notReady(parsed.data.path, 'edit', ['pdfEdit', 'pdfWrite']); - } - default: - return undefined; - } +function stillLoading(filePath: unknown, action: FileAction, options?: { isPdf?: boolean; isUrl?: boolean }): ServerResult | undefined { + const error = typeof filePath === 'string' ? stillLoadingError(filePath, action, options) : undefined; + return error === undefined ? undefined : { content: [{ type: 'text', text: `Error: ${error}` }], isError: true }; } server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest): Promise => { @@ -1347,11 +1305,7 @@ async function handleCallToolRequest(request: CallToolRequest): Promise notReady(filePath, 'read')); + result = await handlers.handleReadMultipleFiles(args, (filePath) => stillLoadingError(filePath, 'read')); break; case "write_file": - result = await handlers.handleWriteFile(args); + result = stillLoading(args?.path, 'write') ?? await handlers.handleWriteFile(args); break; - case "write_pdf": - result = await handlers.handleWritePdf(args); + case "write_pdf": { + // Markdown (a new PDF) or page edits, read as the tool reads them; + // arguments the tool refuses get its own error + const parsed = WritePdfArgsSchema.safeParse(args); + const action = parsed.success && typeof parsed.data.content === 'string' ? 'write' : 'edit'; + result = (parsed.success ? stillLoading(parsed.data.path, action, { isPdf: true }) : undefined) ?? await handlers.handleWritePdf(args); break; + } case "create_directory": result = await handlers.handleCreateDirectory(args); @@ -1552,7 +1511,7 @@ async function handleCallToolRequest(request: CallToolRequest): Promise string | undefined): Promise { +export async function readMultipleFiles(paths: string[], stillLoadingError?: (filePath: string) => string | undefined): Promise { return Promise.all( paths.map(async (filePath: string) => { - const notReadyError = notReady?.(filePath); - if (notReadyError) { - return { path: filePath, error: notReadyError }; + const stillLoading = stillLoadingError?.(filePath); + if (stillLoading) { + return { path: filePath, error: stillLoading }; } try { const validPath = await validatePath(filePath); diff --git a/src/tools/pdf/extract-images.ts b/src/tools/pdf/extract-images.ts index 79e5f5cd6..e7b54af86 100644 --- a/src/tools/pdf/extract-images.ts +++ b/src/tools/pdf/extract-images.ts @@ -1,3 +1,5 @@ +import { LazyPackage } from '../../utils/lazy-package.js'; + export interface ImageInfo { /** Object ID within PDF */ objId: number; @@ -27,14 +29,8 @@ export interface ImageCompressionOptions { maxDimension?: number; } -/** - * unpdf, loaded when first used, not with this module: the server loads the - * PDF tools at startup, before it answers initialize (#715). The server loads - * it right after initialize (utils/heavy-packages.ts). - */ -export function loadUnpdf(): Promise { - return import('unpdf'); -} +// import(): unpdf is an ES module only. If it fails, Node keeps it failed until a restart. +export const unpdfPackage = new LazyPackage('unpdf', 'PDF reading support', () => import('unpdf')); /** * Optimized image extraction from PDF using unpdf's built-in extractImages method @@ -48,7 +44,7 @@ export async function extractImagesFromPdf( pageNumbers?: number[], compressionOptions: ImageCompressionOptions = {} ): Promise> { - const { getDocumentProxy, extractImages } = await loadUnpdf(); + const { getDocumentProxy, extractImages } = await unpdfPackage.load(); const pdfDocument = await getDocumentProxy(pdfBuffer); const pagesToProcess = pageNumbers || Array.from({ length: pdfDocument.numPages }, (_, i) => i + 1); diff --git a/src/tools/pdf/index.ts b/src/tools/pdf/index.ts index f0a807df6..7693477fa 100644 --- a/src/tools/pdf/index.ts +++ b/src/tools/pdf/index.ts @@ -1,8 +1,9 @@ -export { editPdf, insertRenderOptions } from './manipulations.js'; +export { editPdf, insertRenderOptions, pdfLibPackage } from './manipulations.js'; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation } from './manipulations.js'; -export { parsePdfToMarkdown, parseMarkdownToPdf, resolveRender } from './markdown.js'; +export { parsePdfToMarkdown, parseMarkdownToPdf, resolveRender, mdToPdfPackage } from './markdown.js'; export type { IgnoredRenderOption } from './markdown.js'; +export { pdf2mdPackage } from './lib/pdf2md.js'; export type { PdfMetadata, PdfPageItem } from './lib/pdf2md.js'; -export { extractImagesFromPdf } from './extract-images.js'; +export { extractImagesFromPdf, unpdfPackage } from './extract-images.js'; export type { ImageInfo, PageImages } from './extract-images.js'; diff --git a/src/tools/pdf/lib/pdf2md.ts b/src/tools/pdf/lib/pdf2md.ts index 845e5d8ab..b7dbdfa6d 100644 --- a/src/tools/pdf/lib/pdf2md.ts +++ b/src/tools/pdf/lib/pdf2md.ts @@ -1,5 +1,6 @@ import { createRequire } from 'module'; +import { LazyPackage } from '../../../utils/lazy-package.js'; import { generatePageNumbers } from '../utils.js'; import { extractImagesFromPdf, ImageInfo } from '../extract-images.js'; const require = createRequire(import.meta.url); @@ -7,16 +8,11 @@ const require = createRequire(import.meta.url); /** What @opendocsg/pdf2md's parse() returns: its modules are loaded untyped, with require() */ type ParseResult = any; -/** - * @opendocsg/pdf2md, loaded when first used, not with this module: the server - * loads the PDF tools at startup, before it answers initialize (#715). The - * server loads it right after initialize (utils/heavy-packages.ts). - */ -export function loadPdf2md(): { parse: (pdfBuffer: Uint8Array) => Promise; makeTransformations: any; transform: any } { +export const pdf2mdPackage = new LazyPackage('@opendocsg/pdf2md', 'PDF reading support', () => { const { parse } = require('@opendocsg/pdf2md/lib/util/pdf'); const { makeTransformations, transform } = require('@opendocsg/pdf2md/lib/util/transformations'); return { parse, makeTransformations, transform }; -} +}); /** @@ -78,7 +74,7 @@ export type PageRange = { * @returns A Promise that resolves to a PdfParseResult object containing the parsed data. */ export async function pdf2md(pdfBuffer: Uint8Array, pageNumbers: number[] | PageRange = []): Promise { - const { parse, makeTransformations, transform } = loadPdf2md(); + const { parse, makeTransformations, transform } = pdf2mdPackage.load(); const result = await parse(pdfBuffer); const { fonts, pages, pdfDocument } = result; diff --git a/src/tools/pdf/manipulations.ts b/src/tools/pdf/manipulations.ts index d02cd89e9..90e7027a9 100644 --- a/src/tools/pdf/manipulations.ts +++ b/src/tools/pdf/manipulations.ts @@ -1,6 +1,7 @@ import fs from 'fs/promises'; import { createRequire } from 'module'; import type { PDFDocument as PDFDocumentType, PDFPage } from 'pdf-lib'; +import { LazyPackage } from '../../utils/lazy-package.js'; import { normalizePageIndexes } from './utils.js'; import { parseMarkdownToPdf } from './markdown.js'; import type { PdfInsertOperationSchema, PdfDeleteOperationSchema, PdfOperationSchema } from '../schemas.js'; @@ -16,17 +17,10 @@ type PdfOperations = z.infer; export type { PdfOperations, PdfInsertOperation, PdfDeleteOperation }; -/** - * pdf-lib, loaded when first used, not with this module: the server loads the - * PDF tools at startup, before it answers initialize (#715). The server loads - * it right after initialize (utils/heavy-packages.ts). - */ -export function loadPdfLib(): { PDFDocument: typeof PDFDocumentType } { - return require('pdf-lib'); -} +export const pdfLibPackage = new LazyPackage('pdf-lib', 'PDF editing support', (): { PDFDocument: typeof PDFDocumentType } => require('pdf-lib')); async function loadPdfDocumentFromBuffer(filePathOrBuffer: string | Buffer | Uint8Array): Promise { - const { PDFDocument } = loadPdfLib(); + const { PDFDocument } = pdfLibPackage.load(); const buffer = typeof filePathOrBuffer === 'string' ? await fs.readFile(filePathOrBuffer) : filePathOrBuffer; const pdfBytes = new Uint8Array(buffer); return await PDFDocument.load(pdfBytes); diff --git a/src/tools/pdf/markdown.ts b/src/tools/pdf/markdown.ts index 063e7bb6f..62e5ce9c2 100644 --- a/src/tools/pdf/markdown.ts +++ b/src/tools/pdf/markdown.ts @@ -12,6 +12,7 @@ import type { Config as MdToPdfConfig } from 'md-to-pdf/dist/lib/config.js'; import type { PageRange } from './lib/pdf2md.js'; import { PdfParseResult, pdf2md } from './lib/pdf2md.js'; import { CONFIG_FILE } from '../../config.js'; +import { LazyPackage } from '../../utils/lazy-package.js'; const isUrl = (source: string): boolean => source.startsWith('http://') || source.startsWith('https://'); @@ -35,14 +36,9 @@ const RENDER_COOKIE_NAME = 'desktop-commander-pdf-render'; const require = createRequire(import.meta.url); -/** - * md-to-pdf, with its own Puppeteer, file server and front matter parser - * loaded the way md-to-pdf loads them (they are its dependencies, not Desktop - * Commander's). Loaded when first used, not with this module: the server - * loads this module at startup, before it answers initialize (#715). The - * server loads it right after initialize (utils/heavy-packages.ts). - */ -export function loadMdToPdf() { +// md-to-pdf, with its own Puppeteer, file server and front matter parser, loaded the +// way md-to-pdf loads them (they are its dependencies, not Desktop Commander's) +export const mdToPdfPackage = new LazyPackage('md-to-pdf', 'PDF writing support', () => { const requireFromMdToPdf = createRequire(require.resolve('md-to-pdf')); const { convertMdToPdf }: typeof import('md-to-pdf/dist/lib/md-to-pdf.js') = require('md-to-pdf/dist/lib/md-to-pdf.js'); const { defaultConfig }: typeof import('md-to-pdf/dist/lib/config.js') = require('md-to-pdf/dist/lib/config.js'); @@ -51,7 +47,7 @@ export function loadMdToPdf() { requireFromMdToPdf('serve-handler'); const grayMatter: (input: string, options: unknown) => { content: string; data: unknown } = requireFromMdToPdf('gray-matter'); return { convertMdToPdf, defaultConfig, puppeteer, serveHandler, grayMatter }; -} +}); const isPlainObject = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -117,10 +113,10 @@ interface ResolvedRender { * `{}` or null) replaced the defaults and switched it back on, so a `---js` * header, or any header with `language: 'javascript'`, ran in the server. * gray-matter finds an engine by name, then by alias (js -> javascript), so - * both names are switched off. `defaultConfig` is md-to-pdf's, loaded on first use - * (loadMdToPdf()). + * both names are switched off. */ -function safeGrayMatterOptions(callerOptions: unknown, defaultConfig: ReturnType['defaultConfig']): Record { +function safeGrayMatterOptions(callerOptions: unknown): Record { + const { defaultConfig } = mdToPdfPackage.load(); // md-to-pdf's switch-off of gray-matter's JavaScript engine, which evaluates the header's code const disabledJsEngine = (defaultConfig.gray_matter_options as { engines: Record }).engines.javascript; const caller = isPlainObject(callerOptions) ? callerOptions : {}; @@ -141,11 +137,11 @@ function safeGrayMatterOptions(callerOptions: unknown, defaultConfig: ReturnType * md-to-pdf merging the front matter a second time. */ export function resolveRender(markdown: string, options: unknown = {}): ResolvedRender { - const { defaultConfig, grayMatter } = loadMdToPdf(); + const { grayMatter } = mdToPdfPackage.load(); const fromOptions = isPlainObject(options) ? options : {}; // Parse the front matter the way md-to-pdf would, with the caller's // gray_matter_options, but never with gray-matter's JavaScript engine - const grayMatterOptions = safeGrayMatterOptions(fromOptions.gray_matter_options, defaultConfig); + const grayMatterOptions = safeGrayMatterOptions(fromOptions.gray_matter_options); const { content, data } = grayMatter(markdown, grayMatterOptions); const frontMatter = isPlainObject(data) ? data : {}; @@ -574,12 +570,7 @@ function nameGivenPaths(error: unknown, givenPaths: Map): void { * the check resolved, not the URL's path looked up again, so a link on the way * that is changed after the check doesn't lead elsewhere. */ -async function serveAllowedFile( - request: IncomingMessage, - response: ServerResponse, - basedir: string, - serveHandler: ReturnType['serveHandler'] -): Promise { +async function serveAllowedFile(request: IncomingMessage, response: ServerResponse, basedir: string): Promise { // Imported when used: tools/filesystem.js loads this module const { validatePath } = await import('../filesystem.js'); let file: string; @@ -595,7 +586,7 @@ async function serveAllowedFile( // serve-handler serves exactly that file (no .html redirects, which would name it from another folder). // It reads only the request's url and headers; the request itself keeps its URL. const forFile = { url: `/${encodeURIComponent(basename(file))}`, headers: request.headers } as IncomingMessage; - await serveHandler(forFile, response, { public: dirname(file), directoryListing: false, cleanUrls: false }); + await mdToPdfPackage.load().serveHandler(forFile, response, { public: dirname(file), directoryListing: false, cleanUrls: false }); } /** @@ -612,7 +603,6 @@ async function serveAllowedFile( * folder listings, only from inside the allowed folders (serveAllowedFile). */ async function startRenderServer(basedir: string): Promise { - const { serveHandler } = loadMdToPdf(); const cookie = { name: RENDER_COOKIE_NAME, value: randomBytes(32).toString('hex') }; const expected = `${cookie.name}=${cookie.value}`; const server = createServer((request, response) => { @@ -620,7 +610,7 @@ async function startRenderServer(basedir: string): Promise { response.writeHead(403, { 'Content-Type': 'text/plain' }).end('Forbidden'); return; } - serveAllowedFile(request, response, basedir, serveHandler).catch((error) => { + serveAllowedFile(request, response, basedir).catch((error) => { console.error('The PDF render server could not serve a file:', error); if (!response.headersSent) response.writeHead(500); response.end(); @@ -785,7 +775,7 @@ export async function parseMarkdownToPdf(markdown: string, options: any = {}): P // The render files as the caller gave them, by the checked paths the render reads let givenPaths = new Map(); try { - const { convertMdToPdf, defaultConfig, puppeteer } = loadMdToPdf(); + const { convertMdToPdf, defaultConfig, puppeteer } = mdToPdfPackage.load(); // The folder the markdown's files are served from must be inside the allowed folders const { validatePath } = await import('../filesystem.js'); const basedir: string = options.basedir ? await validatePath(options.basedir) : process.cwd(); diff --git a/src/utils/files/docx.ts b/src/utils/files/docx.ts index 37cb9682c..e38de23ae 100644 --- a/src/utils/files/docx.ts +++ b/src/utils/files/docx.ts @@ -19,6 +19,7 @@ import fs from 'fs/promises'; import { createRequire } from 'module'; import type PizZip from 'pizzip'; +import { LazyPackage } from '../lazy-package.js'; import { FileHandler, FileResult, FileInfo, ReadOptions, EditResult } from './base.js'; // ════════════════════════════════════════════════════════════════ @@ -74,18 +75,11 @@ interface DocxZipContents { const require = createRequire(import.meta.url); -/** - * pizzip, loaded when first used, not with this module: the server loads the - * file handlers at startup, before it answers initialize (#715). The server - * loads it right after initialize (utils/heavy-packages.ts). - */ -export function loadPizZip(): typeof PizZip { - return require('pizzip'); -} +export const pizzipPackage = new LazyPackage('pizzip', 'DOCX support', (): typeof PizZip => require('pizzip')); /** Opens a zip from its bytes, or a new, empty one */ function openZip(data?: Buffer): PizZip { - const PizZipClass = loadPizZip(); + const PizZipClass = pizzipPackage.load(); return data === undefined ? new PizZipClass() : new PizZipClass(data); } diff --git a/src/utils/files/excel.ts b/src/utils/files/excel.ts index e2efc4a42..ac10ecc3c 100644 --- a/src/utils/files/excel.ts +++ b/src/utils/files/excel.ts @@ -5,6 +5,8 @@ import type ExcelJS from 'exceljs'; import fs from 'fs/promises'; +import { createRequire } from 'module'; +import { LazyPackage } from '../lazy-package.js'; import { FileHandler, ReadOptions, @@ -14,20 +16,13 @@ import { ExcelSheet } from './base.js'; -/** - * exceljs, loaded when first used, not with this module: the server loads the - * file handlers at startup, before it answers initialize (#715). The server - * loads it right after initialize (utils/heavy-packages.ts). - */ -export async function loadExcelJS(): Promise { - const { default: ExcelJSModule } = await import('exceljs'); - return ExcelJSModule; -} +const require = createRequire(import.meta.url); + +// require(), not import(): Node keeps a failed import() failed, so it couldn't load again +export const exceljsPackage = new LazyPackage('exceljs', 'Excel support', (): typeof ExcelJS => require('exceljs')); -/** A new exceljs Workbook */ async function newWorkbook(): Promise { - const ExcelJSModule = await loadExcelJS(); - return new ExcelJSModule.Workbook(); + return new (exceljsPackage.load().Workbook)(); } // File size limit: 10MB diff --git a/src/utils/files/factory.ts b/src/utils/files/factory.ts index 020280907..7facab073 100644 --- a/src/utils/files/factory.ts +++ b/src/utils/files/factory.ts @@ -6,13 +6,19 @@ * or async (content-based like BinaryFileHandler using isBinaryFile) */ +import path from 'path'; import { FileHandler } from './base.js'; import { TextFileHandler } from './text.js'; import { ImageFileHandler } from './image.js'; import { BinaryFileHandler } from './binary.js'; -import { ExcelFileHandler } from './excel.js'; +import { ExcelFileHandler, exceljsPackage } from './excel.js'; import { PdfFileHandler } from './pdf.js'; -import { DocxFileHandler } from './docx.js'; +import { DocxFileHandler, pizzipPackage } from './docx.js'; +import { pdfLibPackage } from '../../tools/pdf/manipulations.js'; +import { pdf2mdPackage } from '../../tools/pdf/lib/pdf2md.js'; +import { unpdfPackage } from '../../tools/pdf/extract-images.js'; +import { mdToPdfPackage } from '../../tools/pdf/markdown.js'; +import type { LazyPackage } from '../lazy-package.js'; // Singleton instances of each handler let excelHandler: ExcelFileHandler | null = null; @@ -123,3 +129,56 @@ export function isExcelFile(path: string): boolean { export function isImageFile(path: string): boolean { return getImageHandler().canHandle(path); } + +export type FileAction = 'read' | 'write' | 'edit'; + +/** + * Right after initialize (#777): the packages Excel, DOCX and PDF files need, + * loaded in the background one at a time, smaller first, so each pause of the + * main thread is one package. + */ +export function preloadFileSupport(): void { + void (async () => { + for (const pkg of [pizzipPackage, pdfLibPackage, pdf2mdPackage, unpdfPackage, exceljsPackage, mdToPdfPackage]) { + await pkg.preload(); + } + })(); +} + +/** The packages a file needs for `action`, by the handlers' canHandle() (its name, no disk access) */ +function packagesFor(filePath: string, action: FileAction, isPdf: boolean, isUrl: boolean): LazyPackage[] { + if (isPdf) { + // Editing a PDF can insert markdown pages, which are rendered + return action === 'read' ? [pdf2mdPackage, unpdfPackage] : action === 'write' ? [mdToPdfPackage] : [pdfLibPackage, mdToPdfPackage]; + } + // A URL is fetched, not opened by a file handler: only a PDF one is parsed + if (isUrl) return []; + if (getDocxHandler().canHandle(filePath)) return [pizzipPackage]; + if (getExcelHandler().canHandle(filePath)) return [exceljsPackage]; + return []; +} + +/** + * The answer for a tool call that would `action` `filePath` while a package it + * needs is still loading, naming the file; undefined once loaded. A package + * not preloading yet (before initialize, or after a failure) starts here, + * after the call has answered. `isPdf` for a PDF whatever its name (write_pdf), + * `isUrl` for a URL read. + */ +export function stillLoadingError( + filePath: string, + action: FileAction, + { isPdf = getPdfHandler().canHandle(filePath), isUrl = false }: { isPdf?: boolean; isUrl?: boolean } = {} +): string | undefined { + const pending = packagesFor(filePath, action, isPdf, isUrl).find((pkg) => !pkg.loaded); + if (!pending) return undefined; + const file = path.basename(filePath); + const { error, needsRestart, support } = pending; + if (!needsRestart) void pending.preload(); + if (!error) { + return `Can't ${action} ${file} yet: Desktop Commander is still loading its ${support} (it starts right after launch). Try again in a few seconds.`; + } + return needsRestart + ? `Can't ${action} ${file}: Desktop Commander couldn't load its ${support} (${error}). Restart Desktop Commander to load it again.` + : `Can't ${action} ${file}: Desktop Commander couldn't load its ${support} (${error}). It's loading it again; try again in a few seconds.`; +} diff --git a/src/utils/files/index.ts b/src/utils/files/index.ts index 992602309..037440811 100644 --- a/src/utils/files/index.ts +++ b/src/utils/files/index.ts @@ -7,10 +7,12 @@ export * from './base.js'; // Factory function -export { getFileHandler, isExcelFile, isImageFile } from './factory.js'; +export { getFileHandler, isExcelFile, isImageFile, preloadFileSupport, stillLoadingError } from './factory.js'; +export type { FileAction } from './factory.js'; // File handlers export { TextFileHandler } from './text.js'; export { ImageFileHandler, isImageAnswer } from './image.js'; export { BinaryFileHandler } from './binary.js'; -export { ExcelFileHandler } from './excel.js'; +export { ExcelFileHandler, exceljsPackage } from './excel.js'; +export { pizzipPackage } from './docx.js'; diff --git a/src/utils/heavy-packages.ts b/src/utils/heavy-packages.ts deleted file mode 100644 index 598a5fc15..000000000 --- a/src/utils/heavy-packages.ts +++ /dev/null @@ -1,106 +0,0 @@ -/** - * The packages only Excel, DOCX and PDF files need, loaded in the background - * right after initialize, one at a time (#715, review on #777). - * - * Loading them before answering initialize took 25-90 s on one reporter's - * machine and timed the client out, and loading one inside the tool call that - * first needs it can take longer than the few seconds a client gives a call. - * So initialize loads none of them. Right after it, startBackgroundLoad() - * loads them one by one, a turn of the event loop between them, so each pause - * of the main thread is one package. Until a file type's packages are loaded, - * the tool calls that need them answer at once (notReadyMessage(), checked in - * server.ts). A load that fails isn't kept: the next check starts it again. - * - * Each package is loaded with the loader its own module uses on demand, so - * code called without the server (tests, other callers) loads it as before. - */ -import path from 'path'; -import { loadExcelJS } from './files/excel.js'; -import { loadPizZip } from './files/docx.js'; -import { loadPdf2md } from '../tools/pdf/lib/pdf2md.js'; -import { loadUnpdf } from '../tools/pdf/extract-images.js'; -import { loadMdToPdf } from '../tools/pdf/markdown.js'; -import { loadPdfLib } from '../tools/pdf/manipulations.js'; -import { logger } from './logger.js'; - -/** Each package and its module's loader, in the order the background load takes them (smaller first) */ -const PACKAGES = { - pizzip: loadPizZip, - 'pdf-lib': loadPdfLib, - '@opendocsg/pdf2md': loadPdf2md, - unpdf: loadUnpdf, - exceljs: loadExcelJS, - 'md-to-pdf': loadMdToPdf, -} satisfies Record unknown>; -type HeavyPackage = keyof typeof PACKAGES; - -/** What a file type needs, and its name in the answers */ -const SUPPORT = { - excel: { name: 'Excel support', packages: ['exceljs'] }, - docx: { name: 'DOCX support', packages: ['pizzip'] }, - pdfRead: { name: 'PDF reading support', packages: ['@opendocsg/pdf2md', 'unpdf'] }, - pdfWrite: { name: 'PDF writing support', packages: ['md-to-pdf'] }, - pdfEdit: { name: 'PDF editing support', packages: ['pdf-lib'] }, -} as const satisfies Record; -export type HeavySupport = keyof typeof SUPPORT; - -type LoadState = { status: 'loading'; done: Promise } | { status: 'ready' } | { status: 'failed'; error: string }; -const states = new Map(); - -/** - * Loads `pkg` on a later turn of the event loop, never inside the caller's - * (a tool call that starts it answers first). Resolves once loaded or failed. - */ -function load(pkg: HeavyPackage): Promise { - const state = states.get(pkg); - if (state?.status === 'ready') return Promise.resolve(); - if (state?.status === 'loading') return state.done; - const done = new Promise((resolve) => setImmediate(resolve)) - .then(async () => { await PACKAGES[pkg](); }) - .then( - () => { states.set(pkg, { status: 'ready' }); }, - (error) => { - const message = error instanceof Error ? error.message : String(error); - states.set(pkg, { status: 'failed', error: message }); - logger.error(`Loading ${pkg} failed: ${message}`); - }); - states.set(pkg, { status: 'loading', done }); - return done; -} - -/** Loads every package, one at a time; one already loaded or loading isn't loaded again */ -export async function startBackgroundLoad(): Promise { - for (const pkg of Object.keys(PACKAGES) as HeavyPackage[]) { - await load(pkg); - } -} - -/** Whether everything `support` needs is loaded */ -export function isReady(support: HeavySupport): boolean { - return SUPPORT[support].packages.every((pkg) => states.get(pkg)?.status === 'ready'); -} - -/** What a tool call does to the file, as its error says */ -export type FileAction = 'read' | 'write' | 'edit'; - -/** - * The error a tool call that needs `support` to `action` `filePath` answers - * with while it isn't loaded, or undefined once it is. A package not started - * yet (before initialize) or that failed is started here, after the call has - * answered. - */ -export function notReadyMessage(support: HeavySupport, filePath: string, action: FileAction): string | undefined { - const { name, packages } = SUPPORT[support]; - const file = path.basename(filePath); - for (const pkg of packages) { - const state = states.get(pkg); - if (state?.status === 'ready') continue; - if (state?.status === 'failed') { - void load(pkg); - return `Can't ${action} ${file}: Desktop Commander couldn't load its ${name} (${state.error}). It's loading it again; try again in a few seconds.`; - } - if (!state) void load(pkg); - return `Can't ${action} ${file} yet: Desktop Commander is still loading its ${name} (it starts right after launch). Try again in a few seconds.`; - } - return undefined; -} diff --git a/src/utils/lazy-package.ts b/src/utils/lazy-package.ts new file mode 100644 index 000000000..650d3766a --- /dev/null +++ b/src/utils/lazy-package.ts @@ -0,0 +1,41 @@ +import { logger } from './logger.js'; + +/** + * A package only some files need, declared next to the code that uses it, + * whose load() loads it on first use (#715). Loading them before initialize + * took 25-90 s on one reporter's machine and timed the client out, and loading + * one inside the tool call that first needs it can take longer than a client + * waits. So the server preloads them right after initialize, one at a time + * (preloadFileSupport() in utils/files/factory.ts), and until one is loaded the + * calls that need it answer at once that it is still loading. preload() loads + * it on a later turn of the event loop, so a call that starts it answers + * first. `error` says why the last preload failed. Node keeps a failed + * import() failed, so a package loaded with import() then `needsRestart`. + */ +export class LazyPackage { + loaded = false; + error?: string; + needsRestart = false; + private running?: Promise; + + constructor(readonly name: string, readonly support: string, readonly load: () => T) {} + + preload(): Promise { + this.running ??= new Promise((resolve) => setImmediate(resolve)).then(async () => { + let loading: unknown; + try { + loading = this.load(); + await loading; + this.loaded = true; + this.error = undefined; + } catch (error) { + this.error = error instanceof Error ? error.message : String(error); + this.needsRestart = loading instanceof Promise; + logger.error(`Loading ${this.name} failed: ${this.error}`); + } finally { + this.running = undefined; + } + }); + return this.running; + } +} diff --git a/test/test-search-office-completion.js b/test/test-search-office-completion.js index e1a04cab6..19b113018 100644 --- a/test/test-search-office-completion.js +++ b/test/test-search-office-completion.js @@ -17,7 +17,6 @@ import { configManager } from '../dist/config-manager.js'; import { startSearchAndWait } from './helpers/search.js'; import { runIfMain } from './helpers/run-if-main.js'; import { runNode } from './helpers/run-node.js'; -import { hookArgs } from './helpers/module-hooks.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const TEST_DIR = path.join(__dirname, 'search-office-completion-test'); @@ -189,20 +188,20 @@ async function testTimeoutStopsOfficeSearches() { * An Office search that fails as a whole (here: ExcelJS can't be loaded) must * not vanish: the search answers that part of it failed, with the other sources' * matches, and the log says which part failed and why. Runs in a child process - * where exceljs can't be loaded (the search loads it with loadExcelJS(), in - * utils/files/excel.js). + * where exceljs can't be loaded (the search loads it with require(), through + * exceljsPackage in utils/files/excel.js). */ async function testFailedOfficeSearchIsLogged() { console.log('Testing that a failed Office search is logged...'); const REASON = 'exceljs is unavailable in this test'; - const hooks = ` - export async function resolve(specifier, context, nextResolve) { - if (specifier === 'exceljs' && context.parentURL?.endsWith('/utils/files/excel.js')) { - throw new Error(${JSON.stringify(REASON)}); - } - return nextResolve(specifier, context); - }`; + const preload = ` + import Module from 'node:module'; + const resolveFilename = Module._resolveFilename; + Module._resolveFilename = function (request, ...rest) { + if (request === 'exceljs') throw new Error(${JSON.stringify(REASON)}); + return resolveFilename.call(this, request, ...rest); + };`; const dist = (file) => pathToFileURL(path.join(__dirname, '..', 'dist', file)).href; const script = ` import { handleGetMoreSearchResults } from ${JSON.stringify(dist('handlers/search-handlers.js'))}; @@ -213,7 +212,7 @@ async function testFailedOfficeSearchIsLogged() { searchManager.dispose(); console.log(JSON.stringify({ sessionId, isError: !!page.isError, text: page.content[0].text }));`; const child = await runNode([ - ...hookArgs(`data:text/javascript,${encodeURIComponent(hooks)}`), '--input-type=module', '-e', script, + '--import', `data:text/javascript,${encodeURIComponent(preload)}`, '--input-type=module', '-e', script, ], { timeoutMs: 60000 }); assert.strictEqual(child.status, 0, `The search process failed (${child.status}): ${child.stderr}`);