fix(cms): discard empties the draft index first; time out slow R2 draft writes

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);

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<boolean>;
 }
@@ -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);

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) => {

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;