fix(webhook): tear down push-path doc delete surfaces independently [webhook, docs, tests]

docs/0-requirements.ja.md

@@ -114,6 +114,8 @@ Responsibilities:
 
 `push` はリポジトリ内の全 `.md` ファイルの変更検出に使う。同じ `push` event から per-commit diff も index する（1 commit × N files → N vector、各 vector は commit message + file path + patch を embedding input にする）。これにより削除済みファイルや非 `.md` 拡張子の判断履歴も semantic 検索可能になる。
 
+**`push` 経路の doc 削除**は cron reap と同じ 3 面を teardown する — Vectorize / D1 FTS5 / structured store — それぞれ独立に実行するので、1 面の失敗が他を取り残すことはない（issue #206）。graph edge を teardown しないのも cron reap と同じ理由で、doc vector ID が `doc_edges` の端点になりえないため。cron reap と違い per-run の削除枠は持たない。push payload が自分の削除件数を持っており、蓄積した backlog ではなく event で上限が決まるからである。レスポンスの `deleted` は 3 面すべてが落ちた doc を数えるので、`removed - deleted` が部分失敗の件数として delivery log から読める。cron reap 側の counter は代わりに**試行数**を数えるが、あちらではその counter が ETag hold を制御する budget counter を兼ねているためである。
+
 ### 3. Cron Poller
 
 cron poller は fallback path である。

docs/0-requirements.md

@@ -115,6 +115,8 @@ Responsibilities:
 
 `push` is used to detect changes in all `.md` files across the repository. The same `push` events also drive per-commit diff indexing: each commit produces N vectors (one per file with a textual patch), with embedding input being `commit message + file path + patch`. This surface makes deleted files and non-`.md` extensions searchable as judgment history.
 
+**Doc deletions on the `push` path** tear down the same three surfaces the cron reap does — Vectorize, D1 FTS5, and the structured store — each independently, so one failing surface cannot strand the others (issue #206). Graph edges are again not torn down, for the same reason as the cron reap: a doc vector ID is never a `doc_edges` endpoint. Unlike the cron reap there is no per-run deletion cap: a push payload names its own removals, so the loop is bounded by the event rather than by an accumulated backlog. The `deleted` count in the response body counts docs whose three surfaces all came down, so `removed - deleted` is the partial-teardown count visible in the delivery log; the cron reap's counter instead counts attempts, because there it doubles as the budget counter that gates the ETag hold.
+
 ### 3. Cron Poller
 
 The cron poller is the fallback path.

src/webhook-push-docs.test.ts

@@ -0,0 +1,172 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import type { Env } from "./types.js";
+
+// `handlePushEvent` fans out to the embed pipeline, the commit-diff pipeline,
+// Vectorize, and the D1 FTS teardown helper. What is under test here is the
+// *doc delete* fan-out, so the FTS teardown helper is replaced with a
+// controllable fake and Vectorize / the Store DO get in-memory stand-ins.
+// `docVectorId` and the rest of `./pipeline.js` stay real. The payloads below
+// carry commits without an `id`, so the diff-indexing branch short-circuits and
+// no HTTP call is made — global fetch is deliberately left unstubbed, and a
+// test that started making one would fail loudly rather than hit the network.
+const { deleteFtsRowMock } = vi.hoisted(() => ({ deleteFtsRowMock: vi.fn() }));
+
+vi.mock("./fts.js", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("./fts.js")>();
+  return { ...actual, deleteFtsRow: deleteFtsRowMock };
+});
+
+const { handlePushEvent } = await import("./webhook.js");
+const { docVectorId } = await import("./pipeline.js");
+
+const REPO = "acme/widgets";
+
+/** A default-branch push whose commits only remove files. `id` is omitted so
+ *  the per-commit diff indexing branch is skipped. */
+function pushPayload(removed: string[]): Record<string, unknown> {
+  return {
+    repository: { full_name: REPO, default_branch: "main" },
+    ref: "refs/heads/main",
+    head_commit: { id: "headsha" },
+    commits: [{ added: [], modified: [], removed }],
+  };
+}
+
+/** In-memory IssueStore stand-in covering the per-path DELETE the reap issues. */
+function makeDocStore() {
+  const deletes: string[] = [];
+  const stub = {
+    async fetch(request: Request): Promise<Response> {
+      const url = new URL(request.url);
+      if (request.method === "DELETE" && url.pathname === "/doc") {
+        deletes.push(url.searchParams.get("path") ?? "");
+        return new Response("ok");
+      }
+      return new Response("ok");
+    },
+  };
+  return { stub: stub as unknown as DurableObjectStub, deletes };
+}
+
+function makeDocEnv() {
+  const vectorDeletes: string[] = [];
+  const deleteByIds = vi.fn(async (ids: string[]) => {
+    vectorDeletes.push(...ids);
+  });
+  const env = {
+    GITHUB_TOKEN: "test-token",
+    VECTORIZE: { deleteByIds },
+    DB_FTS: {} as unknown,
+  } as unknown as Env;
+  return { env, vectorDeletes };
+}
+
+/** Read back the `docs` block of the 202 the handler returns. */
+async function docsResult(response: Response) {
+  const body = (await response.json()) as {
+    docs: { removed: number; deleted: number; failed: number };
+  };
+  return body.docs;
+}
+
+beforeEach(() => {
+  deleteFtsRowMock.mockReset().mockResolvedValue(undefined);
+});
+
+describe("webhook: push doc delete fan-out", () => {
+  it("tears down all three surfaces for a removed doc", async () => {
+    const store = makeDocStore();
+    const { env, vectorDeletes } = makeDocEnv();
+
+    const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, store.stub);
+
+    const goneId = await docVectorId(REPO, "docs/gone.md");
+    expect(vectorDeletes).toEqual([goneId]);
+    expect(deleteFtsRowMock).toHaveBeenCalledTimes(1);
+    expect(deleteFtsRowMock.mock.calls[0][1]).toBe(goneId);
+    expect(store.deletes).toEqual(["docs/gone.md"]);
+    expect(await docsResult(res)).toMatchObject({ removed: 1, deleted: 1 });
+  });
+
+  it("keeps tearing down the later surfaces when Vectorize fails", async () => {
+    // The defect this issue was filed on: one outer try wrapped the whole item,
+    // so a Vectorize throw jumped straight to the catch and neither the FTS5
+    // row nor the store row was ever touched. The D1 rows are the ones users
+    // actually retrieve, so the stale doc kept coming back in search results.
+    const store = makeDocStore();
+    const { env } = makeDocEnv();
+    (env.VECTORIZE.deleteByIds as ReturnType<typeof vi.fn>).mockRejectedValue(
+      new Error("vectorize down"),
+    );
+
+    const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, store.stub);
+
+    expect(deleteFtsRowMock).toHaveBeenCalledTimes(1);
+    expect(store.deletes).toEqual(["docs/gone.md"]);
+    // `deleted` counts docs whose three surfaces all came down, so a partial
+    // teardown is visible as `removed > deleted` in the delivery-log body.
+    // The cron reap counts attempts instead; see the comment on the loop.
+    expect(await docsResult(res)).toMatchObject({ removed: 1, deleted: 0 });
+  });
+
+  it("keeps tearing down the store record when the FTS5 delete fails", async () => {
+    const store = makeDocStore();
+    const { env, vectorDeletes } = makeDocEnv();
+    deleteFtsRowMock.mockRejectedValue(new Error("d1 down"));
+
+    const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, store.stub);
+
+    expect(vectorDeletes).toHaveLength(1);
+    expect(store.deletes).toEqual(["docs/gone.md"]);
+    expect(await docsResult(res)).toMatchObject({ removed: 1, deleted: 0 });
+  });
+
+  it("keeps tearing down the other surfaces when the store DELETE fails", async () => {
+    const { env, vectorDeletes } = makeDocEnv();
+    const stub = {
+      async fetch(): Promise<Response> {
+        throw new Error("store down");
+      },
+    } as unknown as DurableObjectStub;
+
+    const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, stub);
+
+    expect(vectorDeletes).toHaveLength(1);
+    expect(deleteFtsRowMock).toHaveBeenCalledTimes(1);
+    expect(await docsResult(res)).toMatchObject({ removed: 1, deleted: 0 });
+  });
+
+  it("keeps reaping later docs after one of them fails", async () => {
+    // One failing surface must not abort the loop for the rest of the push.
+    const store = makeDocStore();
+    const { env, vectorDeletes } = makeDocEnv();
+    const firstId = await docVectorId(REPO, "docs/a.md");
+    (env.VECTORIZE.deleteByIds as ReturnType<typeof vi.fn>).mockImplementation(
+      async (ids: string[]) => {
+        if (ids[0] === firstId) throw new Error("vectorize down");
+        vectorDeletes.push(...ids);
+      },
+    );
+
+    const res = await handlePushEvent(
+      pushPayload(["docs/a.md", "docs/b.md"]),
+      env,
+      store.stub,
+    );
+
+    expect(store.deletes).toEqual(["docs/a.md", "docs/b.md"]);
+    expect(vectorDeletes).toEqual([await docVectorId(REPO, "docs/b.md")]);
+    expect(await docsResult(res)).toMatchObject({ removed: 2, deleted: 1 });
+  });
+
+  it("leaves the reap alone when the push removed no docs", async () => {
+    const store = makeDocStore();
+    const { env, vectorDeletes } = makeDocEnv();
+
+    await handlePushEvent(pushPayload(["src/main.ts"]), env, store.stub);
+
+    expect(store.deletes).toEqual([]);
+    expect(vectorDeletes).toEqual([]);
+    expect(deleteFtsRowMock).not.toHaveBeenCalled();
+  });
+});

src/webhook.ts

@@ -354,9 +354,13 @@ async function handleReleaseEvent(
  *
  * Only processes pushes to the default branch.
  * Filters commit file lists for `docs/**\/*.md` and `README.md`.
- * Added/modified files are fetched and embedded; removed files are deleted.
+ * Added/modified files are fetched and embedded; removed files are deleted from
+ * Vectorize, D1 FTS5, and the structured store — each surface independently, so
+ * one failing surface cannot strand the others (issue #206).
+ *
+ * Exported for tests; production callers reach it through `handleWebhook`.
  */
-async function handlePushEvent(
+export async function handlePushEvent(
   payload: Record<string, unknown>,
   env: Env,
   storeStub: DurableObjectStub,
@@ -486,32 +490,59 @@ async function handlePushEvent(
   }
 
   // Delete removed doc files from Vectorize, D1 FTS5, and the structured store.
+  // Each surface is torn down independently, matching the cron reap in
+  // `pollDocs`. Previously one outer try wrapped the whole item: a Vectorize
+  // failure jumped straight to the catch, so the store row (and the FTS5 row
+  // with it) stayed behind and the deleted doc kept surfacing in search — the
+  // D1 rows are the ones users actually retrieve (issue #206).
+  //
+  // No graph-edge teardown here, same as the cron reap: both endpoints of every
+  // `doc_edges` row are wiki vector IDs (`indexWikiEdges` is the only writer,
+  // and the dst ID it computes is a `wikiDocVectorId` too), so a doc vector ID
+  // is never an edge endpoint. Add the teardown if that invariant changes
+  // (issue #203).
+  //
+  // No per-run cap either: a push payload names its own removals, so this loop
+  // is bounded by the event rather than by the accumulated backlog the cron reap
+  // walks (issue #206).
   let deleted = 0;
   for (const path of removed) {
-    try {
-      const dvid = await docVectorId(repo, path);
-      await env.VECTORIZE.deleteByIds([dvid]);
+    const dvid = await docVectorId(repo, path);
+    let allSurfacesTornDown = true;
+
+    for (const [surface, run] of [
+      ["vector", () => env.VECTORIZE.deleteByIds([dvid])],
+      ["FTS5 row", () => deleteFtsRow(env.DB_FTS, dvid)],
+      [
+        "store record",
+        () =>
+          storeStub.fetch(
+            new Request(
+              `http://store/doc?repo=${encodeURIComponent(repo)}&path=${encodeURIComponent(path)}`,
+              { method: "DELETE" },
+            ),
+          ),
+      ],
+    ] as Array<[string, () => Promise<unknown>]>) {
       try {
-        await deleteFtsRow(env.DB_FTS, dvid);
-      } catch (ftsErr) {
+        await run();
+      } catch (err) {
+        allSurfacesTornDown = false;
         console.error(
-          `Webhook: failed to delete FTS5 row for doc ${repo}/${path}:`,
-          ftsErr instanceof Error ? ftsErr.message : String(ftsErr),
+          `Webhook: failed to delete ${surface} for doc ${repo}/${path}:`,
+          err instanceof Error ? err.message : String(err),
         );
       }
-      await storeStub.fetch(
-        new Request(
-          `http://store/doc?repo=${encodeURIComponent(repo)}&path=${encodeURIComponent(path)}`,
-          { method: "DELETE" },
-        ),
-      );
-      deleted++;
-    } catch (err) {
-      console.error(
-        `Webhook: failed to delete doc vector ${repo}/${path}:`,
-        err instanceof Error ? err.message : String(err),
-      );
     }
+
+    // `deleted` counts docs whose three surfaces all came down, so
+    // `removed - deleted` is the partial-teardown count. The cron reap's
+    // counter instead counts *attempts*, because there it is a budget counter
+    // that also gates the ETag hold. There is no budget here, so counting
+    // attempts would only restate `removed` — already in the same response —
+    // and would hide partial failures from the delivery-log body, the cheapest
+    // place an operator sees them.
+    if (allSurfacesTornDown) deleted++;
   }
 
   return jsonResponse(202, {