From d30b5b776be9a0a57183413ddfb6a835db60e0e5 Mon Sep 17 00:00:00 2001 From: Claude Lin & Lay Date: Sun, 2 Aug 2026 21:30:15 +0900 Subject: [PATCH] fix(webhook): tear down push-path doc delete surfaces independently [webhook, docs, tests] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The push-path doc delete wrapped one removed file in a single outer try, so a Vectorize failure jumped to the catch and neither the D1 FTS5 row nor the store row was ever touched. The deleted doc kept surfacing in search — the D1 rows are the ones users actually retrieve. Every other delete path in this file already split its surfaces; docs was the last one left, and it is the primary path (cron is its fallback), so it is the one that actually runs. The three surfaces — Vectorize, D1 FTS5, store row — now each carry their own try, adopting the shape the cron reap took in #205. Graph edges are still not torn down, same invariant as there: a doc vector ID is never a `doc_edges` endpoint. No per-run cap, unlike the cron reap: a push payload names its own removals, so this loop is bounded by the event rather than by an accumulated backlog. `deleted` in the response now counts docs whose three surfaces all came down, which makes `removed - deleted` the partial-teardown count. The cron reap counts attempts instead; the reason for the divergence is in the PR body. handlePushEvent is exported for the new test file. issue 本文の「Vectorize 失敗時に FTS5 だけ消えて store 行が残る」は実測と食い 違っていた。内側 try は Vectorize 呼び出しの後ろにあるので、Vectorize が throw した時点で FTS5 も store も一度も触られない。取り残しの範囲は本文の記述より広 かったが、欠陥そのもの（Vectorize の失敗が D1 側の teardown を丸ごと飛ばす）は そのまま成立する。 Closes #206 --- docs/0-requirements.ja.md | 2 + docs/0-requirements.md | 2 + src/webhook-push-docs.test.ts | 172 ++++++++++++++++++++++++++++++++++ src/webhook.ts | 73 ++++++++++----- 4 files changed, 228 insertions(+), 21 deletions(-) create mode 100644 src/webhook-push-docs.test.ts diff --git a/docs/0-requirements.ja.md b/docs/0-requirements.ja.md index 8df2363..25b0f57 100644 --- a/docs/0-requirements.ja.md +++ b/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 である。 diff --git a/docs/0-requirements.md b/docs/0-requirements.md index 1304ffc..e7d3687 100644 --- a/docs/0-requirements.md +++ b/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. diff --git a/src/webhook-push-docs.test.ts b/src/webhook-push-docs.test.ts new file mode 100644 index 0000000..38977c6 --- /dev/null +++ b/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(); + 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 { + 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 { + 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).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 { + 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).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(); + }); +}); diff --git a/src/webhook.ts b/src/webhook.ts index 0d8ac80..bbc81d2 100644 --- a/src/webhook.ts +++ b/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, 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]>) { 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, {