From 14ec5a8ae9ad44c7776c0ee8d13c3ce3fbf909d3 Mon Sep 17 00:00:00 2001 From: Christoph Leiter Date: Tue, 28 Jul 2026 08:39:36 +0200 Subject: [PATCH] fix(server): materialize provider images under the paseo home Provider images were materialized into a per-process directory created with mkdtemp(os.tmpdir(), "paseo-attachments-"). The OS reaps that location -- macOS clears /var/folders after roughly three days -- so every image in a transcript older than the reap window was silently orphaned: the stored markdown still pointed at a path whose bytes no longer existed, and nothing could recreate them. Materialize into resolvePaseoHome()/paseo-attachments/ instead. The directory keeps its 0700 mode and the files their 0600 mode, and the basename is unchanged so PROVIDER_IMAGE_MARKDOWN still matches history written by older builds (the optional "-" group covers the old mkdtemp shape). A stable directory is no longer self-limiting, so add a best-effort sweep of entries older than 30 days, run once per process on the first image materialized. Re-materializing an image rewrites it and refreshes its mtime, so images still being rendered keep themselves alive and only genuinely cold ones age out. This also makes the existing content-hash filename deduplication work across daemon restarts; previously it only held within a single process. Co-Authored-By: Claude Opus 4.8 --- .../provider-image-output.posix.test.ts | 49 ++++++-- .../providers/provider-image-output.test.ts | 114 ++++++++++++++++-- .../agent/providers/provider-image-output.ts | 71 +++++++++-- 3 files changed, 202 insertions(+), 32 deletions(-) diff --git a/packages/server/src/server/agent/providers/provider-image-output.posix.test.ts b/packages/server/src/server/agent/providers/provider-image-output.posix.test.ts index 1f4da6e04a..d9eae4b87f 100644 --- a/packages/server/src/server/agent/providers/provider-image-output.posix.test.ts +++ b/packages/server/src/server/agent/providers/provider-image-output.posix.test.ts @@ -1,24 +1,49 @@ -import { existsSync, rmSync, statSync } from "node:fs"; +import { existsSync, mkdtempSync, rmSync, statSync } from "node:fs"; +import os from "node:os"; import path from "node:path"; -import { describe, expect, test } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; -import { materializeProviderImage } from "./provider-image-output.js"; +import { + __resetMaterializedImageAttachmentDirForTests, + materializeProviderImage, +} from "./provider-image-output.js"; describe.skipIf(process.platform === "win32")("materializeProviderImage", () => { - test("writes image attachments under a private temp directory", () => { + const originalPaseoHome = process.env.PASEO_HOME; + let testHome: string | null = null; + + beforeEach(() => { + testHome = mkdtempSync(path.join(os.tmpdir(), "paseo-provider-images-posix-")); + process.env.PASEO_HOME = testHome; + __resetMaterializedImageAttachmentDirForTests(); + }); + + afterEach(() => { + if (originalPaseoHome === undefined) { + delete process.env.PASEO_HOME; + } else { + process.env.PASEO_HOME = originalPaseoHome; + } + if (testHome) { + rmSync(testHome, { recursive: true, force: true }); + testHome = null; + } + __resetMaterializedImageAttachmentDirForTests(); + }); + + test("writes image attachments under a private directory in the paseo home", () => { const materialized = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png", }); const attachmentDir = path.dirname(materialized.path); - try { - expect(path.basename(attachmentDir)).toMatch(/^paseo-attachments-/); - expect(existsSync(materialized.path)).toBe(true); - expect(statSync(attachmentDir).mode & 0o777).toBe(0o700); - expect(statSync(materialized.path).mode & 0o777).toBe(0o600); - } finally { - rmSync(attachmentDir, { recursive: true, force: true }); - } + // Exactly "paseo-attachments" — no mkdtemp suffix. The old per-process temp + // directory is what let the OS reap these images out from under the app. + expect(path.basename(attachmentDir)).toBe("paseo-attachments"); + expect(attachmentDir).toBe(path.join(testHome as string, "paseo-attachments")); + expect(existsSync(materialized.path)).toBe(true); + expect(statSync(attachmentDir).mode & 0o777).toBe(0o700); + expect(statSync(materialized.path).mode & 0o777).toBe(0o600); }); }); diff --git a/packages/server/src/server/agent/providers/provider-image-output.test.ts b/packages/server/src/server/agent/providers/provider-image-output.test.ts index 0c32af04fe..19610f9ae0 100644 --- a/packages/server/src/server/agent/providers/provider-image-output.test.ts +++ b/packages/server/src/server/agent/providers/provider-image-output.test.ts @@ -1,8 +1,10 @@ -import { existsSync, rmSync } from "node:fs"; +import { existsSync, mkdtempSync, readdirSync, rmSync, statSync, utimesSync } from "node:fs"; +import os from "node:os"; import path from "node:path"; -import { describe, expect, test } from "vitest"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { + __resetMaterializedImageAttachmentDirForTests, isProviderImageMarkdown, materializeProviderImage, renderProviderImageOutputAsAssistantMarkdown, @@ -10,6 +12,25 @@ import { const HASH = "a".repeat(64); +// Materialized images live under the paseo home now, so without this every test +// run would read and delete the developer's real provider images. +const originalPaseoHome = process.env.PASEO_HOME; +let testHome: string | null = null; + +function useTempPaseoHome(): string { + testHome = mkdtempSync(path.join(os.tmpdir(), "paseo-provider-images-")); + process.env.PASEO_HOME = testHome; + __resetMaterializedImageAttachmentDirForTests(); + return testHome; +} + +function attachmentsDir(): string { + if (!testHome) { + throw new Error("useTempPaseoHome() must run before reading the attachments dir."); + } + return path.join(testHome, "paseo-attachments"); +} + function renderImageMarkdown(imagePath: string): string { const item = renderProviderImageOutputAsAssistantMarkdown({ path: imagePath }); if (!item || item.type !== "assistant_message") { @@ -90,26 +111,99 @@ describe("isProviderImageMarkdown", () => { }); describe("materializeProviderImage", () => { - test("recreates the private temp directory if the cached directory is removed", () => { + beforeEach(() => { + useTempPaseoHome(); + }); + + afterEach(() => { + if (originalPaseoHome === undefined) { + delete process.env.PASEO_HOME; + } else { + process.env.PASEO_HOME = originalPaseoHome; + } + if (testHome) { + rmSync(testHome, { recursive: true, force: true }); + testHome = null; + } + __resetMaterializedImageAttachmentDirForTests(); + }); + + test("recreates the private directory if the cached directory is removed", () => { const first = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png", }); - const firstDir = path.dirname(first.path); expect(existsSync(first.path)).toBe(true); - rmSync(firstDir, { recursive: true, force: true }); + rmSync(path.dirname(first.path), { recursive: true, force: true }); const second = materializeProviderImage({ data: "ZGVmNDU2", mimeType: "image/png", }); - const secondDir = path.dirname(second.path); - try { - expect(existsSync(second.path)).toBe(true); - } finally { - rmSync(secondDir, { recursive: true, force: true }); + expect(existsSync(second.path)).toBe(true); + }); + + test("stores images in a stable directory under the paseo home", () => { + const result = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + + // A fixed directory, not a per-process mkdtemp one — the old shape is what + // let the OS reap these images out from under the app after ~3 days. + expect(path.dirname(result.path)).toBe(attachmentsDir()); + expect(path.basename(path.dirname(result.path))).toBe("paseo-attachments"); + expect(statSync(attachmentsDir()).mode & 0o777).toBe(0o700); + expect(statSync(result.path).mode & 0o777).toBe(0o600); + }); + + test("is idempotent for the same bytes across daemon restarts", () => { + const first = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + + // A fresh daemon process: module state is cleared but the directory persists. + __resetMaterializedImageAttachmentDirForTests(); + const second = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + + expect(second.path).toBe(first.path); + expect(readdirSync(attachmentsDir())).toHaveLength(1); + }); + + test("emits markdown that isProviderImageMarkdown still recognizes", () => { + const item = renderProviderImageOutputAsAssistantMarkdown( + { data: "YWJjMTIz", mimeType: "image/png" }, + { materialize: materializeProviderImage }, + ); + + if (!item || item.type !== "assistant_message") { + throw new Error("Expected provider image output to render as assistant markdown."); } + expect(isProviderImageMarkdown(item.text)).toBe(true); + }); + + test("sweeps images older than the retention window and keeps fresh ones", () => { + const stale = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + const fresh = materializeProviderImage({ data: "ZGVmNDU2", mimeType: "image/png" }); + + const longAgoSeconds = Date.now() / 1000 - 31 * 24 * 60 * 60; + utimesSync(stale.path, longAgoSeconds, longAgoSeconds); + + // The sweep runs once per process, on the first image materialized. + __resetMaterializedImageAttachmentDirForTests(); + materializeProviderImage({ data: "Z2hpNzg5", mimeType: "image/png" }); + + expect(existsSync(stale.path)).toBe(false); + expect(existsSync(fresh.path)).toBe(true); + }); + + test("re-materializing a stale image keeps it alive", () => { + const image = materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + const longAgoSeconds = Date.now() / 1000 - 31 * 24 * 60 * 60; + utimesSync(image.path, longAgoSeconds, longAgoSeconds); + + // Rendering it again rewrites the bytes, refreshing mtime before any sweep. + materializeProviderImage({ data: "YWJjMTIz", mimeType: "image/png" }); + __resetMaterializedImageAttachmentDirForTests(); + materializeProviderImage({ data: "Z2hpNzg5", mimeType: "image/png" }); + + expect(existsSync(image.path)).toBe(true); }); }); diff --git a/packages/server/src/server/agent/providers/provider-image-output.ts b/packages/server/src/server/agent/providers/provider-image-output.ts index 387158b32f..a970f92c7c 100644 --- a/packages/server/src/server/agent/providers/provider-image-output.ts +++ b/packages/server/src/server/agent/providers/provider-image-output.ts @@ -1,9 +1,9 @@ import { createHash } from "node:crypto"; import * as fsSync from "node:fs"; -import os from "node:os"; import path from "node:path"; import type { AgentTimelineItem } from "../agent-sdk-types.js"; +import { resolvePaseoHome } from "../../paseo-home.js"; export interface ProviderImageOutput { path?: string | null; @@ -17,12 +17,22 @@ export interface MaterializedProviderImage { path: string; } +// The directory basename stays "paseo-attachments" because PROVIDER_IMAGE_MARKDOWN +// below matches on it, and history written by older builds used +// mkdtemp(os.tmpdir(), "paseo-attachments-") — the optional "-" group in +// that regex is what lets one pattern match both shapes. const PROVIDER_IMAGE_ATTACHMENT_DIR = "paseo-attachments"; -const PROVIDER_IMAGE_ATTACHMENT_DIR_PREFIX = `${PROVIDER_IMAGE_ATTACHMENT_DIR}-`; const PRIVATE_ATTACHMENT_DIR_MODE = 0o700; const MATERIALIZED_IMAGE_FILE_MODE = 0o600; +// Materialized images used to live in os.tmpdir(), which the OS reaps (macOS +// clears /var/folders after ~3 days). That silently orphaned every image in any +// transcript older than the reap window: the markdown still pointed at a path +// whose bytes were gone, and nothing could re-create them. They now live under +// the paseo home, so this sweep is what bounds the directory instead. +const MATERIALIZED_IMAGE_MAX_AGE_MS = 30 * 24 * 60 * 60 * 1000; let materializedImageAttachmentDir: string | null = null; +let sweptMaterializedImageAttachments = false; function canReuseMaterializedImageAttachmentDir(dir: string): boolean { try { @@ -37,6 +47,38 @@ function canReuseMaterializedImageAttachmentDir(dir: string): boolean { } } +// Runs once per process, on the first image materialized. Re-materializing an +// image rewrites it, refreshing mtime, so an image still being rendered keeps +// itself alive and only genuinely cold ones age out. +function sweepMaterializedImageAttachments(dir: string): void { + if (sweptMaterializedImageAttachments) { + return; + } + sweptMaterializedImageAttachments = true; + + const cutoffMs = Date.now() - MATERIALIZED_IMAGE_MAX_AGE_MS; + let entries: fsSync.Dirent[]; + try { + entries = fsSync.readdirSync(dir, { withFileTypes: true }); + } catch { + return; + } + + for (const entry of entries) { + if (!entry.isFile()) { + continue; + } + const filePath = path.join(dir, entry.name); + try { + if (fsSync.statSync(filePath).mtimeMs < cutoffMs) { + fsSync.rmSync(filePath, { force: true }); + } + } catch { + // Raced with another writer or already gone. Sweeping is best-effort. + } + } +} + function getMaterializedImageAttachmentDir(): string { if ( materializedImageAttachmentDir && @@ -45,11 +87,18 @@ function getMaterializedImageAttachmentDir(): string { return materializedImageAttachmentDir; } - materializedImageAttachmentDir = fsSync.mkdtempSync( - path.join(os.tmpdir(), PROVIDER_IMAGE_ATTACHMENT_DIR_PREFIX), - ); - fsSync.chmodSync(materializedImageAttachmentDir, PRIVATE_ATTACHMENT_DIR_MODE); - return materializedImageAttachmentDir; + const dir = path.join(resolvePaseoHome(), PROVIDER_IMAGE_ATTACHMENT_DIR); + fsSync.mkdirSync(dir, { recursive: true, mode: PRIVATE_ATTACHMENT_DIR_MODE }); + fsSync.chmodSync(dir, PRIVATE_ATTACHMENT_DIR_MODE); + materializedImageAttachmentDir = dir; + sweepMaterializedImageAttachments(dir); + return dir; +} + +/** Test-only hook: the sweep and the resolved directory are process-scoped. */ +export function __resetMaterializedImageAttachmentDirForTests(): void { + materializedImageAttachmentDir = null; + sweptMaterializedImageAttachments = false; } function getImageExtension(mimeType: string): string { @@ -81,9 +130,11 @@ function normalizeImageData(mimeType: string, data: string): { mimeType: string; return { mimeType, data }; } -// Filenames are a content hash of the bytes so re-materializing the same image -// within a process reuses the existing temp file instead of leaking a fresh one -// for repeated image blocks or history replay. +// Filenames are a content hash of the bytes, and the directory is now stable +// across daemon restarts, so re-materializing the same image reuses the existing +// file instead of leaking a fresh one for repeated image blocks or history +// replay. Under the old per-process temp directory this deduplication only held +// within a single daemon process. export function materializeProviderImage(image: { data: string; mimeType: string | null;