diff --git a/apps/api/src/hosted/cms-drafts.test.ts b/apps/api/src/hosted/cms-drafts.test.ts index bc2c32fa43..307aaf4926 100644 --- a/apps/api/src/hosted/cms-drafts.test.ts +++ b/apps/api/src/hosted/cms-drafts.test.ts @@ -77,7 +77,7 @@ describe("cms drafts", () => { expect(found?.state.files.get("ghost")).toEqual({ file: null, etag: null }); }); - it("discard deletes every listed file, then the index, then the row", async () => { + it("discard empties the index, then deletes every listed file, then the index, then the row", async () => { const delivery = memoryDeliveryStore(); const drafts = createCmsDrafts({ table: memoryDraftTable(), @@ -92,6 +92,7 @@ describe("cms drafts", () => { delivery.log.length = 0; expect(await drafts.discard(SITE, "x")).toBe(true); expect(delivery.log).toEqual([ + `put ${deliveryKeys.draftIndex(SITE, row.slug)}`, `delete ${deliveryKeys.draftFile(SITE, row.slug, "a")}`, `delete ${deliveryKeys.draftFile(SITE, row.slug, "b")}`, `delete ${deliveryKeys.draftIndex(SITE, row.slug)}`, @@ -125,6 +126,100 @@ describe("cms drafts", () => { ); }); + it("discard never leaves a listed name whose file is gone", async () => { + const delivery = memoryDeliveryStore(); + const drafts = createCmsDrafts({ + table: memoryDraftTable(), + store: delivery.store, + }); + const { row } = await drafts.ensure(SITE, "x", { + userId: "u1", + via: "token", + }); + await seedDraftFile(delivery.store, SITE, row.slug, "a", setFile({ a: 1 })); + await seedDraftFile(delivery.store, SITE, row.slug, "b", setFile({ b: 1 })); + const indexKey = deliveryKeys.draftIndex(SITE, row.slug); + const remove = delivery.store.delete.bind(delivery.store); + const seen: string[][] = []; + delivery.store.delete = async (key: string) => { + const index = delivery.objects.get(indexKey); + if (index && key !== indexKey) { + seen.push((JSON.parse(index.text) as { names: string[] }).names); + } + return remove(key); + }; + expect(await drafts.discard(SITE, "x")).toBe(true); + expect(seen).toEqual([[], []]); + }); + + it("a failed file delete puts the names it didn't delete back into the index, and a retried discard deletes them", async () => { + const delivery = memoryDeliveryStore(); + const drafts = createCmsDrafts({ + table: memoryDraftTable(), + store: delivery.store, + }); + const { row } = await drafts.ensure(SITE, "x", { + userId: "u1", + via: "token", + }); + await seedDraftFile(delivery.store, SITE, row.slug, "a", setFile({ a: 1 })); + await seedDraftFile(delivery.store, SITE, row.slug, "b", setFile({ b: 1 })); + await seedDraftFile(delivery.store, SITE, row.slug, "c", setFile({ c: 1 })); + const remove = delivery.store.delete.bind(delivery.store); + const bKey = deliveryKeys.draftFile(SITE, row.slug, "b"); + delivery.store.delete = async (key: string) => { + if (key === bKey) throw new DeliveryUnavailableError(key, "503"); + return remove(key); + }; + await expect(drafts.discard(SITE, "x")).rejects.toThrow( + DeliveryUnavailableError, + ); + // a went; b and c are still there, and listed again, so a save guards + // against them instead of tripping over a hidden file. + expect( + (await readDraftIndex(delivery.store, SITE, row.slug)).names, + ).toEqual(["b", "c"]); + expect( + delivery.objects.has(deliveryKeys.draftFile(SITE, row.slug, "a")), + ).toBe(false); + expect((await drafts.resolve(SITE, "x"))?.row.slug).toBe(row.slug); + delivery.store.delete = remove; + expect(await drafts.discard(SITE, "x")).toBe(true); + expect(delivery.objects.size).toBe(0); + }); + + it("discard deletes a name a save listed while the index was being emptied", async () => { + const delivery = memoryDeliveryStore(); + const drafts = createCmsDrafts({ + table: memoryDraftTable(), + store: delivery.store, + }); + const { row } = await drafts.ensure(SITE, "x", { + userId: "u1", + via: "token", + }); + await seedDraftFile(delivery.store, SITE, row.slug, "a", setFile({ a: 1 })); + const indexKey = deliveryKeys.draftIndex(SITE, row.slug); + const putJson = delivery.store.putJson.bind(delivery.store); + let raced = false; + delivery.store.putJson = async (key, value, cacheControl, condition) => { + if (key === indexKey && !raced) { + raced = true; + await seedDraftFile( + delivery.store, + SITE, + row.slug, + "late", + setFile({ late: 1 }), + ); + } + return putJson(key, value, cacheControl, condition); + }; + expect(await drafts.discard(SITE, "x")).toBe(true); + expect(raced).toBe(true); + expect(delivery.objects.size).toBe(0); + }); + it("names follow the branch rule, at most 255 characters", () => { expect(isValidDraftName("threads/abc-123")).toBe(true); expect(isValidDraftName("-x")).toBe(false); diff --git a/apps/api/src/hosted/cms-drafts.ts b/apps/api/src/hosted/cms-drafts.ts index 195cdf9374..1270560e2d 100644 --- a/apps/api/src/hosted/cms-drafts.ts +++ b/apps/api/src/hosted/cms-drafts.ts @@ -13,7 +13,9 @@ * The file is the truth: a listed name whose file is missing, or holds a * `published` marker, is not in the draft. A name new to the draft goes into * the index first, then its file is written, so nothing written is ever - * invisible. Every write is conditional on the object as read + * invisible. A discard runs the other way: it empties the index first, then + * deletes the files it listed (putting back the names it couldn't delete), + * so a reader never meets a listed name whose file is gone. Every write is conditional on the object as read * (`If-None-Match: *` to create, `If-Match` on its ETag otherwise); only the * index's read-modify-write is retried here (on 412, up to 3 attempts). * Draft folders are never listed: every reader goes index → name → key. @@ -401,8 +403,12 @@ export interface CmsDrafts { by: { userId: string; via: CmsVia }, ): Promise<{ row: CmsDraftRow; created: boolean; state: DraftState }>; /** - * Deletes every listed file, then the index, then the row; false when - * there was no row. A failed delete keeps the row, so it can be retried. + * Empties the index (`{ names: [] }`), then deletes every file it listed, + * then the index, then the row; false when there was no row. Readers see + * the draft hold nothing before any file goes, never a listed name whose + * file is gone. A failed delete keeps the row, so it can be retried, and + * puts the names not yet deleted back into the index (best effort), so + * the files left behind stay listed rather than hidden. */ discard(site: string, name: string): Promise; } @@ -456,8 +462,36 @@ export function createCmsDrafts(deps: { const row = await table.get(site, name); if (!row) return false; const index = await readDraftIndex(store, site, row.slug); - for (const listed of index.names) { - await store.delete(deliveryKeys.draftFile(site, row.slug, listed)); + // The names as the emptying write found them: a save that listed a + // name between our read and that write is deleted along with the rest. + let names: readonly string[] = index.names; + if (names.length > 0) { + await updateDraftIndex( + store, + site, + row.slug, + (current) => { + names = current; + return []; + }, + index, + ); + } + for (const [i, listed] of names.entries()) { + try { + await store.delete(deliveryKeys.draftFile(site, row.slug, listed)); + } catch (error) { + // The names not yet deleted go back into the index, so the files + // that are left stay listed (a save then guards against them, and + // a retried discard deletes them) instead of hiding as unlisted + // files a later save's `ifMatch: null` trips over. + const left = names.slice(i); + await updateDraftIndex(store, site, row.slug, (current) => [ + ...current, + ...left, + ]).catch(() => {}); + throw error; + } } await store.delete(deliveryKeys.draftIndex(site, row.slug)); await table.delete(site, name); diff --git a/apps/api/src/hosted/cms-storage.test.ts b/apps/api/src/hosted/cms-storage.test.ts index eee1cbeacb..8dce571842 100644 --- a/apps/api/src/hosted/cms-storage.test.ts +++ b/apps/api/src/hosted/cms-storage.test.ts @@ -26,6 +26,7 @@ import { } from "./cms-test-helpers"; import { CACHE_SCHEMA, + CONDITIONAL_PUT_TIMEOUT_MS, DeliveryUnavailableError, deliveryKeys, newDraftSlug, @@ -526,6 +527,87 @@ describe("blocks.apply (the save)", () => { ]); }); + it("a timed-out file PUT is -32008 with what landed; the index already lists it, and the retry of the rest lands", async () => { + const { delivery, slug, call } = setup(); + const second = deliveryKeys.draftFile(SITE, slug, "b"); + let stalled = false; + delivery.hooks.beforePut = (key) => { + if (key !== second || stalled) return; + stalled = true; + // What the S3 store throws when R2 doesn't answer in time. + throw new DeliveryUnavailableError( + key, + new Error(`no answer within ${CONDITIONAL_PUT_TIMEOUT_MS} ms`), + ); + }; + const answer = await call("blocks.apply", { + set: { a: { v: 1 }, b: { v: 1 } }, + ifMatch: { a: null, b: null }, + }); + expect(answer.error?.code).toBe(-32008); + expect(answer.error?.data).toEqual({ + retryAfterMs: 250, + landed: { a: blockVersion({ v: 1 }) }, + }); + // Index first: b is listed though its file isn't written, so readers + // see it as not in the draft until the retry writes it. + expect((await readDraftIndex(delivery.store, SITE, slug)).names).toEqual([ + "a", + "b", + ]); + expect( + (await readDraftFile(delivery.store, SITE, slug, "b")).file, + ).toBeNull(); + const retry = await call("blocks.apply", { + set: { b: { v: 1 } }, + ifMatch: { b: null }, + }); + expect(retry.result?.versions).toEqual({ b: blockVersion({ v: 1 }) }); + expect( + (await readDraftFile(delivery.store, SITE, slug, "b")).file, + ).not.toBeNull(); + }); + + it("a retry whose PUT meets the first try's own late write counts it as landed; another body is still a lost race", async () => { + for (const late of [{ v: 1 }, { v: 2 }]) { + const { delivery, slug, call } = setup(); + const second = deliveryKeys.draftFile(SITE, slug, "b"); + let tries = 0; + delivery.hooks.beforePut = (key) => { + if (key !== second) return; + tries++; + if (tries === 1) { + throw new DeliveryUnavailableError( + key, + new Error(`no answer within ${CONDITIONAL_PUT_TIMEOUT_MS} ms`), + ); + } + // The given-up PUT commits now: after the retry read the draft, + // before the retry's own PUT. + delivery.objects.set(key, { + text: JSON.stringify(setFile(late)), + etag: '"late"', + cacheControl: "", + }); + }; + const first = await call("blocks.apply", { + set: { a: { v: 1 }, b: { v: 1 } }, + ifMatch: { a: null, b: null }, + }); + expect(first.error?.code).toBe(-32008); + const retry = await call("blocks.apply", { + set: { b: { v: 1 } }, + ifMatch: { b: null }, + }); + if (late.v === 1) { + expect(retry.result?.versions).toEqual({ b: blockVersion({ v: 1 }) }); + } else { + expect(retry.error?.code).toBe(-32008); + expect(retry.error?.data).toEqual({ retryAfterMs: 250, landed: {} }); + } + } + }); + it("an R2 429 before any file landed is -32008 with landed {}", async () => { const { delivery, slug, call } = setup(); delivery.hooks.beforePut = (key) => { diff --git a/apps/api/src/hosted/cms-storage.ts b/apps/api/src/hosted/cms-storage.ts index e2725f07fa..3539af8142 100644 --- a/apps/api/src/hosted/cms-storage.ts +++ b/apps/api/src/hosted/cms-storage.ts @@ -51,6 +51,7 @@ import { editorOf, holdsName, mapBounded, + readDraftFile, readDraftFiles, readDraftIndex, readDraftState, @@ -452,14 +453,25 @@ export function createCmsStorage(options: { read.files.get(name)?.etag ?? null, ); } catch (error) { - partial = { ...landed }; - if ( - error instanceof ObjectChangedError || - error instanceof DeliveryUnavailableError - ) { - throw new PartialApplyError(landed, error); + // A 412 from a file that already holds this very version is this + // save's own earlier try landing late (a PUT given up on as + // Unavailable that R2 still committed): it landed, not a lost race. + const own = + error instanceof ObjectChangedError + ? await readDraftFile(store, site, slug, name).catch(() => null) + : null; + if (own && draftVersion(own) === version) { + last = own.etag; + } else { + partial = { ...landed }; + if ( + error instanceof ObjectChangedError || + error instanceof DeliveryUnavailableError + ) { + throw new PartialApplyError(landed, error); + } + throw error; } - throw error; } landed[name] = version; fileVersions[blockFileName(name)] = version; diff --git a/apps/api/src/hosted/delivery-store-s3.test.ts b/apps/api/src/hosted/delivery-store-s3.test.ts index c5ae6986a7..887e1dc266 100644 --- a/apps/api/src/hosted/delivery-store-s3.test.ts +++ b/apps/api/src/hosted/delivery-store-s3.test.ts @@ -16,6 +16,8 @@ import { } from "./delivery-store"; let answer: { status: number; code: string } = { status: 200, code: "" }; +/** How long the fake S3 sits on a request before it answers. */ +let delayMs = 0; const seen: Array<{ method: string; path: string; @@ -25,7 +27,7 @@ const seen: Array<{ }> = []; const server = Bun.serve({ port: 0, - fetch(req) { + async fetch(req) { const url = new URL(req.url); seen.push({ method: req.method, @@ -34,6 +36,7 @@ const server = Bun.serve({ ifMatch: req.headers.get("if-match"), ifNoneMatch: req.headers.get("if-none-match"), }); + if (delayMs > 0) await Bun.sleep(delayMs); if (answer.status === 200) { return new Response("", { status: 200, headers: { ETag: '"e1"' } }); } @@ -45,11 +48,15 @@ const server = Bun.serve({ }); afterAll(() => server.stop(true)); +const warned: Array<{ message: string; fields: Record }> = []; const store = createS3DeliveryStore({ endpoint: `http://127.0.0.1:${server.port}`, bucket: "b", accessKeyId: "k", secretAccessKey: "s", + conditionalTimeoutMs: 300, + slowMs: 100, + warn: (message, fields) => warned.push({ message, fields }), }); async function putWith(status: number, code: string, conditional: boolean) { @@ -156,6 +163,92 @@ describe("createS3DeliveryStore conditional PUTs", () => { }); }); +describe("createS3DeliveryStore conditional PUT timeout", () => { + const slug = "AAAAAAAAAAAAAAAAAAAAAA"; + const draftKey = `sites/acme/drafts/${slug}/home.json`; + const putDraft = () => + store.putJson(draftKey, { a: 1 }, "", { ifMatch: '"e0"' }).then( + (res) => res, + (error: unknown) => error, + ); + + it("a stalled PUT is given up as Unavailable, sent once, and logged with its duration", async () => { + answer = { status: 200, code: "" }; + delayMs = 2_000; + seen.length = 0; + warned.length = 0; + const started = performance.now(); + try { + const error = await putDraft(); + const took = performance.now() - started; + expect(error).toBeInstanceOf(DeliveryUnavailableError); + expect((error as Error).message).toContain("no answer within 300 ms"); + expect(took).toBeLessThan(1_500); + expect(seen.length).toBe(1); + expect(warned).toHaveLength(1); + const fields = warned[0]!.fields; + expect(fields.outcome).toBe("timeout"); + expect(fields.durationMs as number).toBeGreaterThanOrEqual(300); + expect(fields.bytes).toBe(7); + // The slug is the draft's read grant: never in a log line. + expect(fields.key).toBe("sites/acme/drafts//home.json"); + expect(JSON.stringify(warned)).not.toContain(slug); + } finally { + delayMs = 0; + } + }); + + it("a slow PUT that lands is logged; a quick one isn't", async () => { + answer = { status: 200, code: "" }; + warned.length = 0; + expect(await putDraft()).toEqual({ etag: '"e1"' }); + expect(warned).toHaveLength(0); + delayMs = 150; + try { + expect(await putDraft()).toEqual({ etag: '"e1"' }); + } finally { + delayMs = 0; + } + expect(warned).toHaveLength(1); + expect(warned[0]!.fields.outcome).toBe("ok"); + expect(warned[0]!.fields.durationMs as number).toBeGreaterThanOrEqual(100); + }); + + it("a publish PUT (not a draft key) is never cut off, only logged when slow", async () => { + answer = { status: 200, code: "" }; + delayMs = 500; + seen.length = 0; + warned.length = 0; + try { + expect( + await store.putJson("sites/acme/latest.json", { a: 1 }, "", { + ifMatch: '"e0"', + }), + ).toEqual({ etag: '"e1"' }); + } finally { + delayMs = 0; + } + expect(seen.length).toBe(1); + expect(warned).toHaveLength(1); + expect(warned[0]!.fields.outcome).toBe("ok"); + expect(warned[0]!.fields.timeoutMs).toBeNull(); + expect(warned[0]!.fields.durationMs as number).toBeGreaterThanOrEqual(300); + }); + + it("a 503 is logged as unavailable; a lost race isn't logged", async () => { + warned.length = 0; + expect(await putWith(412, "PreconditionFailed", true)).toBeInstanceOf( + ObjectChangedError, + ); + expect(warned).toHaveLength(0); + expect(await putWith(503, "ServiceUnavailable", true)).toBeInstanceOf( + DeliveryUnavailableError, + ); + expect(warned.map((w) => w.fields.outcome)).toEqual(["unavailable"]); + answer = { status: 200, code: "" }; + }); +}); + describe("deliveryKeys for revisions", () => { it("keys a revision as a folder with index.json and meta.json", () => { const id = newRevisionId(); diff --git a/apps/api/src/hosted/delivery-store.ts b/apps/api/src/hosted/delivery-store.ts index edfb75e145..73cea00ff3 100644 --- a/apps/api/src/hosted/delivery-store.ts +++ b/apps/api/src/hosted/delivery-store.ts @@ -246,6 +246,34 @@ function isUnavailable(error: unknown): boolean { return status === undefined || status === 429 || status >= 500; } +/** + * How long one conditional PUT of a draft object (a draft's index or one of + * its files) may wait for R2's answer before it is given up as Unavailable. + * R2 answers a draft-sized PUT in well under a second (p99 a few hundred + * ms); the largest draft body is one block of a save request capped at + * 8 MiB, which needs ~13 s at a poor 5 Mbit/s uplink, so 15 s never cuts + * off a healthy draft write. A stalled PUT (one sat ~25 s before landing) + * then fails fast, as a `DeliveryUnavailableError`, the Unavailable answer + * the editor already retries, instead of holding the save open. + * + * Only draft keys get it. A publish writes bodies up to 16 MiB (block + * objects, a revision's index, `latest.json`), which a poor link can take + * longer than 15 s to send, and a publish that gave up on a `latest.json` + * PUT that then landed would leave the site live but unpurged; those PUTs + * keep the SDK's defaults (no timeout), as before, and are only logged. + */ +export const CONDITIONAL_PUT_TIMEOUT_MS = 15_000; + +/** A conditional PUT this slow is logged even when it lands. */ +const SLOW_PUT_MS = 2_000; + +const DRAFT_KEY_RE = /\/drafts\/[A-Za-z0-9_-]{22}\//; + +/** A delivery key fit for logs: a draft's slug is its read grant, so it's hidden. */ +function loggableKey(key: string): string { + return key.replace(DRAFT_KEY_RE, "/drafts//"); +} + function isNotModified(error: unknown): boolean { const e = error as { name?: string; $metadata?: { httpStatusCode?: number } }; return e?.name === "NotModified" || e?.$metadata?.httpStatusCode === 304; @@ -261,6 +289,12 @@ export function createS3DeliveryStore(config: { bucket: string; accessKeyId: string; secretAccessKey: string; + /** {@link CONDITIONAL_PUT_TIMEOUT_MS} unless set (tests); draft keys only. */ + conditionalTimeoutMs?: number; + /** {@link SLOW_PUT_MS} unless set (tests). */ + slowMs?: number; + /** Where a slow or failed conditional PUT is reported (`console.warn`). */ + warn?: (message: string, fields: Record) => void; }): DeliveryStore { const options = { endpoint: config.endpoint, @@ -278,6 +312,9 @@ export function createS3DeliveryStore(config: { * look like a lost race (S2). The protocol's attempts handle races. */ const conditionalClient = new S3Client({ ...options, maxAttempts: 1 }); + const timeoutMs = config.conditionalTimeoutMs ?? CONDITIONAL_PUT_TIMEOUT_MS; + const slowMs = config.slowMs ?? SLOW_PUT_MS; + const warn = config.warn ?? console.warn; const Bucket = config.bucket; const put = async ( key: string, @@ -285,31 +322,70 @@ export function createS3DeliveryStore(config: { cacheControl: string, condition: PutCondition | undefined, ) => { + const bytes = Buffer.from(body, "utf-8"); + const command = new PutObjectCommand({ + Bucket, + Key: key, + Body: bytes, + ContentType: "application/json; charset=utf-8", + CacheControl: cacheControl, + ...(condition && "ifMatch" in condition + ? { IfMatch: condition.ifMatch } + : {}), + ...(condition && "ifNoneMatch" in condition + ? { IfNoneMatch: condition.ifNoneMatch } + : {}), + }); + if (!condition) { + const res = await client.send(command); + return { etag: res.ETag ?? null }; + } + const started = performance.now(); + // Only a draft write is cut off ({@link CONDITIONAL_PUT_TIMEOUT_MS}). + const timed = DRAFT_KEY_RE.test(key); + const signal = timed ? AbortSignal.timeout(timeoutMs) : undefined; + let outcome = "ok"; try { - const res = await (condition ? conditionalClient : client).send( - new PutObjectCommand({ - Bucket, - Key: key, - Body: Buffer.from(body, "utf-8"), - ContentType: "application/json; charset=utf-8", - CacheControl: cacheControl, - ...(condition && "ifMatch" in condition - ? { IfMatch: condition.ifMatch } - : {}), - ...(condition && "ifNoneMatch" in condition - ? { IfNoneMatch: condition.ifNoneMatch } - : {}), - }), + const res = await conditionalClient.send( + command, + signal ? { abortSignal: signal } : {}, ); return { etag: res.ETag ?? null }; } catch (error) { - if (condition && isConditionFailed(error)) { + if (isConditionFailed(error)) { + outcome = "changed"; throw new ObjectChangedError(key); } - if (condition && isUnavailable(error)) { + if (signal?.aborted) { + // A PUT given up on may still land: like any Unavailable answer, + // it is neither a lost race nor a success (contract §2.6). + outcome = "timeout"; + throw new DeliveryUnavailableError( + key, + new Error(`no answer within ${timeoutMs} ms`), + ); + } + if (isUnavailable(error)) { + outcome = "unavailable"; throw new DeliveryUnavailableError(key, error); } + outcome = "error"; throw error; + } finally { + const durationMs = Math.round(performance.now() - started); + if ( + outcome === "timeout" || + outcome === "unavailable" || + durationMs >= slowMs + ) { + warn("hosted delivery: slow or failed conditional write", { + key: loggableKey(key), + outcome, + durationMs, + bytes: bytes.byteLength, + timeoutMs: timed ? timeoutMs : null, + }); + } } }; function get(key: string): Promise; diff --git a/apps/web/src/components/sections-editor/content-protocol-api.ts b/apps/web/src/components/sections-editor/content-protocol-api.ts index bb70b7470d..f398cf95d7 100644 --- a/apps/web/src/components/sections-editor/content-protocol-api.ts +++ b/apps/web/src/components/sections-editor/content-protocol-api.ts @@ -34,7 +34,7 @@ import { } from "@decocms/shared/blocks-major"; import { KEYS } from "@/lib/query-keys"; import { useVirtualMCP } from "@/sdk/hooks/use-virtual-mcp"; -import { cmsSiteOf, ensureCmsDraft } from "./cms-api"; +import { cmsBlockVersion, cmsSiteOf, ensureCmsDraft } from "./cms-api"; import { rememberConflictEditor } from "./conflict-editor"; import { keepContentBookkeeping } from "./content-bookkeeping"; import { @@ -902,6 +902,39 @@ function landedOf(error: unknown): Record | undefined { ); } +/** + * The names a hosted Conflict refused only because the draft already holds + * this very write: each refused name's actual version is the version of the + * body `part` sets for it (a tombstone, for a delete). Undefined when the + * error isn't a Conflict, or any refused name holds something else. + */ +async function ownLateWrites( + error: unknown, + part: DecofilePatchBody, +): Promise | undefined> { + if (!(error instanceof ContentProtocolError)) return undefined; + if (error.code !== ErrorCode.Conflict) return undefined; + const entries = (error.data as { entries?: unknown } | null)?.entries; + if (!entries || typeof entries !== "object") return undefined; + const refused = Object.entries(entries as Record); + if (refused.length === 0) return undefined; + const set = (part.set ?? {}) as Record; + const deletes = new Set(part.delete ?? []); + const own: Record = {}; + for (const [name, mismatch] of refused) { + const actual = (mismatch as { actual?: unknown } | null)?.actual; + if (typeof actual !== "string") return undefined; + const mine = Object.hasOwn(set, name) + ? await cmsBlockVersion(set[name]) + : deletes.has(name) + ? DRAFT_TOMBSTONE_VERSION + : undefined; + if (mine !== actual) return undefined; + own[name] = actual; + } + return own; +} + /** `patch` without the names that already landed. */ function withoutNames( patch: DecofilePatchBody, @@ -1003,14 +1036,17 @@ export async function applyProtocolPatch( kind: "edit", adopted: backend.source === "github", write: async () => { - const send = (part: DecofilePatchBody) => + const send = ( + part: DecofilePatchBody, + guards: Record | undefined = ifMatch, + ) => guarded(queryClient, backend, params, () => backend.client.blocksApply({ set: part.set as | Record> | undefined, delete: part.delete, - ...(ifMatch ? { ifMatch: guardsOf(ifMatch, part) } : {}), + ...(guards ? { ifMatch: guardsOf(guards, part) } : {}), }), ); let result: BlocksApplyResult; @@ -1038,7 +1074,22 @@ export async function applyProtocolPatch( if (landedAgain && Object.keys(landedAgain).length > 0) { adoptLanded(queryClient, params, cacheKey, rest, landedAgain); } - throw again; + // The retry was refused only because blocks already hold the + // very bodies it sends: the first try's write was given up on + // but landed late. Those count as landed; the rest is sent + // once more, guarded on them as they now are. + const own = await ownLateWrites(again, rest); + if (!own) throw again; + adoptLanded(queryClient, params, cacheKey, rest, own); + try { + retried = await send(rest, { ...ifMatch, ...own }); + } catch (last) { + const landedLast = landedOf(last); + if (landedLast && Object.keys(landedLast).length > 0) { + adoptLanded(queryClient, params, cacheKey, rest, landedLast); + } + throw last; + } } result = { ...retried, diff --git a/apps/web/src/components/sections-editor/hosted-content.test.ts b/apps/web/src/components/sections-editor/hosted-content.test.ts index 2005d0f433..3e1d2a4623 100644 --- a/apps/web/src/components/sections-editor/hosted-content.test.ts +++ b/apps/web/src/components/sections-editor/hosted-content.test.ts @@ -31,6 +31,7 @@ import { loadBlockBody, stubNames, } from "./hosted-content"; +import { cmsBlockVersion } from "./cms-api"; import { pagesShowingBlock, previewPageForBlock } from "./page-list"; import { findSiteSeoEntry } from "./seo-block"; @@ -824,3 +825,82 @@ describe("a refused save, then the poll (the conflict dialog is open)", () => { expect(epoch()).toBe(before + 1); }); }); + +describe("a retry refused by the first try's own late write", () => { + const home = { __resolveType: "page", path: "/" }; + const Header = { __resolveType: "site/sections/Header.tsx", title: "a" }; + /** + * The first try lands Header and gives up on home (Unavailable), but home's + * PUT still lands, late; the retry's guard (`home: null`) then meets it. + */ + const lateServer = (homeActual: () => Promise) => { + const server = hostedServer(); + const applied: Array<{ set?: string[]; ifMatch?: unknown }> = []; + server.backend.client.blocksApply = (async (patch: { + set?: Record; + ifMatch?: Record; + }) => { + applied.push({ + set: Object.keys(patch.set ?? {}), + ifMatch: structuredClone(patch.ifMatch), + }); + if (applied.length === 1) { + throw new ContentProtocolError(ErrorCode.Unavailable, "no answer", { + retryAfterMs: 250, + landed: { Header: "vHeader2" }, + }); + } + const actual = await homeActual(); + if (patch.ifMatch?.home !== actual) { + throw new ContentProtocolError( + ErrorCode.Conflict, + "a precondition failed", + { entries: { home: { expected: patch.ifMatch?.home, actual } } }, + ); + } + return { revision: "s-9", versions: { home: actual } }; + }) as unknown as ContentClient["blocksApply"]; + return { ...server, applied }; + }; + + it("counts a name holding this very body as landed and resends it guarded on that version", async () => { + const queryClient = new QueryClient(); + const own = await cmsBlockVersion(home); + const { backend, applied } = lateServer(async () => own); + await pollProtocolContent(queryClient, backend, params, CACHE_KEY); + const result = await applyProtocolPatch( + queryClient, + backend, + params, + CACHE_KEY, + { set: { Header, home } }, + { Header: null, home: null }, + ); + expect(applied).toEqual([ + { set: ["Header", "home"], ifMatch: { Header: null, home: null } }, + { set: ["home"], ifMatch: { home: null } }, + { set: ["home"], ifMatch: { home: own } }, + ]); + expect(result.versions).toEqual({ Header: "vHeader2", home: own }); + expect(protocolBlockVersion(queryClient, CACHE_KEY, "home", backend)).toBe( + own, + ); + }); + + it("a name holding another body is a real conflict: nothing more is sent", async () => { + const queryClient = new QueryClient(); + const { backend, applied } = lateServer(async () => "vSomeoneElse"); + await pollProtocolContent(queryClient, backend, params, CACHE_KEY); + await expect( + applyProtocolPatch( + queryClient, + backend, + params, + CACHE_KEY, + { set: { Header, home } }, + { Header: null, home: null }, + ), + ).rejects.toThrow("a precondition failed"); + expect(applied).toHaveLength(2); + }); +});