From ca0f4d29578d28d8dbe641762593f59d74e2c828 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 00:53:38 +0000 Subject: [PATCH] fix(attachments): revalidate before answering an existence probe (TASK-2420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An whose load just failed asks probeForMissing whether the row is gone. It was reading fetchAttachmentMetadata, whose page-lifetime cache holds a prior `ok` observation — so an attachment deleted after that observation still read as live and the permanent placeholder could never latch. A cache of "what is this?" structurally cannot answer "is this still there?". Adds revalidateAttachmentMetadata (invalidate, then fetch) and routes the existence probe through it. Caching of ok/missing is unchanged for the metadata question, and DR-10's Retry gets the invalidate-before-refetch primitive it needs. Found by the orchestrator's Codex pass on TASK-2420. --- .../lib/components/editor/attachment-image.ts | 8 +++- .../editor/attachment-metadata.test.ts | 47 +++++++++++++++++++ .../components/editor/attachment-metadata.ts | 28 +++++++++++ 3 files changed, 82 insertions(+), 1 deletion(-) diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index 171ee3c7..a3bc8af6 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -33,6 +33,7 @@ import { type AttachmentVariant, fetchAttachmentMetadata, invalidateAttachmentMetadata, + revalidateAttachmentMetadata, mimeToFormat } from './attachment-metadata'; import { openCropModal, type CropResult } from './attachment-crop-modal'; @@ -337,10 +338,15 @@ export const AttachmentImage = Node.create({ * HEAD probe is what tells them apart: only a 404 latches, and a * `transient` result leaves the retryable placeholder exactly as * it was (and is not cached, so Retry re-issues the HEAD). + * + * It REVALIDATES rather than reading the cache: the failed load is + * evidence that whatever we last observed about this row is out of + * date, and a cached `ok` from before the deletion would make the + * placeholder permanently unlatchable. */ function probeForMissing(forUuid: string) { if (!forUuid || !opts.workspaceSlug || deleted) return; - void fetchAttachmentMetadata(opts.workspaceSlug, forUuid, opts.getDownloadUrl).then( + void revalidateAttachmentMetadata(opts.workspaceSlug, forUuid, opts.getDownloadUrl).then( (result) => { if (result.status === 'missing') latchMissing(forUuid); } diff --git a/web/src/lib/components/editor/attachment-metadata.test.ts b/web/src/lib/components/editor/attachment-metadata.test.ts index 1e8e279a..5c74a1f5 100644 --- a/web/src/lib/components/editor/attachment-metadata.test.ts +++ b/web/src/lib/components/editor/attachment-metadata.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { fetchAttachmentMetadata, invalidateAttachmentMetadata, + revalidateAttachmentMetadata, mimeToFormat } from './attachment-metadata'; @@ -211,6 +212,52 @@ describe('fetchAttachmentMetadata — caching is per-arm', () => { }); }); +describe('revalidateAttachmentMetadata — existence probes ignore the cache', () => { + // The orchestrator's Codex pass on TASK-2420 caught this: `ok` is cached + // for the page lifetime (correctly — MIME and size are durable facts about + // a content-addressed row), but that makes the cache structurally unable + // to answer "is this row still there?". An whose load just failed + // holds evidence that the cached observation is stale. + it('re-issues the HEAD and observes a 404 that a cached ok would have hidden', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValueOnce( + head(200, { 'content-type': 'image/png', 'content-length': '10' }) + ); + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ + status: 'ok', + mime: 'image/png', + size: 10 + }); + + // The row is deleted by someone else; the cached `ok` still says live. + expect(await fetchAttachmentMetadata('ws', uuid, url)).toMatchObject({ status: 'ok' }); + expect(fetchMock).toHaveBeenCalledTimes(1); + + fetchMock.mockResolvedValueOnce(head(404)); + expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('leaves the authoritative missing result cached for later readers', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValueOnce(head(404)); + expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('does not cache a transient revalidation, so a retry probes again', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValueOnce(head(500)); + expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' }); + + fetchMock.mockResolvedValueOnce(head(404)); + expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); + describe('mimeToFormat', () => { it('maps the recognized image MIMEs to the server-side format names', () => { expect(mimeToFormat('image/jpeg')).toBe('jpeg'); diff --git a/web/src/lib/components/editor/attachment-metadata.ts b/web/src/lib/components/editor/attachment-metadata.ts index 643e2024..d6734265 100644 --- a/web/src/lib/components/editor/attachment-metadata.ts +++ b/web/src/lib/components/editor/attachment-metadata.ts @@ -121,6 +121,34 @@ export function invalidateAttachmentMetadata(workspaceSlug: string, uuid: string cache.delete(`${workspaceSlug}:${uuid}`); } +/** + * Ask the server about this attachment RIGHT NOW, ignoring anything + * already cached. + * + * `fetchAttachmentMetadata` answers "what is this attachment?" and a + * cached `ok` is a perfectly good answer — MIME and size are durable + * facts about a content-addressed row. But an EXISTENCE probe asks a + * different question, "is this row still there?", and a page-lifetime + * cache structurally cannot answer it: the cached `ok` is a memory of + * an earlier observation, so a row deleted since would still read as + * live and a permanent placeholder would never latch (found by the + * orchestrator's Codex pass on TASK-2420). + * + * The callers that need this are the ones holding contrary evidence — + * an whose load just failed, or a user pressing Retry (DR-10, + * which requires invalidating before refetching for exactly this + * reason). Use `fetchAttachmentMetadata` for everything else; this one + * costs a round trip every call by design. + */ +export function revalidateAttachmentMetadata( + workspaceSlug: string, + uuid: string, + getDownloadUrl: AttachmentUrlBuilder +): Promise { + invalidateAttachmentMetadata(workspaceSlug, uuid); + return fetchAttachmentMetadata(workspaceSlug, uuid, getDownloadUrl); +} + /** * Map a MIME type to its canonical short format name as the server's * Capabilities reports it ("png" / "jpeg" / "gif" / "bmp" / "tiff" /