From d58af33fba73936659158ba2167a00475889d1e5 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 1 Mar 2026 19:01:47 -0800 Subject: [PATCH] Fix attachment cleanup to match exact thread segments - Parse thread segments from attachment IDs using a UUID-aware pattern - Prevent revert/delete cleanup from removing files belonging to similarly prefixed threads - Add tests for prefix-collision cases in attachment store and projection pipeline --- apps/server/src/attachmentStore.test.ts | 16 +++++++++++-- apps/server/src/attachmentStore.ts | 19 +++++++++++++++ .../Layers/ProjectionPipeline.test.ts | 22 ++++++++++++++--- .../Layers/ProjectionPipeline.ts | 24 +++++++++++++++---- 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/apps/server/src/attachmentStore.test.ts b/apps/server/src/attachmentStore.test.ts index b3da749dda1154188cf872682d55c98550accac4..147084c56583ee1ca6bb44332cc693b88e057f2f 100644 --- a/apps/server/src/attachmentStore.test.ts +++ b/apps/server/src/attachmentStore.test.ts @@ -4,7 +4,11 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; -import { createAttachmentId, resolveAttachmentPathById } from "./attachmentStore.ts"; +import { + createAttachmentId, + parseThreadSegmentFromAttachmentId, + resolveAttachmentPathById, +} from "./attachmentStore.ts"; describe("attachmentStore", () => { it("sanitizes thread ids when creating attachment ids", () => { @@ -14,7 +18,7 @@ describe("attachmentStore", () => { return; } - const [threadSegment] = attachmentId.split("-", 1); + const threadSegment = parseThreadSegmentFromAttachmentId(attachmentId); expect(threadSegment).toBeTruthy(); expect(threadSegment).toMatch(/^[a-z0-9_-]+$/i); expect(threadSegment).not.toContain("."); @@ -22,6 +26,14 @@ describe("attachmentStore", () => { expect(threadSegment).not.toContain("/"); }); + it("parses exact thread segments from attachment ids without prefix collisions", () => { + const fooId = "foo-00000000-0000-4000-8000-000000000001"; + const fooBarId = "foo-bar-00000000-0000-4000-8000-000000000002"; + + expect(parseThreadSegmentFromAttachmentId(fooId)).toBe("foo"); + expect(parseThreadSegmentFromAttachmentId(fooBarId)).toBe("foo-bar"); + }); + it("resolves attachment path by id using the extension that exists on disk", () => { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3code-attachment-store-")); try { diff --git a/apps/server/src/attachmentStore.ts b/apps/server/src/attachmentStore.ts index 38219d702f5cf0a27527354b254122e3dcb3653c..59b6e499b2fc4a332525b8d3ebcdff86b6a7c3a8 100644 --- a/apps/server/src/attachmentStore.ts +++ b/apps/server/src/attachmentStore.ts @@ -12,6 +12,13 @@ import { inferImageExtension, SAFE_IMAGE_FILE_EXTENSIONS } from "./imageMime.ts" const ATTACHMENT_FILENAME_EXTENSIONS = [...SAFE_IMAGE_FILE_EXTENSIONS, ".bin"]; const ATTACHMENT_ID_THREAD_SEGMENT_MAX_CHARS = 80; +const ATTACHMENT_ID_THREAD_SEGMENT_PATTERN = "[a-z0-9_]+(?:-[a-z0-9_]+)*"; +const ATTACHMENT_ID_UUID_PATTERN = + "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"; +const ATTACHMENT_ID_PATTERN = new RegExp( + `^(${ATTACHMENT_ID_THREAD_SEGMENT_PATTERN})-(${ATTACHMENT_ID_UUID_PATTERN})$`, + "i", +); export function toSafeThreadAttachmentSegment(threadId: string): string | null { const segment = threadId @@ -35,6 +42,18 @@ export function createAttachmentId(threadId: string): string | null { return `${threadSegment}-${randomUUID()}`; } +export function parseThreadSegmentFromAttachmentId(attachmentId: string): string | null { + const normalizedId = normalizeAttachmentRelativePath(attachmentId); + if (!normalizedId || normalizedId.includes("/") || normalizedId.includes(".")) { + return null; + } + const match = normalizedId.match(ATTACHMENT_ID_PATTERN); + if (!match) { + return null; + } + return match[1]?.toLowerCase() ?? null; +} + export function attachmentRelativePath(attachment: ChatAttachment): string { switch (attachment.type) { case "image": { diff --git a/apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts b/apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts index 9aed16fff9ead027d2a2b28b83578d884efe6833..ba1b0afdc4619da67a875a0bd76ba1e940ece898 100644 --- a/apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts +++ b/apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts @@ -734,8 +734,10 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { const eventStore = yield* OrchestrationEventStore; const now = new Date().toISOString(); const threadId = ThreadId.makeUnsafe("thread revert.files"); - const keepAttachmentId = "thread-revert-files-keep"; - const removeAttachmentId = "thread-revert-files-remove"; + const keepAttachmentId = "thread-revert-files-00000000-0000-4000-8000-000000000001"; + const removeAttachmentId = "thread-revert-files-00000000-0000-4000-8000-000000000002"; + const otherThreadAttachmentId = + "thread-revert-files-extra-00000000-0000-4000-8000-000000000003"; const appendAndProject = (event: Parameters[0]) => eventStore @@ -904,8 +906,11 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { fs.mkdirSync(path.join(stateDir, "attachments"), { recursive: true }); fs.writeFileSync(keepPath, Buffer.from("keep")); fs.writeFileSync(removePath, Buffer.from("remove")); + const otherThreadPath = path.join(stateDir, "attachments", `${otherThreadAttachmentId}.png`); + fs.writeFileSync(otherThreadPath, Buffer.from("other")); assert.equal(fs.existsSync(keepPath), true); assert.equal(fs.existsSync(removePath), true); + assert.equal(fs.existsSync(otherThreadPath), true); yield* appendAndProject({ type: "thread.reverted", @@ -925,6 +930,7 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { assert.equal(fs.existsSync(keepPath), true); assert.equal(fs.existsSync(removePath), false); + assert.equal(fs.existsSync(otherThreadPath), true); }).pipe( (effect) => runWithProjectionPipelineLayer(stateDir, effect), Effect.ensuring(Effect.sync(() => fs.rmSync(stateDir, { recursive: true, force: true }))), @@ -943,7 +949,9 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { const eventStore = yield* OrchestrationEventStore; const now = new Date().toISOString(); const threadId = ThreadId.makeUnsafe("thread delete.files"); - const attachmentId = "thread-delete-files-att-1"; + const attachmentId = "thread-delete-files-00000000-0000-4000-8000-000000000001"; + const otherThreadAttachmentId = + "thread-delete-files-extra-00000000-0000-4000-8000-000000000002"; const appendAndProject = (event: Parameters[0]) => eventStore @@ -1025,9 +1033,16 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { }); const threadAttachmentPath = path.join(stateDir, "attachments", `${attachmentId}.png`); + const otherThreadAttachmentPath = path.join( + stateDir, + "attachments", + `${otherThreadAttachmentId}.png`, + ); fs.mkdirSync(path.join(stateDir, "attachments"), { recursive: true }); fs.writeFileSync(threadAttachmentPath, Buffer.from("delete")); + fs.writeFileSync(otherThreadAttachmentPath, Buffer.from("other-thread")); assert.equal(fs.existsSync(threadAttachmentPath), true); + assert.equal(fs.existsSync(otherThreadAttachmentPath), true); yield* appendAndProject({ type: "thread.deleted", @@ -1046,6 +1061,7 @@ projectionLayer("OrchestrationProjectionPipeline", (it) => { }); assert.equal(fs.existsSync(threadAttachmentPath), false); + assert.equal(fs.existsSync(otherThreadAttachmentPath), true); }).pipe( (effect) => runWithProjectionPipelineLayer(stateDir, effect), Effect.ensuring(Effect.sync(() => fs.rmSync(stateDir, { recursive: true, force: true }))), diff --git a/apps/server/src/orchestration/Layers/ProjectionPipeline.ts b/apps/server/src/orchestration/Layers/ProjectionPipeline.ts index 9f746f54d20393c151b13dded731b0f02c073505..f4c8728f39fd53aac54b623c45631b1e161d72b2 100644 --- a/apps/server/src/orchestration/Layers/ProjectionPipeline.ts +++ b/apps/server/src/orchestration/Layers/ProjectionPipeline.ts @@ -37,7 +37,12 @@ import { OrchestrationProjectionPipeline, type OrchestrationProjectionPipelineShape, } from "../Services/ProjectionPipeline.ts"; -import { attachmentRelativePath, toSafeThreadAttachmentSegment } from "../../attachmentStore.ts"; +import { + attachmentRelativePath, + parseAttachmentIdFromRelativePath, + parseThreadSegmentFromAttachmentId, + toSafeThreadAttachmentSegment, +} from "../../attachmentStore.ts"; export const ORCHESTRATION_PROJECTOR_NAMES = { projects: "projection.projects", @@ -198,7 +203,8 @@ function collectThreadAttachmentRelativePaths( if (attachment.type !== "image") { continue; } - if (!attachment.id.startsWith(`${threadSegment}-`)) { + const attachmentThreadSegment = parseThreadSegmentFromAttachmentId(attachment.id); + if (!attachmentThreadSegment || attachmentThreadSegment !== threadSegment) { continue; } relativePaths.add(attachmentRelativePath(attachment)); @@ -236,7 +242,12 @@ const runAttachmentSideEffects = Effect.fn(function* (sideEffects: AttachmentSid if (normalizedEntry.length === 0 || normalizedEntry.includes("/")) { return; } - if (!normalizedEntry.startsWith(`${threadSegment}-`)) { + const attachmentId = parseAttachmentIdFromRelativePath(normalizedEntry); + if (!attachmentId) { + return; + } + const attachmentThreadSegment = parseThreadSegmentFromAttachmentId(attachmentId); + if (!attachmentThreadSegment || attachmentThreadSegment !== threadSegment) { return; } yield* fileSystem.remove(path.join(attachmentsRootDir, normalizedEntry), { @@ -272,7 +283,12 @@ const runAttachmentSideEffects = Effect.fn(function* (sideEffects: AttachmentSid if (relativePath.length === 0 || relativePath.includes("/")) { return; } - if (!relativePath.startsWith(`${threadSegment}-`)) { + const attachmentId = parseAttachmentIdFromRelativePath(relativePath); + if (!attachmentId) { + return; + } + const attachmentThreadSegment = parseThreadSegmentFromAttachmentId(attachmentId); + if (!attachmentThreadSegment || attachmentThreadSegment !== threadSegment) { return; } -- 2.54.0