improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path (#6149)

* improvement(files): cache stream binding meta, tighten file cache-control, prune dead persist path - Cache the agent-stream ProseMirror↔Yjs binding metadata per session and reuse it across streamed frames instead of rebuilding it (O(doc)) every frame; matches how y-tiptap's own binding maintains the mapping in place. Safe because the shadow doc only ever sees the agent's own reconciles. - Default createFileResponse to a private, no-cache policy so auth-gated bytes are never stored in a shared cache; genuinely-public serve routes opt into public caching explicitly. - Serve content-addressed (key=) embedded images with an immutable private cache to avoid re-downloading them on every doc re-open; fileId= embeds and public shares keep revalidating (the underlying key can change / a share can be revoked). - Drop the dead conflict.version field from the collab-doc persist result and remove the wasted getWorkspaceFile re-read on the conflict path (the relay treats missing and conflict identically and never read version). - Decorate-sort the files/folders lists (compute each sort key once) and index the move-menu subtree build (O(N) vs O(N^2)); ordering is preserved exactly. * fix(files): serve inline images with no-cache so deletion/authorization is enforced per request Embedded images are authenticated content whose backing file can be deleted or have its access revoked at any time. Both key= and fileId= embeds serve private, no-cache, must-revalidate, so every request re-runs the server-side deletion/authorization check instead of serving a possibly-stale image from cache.
2026-09-24 15:45:35 +08:00 · 2026-07-31 20:09:45 -07:00
parent 10bfb5d139
commit 06506bbd38
13 changed files with 166 additions and 85 deletions
@@ -77,13 +77,13 @@ export async function fetchFileDocMerge(
 * Result of a persist attempt (mirrors the app's `persistFileDoc` contract):
 * - `persisted` — written; `version` is the new durable version the relay records as synced.
 * - `missing` — the file is gone.
- * - `conflict` — the file changed out-of-band since `expectedVersion`; NOT written. `version` is the
- *   current durable version the relay adopts as its new If-Match to re-persist the current live stream.
+ * - `conflict` — the file changed out-of-band since `expectedVersion`; NOT written. The relay leaves the
+ *   durable content authoritative and reads nothing off this result beyond the status.
 */
 export type PersistResult =
  | { status: 'persisted'; version: number }
  | { status: 'missing' }
-  | { status: 'conflict'; version: number }
+  | { status: 'conflict' }
  | { status: 'deferred' }

 /**
@@ -338,7 +338,6 @@ describe('setupWorkspaceFileDocHandlers', () => {
    // authoritative; a later flush projects the converged stream once the merge lands.
    mockFetchFileDocPersist.mockResolvedValue({
      status: 'conflict',
-      version: 999,
    })
    const { io } = createIo()
    const { handlers } = setup('socket-1', io)
@@ -8,8 +8,10 @@ import { createFileResponse, FileNotFoundError } from '@/app/api/files/utils'
 const logger = createLogger('InlineImageServe')

 /**
- * A shared/edited/deleted file must never serve stale bytes from its fixed inline URL, so every inline
- * image revalidates on each request.
+ * An embedded image is authenticated content served from a fixed inline URL, and the file behind it can
+ * be DELETED or its access REVOKED at any time — so it always revalidates, letting each request re-run the
+ * server-side deletion/authorization check rather than serving a stale (possibly no-longer-authorized)
+ * image from cache. Private so no shared cache/CDN ever stores it.
 */
 const INLINE_CACHE_CONTROL = 'private, no-cache, must-revalidate'

@@ -60,6 +60,9 @@ function getWorkspaceIdForCompile(key: string): string | undefined {

 const IMMUTABLE_CACHE_CONTROL = 'private, max-age=31536000, immutable'
 const WORKSPACE_REVALIDATE_CACHE_CONTROL = 'private, no-cache, must-revalidate'
+/** For the genuinely-public, pre-auth asset routes (avatars, OG images, workspace logos) — these are
+ *  intentionally shared-cacheable. Passed EXPLICITLY so the default response cache stays `private`. */
+const PUBLIC_ASSET_CACHE_CONTROL = 'public, max-age=31536000'

 /**
 * Cache-Control for a served file. A versioned request (`?v=<updatedAt>`) addresses
@@ -314,6 +317,7 @@ async function handleCloudProxyPublic(
      buffer: fileBuffer,
      contentType,
      filename,
+      cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
    })
  } catch (error) {
    logger.error('Error serving public cloud file:', error)
@@ -338,6 +342,7 @@ async function handleLocalFilePublic(filename: string): Promise<NextResponse> {
      buffer: fileBuffer,
      contentType,
      filename,
+      cacheControl: PUBLIC_ASSET_CACHE_CONTROL,
    })
  } catch (error) {
    logger.error('Error reading public local file:', error)
@@ -173,6 +173,26 @@ describe('extractFilename', () => {
        expect(response.headers.get('Content-Security-Policy')).toBeNull()
      })

+      it('defaults to a PRIVATE cache so access-verified content is never shared-cached', () => {
+        const response = createFileResponse({
+          buffer: Buffer.from('fake-image-data'),
+          contentType: 'image/png',
+          filename: 'safe-image.png',
+        })
+        // No explicit cacheControl → must NOT be `public` (a shared cache/CDN could re-serve authed bytes).
+        expect(response.headers.get('Cache-Control')).toBe('private, no-cache')
+      })
+
+      it('honors an explicit cacheControl (e.g. public assets opt in)', () => {
+        const response = createFileResponse({
+          buffer: Buffer.from('fake-image-data'),
+          contentType: 'image/png',
+          filename: 'avatar.png',
+          cacheControl: 'public, max-age=31536000',
+        })
+        expect(response.headers.get('Cache-Control')).toBe('public, max-age=31536000')
+      })
+
      it('should serve PDFs inline safely', () => {
        const response = createFileResponse({
          buffer: Buffer.from('fake-pdf-data'),
@@ -211,7 +211,10 @@ export function createFileResponse(file: FileResponse): NextResponse {
  const headers: Record<string, string> = {
    'Content-Type': contentType,
    'Content-Disposition': `${disposition}; ${encodeFilenameForHeader(file.filename)}`,
-    'Cache-Control': file.cacheControl || 'public, max-age=31536000',
+    // Default to PRIVATE: this response is served only after access verification, so it must never be
+    // stored by a shared cache/CDN and re-served cross-user. Genuinely public assets (avatars, OG images,
+    // workspace logos) pass an explicit `cacheControl` (see PUBLIC_ASSET_CACHE_CONTROL in the serve route).
+    'Cache-Control': file.cacheControl || 'private, no-cache',
    'X-Content-Type-Options': 'nosniff',
  }

@@ -38,15 +38,20 @@ describe('GET /api/workspaces/[id]/files/inline', () => {
    mockDownloadFile.mockResolvedValue(PNG)
  })

-  it('serves a workspace-scoped image by fileId', async () => {
+  it('serves a workspace-scoped image by fileId, always revalidating', async () => {
    const res = await GET(req('fileId=wf_abc'), params)
    expect(res.status).toBe(200)
    expect(mockResolveImage).toHaveBeenCalledWith('ws-1', { fileId: 'wf_abc' })
+    // Authenticated content: always revalidate so a deletion/revocation is enforced on the next request.
+    expect(res.headers.get('Cache-Control')).toBe('private, no-cache, must-revalidate')
  })

-  it('serves a workspace-scoped image by key', async () => {
+  it('serves a workspace-scoped image by key, always revalidating', async () => {
    const res = await GET(req(`key=${encodeURIComponent('workspace/ws-1/x-photo.png')}`), params)
    expect(res.status).toBe(200)
+    // Same policy as fileId: authenticated content never cached past a revalidation, so a deleted or
+    // access-revoked image drops out immediately rather than lingering in a private browser cache.
+    expect(res.headers.get('Cache-Control')).toBe('private, no-cache, must-revalidate')
  })

  it('404s when the reference does not resolve in the workspace (cross-workspace)', async () => {
@@ -7,7 +7,12 @@ import { afterEach, beforeAll, describe, expect, it } from 'vitest'
 import { Awareness } from 'y-protocols/awareness'
 import * as Y from 'yjs'
 import { createMarkdownEditorExtensions } from '../editor-extensions'
-import { applyAgentStreamFrame, beginAgentStream, endAgentStream } from './apply-streamed-markdown'
+import {
+  AGENT_STREAM_ORIGIN,
+  applyAgentStreamFrame,
+  beginAgentStream,
+  endAgentStream,
+} from './apply-streamed-markdown'

 beforeAll(() => {
  // jsdom does not implement elementFromPoint; the Placeholder extension's viewport tracking calls it
@@ -186,4 +191,35 @@ describe('agent-stream applier', () => {
    expect(live).toContain('EDITED')
    expect(live).toContain('Gamma paragraph')
  })
+
+  it('reuses cached binding metadata across frames, still emitting minimal per-frame deltas', () => {
+    // The binding `meta` is built ONCE (first frame) and reused — `updateYFragment` maintains it in place,
+    // so we skip an O(doc) `initProseMirrorDoc` rebuild per frame. This guards that caching preserves the
+    // minimal-delta + no-duplication behavior across frames (a stale/rebuilt mapping would re-emit existing
+    // paragraphs → duplication).
+    const { editor, doc } = track(makeCollabEditor())
+    const session = beginAgentStream(editor)!
+
+    applyAgentStreamFrame(editor, session, 'One.')
+    expect(session.meta).not.toBeNull() // built and cached on the first frame
+
+    const deltas: Uint8Array[] = []
+    const onUpdate = (u: Uint8Array, origin: unknown) => {
+      if (origin === AGENT_STREAM_ORIGIN) deltas.push(u)
+    }
+    doc.on('update', onUpdate)
+    applyAgentStreamFrame(editor, session, 'One.\n\nTwo.')
+    applyAgentStreamFrame(editor, session, 'One.\n\nTwo.\n\nThree.')
+    doc.off('update', onUpdate)
+
+    // Two later frames → two incremental deltas; the cached mapping kept diffs minimal and correct.
+    expect(deltas.length).toBe(2)
+    const text = editor.getText()
+    expect(text).toContain('One')
+    expect(text).toContain('Two')
+    expect(text).toContain('Three')
+    expect(text.match(/One/g)?.length).toBe(1)
+    expect(text.match(/Two/g)?.length).toBe(1)
+    endAgentStream(session)
+  })
 })
@@ -23,6 +23,17 @@ export const AGENT_STREAM_ORIGIN = Symbol('agent-stream')
 export interface AgentStreamSession {
  shadow: Y.Doc
  fragment: Y.XmlFragment
+  /**
+   * The fragment↔PM binding metadata (`mapping`/`isOMark`) `updateYFragment` diffs against. Built ONCE
+   * from the seeded fragment on the first frame, then maintained IN PLACE by `updateYFragment` on every
+   * subsequent frame — the same persistent structure y-tiptap's own `ProsemirrorBinding` keeps for a
+   * doc's whole life. Caching it avoids an O(doc) `initProseMirrorDoc` tree rebuild per streamed frame.
+   * Safe ONLY because the shadow receives the agent's OWN reconciles and nothing else (never peer
+   * updates), so the fragment never changes outside `updateYFragment`; it is torn down with the session
+   * on leadership loss, so it can't outlive its fragment. A future change that ever applies an external
+   * update to `shadow` MUST reset this to `null`.
+   */
+  meta: ReturnType<typeof initProseMirrorDoc>['meta'] | null
 }

 /**
@@ -34,7 +45,7 @@ export function beginAgentStream(editor: Editor): AgentStreamSession | null {
  if (!binding) return null
  const shadow = new Y.Doc()
  Y.applyUpdate(shadow, Y.encodeStateAsUpdate(binding.doc))
-  return { shadow, fragment: shadow.getXmlFragment(COLLAB_DOC_FIELD) }
+  return { shadow, fragment: shadow.getXmlFragment(COLLAB_DOC_FIELD), meta: null }
 }

 /**
@@ -60,10 +71,10 @@ export function applyAgentStreamFrame(
  session.shadow.on('update', capture)
  try {
    session.shadow.transact(() => {
-      // `updateYFragment` diffs against the fragment's CURRENT content, so it needs the fragment↔PM
-      // binding metadata; `initProseMirrorDoc` reconstructs it from the fragment's present state.
-      const { meta } = initProseMirrorDoc(session.fragment, editor.schema)
-      updateYFragment(session.shadow, session.fragment, target, meta)
+      // Build the binding metadata once, then reuse it; updateYFragment maintains it in place. See
+      // AgentStreamSession.meta for why per-frame reuse is safe (and why a rebuild would be wasteful).
+      session.meta ??= initProseMirrorDoc(session.fragment, editor.schema).meta
+      updateYFragment(session.shadow, session.fragment, target, session.meta)
    }, AGENT_STREAM_ORIGIN)
  } finally {
    session.shadow.off('update', capture)
@@ -810,10 +810,10 @@ export function LoadedRichMarkdownEditor({
    if (editor.isEditable === isEditable) return
    // Defer out of the render/commit phase. `isEditable` flips from collab readiness (synced + seeded),
    // which is driven by a Yjs `config.observe` firing synchronously inside `Y.applyUpdate` — so this
-    // effect can run while React is mid-render. `setEditable` dispatches a TipTap transaction that the
-    // React binding commits with `flushSync`, which throws ("cannot flush while rendering") in that
-    // window. A microtask runs right after the current commit, before paint; re-check liveness/value
-    // since either can change before it fires.
+    // effect can run while React is mid-render. `setEditable` re-applies the view state and emits an
+    // `update` that the React binding commits with `flushSync`, which throws ("cannot flush while
+    // rendering") in that window. A microtask runs right after the current commit, before paint; re-check
+    // liveness/value since either can change before it fires.
    let cancelled = false
    queueMicrotask(() => {
      if (cancelled || editor.isDestroyed) return
@@ -468,23 +468,28 @@ export function Files() {
      : siblings
    const col = activeSort?.column ?? 'name'
    const dir = activeSort?.direction ?? 'asc'
-    return [...searched].sort((a, b) => {
+    // Decorate-sort: compute each key + pinned flag once (O(N)) rather than parsing dates per comparison.
+    const decorated = searched.map((folder) => ({
+      folder,
+      pinned: pinnedFolderIds.has(folder.id),
+      key:
+        col === 'updated'
+          ? new Date(folder.updatedAt).getTime()
+          : col === 'created'
+            ? new Date(folder.createdAt).getTime()
+            : folder.name,
+    }))
+    decorated.sort((a, b) => {
      // Pinned folders float to the top of every sort/direction — pinning is a
      // user-declared priority, not another sort key to be inverted by `desc`.
-      const aPinned = pinnedFolderIds.has(a.id)
-      const bPinned = pinnedFolderIds.has(b.id)
-      if (aPinned !== bPinned) return aPinned ? -1 : 1
-
-      let cmp = 0
-      if (col === 'updated') {
-        cmp = new Date(a.updatedAt).getTime() - new Date(b.updatedAt).getTime()
-      } else if (col === 'created') {
-        cmp = new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime()
-      } else {
-        cmp = a.name.localeCompare(b.name)
-      }
+      if (a.pinned !== b.pinned) return a.pinned ? -1 : 1
+      const cmp =
+        typeof a.key === 'number' && typeof b.key === 'number'
+          ? a.key - b.key
+          : String(a.key).localeCompare(String(b.key))
      return dir === 'asc' ? cmp : -cmp
    })
+    return decorated.map((d) => d.folder)
  }, [folders, currentFolderId, debouncedSearchTerm, activeSort, pinnedFolderIds])

  const filteredFiles = useMemo(() => {
@@ -522,38 +527,36 @@ export function Files() {

    const col = activeSort?.column ?? 'updated'
    const dir = activeSort?.direction ?? 'desc'
-    return [...result].sort((a, b) => {
+    // Decorate-sort: compute each row's sort key + pinned flag ONCE (O(N)), then compare precomputed
+    // keys — the comparator ran per-comparison work (Date parsing, `formatFileType`, member lookups) at
+    // O(N log N). Stable sort + pinned-primary ordering are preserved.
+    const decorated = result.map((f) => ({
+      f,
+      pinned: pinnedFileIds.has(f.id),
+      key:
+        col === 'size'
+          ? f.size
+          : col === 'type'
+            ? formatFileType(f.type, f.name)
+            : col === 'created'
+              ? new Date(f.uploadedAt).getTime()
+              : col === 'updated'
+                ? new Date(f.updatedAt).getTime()
+                : col === 'owner'
+                  ? (membersById.get(f.uploadedBy)?.name ?? '')
+                  : f.name,
+    }))
+    decorated.sort((a, b) => {
      // Pinned files float to the top of every sort/direction — pinning is a
      // user-declared priority, not another sort key to be inverted by `desc`.
-      const aPinned = pinnedFileIds.has(a.id)
-      const bPinned = pinnedFileIds.has(b.id)
-      if (aPinned !== bPinned) return aPinned ? -1 : 1
-
-      let cmp = 0
-      switch (col) {
-        case 'name':
-          cmp = a.name.localeCompare(b.name)
-          break
-        case 'size':
-          cmp = a.size - b.size
-          break
-        case 'type':
-          cmp = formatFileType(a.type, a.name).localeCompare(formatFileType(b.type, b.name))
-          break
-        case 'created':
-          cmp = new Date(a.uploadedAt).getTime() - new Date(b.uploadedAt).getTime()
-          break
-        case 'updated':
-          cmp = new Date(a.updatedAt).getTime() - new Date(b.updatedAt).getTime()
-          break
-        case 'owner':
-          cmp = (membersById.get(a.uploadedBy)?.name ?? '').localeCompare(
-            membersById.get(b.uploadedBy)?.name ?? ''
-          )
-          break
-      }
+      if (a.pinned !== b.pinned) return a.pinned ? -1 : 1
+      const cmp =
+        typeof a.key === 'number' && typeof b.key === 'number'
+          ? a.key - b.key
+          : String(a.key).localeCompare(String(b.key))
      return dir === 'asc' ? cmp : -cmp
    })
+    return decorated.map((d) => d.f)
  }, [
    files,
    currentFolderId,
@@ -1818,10 +1821,18 @@ export function Files() {
  )

  const contextMenuMoveOptions = useMemo((): MoveOptionNode[] => {
+    // Index children by parent ONCE (the same pattern used for folder sizes + descendant maps above),
+    // so building the tree is O(N) instead of a full `folders.filter` scan at every node (O(N²)).
+    const childrenByParent = new Map<string | null, typeof folders>()
+    for (const f of folders) {
+      const key = f.parentId ?? null
+      const arr = childrenByParent.get(key)
+      if (arr) arr.push(f)
+      else childrenByParent.set(key, [f])
+    }
    const buildSubtree = (parentId: string | null): MoveOptionNode[] =>
-      folders
+      (childrenByParent.get(parentId) ?? [])
        .filter((f) => {
-          if ((f.parentId ?? null) !== parentId) return false
          if (selectedFolderIds.includes(f.id)) return false
          return selectedFolderIds.every(
            (sid) => !descendantFolderIdsByFolderId.get(sid)?.has(f.id)
@@ -125,13 +125,11 @@ export const persistFileDocResponseSchema = z.discriminatedUnion('status', [
  }),
  z.object({
    /**
-     * The file changed out-of-band since `expectedVersion` — the write was refused to avoid clobbering
-     * it. `version` is the current durable version the relay adopts as its new If-Match to re-persist the
-     * current live stream (which already holds the out-of-band change via the write chokepoint). No
-     * markdown body: the relay never projects the durable body back over the live doc.
+     * The file changed out-of-band since `expectedVersion` — the write was refused to avoid clobbering it.
+     * The relay leaves the durable content authoritative and does not re-persist here (a later flush
+     * reconciles once the chokepoint merge lands), so it reads nothing off this result beyond the status.
     */
    status: z.literal('conflict'),
-    version: z.number().int(),
  }),
 ])
 export type PersistFileDocResponse = z.output<typeof persistFileDocResponseSchema>
@@ -17,14 +17,14 @@ const logger = createLogger('FileDocPersist')
 *   CONTENT version (`content_updated_at`, epoch ms) the relay records as what its live doc is synced to.
 * - `missing` — the file is gone (deleted); nothing to write.
 * - `conflict` — the file changed out-of-band since the relay's live doc last synced, so writing the
- *   projection would clobber that change (RFC 7232 `If-Match` failure). NOT written. `version` is the
- *   current durable version the relay adopts as its new If-Match to re-persist the current live stream
- *   (which already holds the out-of-band change via the write chokepoint).
+ *   projection would clobber that change (RFC 7232 `If-Match` failure). NOT written; the relay leaves the
+ *   durable content authoritative and does not advance its synced version (a later flush reconciles once
+ *   the chokepoint merge lands). No `version` is returned — the relay never reads one on this path.
 */
 export type PersistFileDocResult =
  | { status: 'persisted'; version: number }
  | { status: 'missing' }
-  | { status: 'conflict'; version: number }
+  | { status: 'conflict' }
  | { status: 'deferred' }

 /**
@@ -113,22 +113,13 @@ export async function persistFileDoc(
    }
  } catch (error) {
    if (!(error instanceof ContentVersionConflictError)) throw error
-    // Out-of-band content change since the live doc last synced — DON'T clobber. Return the current
-    // durable version so the relay adopts it as its new If-Match and re-persists the current live stream
-    // (which already holds the out-of-band change, merged in via the write chokepoint). No markdown body
-    // is returned: the relay never projects the durable body back over the live doc (that would be a
-    // destructive "make it match" that could move the doc backward and wipe newer edits).
-    const current = await getWorkspaceFile(workspaceId, fileId, { throwOnError: true })
-    if (!current) return { status: 'missing' }
+    // Out-of-band content change since the live doc last synced — DON'T clobber. The relay leaves the
+    // durable content authoritative and does NOT re-persist or advance its synced version here (a later
+    // flush reconciles once the chokepoint merge lands), so it reads nothing off this result beyond the
+    // `conflict` status — no durable version re-read is needed.
    logger.warn(
      `Persist conflict for file ${fileId}; durable content changed out-of-band since sync`
    )
-    return {
-      status: 'conflict',
-      // The CONTENT version, not `updatedAt`: the relay adopts this as its If-Match. If a metadata write
-      // bumped `updatedAt` past `contentUpdatedAt`, returning `updatedAt` would make the re-persist's CAS
-      // (which checks `contentUpdatedAt`) never match → perpetual conflict.
-      version: (current.contentUpdatedAt ?? current.updatedAt).getTime(),
-    }
+    return { status: 'conflict' }
  }
 }