mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 11:26:34 +00:00
fix(attachments): revalidate before answering an existence probe (TASK-2420)
An <img> 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.
This commit is contained in:
@@ -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<AttachmentImageOptions>({
|
||||
* 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);
|
||||
}
|
||||
|
||||
@@ -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 <img> 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');
|
||||
|
||||
@@ -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 <img> 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<AttachmentMetadataResult> {
|
||||
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" /
|
||||
|
||||
Reference in New Issue
Block a user