fix(web): make the draft claim best effort on get/put, not KV list

Workers KV list results can lag writes by up to 60 seconds, even in the
location that wrote them, so the list-based claim could 409 every post
from a stale listing (and still let two stale listings both proceed).
The claim is now one key holding the request's token: refuse if a claim
is visible, write the token, proceed only if reading it back returns our
token and no decision marker exists. A read-back that misses our own
write answers 503 (retryable) instead of a lost race. Release deletes
the key only while it still holds our token. The code comment no longer
claims exclusivity: KV has no compare-and-set, so simultaneous or
cross-location requests can still both proceed; a Durable Object would
be needed for that.

A repeated discard (marker already "discarded", seen by the route or by
the claim) is now a 200 no-op instead of a 409.

Tests: vitest community-agent-review-state 34 passed (4 new; all 4 fail
against 989081cb8); community-agent + content-watch + public-api-security
5 files, 64 passed. tsc --noEmit exit 0. eslint on touched files exit 0.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_014ZwqatxgVFxHvovngywnks

web/app/api/admin/post/route.ts

@@ -16,7 +16,7 @@ import {
   reviewedBodyHash,
   validateSession,
   type CommunityAgentEnv,
-  type DraftClaim,
+  type DraftClaimResult,
 } from "@/lib/community-agent";
 
 export const dynamic = "force-dynamic";
@@ -65,6 +65,22 @@ function githubDefinitelyRejected(status: number): boolean {
   return status >= 400 && status < 500 && status !== 408 && status !== 429;
 }
 
+/** The answer for a claim that was not taken. */
+function claimRefused(result: Exclude<DraftClaimResult, { ok: true }>) {
+  if (result.reason === "unconfirmed") {
+    return NextResponse.json({ error: "could not confirm the draft claim; nothing was done, retry" }, { status: 503 });
+  }
+  if (result.reason === "resolved") {
+    return NextResponse.json({ error: `draft already ${result.resolution.state}` }, { status: 409 });
+  }
+  return NextResponse.json(
+    { error: "another request is acting on this draft; check GitHub, then retry in a minute" },
+    { status: 409 }
+  );
+}
+
+const discarded = () => NextResponse.json({ ok: true, action: "discarded" });
+
 export async function POST(req: Request) {
   const env = await getAgentEnv();
 
@@ -149,17 +165,21 @@ export async function POST(req: Request) {
     if (draft.posted || resolution?.state === "posted" || resolution?.state === "posting") {
       return NextResponse.json({ error: "draft already posted" }, { status: 409 });
     }
+    // A repeated discard (double click) is a no-op, not an error.
+    if (resolution?.state === "discarded") return discarded();
     // Hold the same claim a post takes, so a discard and a post of one draft
-    // cannot both run.
-    let claim: DraftClaim | null = null;
+    // do not both run.
+    let result: DraftClaimResult;
     try {
-      claim = await claimDraft(env.CURATED_KV, parsedKey.type, parsedKey.id);
+      result = await claimDraft(env.CURATED_KV, parsedKey.type, parsedKey.id);
     } catch (e) {
       return NextResponse.json({ error: `could not claim draft: ${String(e)}` }, { status: 500 });
     }
-    if (!claim) {
-      return NextResponse.json({ error: "draft is being posted or was already resolved" }, { status: 409 });
+    if (!result.ok) {
+      if (result.reason === "resolved" && result.resolution.state === "discarded") return discarded();
+      return claimRefused(result);
     }
+    const claim = result.claim;
     try {
       // The marker stops the next cron run from regenerating this draft.
       await markDraftResolved(env.CURATED_KV, parsedKey.type, parsedKey.id, "discarded");
@@ -174,7 +194,7 @@ export async function POST(req: Request) {
       await releaseDraftClaim(env.CURATED_KV, claim).catch(() => undefined);
     }
     if (draft.type === "digest") revalidateDigestPage();
-    return NextResponse.json({ ok: true, action: "discarded" });
+    return discarded();
   }
 
   if (action === "post") {
@@ -195,19 +215,21 @@ export async function POST(req: Request) {
     if (resolution) {
       return NextResponse.json({ error: `draft already ${resolution.state}` }, { status: 409 });
     }
-    let claim: DraftClaim | null = null;
+    let result: DraftClaimResult;
+    try {
+      result = await claimDraft(env.CURATED_KV, parsedKey.type, parsedKey.id);
+    } catch (e) {
+      return NextResponse.json({ error: `could not claim draft: ${String(e)}` }, { status: 500 });
+    }
+    if (!result.ok) return claimRefused(result);
+    const heldClaim = result.claim;
     try {
-      claim = await claimDraft(env.CURATED_KV, parsedKey.type, parsedKey.id);
-      if (!claim) {
-        return NextResponse.json({ error: "draft already posting or resolved" }, { status: 409 });
-      }
       // Keeps the cron from regenerating the draft while the post is in flight.
       await markDraftResolved(env.CURATED_KV, parsedKey.type, parsedKey.id, "posting");
     } catch (e) {
-      if (claim) await releaseDraftClaim(env.CURATED_KV, claim).catch(() => undefined);
+      await releaseDraftClaim(env.CURATED_KV, heldClaim).catch(() => undefined);
       return NextResponse.json({ error: `could not claim draft: ${String(e)}` }, { status: 500 });
     }
-    const heldClaim = claim;
 
     const commentBody = editedBody ?? originalBody;
 

web/lib/community-agent-review-state.test.ts

@@ -537,7 +537,8 @@ describe("admin post action is idempotent and bounded", () => {
       act(kv, { action: "post", draftKey: "draft:triage:42" }),
     ]);
     const statuses = results.map((r) => r.status);
-    // Overlapping claims may both refuse (fail closed), never both proceed.
+    // This fake KV makes every write visible at once, so the claim is exact
+    // here; on Workers KV it is best effort (see claimDraft).
     expect(statuses.filter((s) => s === 200)).toHaveLength(posts.length);
     expect(posts.length).toBeLessThanOrEqual(1);
     expect(statuses.every((s) => s === 200 || s === 409)).toBe(true);
@@ -570,8 +571,6 @@ describe("admin post action is idempotent and bounded", () => {
     };
     let claimPuts = 0;
     kv.beforePut = async (key, value) => {
-      // A claim write: its own key, or (the earlier design) a token written
-      // over the shared marker.
       const isClaim = key.startsWith("draft-claim:") || (key.startsWith("draft-resolved:") && value.includes('"claim"'));
       if (!isClaim) return;
       claimPuts += 1;
@@ -623,6 +622,92 @@ describe("admin post action is idempotent and bounded", () => {
     expect(JSON.parse(kv.values.get("draft:triage:42")!)).toMatchObject({ posted: true });
   });
 
+  it("claims without KV list, whose results can lag writes by up to a minute", async () => {
+    const kv = new FakeKv();
+    await saveDraft(kv, draft());
+    useAdminEnv(kv);
+    const posts = stubGitHub();
+    // A stale listing that never shows claim keys must not affect the claim.
+    const realList = kv.list.bind(kv);
+    kv.list = async (options) => {
+      if (options?.prefix?.startsWith("draft-claim:")) throw new Error("claim must not list");
+      return realList(options);
+    };
+
+    const res = await act(kv, { action: "post", draftKey: "draft:triage:42" });
+    expect(res.status).toBe(200);
+    expect(posts).toHaveLength(1);
+    expect([...kv.values.keys()].filter((k) => k.startsWith("draft-claim:"))).toEqual([]);
+  });
+
+  it("answers a claim it cannot read back as retryable, not as a lost race, and the retry posts", async () => {
+    const kv = new FakeKv();
+    await saveDraft(kv, draft());
+    useAdminEnv(kv);
+    const posts = stubGitHub();
+    // The first read-back of the request's own claim misses the write.
+    const realGet = kv.get.bind(kv);
+    let claimReads = 0;
+    kv.get = async (key: string) => {
+      if (key.startsWith("draft-claim:") && ++claimReads === 2) return null;
+      return realGet(key);
+    };
+
+    const first = await act(kv, { action: "post", draftKey: "draft:triage:42" });
+    expect(first.status).toBe(503);
+    expect(posts).toEqual([]);
+    expect([...kv.values.keys()].filter((k) => k.startsWith("draft-claim:") || k.startsWith("draft-resolved:"))).toEqual([]);
+
+    const retry = await act(kv, { action: "post", draftKey: "draft:triage:42" });
+    expect(retry.status).toBe(200);
+    expect(posts).toHaveLength(1);
+  });
+
+  it("refuses a post while another request's claim is visible, and leaves that claim alone", async () => {
+    const kv = new FakeKv();
+    await saveDraft(kv, draft());
+    useAdminEnv(kv);
+    const posts = stubGitHub();
+    kv.values.set("draft-claim:triage:42", "other-request");
+
+    for (const action of ["post", "discard"]) {
+      const res = await act(kv, { action, draftKey: "draft:triage:42" });
+      expect(res.status).toBe(409);
+    }
+    expect(posts).toEqual([]);
+    expect(kv.values.get("draft-claim:triage:42")).toBe("other-request");
+  });
+
+  it("treats a repeated discard as a no-op, including one that sees the marker only at claim time", async () => {
+    const kv = new FakeKv();
+    await saveDraft(kv, draft());
+    useAdminEnv(kv);
+    stubGitHub();
+
+    const first = await act(kv, { action: "discard", draftKey: "draft:triage:42" });
+    expect(first.status).toBe(200);
+    // A double click whose request still reads the draft (its delete not yet
+    // visible), first past the marker, then with the marker missed by the
+    // route's own check and seen only by the claim.
+    const staleDraft = JSON.stringify(draft());
+    kv.values.set("draft:triage:42", staleDraft);
+    const again = await act(kv, { action: "discard", draftKey: "draft:triage:42" });
+    expect(again.status).toBe(200);
+    await expect(again.json()).resolves.toEqual({ ok: true, action: "discarded" });
+
+    kv.values.set("draft:triage:42", staleDraft);
+    const realGet = kv.get.bind(kv);
+    let markerReads = 0;
+    kv.get = async (key: string) => {
+      if (key.startsWith("draft-resolved:") && ++markerReads === 1) return null;
+      return realGet(key);
+    };
+    const racing = await act(kv, { action: "discard", draftKey: "draft:triage:42" });
+    expect(racing.status).toBe(200);
+    expect(JSON.parse(kv.values.get("draft-resolved:triage:42")!)).toMatchObject({ state: "discarded" });
+    expect([...kv.values.keys()].filter((k) => k.startsWith("draft-claim:"))).toEqual([]);
+  });
+
   it("refuses an action on a draft regenerated after the page loaded, before calling GitHub", async () => {
     const kv = new FakeKv();
     await saveDraft(kv, draft({ bodyEn: "reviewed A" }));

web/lib/community-agent.ts

@@ -382,64 +382,88 @@ export async function markDraftResolved(
 }
 
 /**
- * An exclusive hold on one draft identity while a maintainer action (post or
- * discard) runs. Each request writes its own claim key, so no request can
- * overwrite another's claim, and proceeds only if its key is the only one
- * under the draft's claim prefix.
+ * A hold on one draft identity while a maintainer action (post or discard)
+ * runs. It is a single KV key holding the claiming request's random token.
  */
 export interface DraftClaim {
   key: string;
-}
-
-const CLAIM_PREFIX = "draft-claim:";
-
-function claimPrefix(type: AgentDraftType, id: string): string {
-  return `${CLAIM_PREFIX}${draftKey(type, id).slice("draft:".length)}:`;
+  token: string;
 }
 
 /**
- * Claim a draft before acting on it. Returns null when another request holds
- * a claim, or when a decision (posted, discarded, a post in flight) was
- * recorded before this claim.
+ * Why a claim was not taken:
+ * - `held`: another request's claim is visible (a post in flight, or one whose
+ *   outcome is unknown, which holds the draft until the claim expires).
+ * - `resolved`: a decision (posted, discarded, a post in flight) exists.
+ * - `unconfirmed`: this request could not read its own claim back; nothing
+ *   was done and a retry is safe.
+ */
+export type DraftClaimResult =
+  | { ok: true; claim: DraftClaim }
+  | { ok: false; reason: "held" | "unconfirmed" }
+  | { ok: false; reason: "resolved"; resolution: DraftResolution };
+
+const CLAIM_PREFIX = "draft-claim:";
+
+function claimKey(type: AgentDraftType, id: string): string {
+  return CLAIM_PREFIX + draftKey(type, id).slice("draft:".length);
+}
+
+/**
+ * Claim a draft before acting on it.
  *
- * Why this is exclusive: two overlapping claimants each write their key before
- * they list. Whichever lists second sees both keys, so at most one sees only
- * its own. If they overlap exactly, both see two keys and both refuse (the
- * maintainer retries); two can never both proceed. A claimant that arrives
- * after the winner still sees the winner's key, which is kept until it
- * expires unless the winner's action definitely did not happen.
+ * This is best effort, not a lock. Workers KV has no compare-and-set: a
+ * write is usually visible at once to reads in the location that made it,
+ * but can take 60 seconds or more to reach other locations, and list results
+ * can lag even locally. So the claim uses only get and put on one key:
+ * refuse if a claim is already visible, write our token, and proceed only if
+ * reading the key back returns our token and no decision has been recorded.
+ * That stops the double-post cases this admin page actually produces (a
+ * second tab, a retry, a discard during a post), since each re-reads a key
+ * that a finished request wrote. Two requests whose writes land within the
+ * same moment, or that run in different locations, can still both proceed;
+ * a Durable Object would be needed to rule that out.
  *
- * This holds when a list reflects every put that finished before it. Workers
- * KV guarantees that within one location, not across locations (its writes
- * can take up to 60 seconds to reach other locations), so two maintainers
- * posting the same draft from different regions in the same instant are not
- * serialized. A real compare-and-set authority (a Durable Object) would be
- * needed for that; this admin surface has one maintainer.
+ * A released claim can also stay visible to a retry from another location
+ * for up to about a minute, which surfaces as `held`.
  */
 export async function claimDraft(
   kv: KVNamespace | undefined,
   type: AgentDraftType,
   id: string
-): Promise<DraftClaim | null> {
-  if (!kv) return { key: "" };
-  const prefix = claimPrefix(type, id);
-  const key = prefix + crypto.randomUUID();
-  await kv.put(key, new Date().toISOString(), { expirationTtl: POSTING_CLAIM_TTL_SEC });
-  let won = false;
-  try {
-    const { keys } = await kv.list({ prefix });
-    won = keys.length === 1 && keys[0].name === key;
-    // A decision recorded after the caller's own check still wins.
-    if (won && (await getDraftResolution(kv, type, id))) won = false;
-  } finally {
-    if (!won) await kv.delete(key).catch(() => undefined);
+): Promise<DraftClaimResult> {
+  const token = crypto.randomUUID();
+  if (!kv) return { ok: true, claim: { key: "", token } };
+  const key = claimKey(type, id);
+  if (await kv.get(key)) return { ok: false, reason: "held" };
+  await kv.put(key, token, { expirationTtl: POSTING_CLAIM_TTL_SEC });
+  const seen = await kv.get(key);
+  if (seen !== token) {
+    // Someone else's token is theirs to keep. Our own write not being visible
+    // is not a lost race: drop it so it cannot block the retry.
+    if (seen === null) {
+      await kv.delete(key).catch(() => undefined);
+      return { ok: false, reason: "unconfirmed" };
+    }
+    return { ok: false, reason: "held" };
   }
-  return won ? { key } : null;
+  // A decision recorded after the caller's own check still wins.
+  const resolution = await getDraftResolution(kv, type, id).catch(async (e) => {
+    await kv.delete(key).catch(() => undefined);
+    throw e;
+  });
+  if (resolution) {
+    await kv.delete(key).catch(() => undefined);
+    return { ok: false, reason: "resolved", resolution };
+  }
+  return { ok: true, claim: { key, token } };
 }
 
-/** Give up a claim, for an action that definitely did not happen. */
+/** Give up a claim, for an action that definitely did not happen or is recorded. */
 export async function releaseDraftClaim(kv: KVNamespace | undefined, claim: DraftClaim): Promise<void> {
   if (!kv || !claim.key) return;
+  // Leave a claim another request has since written in place.
+  if ((await kv.get(claim.key)) !== claim.token) return;
   await kv.delete(claim.key);
 }