fix(files): name every collaborative document, and only cache what the URL names

Two findings from review, both real.

A document stored before identities existed is returned by the seed's fast path
on every open, and that path never named one — so those files could never
acquire an identity, and the join-ack guard could never fire for them. That is
the population most likely to have a tab that outlived its room, which is the
case the guard exists for. The fast path now names an unnamed document and
stores it, once: minting without storing would name it differently on every
open and the guard would start refusing clients that hold the very same
document.

The inline route marked a response immutable whenever the caller passed a key,
but a key is resolved to a FILE and the file's current key is what gets
streamed. A content write landing between those two reads would serve the new
bytes under a URL naming the old object — and cached for a year, that is wrong
forever. The flag is now what it always meant: the URL names the exact object
that was streamed.

Author: icecrasher321

apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts

@@ -30,7 +30,7 @@ describe('GET /api/workspaces/[id]/files/inline', () => {
     mockReadInline.mockResolvedValue({
       file: { name: 'photo.png', type: 'image/png', size: PNG.length },
       stream: new Blob([new Uint8Array(PNG)]).stream(),
-      addressedBy: 'fileId',
+      contentAddressed: false,
     })
   })
 
@@ -66,11 +66,11 @@ describe('GET /api/workspaces/[id]/files/inline', () => {
    * rendered by two editors (the read-only placeholder, then the live one) and each renders the image
    * twice, so the image was fetched again on every one of those passes.
    */
-  it('lets the browser keep an image addressed by storage key', async () => {
+  it('lets the browser keep an image whose URL names the object that was streamed', async () => {
     mockReadInline.mockResolvedValue({
       file: { name: 'photo.png', type: 'image/png', size: PNG.length },
       stream: new Blob([new Uint8Array(PNG)]).stream(),
-      addressedBy: 'key',
+      contentAddressed: true,
     })
 
     const res = await GET(req('key=workspace%2Fws-1%2Fphoto.png'), params)

apps/sim/app/api/workspaces/[id]/files/inline/route.ts

@@ -11,23 +11,22 @@ import { encodeFilenameForHeader, getSecureFileHeaders } from '@/app/api/files/u
 export const dynamic = 'force-dynamic'
 
 /**
- * How long the browser may reuse an embedded image, by how the request addressed it (see
- * {@link ReadWorkspaceInlineFileResult.addressedBy}).
+ * How long the browser may reuse an embedded image, decided by whether the URL names the exact object
+ * that was streamed (see {@link ReadWorkspaceInlineFileResult.contentAddressed}).
  *
- * A `key` names one storage object and a content write never rewrites one, so those bytes are
- * immutable and the browser needs no round trip — which is the difference between an embedded image
- * reappearing instantly and it being downloaded again. Every document render asks for the same image
- * at least twice (ProseMirror's own DOM, then the React node view) and every editor mounts twice (the
+ * A content write never rewrites a storage object, so a URL that names one addresses bytes that can
+ * never change and the browser needs no round trip — which is the difference between an embedded image
+ * reappearing instantly and being downloaded again. Every document render asks for the same image at
+ * least twice (ProseMirror's own DOM, then the React node view) and every editor mounts twice (the
  * read-only placeholder, then the live editor), so revalidating each time meant re-fetching the whole
  * image on every open and reload — measured at ~1 MB per open on a real document, with the image area
  * blank until it landed. `private` keeps it out of shared caches: the bytes are authorized per user.
  *
- * A `fileId` names the FILE, and its bytes move under it on every edit, so that form keeps revalidating.
+ * Anything else — a request that names the FILE, whose bytes move under it, or one whose object was
+ * rotated away mid-request — keeps revalidating.
  */
-const INLINE_CACHE_CONTROL = {
-  key: 'private, max-age=31536000, immutable',
-  fileId: 'private, no-cache, must-revalidate',
-} as const
+const IMMUTABLE_CACHE_CONTROL = 'private, max-age=31536000, immutable'
+const REVALIDATE_CACHE_CONTROL = 'private, no-cache, must-revalidate'
 
 /**
  * GET /api/workspaces/[id]/files/inline?key=<cloudKey>|fileId=<id>
@@ -48,12 +47,12 @@ export const GET = defineInternalBinaryRoute({
     fileId: query.fileId,
   }),
   useCase: readWorkspaceInlineFile,
-  present: ({ file, stream, addressedBy }) => {
+  present: ({ file, stream, contentAddressed }) => {
     const secure = getSecureFileHeaders(file.name, file.type)
     const headers = new Headers({
       'Content-Type': secure.contentType,
       'Content-Disposition': `${secure.disposition}; ${encodeFilenameForHeader(file.name)}`,
-      'Cache-Control': INLINE_CACHE_CONTROL[addressedBy],
+      'Cache-Control': contentAddressed ? IMMUTABLE_CACHE_CONTROL : REVALIDATE_CACHE_CONTROL,
       'X-Content-Type-Options': 'nosniff',
     })
     if (secure.contentType === 'image/svg+xml') {

apps/sim/lib/collab-doc/seed.test.ts

@@ -69,6 +69,9 @@ describe('buildFileDocSeed', () => {
     // repaired on the way through and this would assert the fast path while never taking it.
     const cachedDoc = markdownToYDoc('# Anything')
     cachedDoc.getText('marker').insert(0, 'cached')
+    // Named, as anything the current seed stored would be — an unnamed document is rewritten once to
+    // give it an identity, which has its own test below.
+    cachedDoc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-already-named')
     const cached = Y.encodeStateAsUpdate(cachedDoc)
     mockFetchBuffer.mockResolvedValue(Buffer.from('# Anything', 'utf-8'))
     mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' })
@@ -273,6 +276,35 @@ describe('buildFileDocSeed — document identity', () => {
     doc.destroy()
   })
 
+  /**
+   * A document stored before identities existed is returned by the fast path on every open, so if it
+   * were named only where documents are BUILT those files would never acquire one — and the join-ack
+   * guard could never fire for them, which is the population most likely to have a tab that outlived
+   * its room. Naming it must also be stored, or every open would name it differently and the guard
+   * would refuse a client holding the very same document.
+   */
+  it('names a stored document that predates identities, once, and keeps that name', async () => {
+    const legacy = markdownToYDoc('# Legacy')
+    mockFetchBuffer.mockResolvedValue(Buffer.from('# Legacy', 'utf-8'))
+    mockLoadState.mockResolvedValue({
+      docState: Y.encodeStateAsUpdate(legacy),
+      sourceHash: 'test-source-hash',
+    })
+
+    const first = await buildFileDocSeed('ws-1', 'file-1')
+    const docId = docIdOf(first!.update)
+    expect(typeof docId).toBe('string')
+    expect(mockSaveState).toHaveBeenCalledWith('file-1', first!.update, 'test-source-hash')
+
+    // The next open finds it named and hands back the stored bytes untouched.
+    mockSaveState.mockClear()
+    mockLoadState.mockResolvedValue({ docState: first!.update, sourceHash: 'test-source-hash' })
+    const second = await buildFileDocSeed('ws-1', 'file-1')
+    expect(docIdOf(second!.update)).toBe(docId)
+    expect(mockSaveState).not.toHaveBeenCalled()
+    legacy.destroy()
+  })
+
   it('still seeds when the document cannot be stored (the write is best-effort)', async () => {
     mockFetchBuffer.mockResolvedValue(Buffer.from('# Title', 'utf-8'))
     mockSaveState.mockRejectedValue(new Error('db down'))

apps/sim/lib/collab-doc/seed.ts

@@ -41,19 +41,57 @@ export interface FileDocSeed {
  * that ever repairs it. Running the round-trip here is that repair, and it doubles as the bound on this
  * branch: the cached path never calls `parseMarkdownToDoc`, so it is the one way into a room that the
  * parse-side limits do not cover. Returns the original bytes untouched when the snapshot is already
- * canonical (the common case) — no re-encode — and a fresh encode, preserving the CRDT's client ids, when
- * it repaired one.
+ * canonical AND already named (the common case) — no re-encode — and a fresh encode, preserving the
+ * CRDT's client ids, when it had to repair or name one.
+ *
+ * Naming happens here too, not only where a document is built: a document stored before identities
+ * existed is returned by this path on every open, so if it were skipped here those files would never
+ * acquire one and the join-ack guard could never fire for them — which is the population most likely to
+ * have a tab that outlived its room. `changed` tells the caller to store what it got back, so the
+ * identity is minted ONCE and every later open agrees with it (a re-minted one would make the guard
+ * refuse a client holding the very same document).
  */
-function canonicalSeedUpdate(cached: Uint8Array): Uint8Array {
+function prepareCachedSeed(cached: Uint8Array): { update: Uint8Array; changed: boolean } {
   const doc = new Y.Doc()
   try {
     Y.applyUpdate(doc, cached)
-    return canonicalizeYDoc(doc) ? Y.encodeStateAsUpdate(doc) : cached
+    const repaired = canonicalizeYDoc(doc)
+    const named = ensureDocumentIdentity(doc)
+    return repaired || named
+      ? { update: Y.encodeStateAsUpdate(doc), changed: true }
+      : { update: cached, changed: false }
   } finally {
     doc.destroy()
   }
 }
 
+/**
+ * Give a document an identity if it has none, and report whether it needed one. A resumed document
+ * keeps the identity its clients already know it by — re-minting would make the join-ack guard refuse
+ * a client that holds this exact document.
+ */
+function ensureDocumentIdentity(ydoc: Y.Doc): boolean {
+  const config = ydoc.getMap(FILE_DOC_SEED.configMap)
+  if (typeof config.get(FILE_DOC_SEED.docIdKey) === 'string') return false
+  config.set(FILE_DOC_SEED.docIdKey, generateId())
+  return true
+}
+
+/** Store the file's collaborative document. Best-effort: the durable markdown is the source of truth. */
+async function storeDocument(
+  fileId: string,
+  update: Uint8Array,
+  sourceHash: string
+): Promise<void> {
+  try {
+    await saveCollabDocState(fileId, update, sourceHash)
+  } catch (error) {
+    logger.warn(`Failed to store the collaborative document for file ${fileId}`, {
+      error: getErrorMessage(error),
+    })
+  }
+}
+
 /**
  * Build the server-side seed for a file's collaborative document: load the file's current markdown
  * and convert it — through the exact client engine (see {@link markdownToYDoc}) — into a Yjs update.
@@ -97,7 +135,11 @@ export async function buildFileDocSeed(
   // Cold-start fast path: the stored document already projects to THIS markdown, so apply it verbatim
   // (the Hocuspocus load-document pattern) instead of re-converting.
   if (stored?.sourceHash === sourceHash) {
-    return { update: canonicalSeedUpdate(stored.docState), version }
+    const prepared = prepareCachedSeed(stored.docState)
+    // Store a repair or a freshly-minted identity so the next open finds it done — and, for the
+    // identity, so every open names the SAME document.
+    if (prepared.changed) await storeDocument(fileId, prepared.update, sourceHash)
+    return { update: prepared.update, version }
   }
 
   const { frontmatter, body } = splitFrontmatter(buffer.toString('utf-8'))
@@ -113,23 +155,13 @@ export async function buildFileDocSeed(
     // Carry the frontmatter in the doc (not the body) so it merges across clients and a later
     // server-side edit can update it — the editor re-attaches this on autosave.
     config.set(FILE_DOC_SEED.frontmatterKey, frontmatter)
-    // Mint an identity only for a document that has none; a resumed one keeps the identity its clients
-    // already know it by.
-    if (typeof config.get(FILE_DOC_SEED.docIdKey) !== 'string') {
-      config.set(FILE_DOC_SEED.docIdKey, generateId())
-    }
+    ensureDocumentIdentity(ydoc)
     const update = Y.encodeStateAsUpdate(ydoc)
     // Store it NOW, not at the next persist. Until this row exists every cold open builds the document
     // again from markdown, minting a new identity each time — so a file that is opened but never edited
     // has a different document on every open, and any client that outlives a room (a laptop that slept
     // past the shared stream's TTL) reconnects into one and merges its content in twice.
-    try {
-      await saveCollabDocState(fileId, update, sourceHash)
-    } catch (error) {
-      logger.warn(`Failed to store the collaborative document for file ${fileId}`, {
-        error: getErrorMessage(error),
-      })
-    }
+    await storeDocument(fileId, update, sourceHash)
     return { update, version }
   } finally {
     ydoc.destroy()

apps/sim/lib/workspace-files/application/read-workspace-inline-file.test.ts

@@ -76,6 +76,39 @@ describe('readWorkspaceInlineFile', () => {
     expect(mockLoadContext).toHaveBeenCalledWith('f1')
   })
 
+  /**
+   * The response is marked immutable only when the URL names the object that was streamed. The key is
+   * resolved to a file and the file's CURRENT key is what gets downloaded, so a rotation landing
+   * between those two reads would serve new bytes under a URL naming the old object — cached, that
+   * would be wrong forever.
+   */
+  it('is content-addressed only when the requested key is the one it streamed', async () => {
+    mockGetMetadataByKey.mockResolvedValue({ id: 'f1', workspaceId: 'ws-1' })
+
+    const match = await readWorkspaceInlineFile.execute({
+      principal,
+      input: { workspaceId: 'ws-1', key: file.key },
+    })
+    expect(match.contentAddressed).toBe(true)
+
+    // The file's content was replaced between resolving the key and reading the row.
+    mockGetWorkspaceFile.mockResolvedValue({ ...file, key: 'workspace/ws-1/rotated-photo.png' })
+    const rotated = await readWorkspaceInlineFile.execute({
+      principal,
+      input: { workspaceId: 'ws-1', key: file.key },
+    })
+    expect(rotated.contentAddressed).toBe(false)
+  })
+
+  it('is never content-addressed when the caller named the file rather than an object', async () => {
+    const result = await readWorkspaceInlineFile.execute({
+      principal,
+      input: { workspaceId: 'ws-1', fileId: 'f1' },
+    })
+
+    expect(result.contentAddressed).toBe(false)
+  })
+
   it('conceals a key belonging to another workspace before authorization', async () => {
     mockGetMetadataByKey.mockResolvedValue({ id: 'other', workspaceId: 'ws-other' })
 

apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts

@@ -22,15 +22,16 @@ export interface ReadWorkspaceInlineFileResult {
   file: WorkspaceFileRecord
   stream: ReadableStream<Uint8Array>
   /**
-   * How the caller addressed the file, which decides whether the response may be cached.
+   * Whether the URL that produced this response names the exact storage object it streamed — the only
+   * condition under which the bytes may be cached, since a content write never rewrites an object (it
+   * uploads under a fresh key and repoints the row).
    *
-   * `key` names a storage object, and a content write never rewrites one — it uploads under a fresh
-   * key and repoints the row, so the lookup that resolved this request (an exact match on the CURRENT
-   * key) either found bytes that can never change or found nothing at all. The response is therefore
-   * immutable. `fileId` names the FILE, whose bytes move as it is edited, so those responses must keep
-   * revalidating.
+   * It is deliberately not "the caller passed a key": the key is resolved to a file and the file's
+   * CURRENT key is what gets streamed, so a rotation landing between those two reads would serve the
+   * new bytes under a URL naming the old object — cached, that would be wrong forever. A `fileId`
+   * request names the FILE, whose bytes move as it is edited, and is never content-addressed.
    */
-  addressedBy: 'key' | 'fileId'
+  contentAddressed: boolean
 }
 
 async function executeReadWorkspaceInlineFile({
@@ -50,7 +51,7 @@ async function executeReadWorkspaceInlineFile({
   return {
     file,
     stream: nodeReadableToWebStream(stream),
-    addressedBy: input.key ? 'key' : 'fileId',
+    contentAddressed: input.key !== undefined && input.key === file.key,
   }
 }