Fix scratchpad asset sweep data loss and hardening from code review

Addresses correctness findings in the file-backed scratchpad assets work:

- Sweep race / clear resurrection (data loss): the orphan sweep keyed its
  grace window on file mtime, so a long-referenced image got zero grace the
  instant it became unreferenced (e.g. `clear` racing an open tab's pending
  autosave), letting the sweep permanently delete a file the surviving
  snapshot still pointed at. Anchor the grace to when a file is FIRST observed
  unreferenced instead, tracked in-memory. This also subsumes the dedup
  mtime-not-refreshed case (mtime no longer factors into the decision).

- `delete`/re-add leaked asset files: `delete` removed only the shape and its
  bindings, leaving the asset record to pin its file against the sweep forever
  (asymmetric with the `clear` fix). Drop the asset record when the deleted or
  replaced shape was its last user (shared assets are preserved).

- Atomic asset writes: write via a temp sibling + rename so a crash or full
  disk mid-write can't leave a truncated file at the content-addressed path
  that the exists() skip would then serve forever.

- Stored-XSS hardening: serve assets with Content-Disposition: attachment and
  X-Content-Type-Options: nosniff so a pasted/uploaded SVG can't execute as
  script in the app origin on direct navigation (img/video/fetch ignore these).

- assetStore held in a ref, not useMemo (which React may discard): a fresh
  `assets` identity makes <Tldraw> rebuild its store and remount the editor,
  dropping unsaved edits and resetting the camera.

- Flag `missing` in `read` for shapes whose asset RECORD is absent, not only
  when the file is gone, so the agent isn't handed a healthy-looking image.

- Sync the CLI's read-image extension map with the server (avif/apng/video),
  so those assets save with a real extension instead of `.bin`.

- Derive ASSET_FILE_RE and the .bak scanner from one shared pattern so they
  can't drift and delete backup-referenced files; add image/apng to MIME_EXT.

Tests updated for the new grace semantics; new regression tests cover the
clear-race data loss and the delete reclaim path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HCbMMxrNwyPAtJP5qryT6W

Author: claude

client/components/Scratchpad.tsx

@@ -1,12 +1,4 @@
-import {
-  Fragment,
-  type ReactElement,
-  useCallback,
-  useEffect,
-  useMemo,
-  useRef,
-  useState
-} from 'react'
+import { Fragment, type ReactElement, useCallback, useEffect, useRef, useState } from 'react'
 
 import {
   type Editor,
@@ -785,8 +777,14 @@ export function Scratchpad() {
   // Reactive handle to the mounted editor, used to render the custom tool bar.
   const [editor, setEditor] = useState<Editor | null>(null)
   const { loaded, snapshot, skew, flagSkew } = useScratchpadSnapshot(workspaceId)
-  // Stable per workspace — a new identity would remount tldraw's asset handling.
-  const assetStore = useMemo(() => makeAssetStore(workspaceId), [workspaceId])
+  // Stable identity per workspace, held in a ref rather than useMemo (which React
+  // may discard): a fresh `assets` identity makes <Tldraw> rebuild its store and
+  // remount the editor, dropping unsaved edits and resetting the camera.
+  const assetStoreRef = useRef<{ id: string; store: TLAssetStore } | null>(null)
+  if (assetStoreRef.current?.id !== workspaceId) {
+    assetStoreRef.current = { id: workspaceId, store: makeAssetStore(workspaceId) }
+  }
+  const assetStore = assetStoreRef.current.store
 
   const save = useCallback(() => {
     const editor = editorRef.current

server/api.ts

@@ -417,7 +417,12 @@ one.get('/scratchpad/assets/:file', async c => {
   return new Response(resolved.file, {
     headers: {
       'Content-Type': resolved.mimeType,
-      'Cache-Control': 'public, max-age=31536000, immutable'
+      'Cache-Control': 'public, max-age=31536000, immutable',
+      // Assets are only ever consumed via <img>/<video>/fetch (which all ignore
+      // this), so force a download on direct navigation and block MIME sniffing:
+      // a pasted/uploaded SVG must not execute as script in the app origin.
+      'Content-Disposition': 'attachment',
+      'X-Content-Type-Options': 'nosniff'
     }
   })
 })

server/cli.ts

@@ -1310,14 +1310,21 @@ const scratchView = defineCommand({
   }
 })
 
-// Image data URL mime → file extension, for naming the saved file.
+// Image/video data URL mime → file extension, for naming the saved file. Kept in
+// sync with the server's MIME_EXT (scratchpad-assets.ts) so avif/apng/video assets
+// round-trip through `read-image` with a real extension instead of `.bin`.
 const IMAGE_EXT: Record<string, string> = {
   'image/png': 'png',
   'image/jpeg': 'jpg',
   'image/jpg': 'jpg',
   'image/webp': 'webp',
   'image/gif': 'gif',
-  'image/svg+xml': 'svg'
+  'image/svg+xml': 'svg',
+  'image/avif': 'avif',
+  'image/apng': 'apng',
+  'video/mp4': 'mp4',
+  'video/webm': 'webm',
+  'video/quicktime': 'mov'
 }
 
 const scratchReadImage = defineCommand({

server/scratchpad-assets.ts

@@ -1,4 +1,4 @@
-import { readdir, stat, unlink } from 'node:fs/promises'
+import { readdir, rename, stat, unlink } from 'node:fs/promises'
 import { join } from 'node:path'
 
 import type { ScratchpadDoc } from './scratchpad'
@@ -30,8 +30,11 @@ const ASSET_SRC_PREFIX = 'asset:'
 // The only file names we ever create or serve: `asset-<sha256 hex>.<short ext>`.
 // The `asset-` prefix marks the files as canvas assets, keeping the sidecar dir
 // unambiguous if it ever holds anything else. Anything else (traversal attempts
-// included) is rejected at the boundary.
-const ASSET_FILE_RE = /^asset-[0-9a-f]{64}\.[a-z0-9]{1,8}$/
+// included) is rejected at the boundary. The core pattern is shared with the
+// `.bak` scanner below (BAK_SRC_RE) so the two can never drift — a drift would
+// let the sweep delete files the schema-change backup still references.
+const ASSET_NAME_PATTERN = 'asset-[0-9a-f]{64}\\.[a-z0-9]{1,8}'
+const ASSET_FILE_RE = new RegExp(`^${ASSET_NAME_PATTERN}$`)
 
 const MIME_EXT: Record<string, string> = {
   'image/png': 'png',
@@ -40,6 +43,7 @@ const MIME_EXT: Record<string, string> = {
   'image/gif': 'gif',
   'image/svg+xml': 'svg',
   'image/avif': 'avif',
+  'image/apng': 'apng',
   'video/mp4': 'mp4',
   'video/webm': 'webm',
   'video/quicktime': 'mov'
@@ -82,8 +86,17 @@ export async function storeScratchpadAsset(
   hasher.update(bytes)
   const ext = MIME_EXT[mimeType.split(';')[0].trim().toLowerCase()] ?? 'bin'
   const fileName = `asset-${hasher.digest('hex')}.${ext}`
-  const path = join(getScratchpadAssetsDir(workspacePath), fileName)
-  if (!(await Bun.file(path).exists())) await Bun.write(path, bytes)
+  const dir = getScratchpadAssetsDir(workspacePath)
+  const path = join(dir, fileName)
+  // Content-addressed, so an existing file already holds these exact bytes.
+  // Write through a temp sibling + atomic rename: a crash or full disk mid-write
+  // must never leave a truncated file at `path`, whose name would then claim a
+  // hash its bytes don't match and be served forever by the exists() skip above.
+  if (!(await Bun.file(path).exists())) {
+    const tmp = join(dir, `.tmp-${crypto.randomUUID()}`)
+    await Bun.write(tmp, bytes)
+    await rename(tmp, path)
+  }
   return { src: `${ASSET_SRC_PREFIX}${fileName}` }
 }
 
@@ -124,20 +137,30 @@ export async function extractInlineAssets(
   return rewritten ? { ...document, store: rewritten } : document
 }
 
-// Files uploaded but not yet referenced by a saved snapshot: the browser's
-// TLAssetStore uploads first, and the asset record only lands on disk with the
-// next autosave (~500ms-1s later). The grace keeps the sweep from eating that
-// window; anything unreferenced *and* old is a genuine orphan (deleted image,
-// cleared canvas, abandoned upload).
+// A file is reclaimed only after it has stayed unreferenced for this long. The
+// clock starts when the sweep FIRST observes the file unreferenced — NOT when
+// the file was written — so every reference drop gets the same recovery window a
+// fresh upload gets. (Keying on file mtime instead gave a long-referenced image
+// zero grace the instant it went unreferenced — e.g. `clear` racing an open
+// tab's pending autosave — letting the sweep permanently delete a file the
+// surviving snapshot still pointed at.) The window covers the upload→autosave
+// gap and a stale tab that re-PUTs an old, still-referencing document alike.
 const SWEEP_GRACE_MS = 5 * 60_000
 
+// When each still-unreferenced asset file was first seen unreferenced, keyed by
+// absolute path. In-memory and per-process: a restart just restarts the clock
+// (delaying cleanup, never losing data). Entries drop as soon as a file is
+// referenced again or unlinked, so this only ever holds currently-unreferenced
+// files within their grace window.
+const firstUnreferencedAt = new Map<string, number>()
+
 // Asset file names referenced by the schema-change backup (`.scratchpad.json.bak`
 // — see backupOnSchemaChange in scratchpad.ts). The sweep must not eat those:
 // the .bak is the manual escape hatch after a downgrade, and restoring it with
 // its images gone would defeat the point. A raw regex scan (no JSON parse — the
 // .bak may be a huge legacy file), cached by mtime since the file only changes
-// on a schema upgrade. The pattern is `asset:` + ASSET_FILE_RE.
-const BAK_SRC_RE = /asset:(asset-[0-9a-f]{64}\.[a-z0-9]{1,8})/g
+// on a schema upgrade. Built from the same ASSET_NAME_PATTERN as ASSET_FILE_RE.
+const BAK_SRC_RE = new RegExp(`asset:(${ASSET_NAME_PATTERN})`, 'g')
 const bakRefsCache = new Map<string, { mtimeMs: number; refs: Set<string> }>()
 async function bakReferencedAssets(bakFile: string): Promise<Set<string>> {
   try {
@@ -159,7 +182,8 @@ async function bakReferencedAssets(bakFile: string): Promise<Set<string>> {
 export async function sweepOrphanAssets(
   workspacePath: string,
   document: ScratchpadDoc,
-  bakFile?: string
+  bakFile?: string,
+  now: number = Date.now()
 ): Promise<void> {
   try {
     const dir = getScratchpadAssetsDir(workspacePath)
@@ -176,14 +200,24 @@ export async function sweepOrphanAssets(
     }
     if (bakFile) for (const name of await bakReferencedAssets(bakFile)) referenced.add(name)
 
-    const now = Date.now()
     for (const name of files) {
-      if (!ASSET_FILE_RE.test(name) || referenced.has(name)) continue
+      if (!ASSET_FILE_RE.test(name)) continue
       const path = join(dir, name)
-      try {
-        const { mtimeMs } = await stat(path)
-        if (now - mtimeMs > SWEEP_GRACE_MS) await unlink(path)
-      } catch {}
+      if (referenced.has(name)) {
+        firstUnreferencedAt.delete(path) // referenced (again) — reset its clock
+        continue
+      }
+      const since = firstUnreferencedAt.get(path)
+      if (since === undefined) {
+        firstUnreferencedAt.set(path, now) // first seen unreferenced — start clock
+        continue
+      }
+      if (now - since > SWEEP_GRACE_MS) {
+        try {
+          await unlink(path)
+          firstUnreferencedAt.delete(path)
+        } catch {}
+      }
     }
   } catch {}
 }

server/scratchpad-executor.ts

@@ -214,8 +214,14 @@ async function applyAddImage(
   op: Extract<ScratchOp, { kind: 'add-image' }>
 ): Promise<ScratchOpResult> {
   const pageId = firstPageId(store)
+  // Re-adding under an existing id replaces that shape; note its current asset so
+  // we can drop it below once nothing else references it (else its file leaks).
+  const prev = store.get(createShapeId(op.name)) as unknown as
+    | { props?: { assetId?: unknown } }
+    | undefined
+  const prevAssetId = typeof prev?.props?.assetId === 'string' ? prev.props.assetId : undefined
   const { data, w, h, mimeType, name } = await processCanvasImage(op.path, op.quality ?? 'lo')
-  const { src } = await storeScratchpadAsset(workspacePath, new Uint8Array(data), mimeType)
+  const { src } = await storeScratchpadAsset(workspacePath, data, mimeType)
   const assetId = AssetRecordType.createId()
   store.put([
     {
@@ -237,6 +243,11 @@ async function applyAddImage(
       props: { ...defaultProps('image'), w, h, assetId }
     })
   ])
+  // The replaced shape's old asset is now unreachable — drop it so the sweep can
+  // reclaim its file.
+  if (prevAssetId && prevAssetId !== assetId && !anyShapeUsesAsset(store, prevAssetId)) {
+    store.remove([prevAssetId as unknown as TLRecord['id']])
+  }
   return { name: op.name }
 }
 
@@ -346,10 +357,17 @@ function applyOp(store: TLStore, op: ScratchOp): ScratchOpResult {
     }
     case 'delete': {
       const id = createShapeId(op.name)
+      const shape = store.get(id) as unknown as { props?: { assetId?: unknown } } | undefined
+      const assetId = typeof shape?.props?.assetId === 'string' ? shape.props.assetId : undefined
       // Remove the shape plus any binding that references it, or the leftover
       // binding would dangle and invalidate the snapshot.
       const ids = [id, ...bindingsTouching(store, op.name)]
       store.remove(ids)
+      // If that was the last shape using the image's asset, drop the asset record
+      // too so the post-save sweep can reclaim its file (mirrors `clear`).
+      if (assetId && !anyShapeUsesAsset(store, assetId)) {
+        store.remove([assetId as unknown as TLRecord['id']])
+      }
       return { ok: true }
     }
     case 'clear': {
@@ -369,6 +387,18 @@ function applyOp(store: TLStore, op: ScratchOp): ScratchOpResult {
   }
 }
 
+// True if any shape still references the given asset id. tldraw lets several
+// shapes share one asset (e.g. a duplicated image), so delete/replace only drops
+// the asset record when its last user is gone — leaving the file for the sweep.
+function anyShapeUsesAsset(store: TLStore, assetId: string): boolean {
+  for (const record of store.allRecords()) {
+    if (record.typeName !== 'shape') continue
+    const props = (record as unknown as { props?: { assetId?: unknown } }).props
+    if (props?.assetId === assetId) return true
+  }
+  return false
+}
+
 // Ids of bindings whose start/end is the named shape.
 function bindingsTouching(store: TLStore, name: string): TLRecord['id'][] {
   const target = createShapeId(name)

server/scratchpad.ts

@@ -47,9 +47,10 @@ export type ScratchShape = {
   // as-is. A legacy inline base64 blob is omitted (see omitBase64) — the agent
   // calls `moi scratch read-image`/`view` to actually see pixels.
   src?: string
-  // True when `src` is an `asset:` reference whose backing file is gone from
-  // .moi/.scratchpad (sidecar dir lost or pruned) — `read-image` will error and
-  // the canvas shows a broken image. Absent otherwise.
+  // True when the shape references an asset that can't be resolved to pixels: an
+  // `asset:` file gone from .moi/.scratchpad (sidecar dir lost or pruned), or the
+  // asset record absent entirely — `read-image` will error and the canvas shows a
+  // broken image. Absent otherwise.
   missing?: true
 }
 
@@ -236,6 +237,11 @@ export async function readScratchpadShapes(workspacePath: string): Promise<Scrat
     const h = typeof r.props?.h === 'number' ? r.props.h : undefined
     const assetId = typeof r.props?.assetId === 'string' ? r.props.assetId : undefined
     const rawSrc = assetId !== undefined ? assetSrc.get(assetId) : undefined
+    // Flag `missing` when a shape references an asset we can't resolve to pixels:
+    // its backing file is gone (assetGone), or the asset record is absent / has a
+    // non-string src (rawSrc undefined). Either way `read-image` can't return
+    // bytes, so warn from `read` rather than let it fail later.
+    const missing = assetId !== undefined && (assetGone.has(assetId) || rawSrc === undefined)
     shapes.push({
       id: (r.id ?? '').replace(/^shape:/, ''),
       type: r.type ?? 'unknown',
@@ -248,7 +254,7 @@ export async function readScratchpadShapes(workspacePath: string): Promise<Scrat
         return text !== undefined ? { text: omitBase64(text) } : {}
       })(),
       ...(rawSrc !== undefined ? { src: omitBase64(rawSrc) } : {}),
-      ...(assetId !== undefined && assetGone.has(assetId) ? { missing: true as const } : {})
+      ...(missing ? { missing: true as const } : {})
     })
   }
   return shapes

server/test/scratchpad-assets.test.ts

@@ -1,6 +1,6 @@
 import { afterEach, beforeEach, describe, expect, test } from 'bun:test'
 import { mkdtempSync, rmSync } from 'node:fs'
-import { readdir, utimes } from 'node:fs/promises'
+import { readdir } from 'node:fs/promises'
 import { join } from 'path'
 
 import {
@@ -161,34 +161,62 @@ describe('missing asset files (dangling references)', () => {
 })
 
 describe('sweepOrphanAssets', () => {
-  test('deletes old unreferenced files, keeps referenced and recent ones', async () => {
+  test('reclaims a file only after it has stayed unreferenced past the grace window', async () => {
     const { src } = await storeScratchpadAsset(WS, PNG_BYTES, 'image/png')
     const referenced = assetSrcFileName(src)!
     const { src: orphanSrc } = await storeScratchpadAsset(
       WS,
       new Uint8Array([1, 2, 3]),
       'image/png'
     )
-    const oldOrphan = assetSrcFileName(orphanSrc)!
-    const { src: freshSrc } = await storeScratchpadAsset(WS, new Uint8Array([4, 5, 6]), 'image/png')
-    const freshOrphan = assetSrcFileName(freshSrc)!
-
-    // Age everything past the grace window, then re-touch the fresh orphan.
+    const orphan = assetSrcFileName(orphanSrc)!
     const dir = getScratchpadAssetsDir(WS)
-    const past = new Date(Date.now() - 60 * 60_000)
-    for (const name of [referenced, oldOrphan]) await utimes(join(dir, name), past, past)
-
     const doc: ScratchpadDoc = {
       store: {
         'asset:a1': { id: 'asset:a1', typeName: 'asset', type: 'image', props: { src } }
       }
     }
-    await sweepOrphanAssets(WS, doc)
 
+    const t0 = 1_000_000
+    // First sweep only starts the orphan's clock; nothing is deleted yet.
+    await sweepOrphanAssets(WS, doc, undefined, t0)
+    expect(await readdir(dir)).toContain(orphan)
+
+    // Still within the grace window — kept.
+    await sweepOrphanAssets(WS, doc, undefined, t0 + 60_000)
+    expect(await readdir(dir)).toContain(orphan)
+
+    // Past the window, the still-unreferenced orphan is reclaimed; the referenced
+    // file was never on the clock and survives.
+    await sweepOrphanAssets(WS, doc, undefined, t0 + 6 * 60_000)
     const left = await readdir(dir)
     expect(left).toContain(referenced)
-    expect(left).toContain(freshOrphan)
-    expect(left).not.toContain(oldOrphan)
+    expect(left).not.toContain(orphan)
+  })
+
+  test('a long-referenced file gets a fresh grace window when it becomes unreferenced', async () => {
+    const { src } = await storeScratchpadAsset(WS, PNG_BYTES, 'image/png')
+    const name = assetSrcFileName(src)!
+    const dir = getScratchpadAssetsDir(WS)
+    const withImg: ScratchpadDoc = {
+      store: { 'asset:a1': { id: 'asset:a1', typeName: 'asset', type: 'image', props: { src } } }
+    }
+    const empty: ScratchpadDoc = { store: {} }
+
+    const t0 = 5_000_000
+    // Referenced across sweeps spanning an hour.
+    await sweepOrphanAssets(WS, withImg, undefined, t0)
+    await sweepOrphanAssets(WS, withImg, undefined, t0 + 60 * 60_000)
+
+    // It becomes unreferenced (e.g. `clear`). Despite the file being an hour old,
+    // the next sweep must NOT delete it — the clock starts here, not at write time.
+    // This is the sweep-race data-loss regression the grace anchoring fixes.
+    await sweepOrphanAssets(WS, empty, undefined, t0 + 60 * 60_000)
+    expect(await readdir(dir)).toContain(name)
+
+    // Only after the grace elapses from the un-reference moment is it reclaimed.
+    await sweepOrphanAssets(WS, empty, undefined, t0 + 60 * 60_000 + 6 * 60_000)
+    expect(await readdir(dir)).not.toContain(name)
   })
 
   test('keeps files the .bak backup still references', async () => {
@@ -209,14 +237,13 @@ describe('sweepOrphanAssets', () => {
     )
 
     const dir = getScratchpadAssetsDir(WS)
-    const past = new Date(Date.now() - 60 * 60_000)
-    for (const name of [bakKept, orphan]) await utimes(join(dir, name), past, past)
-
-    // Current document references neither file.
-    await sweepOrphanAssets(WS, { store: {} }, bakFile)
+    // Current document references neither file; run twice past the grace window.
+    const t0 = 2_000_000
+    await sweepOrphanAssets(WS, { store: {} }, bakFile, t0)
+    await sweepOrphanAssets(WS, { store: {} }, bakFile, t0 + 6 * 60_000)
 
     const left = await readdir(dir)
-    expect(left).toContain(bakKept)
+    expect(left).toContain(bakKept) // pinned by the .bak, never on the clock
     expect(left).not.toContain(orphan)
   })