From 3bfa4a625a8fd3608c5815570d1cbe32c63f9a30 Mon Sep 17 00:00:00 2001 From: gimenes Date: Tue, 10 Nov 2026 15:44:58 -0300 Subject: [PATCH 1/5] fix(cms): discard empties the draft index before deleting its files A discard deleted every listed draft file, then the index. A reader (the SDK through the CDN) that read the index in between met listed names whose files were gone, and a CDN may hold those 404s. Now the index is emptied ({ names: [] }) first, then the files it listed are deleted, then the index, then the row. The names come from the emptying write itself, so a name a save listed between the read and that write is deleted too. A failed delete still keeps the row. Co-Authored-By: Claude Opus 5.5 --- apps/api/src/hosted/cms-drafts.test.ts | 61 +++++++++++++++++++++++++- apps/api/src/hosted/cms-drafts.ts | 28 ++++++++++-- 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/apps/api/src/hosted/cms-drafts.test.ts b/apps/api/src/hosted/cms-drafts.test.ts index bc2c32fa43..78e54decfd 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,64 @@ 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("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..85d36b7873 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, 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,11 @@ 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 (a + * file whose delete failed is then unlisted: an orphan nobody reads). */ discard(site: string, name: string): Promise; } @@ -456,7 +461,22 @@ 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) { + // 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 listed of names) { await store.delete(deliveryKeys.draftFile(site, row.slug, listed)); } await store.delete(deliveryKeys.draftIndex(site, row.slug)); From 9b272bad49f05336564f99ea803ddcb490992983 Mon Sep 17 00:00:00 2001 From: gimenes Date: Tue, 10 Nov 2026 15:46:51 -0300 Subject: [PATCH 2/5] fix(cms): time out and log slow R2 draft writes Conditional PUTs go through an S3 client with the SDK retries off and no request timeout, so a stalled R2 PUT held the save open until R2 answered (one sat ~25 s before landing), and nothing logged it. Each conditional PUT now gives up after 15 s (CONDITIONAL_PUT_TIMEOUT_MS) and throws DeliveryUnavailableError, the Unavailable answer a save already turns into PartialApplyError (-32008 with what landed) and the editor retries. 15 s is far above R2 latency for a draft-sized body and still fits the largest block an 8 MiB request can carry on a slow link. A conditional PUT that times out, answers 429/5xx, or takes 2 s or more is logged through console.warn (exported as OTel logs) with its outcome, duration, size and key, the draft slug hidden since it is the draft read grant. Save order is unchanged: draft index first, then files. Co-Authored-By: Claude Opus 5.5 --- apps/api/src/hosted/cms-storage.test.ts | 42 ++++++++ apps/api/src/hosted/delivery-store-s3.test.ts | 74 +++++++++++++- apps/api/src/hosted/delivery-store.ts | 99 +++++++++++++++---- 3 files changed, 197 insertions(+), 18 deletions(-) diff --git a/apps/api/src/hosted/cms-storage.test.ts b/apps/api/src/hosted/cms-storage.test.ts index eee1cbeacb..70256316eb 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,47 @@ 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("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/delivery-store-s3.test.ts b/apps/api/src/hosted/delivery-store-s3.test.ts index c5ae6986a7..494a955b38 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,71 @@ 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 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..47c7863261 100644 --- a/apps/api/src/hosted/delivery-store.ts +++ b/apps/api/src/hosted/delivery-store.ts @@ -246,6 +246,26 @@ function isUnavailable(error: unknown): boolean { return status === undefined || status === 429 || status >= 500; } +/** + * How long one conditional PUT 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 body a save can send is one block of + * a request capped at 8 MiB, which needs ~13 s at a poor 5 Mbit/s uplink, + * so 15 s never cuts off a healthy 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 (and every write queued behind it) open. + */ +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; + +/** 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(/\/drafts\/[A-Za-z0-9_-]{22}\//, "/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 +281,12 @@ export function createS3DeliveryStore(config: { bucket: string; accessKeyId: string; secretAccessKey: string; + /** {@link CONDITIONAL_PUT_TIMEOUT_MS} unless set (tests). */ + 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 +304,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 +314,67 @@ 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(); + const signal = AbortSignal.timeout(timeoutMs); + 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, { + 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, + }); + } } }; function get(key: string): Promise; From 40d41cbcf3864568d860a6f3ce6dae20b0f061f4 Mon Sep 17 00:00:00 2001 From: gimenes Date: Tue, 10 Nov 2026 16:00:25 -0300 Subject: [PATCH 3/5] fix(cms): a discard whose file delete fails relists what it left Discard empties the draft index before it deletes the files. When one delete fails, the names not yet deleted are put back into the index (best effort) before the error is rethrown. The files left behind stay listed, so a later save guards against them and a retried discard deletes them. Without this they were hidden files that a save with ifMatch null hit as a conflict. Co-Authored-By: Claude Opus 5.5 --- apps/api/src/hosted/cms-drafts.test.ts | 36 ++++++++++++++++++++++++++ apps/api/src/hosted/cms-drafts.ts | 26 ++++++++++++++----- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/apps/api/src/hosted/cms-drafts.test.ts b/apps/api/src/hosted/cms-drafts.test.ts index 78e54decfd..307aaf4926 100644 --- a/apps/api/src/hosted/cms-drafts.test.ts +++ b/apps/api/src/hosted/cms-drafts.test.ts @@ -152,6 +152,42 @@ describe("cms drafts", () => { 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({ diff --git a/apps/api/src/hosted/cms-drafts.ts b/apps/api/src/hosted/cms-drafts.ts index 85d36b7873..1270560e2d 100644 --- a/apps/api/src/hosted/cms-drafts.ts +++ b/apps/api/src/hosted/cms-drafts.ts @@ -14,8 +14,8 @@ * `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. A discard runs the other way: it empties the index first, then - * deletes the files it listed, so a reader never meets a listed name whose - * file is gone. Every write is conditional on the object as read + * 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. @@ -406,8 +406,9 @@ export interface CmsDrafts { * 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 (a - * file whose delete failed is then unlisted: an orphan nobody reads). + * 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; } @@ -476,8 +477,21 @@ export function createCmsDrafts(deps: { index, ); } - for (const listed of names) { - await store.delete(deliveryKeys.draftFile(site, row.slug, listed)); + 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); From 5f55c644b688872592792a2472ab0f80739c9295 Mon Sep 17 00:00:00 2001 From: gimenes Date: Tue, 10 Nov 2026 16:00:25 -0300 Subject: [PATCH 4/5] fix(cms): time out only draft PUTs, not publish writes The 15 s timeout is sized for draft bodies (one block of a save capped at 8 MiB). Publish writes go up to 16 MiB, and a publish that gave up on a latest.json PUT that then landed would leave the site live but not purged. So only keys under drafts// get the timeout. Every other conditional PUT keeps no timeout, as on main, and is only logged when slow or failed (timeoutMs null in the log). Co-Authored-By: Claude Opus 5.5 --- apps/api/src/hosted/delivery-store-s3.test.ts | 21 +++++++++ apps/api/src/hosted/delivery-store.ts | 43 ++++++++++++------- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/apps/api/src/hosted/delivery-store-s3.test.ts b/apps/api/src/hosted/delivery-store-s3.test.ts index 494a955b38..887e1dc266 100644 --- a/apps/api/src/hosted/delivery-store-s3.test.ts +++ b/apps/api/src/hosted/delivery-store-s3.test.ts @@ -214,6 +214,27 @@ describe("createS3DeliveryStore conditional PUT timeout", () => { 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( diff --git a/apps/api/src/hosted/delivery-store.ts b/apps/api/src/hosted/delivery-store.ts index 47c7863261..73cea00ff3 100644 --- a/apps/api/src/hosted/delivery-store.ts +++ b/apps/api/src/hosted/delivery-store.ts @@ -247,23 +247,31 @@ function isUnavailable(error: unknown): boolean { } /** - * How long one conditional PUT 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 body a save can send is one block of - * a request capped at 8 MiB, which needs ~13 s at a poor 5 Mbit/s uplink, - * so 15 s never cuts off a healthy 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 (and every write queued behind it) open. + * 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(/\/drafts\/[A-Za-z0-9_-]{22}\//, "/drafts//"); + return key.replace(DRAFT_KEY_RE, "/drafts//"); } function isNotModified(error: unknown): boolean { @@ -281,7 +289,7 @@ export function createS3DeliveryStore(config: { bucket: string; accessKeyId: string; secretAccessKey: string; - /** {@link CONDITIONAL_PUT_TIMEOUT_MS} unless set (tests). */ + /** {@link CONDITIONAL_PUT_TIMEOUT_MS} unless set (tests); draft keys only. */ conditionalTimeoutMs?: number; /** {@link SLOW_PUT_MS} unless set (tests). */ slowMs?: number; @@ -333,19 +341,22 @@ export function createS3DeliveryStore(config: { return { etag: res.ETag ?? null }; } const started = performance.now(); - const signal = AbortSignal.timeout(timeoutMs); + // 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 conditionalClient.send(command, { - abortSignal: signal, - }); + const res = await conditionalClient.send( + command, + signal ? { abortSignal: signal } : {}, + ); return { etag: res.ETag ?? null }; } catch (error) { if (isConditionFailed(error)) { outcome = "changed"; throw new ObjectChangedError(key); } - if (signal.aborted) { + 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"; @@ -372,7 +383,7 @@ export function createS3DeliveryStore(config: { outcome, durationMs, bytes: bytes.byteLength, - timeoutMs, + timeoutMs: timed ? timeoutMs : null, }); } } From d4d94957e2b40456707d8a6421f12709bc6a6a57 Mon Sep 17 00:00:00 2001 From: gimenes Date: Tue, 10 Nov 2026 16:00:25 -0300 Subject: [PATCH 5/5] fix(cms): a save meeting its own late write counts it as landed A draft PUT given up on after the timeout can still land. The editor retries the rest right away with the same guards, so the retry could meet the first try's own write and be refused. - Server: in blocks.apply, a 412 on a file that already holds the version being written is counted as landed, not a lost race. - Editor: when the retry is refused with a Conflict, and every refused block already holds the exact body this save sends (same version), those blocks are adopted as landed and the rest is sent once more, guarded on those versions. Any other conflict is shown as before. Co-Authored-By: Claude Opus 5.5 --- apps/api/src/hosted/cms-storage.test.ts | 40 ++++++++++ apps/api/src/hosted/cms-storage.ts | 26 ++++-- .../sections-editor/content-protocol-api.ts | 59 +++++++++++++- .../sections-editor/hosted-content.test.ts | 80 +++++++++++++++++++ 4 files changed, 194 insertions(+), 11 deletions(-) diff --git a/apps/api/src/hosted/cms-storage.test.ts b/apps/api/src/hosted/cms-storage.test.ts index 70256316eb..8dce571842 100644 --- a/apps/api/src/hosted/cms-storage.test.ts +++ b/apps/api/src/hosted/cms-storage.test.ts @@ -568,6 +568,46 @@ describe("blocks.apply (the save)", () => { ).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/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); + }); +});