From bf9b561cc731008e2ef51e0cdc4f24ac33383376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Sat, 11 Apr 2026 15:38:05 +0800 Subject: [PATCH] fix(adapters): eliminate AttachmentStore collision race and orphan .part cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two production issues in the AttachmentStore: 1. TOCTOU race in resolvePath() — concurrent downloads with same filename in the same millisecond could collide. Fixed by appending a 6-char random suffix to the timestamp-based path, ensuring millisecond-level collisions never occur. 2. Orphan .part files from crashed writes never cleaned up. Fixed by extending gc() to recognize .part files and clean them with a shorter grace period (10 minutes by default) rather than the normal retention window. Added two test cases covering heavy collision pressure and .part cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../__tests__/attachment-store.test.ts | 32 +++++++++++++++++++ .../common/attachment/attachment-store.ts | 16 ++++++++-- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/adapters/common/attachment/__tests__/attachment-store.test.ts b/adapters/common/attachment/__tests__/attachment-store.test.ts index 6f59c578..9b54c6b6 100644 --- a/adapters/common/attachment/__tests__/attachment-store.test.ts +++ b/adapters/common/attachment/__tests__/attachment-store.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach } from 'bun:test' import * as fs from 'node:fs/promises' +import * as fsSync from 'node:fs' import * as path from 'node:path' import * as os from 'node:os' import { AttachmentStore } from '../attachment-store.js' @@ -73,4 +74,35 @@ describe('AttachmentStore', () => { expect(result.removed).toBe(0) await fs.access(target) }) + + it('resolvePath under heavy collision pressure returns unique paths', () => { + const store = new AttachmentStore({ root: tmpRoot, retentionMs: 60_000 }) + // First create a file so subsequent resolves hit the collision branch + const a = store.resolvePath('feishu', 'sess-1', 'race.png') + fsSync.writeFileSync(a, 'first') + // Collect 50 resolved paths in a tight loop — none should clash + const seen = new Set() + for (let i = 0; i < 50; i++) { + seen.add(store.resolvePath('feishu', 'sess-1', 'race.png')) + } + expect(seen.size).toBe(50) + for (const p of seen) { + expect(p).not.toBe(a) + } + }) + + it('gc() cleans orphan .part files after a short grace period', async () => { + const store = new AttachmentStore({ root: tmpRoot, retentionMs: 10_000, orphanGraceMs: 50 }) + // Simulate a crashed write — leave a .part tmp file behind + const dir = path.join(tmpRoot, 'feishu', 'sess-1') + await fs.mkdir(dir, { recursive: true }) + const orphan = path.join(dir, 'image.png.1234.5678.part') + await fs.writeFile(orphan, 'ORPHAN') + // Age the orphan so gc considers it stale + const past = new Date(Date.now() - 1000) + await fs.utimes(orphan, past, past) + const result = await store.gc() + expect(result.removed).toBeGreaterThanOrEqual(1) + await expect(fs.access(orphan)).rejects.toThrow() + }) }) diff --git a/adapters/common/attachment/attachment-store.ts b/adapters/common/attachment/attachment-store.ts index 12937b08..75cdd7c6 100644 --- a/adapters/common/attachment/attachment-store.ts +++ b/adapters/common/attachment/attachment-store.ts @@ -19,9 +19,13 @@ import type { ImPlatform } from './attachment-types.js' export interface AttachmentStoreConfig { root: string retentionMs: number + /** Grace window before a `.part` orphan (left behind by a crashed writer) + * is eligible for GC. Default 10 minutes. */ + orphanGraceMs: number } const DEFAULT_RETENTION_MS = 24 * 60 * 60 * 1000 +const DEFAULT_ORPHAN_GRACE_MS = 10 * 60 * 1000 function defaultRoot(): string { return path.join(os.homedir(), '.claude', 'im-downloads') @@ -38,10 +42,12 @@ function sanitizeFilename(name: string): string { export class AttachmentStore { private readonly root: string private readonly retentionMs: number + private readonly orphanGraceMs: number constructor(config?: Partial) { this.root = config?.root ?? defaultRoot() this.retentionMs = config?.retentionMs ?? DEFAULT_RETENTION_MS + this.orphanGraceMs = config?.orphanGraceMs ?? DEFAULT_ORPHAN_GRACE_MS } /** Compute the target path. Creates parent dirs on demand. @@ -55,7 +61,11 @@ export class AttachmentStore { const candidate = path.join(dir, safeName) if (!fsSync.existsSync(candidate)) return candidate const { name: base, ext } = path.parse(safeName) - return path.join(dir, `${base}-${Date.now()}${ext}`) + // Collisions are rare in practice, but multiple downloads landing in the + // same millisecond must still produce unique paths — append a random + // suffix so the bare timestamp alone never clashes. + const rand = Math.random().toString(36).slice(2, 8) + return path.join(dir, `${base}-${Date.now()}-${rand}${ext}`) } /** Write atomically: stream to {target}.part, then rename. */ @@ -88,7 +98,9 @@ export class AttachmentStore { try { const stat = await fs.stat(full) const age = now - stat.mtimeMs - if (age > this.retentionMs) { + const isOrphanPart = entry.name.endsWith('.part') + const threshold = isOrphanPart ? this.orphanGraceMs : this.retentionMs + if (age > threshold) { bytes += stat.size await fs.unlink(full) removed++