diff --git a/src/document-conversion-limiter.ts b/src/document-conversion-limiter.ts new file mode 100644 index 00000000..f1ce8a25 --- /dev/null +++ b/src/document-conversion-limiter.ts @@ -0,0 +1,67 @@ +/** + * @fileoverview Global concurrency limiter for spawning external document + * converters (pdftoppm / LibreOffice `soffice` / Word-COM `powershell.exe`). + * + * Without a cap, N simultaneous thumbnail/preview requests for *distinct* + * documents fork N converter processes at once — each held open for up to the + * multi-minute conversion timeout. That is a localhost resource-exhaustion + * (fork-bomb-shaped) vector: a handful of large PDFs detected at once can pin + * CPU and RAM. This module serializes converter spawns down to a small fixed + * pool; excess spawns queue (FIFO) until a slot frees. The in-flight cache in + * `document-preview-cache.ts` already de-dups *identical* inputs; this bounds + * the *distinct* case the cache can't. + * + * Permit accounting transfers the slot directly to the next waiter on release + * (rather than decrement-then-reacquire) so the active count can never exceed + * the cap even under interleaved async resumption. + * + * NOT re-entrant: never call `runWithConversionLimit` from inside a task that is + * already holding a slot — a nested acquire under a full pool would deadlock. + * The converter call sites only ever acquire once per request (the office path + * acquires for `soffice` and `pdftoppm` sequentially, not nested). + */ + +/** + * Max converter processes allowed to run concurrently across the whole process. + * Override with CODEMAN_MAX_DOCUMENT_CONVERSIONS (clamped to >= 1). + */ +const MAX_CONCURRENT_DOCUMENT_CONVERSIONS = (() => { + const raw = Number(process.env.CODEMAN_MAX_DOCUMENT_CONVERSIONS); + return Number.isFinite(raw) && raw >= 1 ? Math.floor(raw) : 3; +})(); + +let active = 0; +const waiters: Array<() => void> = []; + +/** Test/diagnostic hook: converters currently holding a slot. */ +export function getActiveConversionCount(): number { + return active; +} + +function acquire(): Promise { + if (active < MAX_CONCURRENT_DOCUMENT_CONVERSIONS) { + active++; + return Promise.resolve(); + } + return new Promise((resolve) => waiters.push(resolve)); +} + +function release(): void { + const next = waiters.shift(); + if (next) { + // Hand the slot straight to the next waiter — `active` stays at the cap. + next(); + } else { + active--; + } +} + +/** Run `task` once a converter slot is free, releasing the slot afterward. */ +export async function runWithConversionLimit(task: () => Promise): Promise { + await acquire(); + try { + return await task(); + } finally { + release(); + } +} diff --git a/src/document-preview-cache.ts b/src/document-preview-cache.ts index 5bd206ca..33fe1375 100644 --- a/src/document-preview-cache.ts +++ b/src/document-preview-cache.ts @@ -9,11 +9,22 @@ import { tmpdir } from 'node:os'; import { basename, dirname, extname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; import { promisify } from 'node:util'; +import { runWithConversionLimit } from './document-conversion-limiter.js'; const execFileAsync = promisify(execFile); const OFFICE_CONVERSION_TIMEOUT_MS = 5 * 60_000; const DOCUMENT_PREVIEW_CACHE_DIR = join(tmpdir(), 'codeman-document-preview-cache'); +/** + * Cap on persistent converted-PDF files kept in DOCUMENT_PREVIEW_CACHE_DIR. + * The cache key embeds the source mtime, so every edit to a doc orphans its + * prior PDF; without a cap the dir grows unbounded across long-running sessions. + * Override with CODEMAN_MAX_PREVIEW_CACHE_FILES (clamped to >= 1). + */ +const MAX_PREVIEW_CACHE_FILES = (() => { + const raw = Number(process.env.CODEMAN_MAX_PREVIEW_CACHE_FILES); + return Number.isFinite(raw) && raw >= 1 ? Math.floor(raw) : 100; +})(); function buildWordExportPdfScript(sourcePath: string, outputPath: string): string { return ` $ErrorActionPreference = "Stop" @@ -50,6 +61,40 @@ export function clearDocumentPreviewCache(): void { inFlightOfficeConversions.clear(); } +/** + * Best-effort LRU-ish eviction for the persistent converted-PDF cache: keeps at + * most MAX_PREVIEW_CACHE_FILES `*.pdf` files in `cacheDir`, deleting the oldest + * by mtime once over the cap. Never throws — a pruning failure must not fail the + * conversion that triggered it. Only `*.pdf` files are considered, so the + * transient `work-*` mkdtemp dirs are ignored. + */ +export async function pruneDocumentPreviewCache(cacheDir: string): Promise { + try { + const entries = await fs.readdir(cacheDir); + const pdfs = entries.filter((name) => name.toLowerCase().endsWith('.pdf')); + if (pdfs.length <= MAX_PREVIEW_CACHE_FILES) return; + + const stats = await Promise.all( + pdfs.map(async (name) => { + const fullPath = join(cacheDir, name); + try { + const stat = await fs.stat(fullPath); + return { fullPath, mtimeMs: stat.mtimeMs ?? 0 }; + } catch { + return null; + } + }) + ); + + const sorted = stats.filter((s): s is { fullPath: string; mtimeMs: number } => s !== null); + sorted.sort((a, b) => a.mtimeMs - b.mtimeMs); // oldest first + const toRemove = sorted.slice(0, Math.max(0, sorted.length - MAX_PREVIEW_CACHE_FILES)); + await Promise.all(toRemove.map((entry) => fs.rm(entry.fullPath, { force: true }).catch(() => {}))); + } catch { + // Best-effort: pruning must never break a conversion. + } +} + export async function getOfficePreviewPdfPath(filePath: string, extension: string): Promise { const ext = extension.toLowerCase().replace(/^\./, ''); if (ext !== 'docx' && ext !== 'pptx') return null; @@ -147,23 +192,26 @@ async function convertWordDocumentToCachedPdf(filePath: string, cachePath: strin const sourcePath = wslMountPathToWindowsPath(sourceCopyPath); if (!sourcePath) return null; - await execFileAsync( - 'powershell.exe', - [ - '-NoProfile', - '-NonInteractive', - '-ExecutionPolicy', - 'Bypass', - '-EncodedCommand', - encodePowerShellCommand(buildWordExportPdfScript(sourcePath, outputPath)), - ], - { - timeout: OFFICE_CONVERSION_TIMEOUT_MS, - maxBuffer: 1024 * 1024, - } + await runWithConversionLimit(() => + execFileAsync( + 'powershell.exe', + [ + '-NoProfile', + '-NonInteractive', + '-ExecutionPolicy', + 'Bypass', + '-EncodedCommand', + encodePowerShellCommand(buildWordExportPdfScript(sourcePath, outputPath)), + ], + { + timeout: OFFICE_CONVERSION_TIMEOUT_MS, + maxBuffer: 1024 * 1024, + } + ) ); if (await fileExists(cachePath)) { + await pruneDocumentPreviewCache(dirname(cachePath)); return cachePath; } @@ -186,33 +234,37 @@ async function convertLibreOfficeDocumentToCachedPdf(filePath: string, cachePath let workDir: string | undefined; try { await fs.mkdir(DOCUMENT_PREVIEW_CACHE_DIR, { recursive: true }); - workDir = await fs.mkdtemp(join(DOCUMENT_PREVIEW_CACHE_DIR, 'work-')); - const profileDir = join(workDir, 'profile'); + const outDir = await fs.mkdtemp(join(DOCUMENT_PREVIEW_CACHE_DIR, 'work-')); + workDir = outDir; + const profileDir = join(outDir, 'profile'); await fs.mkdir(profileDir, { recursive: true }); - await execFileAsync( - 'soffice', - [ - '--headless', - '--nologo', - '--nofirststartwizard', - `-env:UserInstallation=${pathToFileURL(profileDir).href}`, - '--convert-to', - 'pdf', - '--outdir', - workDir, - filePath, - ], - { - timeout: OFFICE_CONVERSION_TIMEOUT_MS, - maxBuffer: 1024 * 1024, - } + await runWithConversionLimit(() => + execFileAsync( + 'soffice', + [ + '--headless', + '--nologo', + '--nofirststartwizard', + `-env:UserInstallation=${pathToFileURL(profileDir).href}`, + '--convert-to', + 'pdf', + '--outdir', + outDir, + filePath, + ], + { + timeout: OFFICE_CONVERSION_TIMEOUT_MS, + maxBuffer: 1024 * 1024, + } + ) ); - const converted = (await fs.readdir(workDir)).find((name) => name.toLowerCase().endsWith('.pdf')); + const converted = (await fs.readdir(outDir)).find((name) => name.toLowerCase().endsWith('.pdf')); if (!converted) return null; await fs.rename(join(workDir, converted), cachePath); + await pruneDocumentPreviewCache(DOCUMENT_PREVIEW_CACHE_DIR); return cachePath; } catch (err) { console.warn( diff --git a/src/document-thumbnailer.ts b/src/document-thumbnailer.ts index 9c84e839..8972bebb 100644 --- a/src/document-thumbnailer.ts +++ b/src/document-thumbnailer.ts @@ -8,6 +8,7 @@ import { tmpdir } from 'node:os'; import { basename, extname, join } from 'node:path'; import { promisify } from 'node:util'; import { getOfficePreviewPdfPath } from './document-preview-cache.js'; +import { runWithConversionLimit } from './document-conversion-limiter.js'; const execFileAsync = promisify(execFile); const THUMBNAIL_CONVERSION_TIMEOUT_MS = 5 * 60_000; @@ -61,13 +62,11 @@ async function renderPdfFirstPage(filePath: string): Promise + execFileAsync('pdftoppm', ['-png', '-singlefile', '-f', '1', '-l', '1', '-scale-to', '520', filePath, prefix], { timeout: THUMBNAIL_CONVERSION_TIMEOUT_MS, maxBuffer: 1024 * 1024, - } + }) ); const content = await fs.readFile(`${prefix}.png`); return { content, contentType: 'image/png' }; diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 62c2a0bd..c2577898 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -2509,6 +2509,23 @@ Object.assign(CodemanApp.prototype, { return; } + // Workspace-path (auto-detected, unregistered) attachments: Office docs are + // converted to PDF server-side via the file-preview route; PDFs stream raw. + // Both render inline in an iframe. Without this, docx/pptx/pdf fall through + // to file-content below, which would dump the binary bytes as mojibake. + if (ext === 'docx' || ext === 'pptx') { + footerEl.textContent = ext.toUpperCase(); + const previewSrc = `/api/sessions/${sessionId}/file-preview?path=${encodeURIComponent(filePath)}`; + bodyEl.innerHTML = ``; + return; + } + if (ext === 'pdf') { + footerEl.textContent = 'PDF'; + const rawSrc = `/api/sessions/${sessionId}/file-raw?path=${encodeURIComponent(filePath)}`; + bodyEl.innerHTML = ``; + return; + } + try { const res = await fetch(`/api/sessions/${sessionId}/file-content?path=${encodeURIComponent(filePath)}&lines=500`); if (!res.ok) throw new Error('Failed to load file'); diff --git a/test/document-conversion-limiter.test.ts b/test/document-conversion-limiter.test.ts new file mode 100644 index 00000000..c194dc51 --- /dev/null +++ b/test/document-conversion-limiter.test.ts @@ -0,0 +1,47 @@ +import { describe, it, expect } from 'vitest'; +import { runWithConversionLimit, getActiveConversionCount } from '../src/document-conversion-limiter.js'; + +describe('document-conversion-limiter', () => { + it('never runs more converters than the cap (default 3) concurrently', async () => { + let running = 0; + let maxObserved = 0; + + const task = () => async () => { + running++; + maxObserved = Math.max(maxObserved, running); + // The module's own accounting must also stay within the cap. + expect(getActiveConversionCount()).toBeLessThanOrEqual(3); + await new Promise((resolve) => setTimeout(resolve, 5)); + running--; + }; + + await Promise.all(Array.from({ length: 12 }, () => runWithConversionLimit(task()))); + + expect(maxObserved).toBeLessThanOrEqual(3); + expect(maxObserved).toBeGreaterThan(1); // proves it genuinely parallelizes, not serializes + expect(getActiveConversionCount()).toBe(0); // every slot released + }); + + it('processes every queued task even when far more are submitted than the cap', async () => { + let completed = 0; + await Promise.all( + Array.from({ length: 25 }, () => + runWithConversionLimit(async () => { + await new Promise((resolve) => setTimeout(resolve, 1)); + completed++; + }) + ) + ); + expect(completed).toBe(25); + expect(getActiveConversionCount()).toBe(0); + }); + + it('releases the slot when a task throws', async () => { + await expect( + runWithConversionLimit(async () => { + throw new Error('boom'); + }) + ).rejects.toThrow('boom'); + expect(getActiveConversionCount()).toBe(0); + }); +}); diff --git a/test/document-preview-cache-eviction.test.ts b/test/document-preview-cache-eviction.test.ts new file mode 100644 index 00000000..2aeab44b --- /dev/null +++ b/test/document-preview-cache-eviction.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import fs from 'node:fs/promises'; +import { pruneDocumentPreviewCache } from '../src/document-preview-cache.js'; + +vi.mock('node:fs/promises', () => ({ + default: { + readdir: vi.fn(), + stat: vi.fn(), + rm: vi.fn(async () => undefined), + }, +})); + +const mockedReaddir = vi.mocked(fs.readdir); +const mockedStat = vi.mocked(fs.stat); +const mockedRm = vi.mocked(fs.rm); + +describe('pruneDocumentPreviewCache', () => { + beforeEach(() => { + vi.clearAllMocks(); + // mtime encoded in the filename (f0 oldest ... f101 newest) + mockedStat.mockImplementation(async (p) => { + const m = /f(\d+)\.pdf$/.exec(String(p)); + return { mtimeMs: m ? Number(m[1]) : 0, isFile: () => true } as never; + }); + }); + + it('evicts the oldest *.pdf files once the cache exceeds the cap (default 100)', async () => { + const names = Array.from({ length: 102 }, (_, i) => `f${i}.pdf`); + mockedReaddir.mockResolvedValue(names as never); + + await pruneDocumentPreviewCache('/tmp/codeman-document-preview-cache'); + + // 102 - 100 = 2 oldest removed + expect(mockedRm).toHaveBeenCalledTimes(2); + const removed = mockedRm.mock.calls.map((c) => String(c[0])); + expect(removed.some((p) => p.endsWith('f0.pdf'))).toBe(true); + expect(removed.some((p) => p.endsWith('f1.pdf'))).toBe(true); + expect(removed.some((p) => p.endsWith('f101.pdf'))).toBe(false); // newest kept + }); + + it('ignores non-pdf entries (e.g. transient work-* dirs) when counting', async () => { + const names = [...Array.from({ length: 50 }, (_, i) => `f${i}.pdf`), 'work-abc', 'work-def']; + mockedReaddir.mockResolvedValue(names as never); + + await pruneDocumentPreviewCache('/tmp/codeman-document-preview-cache'); + + expect(mockedRm).not.toHaveBeenCalled(); // 50 pdfs <= cap + }); + + it('never throws when the cache dir cannot be read', async () => { + mockedReaddir.mockRejectedValue(new Error('ENOENT')); + await expect(pruneDocumentPreviewCache('/tmp/missing')).resolves.toBeUndefined(); + expect(mockedRm).not.toHaveBeenCalled(); + }); +}); diff --git a/test/routes/file-routes-preview-thumbnail.test.ts b/test/routes/file-routes-preview-thumbnail.test.ts new file mode 100644 index 00000000..ecffbe95 --- /dev/null +++ b/test/routes/file-routes-preview-thumbnail.test.ts @@ -0,0 +1,156 @@ +/** + * @fileoverview Route coverage for the document preview/thumbnail endpoints + * added in COD-38 (PR #120): the by-attachmentId routes + * (/attachments/:id/preview|thumbnail) and the workspace-path routes + * (/file-preview|/file-thumbnail). Converters are mocked, so no real + * pdftoppm/LibreOffice is needed. Uses app.inject() — no real ports. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js'; +import { registerFileRoutes } from '../../src/web/routes/file-routes.js'; + +vi.mock('node:fs/promises', () => ({ + default: { + readFile: vi.fn(async () => Buffer.from('%PDF-1.4 fake pdf bytes')), + stat: vi.fn(async () => ({ size: 100, isFile: () => true, mtimeMs: 1 })), + readdir: vi.fn(async () => []), + mkdir: vi.fn(async () => undefined), + mkdtemp: vi.fn(async () => '/tmp/codeman-preview-test'), + rename: vi.fn(async () => undefined), + rm: vi.fn(async () => undefined), + }, +})); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, realpathSync: vi.fn((p: string) => p) }; +}); + +// Mock the converters so the routes don't shell out to real binaries. +vi.mock('../../src/document-thumbnailer.js', () => ({ + generateFirstPageThumbnail: vi.fn(async () => ({ content: Buffer.from('\x89PNG fake'), contentType: 'image/png' })), +})); +vi.mock('../../src/document-preview-cache.js', () => ({ + getOfficePreviewPdfPath: vi.fn(async () => '/tmp/codeman-document-preview-cache/out.pdf'), + getPreviewPdfDownloadName: vi.fn((name: string) => `${name.replace(/\.[^.]+$/, '')}.pdf`), +})); + +import { generateFirstPageThumbnail } from '../../src/document-thumbnailer.js'; +import { getOfficePreviewPdfPath } from '../../src/document-preview-cache.js'; +import { attachmentRegistry, type AttachmentRecord } from '../../src/attachment-registry.js'; + +const SID = 'test-session-1'; +const WORKDIR = '/tmp/test-workdir'; + +function makeRecord(over: Partial): AttachmentRecord { + return { + attachmentId: 'att_x', + sessionId: SID, + filePath: `${WORKDIR}/file`, + fileName: 'file', + extension: 'pdf', + attachmentType: 'document', + size: 100, + mtimeMs: 1, + timestamp: 1, + source: 'detected', + ...over, + }; +} + +describe('file-routes preview/thumbnail (COD-38)', () => { + let harness: RouteTestHarness; + + beforeEach(async () => { + harness = await createRouteTestHarness(registerFileRoutes, { sessionId: SID }); + vi.clearAllMocks(); + attachmentRegistry.clearSession(SID); + }); + + afterEach(async () => { + await harness.app.close(); + attachmentRegistry.clearSession(SID); + }); + + // ---- workspace-path routes ---- + + it('file-preview converts a workspace DOCX to an inline PDF', async () => { + const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${SID}/file-preview?path=deck.docx` }); + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toContain('application/pdf'); + expect(res.headers['content-disposition']).toContain('inline'); + expect(getOfficePreviewPdfPath).toHaveBeenCalled(); + }); + + it('file-preview redirects a non-Office workspace file (PDF) to the raw route', async () => { + const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${SID}/file-preview?path=report.pdf` }); + expect(res.statusCode).toBeGreaterThanOrEqual(300); + expect(res.statusCode).toBeLessThan(400); + expect(res.headers.location).toContain('/file-raw?path=report.pdf'); + expect(getOfficePreviewPdfPath).not.toHaveBeenCalled(); + }); + + it('file-preview 400s for a missing path parameter', async () => { + const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${SID}/file-preview` }); + expect(res.statusCode).toBe(400); + }); + + it('file-thumbnail returns a PNG for a supported workspace file', async () => { + const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${SID}/file-thumbnail?path=deck.pdf` }); + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toContain('image/png'); + expect(res.headers['x-content-type-options']).toBe('nosniff'); + expect(generateFirstPageThumbnail).toHaveBeenCalled(); + }); + + it('file-thumbnail 400s for an unsupported extension', async () => { + const res = await harness.app.inject({ method: 'GET', url: `/api/sessions/${SID}/file-thumbnail?path=notes.exe` }); + expect(res.statusCode).toBe(400); + expect(generateFirstPageThumbnail).not.toHaveBeenCalled(); + }); + + // ---- by-attachmentId routes ---- + + it('by-id preview converts a registered DOCX attachment', async () => { + attachmentRegistry.register( + makeRecord({ + attachmentId: 'att_docx', + filePath: `${WORKDIR}/deck.docx`, + fileName: 'deck.docx', + extension: 'docx', + }) + ); + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${SID}/attachments/att_docx/preview`, + }); + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toContain('application/pdf'); + }); + + it('by-id preview redirects a non-Office attachment (PNG) to its raw route', async () => { + attachmentRegistry.register( + makeRecord({ attachmentId: 'att_png', filePath: `${WORKDIR}/shot.png`, fileName: 'shot.png', extension: 'png' }) + ); + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${SID}/attachments/att_png/preview`, + }); + expect(res.statusCode).toBeGreaterThanOrEqual(300); + expect(res.statusCode).toBeLessThan(400); + expect(res.headers.location).toContain('/attachments/att_png/raw'); + }); + + it('by-id thumbnail returns a PNG for a registered attachment', async () => { + attachmentRegistry.register( + makeRecord({ attachmentId: 'att_pdf', filePath: `${WORKDIR}/deck.pdf`, fileName: 'deck.pdf', extension: 'pdf' }) + ); + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${SID}/attachments/att_pdf/thumbnail`, + }); + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toContain('image/png'); + }); +});