diff --git a/docs/0-requirements.ja.md b/docs/0-requirements.ja.md index 03239b4..40a3531 100644 --- a/docs/0-requirements.ja.md +++ b/docs/0-requirements.ja.md @@ -140,7 +140,7 @@ 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 を兼ねているためである。 +**`push` 経路の doc 削除**は cron reap と同じ 3 面を teardown する — Vectorize / D1 FTS5 / structured store — それぞれ独立に実行するので、1 面の失敗が他を取り残すことはない(issue #206)。store record は最後に、他の 2 面が落ちたときだけ消す。record が残っていれば doc は cron reap の候補に残るので、部分失敗は次の cron run が片付け、残った行が取り残されることはない(issue #282)。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 @@ -195,9 +195,9 @@ file 予算は 1 commit を複数 run に分割しうる。間引きはしない トレードオフは liveness で、毎回失敗し続ける commit はその phase の watermark を止める。run log に境界 commit の SHA が出るほか、`POST /admin/diff-watermark`(installation guide 参照)で watermark を手動で移動できる。旧版 poller が取りこぼした期間を再走査させる経路も同じ endpoint。 -docs poller は `If-None-Match` 付きの条件付きリクエストで repository tree を読み、保存済みの doc record と差分を取る。blob SHA が動いた entry は re-embed し、store にあって tree に無い entry は削除する。**削除は 3 面を teardown する** — Vectorize / D1 FTS5 / structured store — それぞれ独立に実行するので、Vectorize の失敗が実際に retrieval される D1 行を取り残すことはない。wiki 側の 4 面に対してここが 3 面なのは、doc vector ID が `doc_edges` の端点になりえないため。`indexWikiEdges` が唯一の writer であり、src 側も算出される dst 側も wiki vector ID になる。この不変条件が変わったら、ここに edge の teardown を足すこと(issue #203)。なお削除が届くのは現行の vector ID 世代だけで、移行前世代は構造上到達できず、別経路で掃除する(Vector Store の「移行前世代」を参照)。 +docs poller は `If-None-Match` 付きの条件付きリクエストで repository tree を読み、保存済みの doc record と差分を取る。blob SHA が動いた entry は re-embed し、store にあって tree に無い entry は削除する。**削除は 3 面を teardown する** — Vectorize / D1 FTS5 / structured store — それぞれ独立に実行するので、Vectorize の失敗が実際に retrieval される D1 行を取り残すことはない。store record は最後に、他の面がすべて落ちたときだけ消す。reap は候補を store から読むので、他の面に行が残ったまま record を消すとその行は二度と回収されず、削除済みの doc が検索に出続けていた(issue #282)。wiki の reap も同じ規則に従う。書き込み側も同様で、FTS5 の upsert が失敗したら doc / wiki の upsert 全体を失敗にする。store は古い版を保持し、次の poll がその page をやり直す。wiki 側の 4 面に対してここが 3 面なのは、doc vector ID が `doc_edges` の端点になりえないため。`indexWikiEdges` が唯一の writer であり、src 側も算出される dst 側も wiki vector ID になる。この不変条件が変わったら、ここに edge の teardown を足すこと(issue #203)。なお削除が届くのは現行の vector ID 世代だけで、移行前世代は構造上到達できず、別経路で掃除する(Vector Store の「移行前世代」を参照)。 -**削除の枠.** 削除は 1 repo 1 run あたり `MAX_DOC_DELETIONS_PER_REPO_PER_RUN`(既定 5)で cap する。wiki の削除と同じ guard で、1 件あたり 3 subrequest かかるため、大量削除に対して上限なく回すと light cron の invocation 予算を単独で食い潰し、後ろに並ぶ repo を飢えさせうる — 1 つの PR が `.md` を 66 件削除すれば、その全件が 1 run に集中する。削除済み doc の store 行は消えるので残りの集合は縮む一方であり、drain は単調。したがってこの surface にはやり残しの経路が 3 本あり、**どれが効いても tree ETag は据え置く**(上記「ETag の据え置き」に従う): fetch 枠は未処理の変更 doc を残し(issue #149)、削除枠は未削除の doc を残し(issue #203)、embed 失敗は store 行が古い `blobSha` を保持したままの doc を残す(issue #211)。 +**削除の枠.** 削除は 1 repo 1 run あたり `MAX_DOC_DELETIONS_PER_REPO_PER_RUN`(既定 5)で cap する。wiki の削除と同じ guard で、1 件あたり 3 subrequest かかるため、大量削除に対して上限なく回すと light cron の invocation 予算を単独で食い潰し、後ろに並ぶ repo を飢えさせうる — 1 つの PR が `.md` を 66 件削除すれば、その全件が 1 run に集中する。削除済み doc の store 行は消えるので残りの集合は縮む一方であり、drain は単調。したがってこの surface にはやり残しの経路が 4 本あり、**どれが効いても tree ETag は据え置く**(上記「ETag の据え置き」に従う): fetch 枠は未処理の変更 doc を残し(issue #149)、削除枠は未削除の doc を残し(issue #203)、embed 失敗は store 行が古い `blobSha` を保持したままの doc を残し(issue #211)、削除の部分失敗は store record を残した削除済み doc を残す(issue #282)。 docs / releases のどちらの loop も、自前の embedding cap を持たない。両者の fan-out cap(各 10)が `MAX_EMBEDDINGS_PER_RUN` = 50 より小さく、embed 件数を構造的に抑えているため、かつて置かれていた guard は到達不能だった。到達不能な分岐は「それも ETag の据え置きを負うべきか」を読み手が判断できない状態を残し、issue #211 の穴が欠陥ではなく曖昧さとして読めてしまう原因になっていた。**この定数関係はテストで検証している**——各 cap の位置のコメントに委ねてはいない。この repo の fan-out cap は実際に調整される(issue #134 は comment fetch cap を 30 から 10 に下げた)ので、50 より上に引き上げた時に、その面の embed 上限が黙って消えてしまうからである。assertion が落ちることは「新しい cap が誤り」という判定ではない。その loop に embedding guard を戻し、見送った項目が ETag を据え置けるよう、その loop の ETag 据え置き条件に組み込む必要がある、という意味である。 diff --git a/docs/0-requirements.md b/docs/0-requirements.md index 5afe028..fb505ec 100644 --- a/docs/0-requirements.md +++ b/docs/0-requirements.md @@ -144,7 +144,7 @@ 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. +**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). The store record goes last and only when the other two came down: it is what keeps the doc on the cron reap's candidate list, so a partial teardown is finished by the next cron run instead of stranding the surviving row (issue #282). 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 @@ -199,9 +199,9 @@ Both phases obey one watermark invariant: **a watermark never moves past a commi The tradeoff is liveness: a commit that fails on every attempt blocks its phase's watermark. The run log names the boundary commit, and `POST /admin/diff-watermark` (see the installation guide) moves the watermark manually — the same endpoint used to replay a period whose commits were lost by an earlier version of the poller. -The docs poller reads the repository tree with a conditional `If-None-Match` request and diffs it against the stored doc records: entries whose blob SHA moved are re-embedded, entries present in the store but absent from the tree are reaped. **The reap tears down three surfaces** — Vectorize, D1 FTS5, and the structured store — each independently, so a Vectorize failure cannot strand the D1 rows users actually retrieve. It is three and not the wiki reap's four because a doc vector ID is never a `doc_edges` endpoint: `indexWikiEdges` is the only writer and both the source and the computed destination IDs are wiki vector IDs. Add the edge teardown here if that invariant changes (issue #203). The reap reaches only the current vector-ID generation; the pre-migration generation is unreachable by construction and is cleaned out separately (see Vector Store, "Pre-migration generation"). +The docs poller reads the repository tree with a conditional `If-None-Match` request and diffs it against the stored doc records: entries whose blob SHA moved are re-embedded, entries present in the store but absent from the tree are reaped. **The reap tears down three surfaces** — Vectorize, D1 FTS5, and the structured store — each independently, so a Vectorize failure cannot strand the D1 rows users actually retrieve. The store record goes last and only when the other surfaces came down: the reap reads its candidates from the store, so dropping the record while a row survived elsewhere stranded that row for good and the deleted doc kept answering searches (issue #282). The wiki reap follows the same rule. On the write side, a failed FTS5 upsert fails the whole doc or wiki upsert, so the store keeps the old version and the next poll retries the page. It is three and not the wiki reap's four because a doc vector ID is never a `doc_edges` endpoint: `indexWikiEdges` is the only writer and both the source and the computed destination IDs are wiki vector IDs. Add the edge teardown here if that invariant changes (issue #203). The reap reaches only the current vector-ID generation; the pre-migration generation is unreachable by construction and is cleaned out separately (see Vector Store, "Pre-migration generation"). -**Reap budget.** The reap is capped at `MAX_DOC_DELETIONS_PER_REPO_PER_RUN` (default 5) per repo per run, the same guard the wiki reap carries: at 3 subrequests per deletion an unbounded loop over a mass deletion could exhaust the light cron's invocation budget on its own and starve every repo behind it — a single PR removing 66 `.md` files puts all of them on one run. The drain is monotonic, since a reaped doc's store row is gone and the leftover set only shrinks. This surface therefore has three ways to leave work behind, and **any one of them holds the tree ETag back** per the ETag hold above: the fetch cap leaves changed docs unprocessed (issue #149), the delete cap leaves deletions unreaped (issue #203), and a failed embed leaves a doc whose store row still carries the old `blobSha` (issue #211). +**Reap budget.** The reap is capped at `MAX_DOC_DELETIONS_PER_REPO_PER_RUN` (default 5) per repo per run, the same guard the wiki reap carries: at 3 subrequests per deletion an unbounded loop over a mass deletion could exhaust the light cron's invocation budget on its own and starve every repo behind it — a single PR removing 66 `.md` files puts all of them on one run. The drain is monotonic, since a reaped doc's store row is gone and the leftover set only shrinks. This surface therefore has four ways to leave work behind, and **any one of them holds the tree ETag back** per the ETag hold above: the fetch cap leaves changed docs unprocessed (issue #149), the delete cap leaves deletions unreaped (issue #203), a failed embed leaves a doc whose store row still carries the old `blobSha` (issue #211), and a partial teardown leaves a deleted doc whose store record was kept (issue #282). Neither the docs nor the releases loop carries an embedding cap of its own. Their fan-out caps (10 each) sit below `MAX_EMBEDDINGS_PER_RUN` = 50 and bound the embed count by construction, so the guards those loops used to hold were unreachable — and unreachable code left the reader unable to tell whether it also owed the ETag hold, which is how the issue #211 hole read as ambiguous rather than as a defect. **That constant relation is asserted by a test**, not left to the comments at the cap sites: fan-out caps on this repo get retuned (issue #134 took the comment fetch cap from 30 to 10), and raising one past 50 would otherwise remove the surface's embed bound in silence. A failing assertion is not a verdict that the new cap is wrong — it says the loop now needs its embedding guard back, folded into that loop's ETag-hold condition so a deferred item still holds the ETag. diff --git a/src/pipeline/embed-doc.test.ts b/src/pipeline/embed-doc.test.ts new file mode 100644 index 0000000..edcaaee --- /dev/null +++ b/src/pipeline/embed-doc.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, vi } from "vitest"; +import type { Env } from "../types.js"; +import { processAndUpsertDoc, processAndUpsertWikiDoc } from "./embed-doc.js"; + +// A failed FTS5 write has to fail the whole upsert. The store record carries the +// version the next poll compares against, so writing it after a lost FTS5 row +// told the next poll the page was current, and the row stayed stale for good +// (issue #282). + +const REPO = "acme/widgets"; + +function mkEnv(ftsFails: boolean) { + const env = { + AI: { run: vi.fn(async () => ({ data: [[0.1, 0.2]] })) }, + VECTORIZE: { upsert: vi.fn(async () => undefined) }, + DB_FTS: { + prepare: () => ({ + bind: () => ({ + run: async () => { + if (ftsFails) throw new Error("d1 down"); + return {}; + }, + }), + }), + }, + } as unknown as Env; + const storeWrites: string[] = []; + const storeStub = { + fetch: vi.fn(async (req: Request) => { + storeWrites.push(new URL(req.url).pathname); + return new Response("ok"); + }), + } as unknown as DurableObjectStub; + return { env, storeStub, storeWrites }; +} + +describe("embed-doc: FTS5 failure keeps the stored version", () => { + it("reports a doc as failed and leaves its store record alone", async () => { + const { env, storeStub, storeWrites } = mkEnv(true); + + const result = await processAndUpsertDoc(env, storeStub, REPO, "docs/a.md", "body", "sha1"); + + expect(result.failed).toBe(true); + expect(storeWrites).toEqual([]); + }); + + it("reports a wiki page as failed and leaves its store record alone", async () => { + const { env, storeStub, storeWrites } = mkEnv(true); + + const result = await processAndUpsertWikiDoc(env, storeStub, REPO, "Page", "md", "body"); + + expect(result.failed).toBe(true); + expect(storeWrites).toEqual([]); + }); + + it("writes the store record once the FTS5 row is in", async () => { + const { env, storeStub, storeWrites } = mkEnv(false); + + const result = await processAndUpsertDoc(env, storeStub, REPO, "docs/a.md", "body", "sha1"); + + expect(result.embedded).toBe(true); + expect(storeWrites).toEqual(["/upsert-doc"]); + }); +}); diff --git a/src/pipeline/embed-doc.ts b/src/pipeline/embed-doc.ts index 5cad10c..2b6bfc7 100644 --- a/src/pipeline/embed-doc.ts +++ b/src/pipeline/embed-doc.ts @@ -68,25 +68,20 @@ export async function processAndUpsertDoc( ]); // Mirror into D1 FTS5 sparse index. - try { - await upsertFtsRow(env.DB_FTS, { - vectorId: dvid, - repo, - type: "doc", - state: "active", - labels: "", - milestone: "", - assignees: "", - updatedAt: now, - docPath: path, - content: embeddingInput, - }); - } catch (ftsErr) { - console.error( - `Failed to upsert FTS5 row for doc ${repo}/${path}:`, - ftsErr instanceof Error ? ftsErr.message : String(ftsErr), - ); - } + // Not caught here: a failed FTS write must fail the whole upsert so the store + // keeps the old version and the next poll retries the page (issue #282). + await upsertFtsRow(env.DB_FTS, { + vectorId: dvid, + repo, + type: "doc", + state: "active", + labels: "", + milestone: "", + assignees: "", + updatedAt: now, + docPath: path, + content: embeddingInput, + }); // Upsert doc record into store const record: DocRecord = { @@ -172,25 +167,20 @@ export async function processAndUpsertWikiDoc( // Mirror into D1 FTS5. We reuse the existing `doc_path` column to store // the wiki page slug — semantically the same kind of "where did this come // from" field, distinguished by the row's `type='wiki_doc'`. - try { - await upsertFtsRow(env.DB_FTS, { - vectorId: wvid, - repo, - type: "wiki_doc", - state: "active", - labels: "", - milestone: "", - assignees: "", - updatedAt: now, - docPath: pageName, - content: embeddingInput, - }); - } catch (ftsErr) { - console.error( - `Failed to upsert FTS5 row for wiki ${repo}/${pageName}:`, - ftsErr instanceof Error ? ftsErr.message : String(ftsErr), - ); - } + // Not caught here: a failed FTS write must fail the whole upsert so the store + // keeps the old version and the next poll retries the page (issue #282). + await upsertFtsRow(env.DB_FTS, { + vectorId: wvid, + repo, + type: "wiki_doc", + state: "active", + labels: "", + milestone: "", + assignees: "", + updatedAt: now, + docPath: pageName, + content: embeddingInput, + }); // Extract + index graph "mention" edges to other wiki pages in this repo. // Additive: the default retrieval path never reads these. Failures here must diff --git a/src/poller-docs.test.ts b/src/poller-docs.test.ts index af6375b..7027cc0 100644 --- a/src/poller-docs.test.ts +++ b/src/poller-docs.test.ts @@ -191,11 +191,9 @@ describe("poller: pollDocs delete fan-out", () => { expect(store.records.has("docs/keep.md")).toBe(true); }); - it("keeps tearing down the later surfaces when Vectorize fails", async () => { - // The surfaces are torn down independently. Pre-fix one outer try wrapped - // the whole item, so a Vectorize failure skipped the store DELETE and left - // the row behind — while the FTS5 row, guarded by its own inner try, was - // already gone. The D1 rows are the ones users actually retrieve. + it("keeps tearing down the FTS5 row when Vectorize fails", async () => { + // The surfaces are torn down independently: a Vectorize failure must not + // leave the FTS5 row, the one users actually retrieve, behind. stubTree(["docs/keep.md"]); const store = makeDocStore([stored("docs/keep.md"), stored("docs/gone.md")]); const { env } = makeDocEnv(); @@ -206,19 +204,54 @@ describe("poller: pollDocs delete fan-out", () => { await pollDocs(REPO, env, store.stub); expect(deleteFtsRowMock).toHaveBeenCalledTimes(1); - expect(store.deletes).toEqual(["docs/gone.md"]); }); - it("keeps tearing down the store record when the FTS5 delete fails", async () => { - stubTree(["docs/keep.md"]); - const store = makeDocStore([stored("docs/keep.md"), stored("docs/gone.md")]); + it("keeps the store record when another surface failed to delete", async () => { + // Deletion candidates are read from the store. Dropping the record while the + // FTS5 row survived stranded that row for good, and the deleted doc kept + // answering searches (issue #282). + for (const fail of ["vector", "fts"] as const) { + stubTree(["docs/keep.md"]); + const store = makeDocStore([stored("docs/keep.md"), stored("docs/gone.md")]); + const { env } = makeDocEnv(); + if (fail === "vector") { + (env.VECTORIZE.deleteByIds as ReturnType).mockRejectedValue( + new Error("vectorize down"), + ); + } else { + deleteFtsRowMock.mockRejectedValueOnce(new Error("d1 down")); + } + + await pollDocs(REPO, env, store.stub); + + expect(store.deletes).toEqual([]); + expect(store.records.has("docs/gone.md")).toBe(true); + } + }); + + it("retries a half-deleted doc on the next run until every surface is gone", async () => { + const PRIOR_ETAG = 'W/"tree-prior"'; + const store = makeDocStore([stored("docs/keep.md"), stored("docs/gone.md")], PRIOR_ETAG); const { env, vectorDeletes } = makeDocEnv(); - deleteFtsRowMock.mockRejectedValue(new Error("d1 down")); + deleteFtsRowMock.mockRejectedValueOnce(new Error("d1 down")); + stubTree(["docs/keep.md"]); + await pollDocs(REPO, env, store.stub); + + // The tree ETag is held, or the next run would answer 304 and never reach + // the reap at all. + expect(store.etag()).toBe(PRIOR_ETAG); + + const second = stubTree(["docs/keep.md"]); await pollDocs(REPO, env, store.stub); - expect(vectorDeletes).toHaveLength(1); + const goneId = await docVectorId(REPO, "docs/gone.md"); + expect(second.conditionalHits).toEqual([PRIOR_ETAG]); + expect(deleteFtsRowMock).toHaveBeenCalledTimes(2); + expect(deleteFtsRowMock.mock.calls[1][1]).toBe(goneId); + expect(vectorDeletes).toEqual([goneId, goneId]); expect(store.deletes).toEqual(["docs/gone.md"]); + expect(store.etag()).toBe('W/"tree-1"'); }); it("leaves the reap alone when nothing was deleted", async () => { diff --git a/src/poller-wiki.test.ts b/src/poller-wiki.test.ts index e0e2674..28bed5e 100644 --- a/src/poller-wiki.test.ts +++ b/src/poller-wiki.test.ts @@ -618,6 +618,35 @@ describe("poller: pollWiki orphan reap", () => { await pollWiki(REPO, env, store.stub); expect(deleteFtsRowMock).toHaveBeenCalledTimes(1); + // The store record is the reap's candidate list: it stays until every other + // surface is gone, so the next run retries the page (issue #282). + expect(store.deletes).toEqual([]); + + deleteByIds.mockResolvedValue(undefined); + stubWiki(wiki); + await pollWiki(REPO, env, store.stub); + + expect(deleteFtsRowMock).toHaveBeenCalledTimes(2); + expect(store.deletes).toEqual(["orphan"]); + }); + + it("keeps the store record when the FTS5 delete fails, and retries it next run", async () => { + const wiki: FakeWiki = { + listed: [{ slug: "current" }], + files: { current: "c", Home: "h" }, + }; + const store = makeWikiStore(); + const { env } = makeWikiEnv(["orphan"]); + deleteFtsRowMock.mockRejectedValueOnce(new Error("d1 down")); + stubWiki(wiki); + + await pollWiki(REPO, env, store.stub); + expect(store.deletes).toEqual([]); + + stubWiki(wiki); + await pollWiki(REPO, env, store.stub); + + expect(deleteFtsRowMock).toHaveBeenCalledTimes(2); expect(store.deletes).toEqual(["orphan"]); }); diff --git a/src/poller.ts b/src/poller.ts index f574468..c8c3f89 100644 --- a/src/poller.ts +++ b/src/poller.ts @@ -1166,6 +1166,7 @@ export async function pollDocs( // vector ID is never an edge endpoint. Add the teardown here if that // invariant changes (issue #203). let removedDocs = 0; + let deleteFailed = 0; let deleteBudgetExhausted = false; for (const doc of deletedDocs) { if (removedDocs >= MAX_DOC_DELETIONS_PER_REPO_PER_RUN) { @@ -1181,6 +1182,7 @@ export async function pollDocs( const dvid = await docVectorId(repo, doc.path); // Each surface is torn down independently: a Vectorize failure must not // strand the D1 rows, which are the ones users actually retrieve. + let surfaceFailed = false; for (const [surface, run] of [ ["vector", () => env.VECTORIZE.deleteByIds([dvid])], ["FTS5 row", () => deleteFtsRow(env.DB_FTS, dvid)], @@ -1195,30 +1197,37 @@ export async function pollDocs( ), ], ] as Array<[string, () => Promise]>) { + // The store record goes last and only when every other surface is gone: + // deletion candidates are read from the store, so dropping it while a row + // survives elsewhere strands that row for good (issue #282). + if (surface === "store record" && surfaceFailed) continue; try { await run(); } catch (err) { + surfaceFailed = true; console.error( `Failed to delete ${surface} for doc ${repo}/${doc.path}:`, err instanceof Error ? err.message : String(err), ); } } + if (surfaceFailed) deleteFailed++; removedDocs++; } // Update watermark with ETag — but only when the run left no work behind, the // same condition the issue / PR and releases surfaces use (issue #215 / #211). - // Three ways work is left: changed docs still to embed (fetch cap, issue - // #149), deleted docs still to reap (delete cap, issue #203), or a doc whose + // Four ways work is left: changed docs still to embed (fetch cap, issue + // #149), deleted docs still to reap (delete cap, issue #203), a doc whose // embed failed (issue #211 — its store row keeps the old `blobSha`, so it is - // still "changed" next run). Persisting the new ETag would cause the next cron + // still "changed" next run), or a deleted doc with a surface left behind + // (issue #282 — its store record is kept, so it is still "deleted" next run). Persisting the new ETag would cause the next cron // to short-circuit on 304 and never see any of them. Holding the prior ETag // forces a fresh tree fetch next run so `changedEntries` and `deletedDocs` // both repopulate — this run sent that ETag and got 200 back, so the tree // already differs from it. lastPolledAt is still bumped so observability sees // the run. - const holdEtag = fetchBudgetExhausted || deleteBudgetExhausted || failed > 0; + const holdEtag = fetchBudgetExhausted || deleteBudgetExhausted || failed > 0 || deleteFailed > 0; await storeStub.fetch( new Request("http://store/watermark", { method: "POST", @@ -2164,6 +2173,7 @@ export async function pollWiki( let failed = 0; let inconclusive = 0; let removed = 0; + let deleteFailed = 0; let orphansWithheld = 0; let fetches = 0; let visited = 0; @@ -2396,6 +2406,7 @@ export async function pollWiki( subrequests += WIKI_SUBREQUESTS_PER_DELETE; // Each surface is torn down independently: a Vectorize failure must not // strand the D1 rows, which are the ones users actually retrieve. + let surfaceFailed = false; for (const [surface, run] of [ ["vector", () => env.VECTORIZE.deleteByIds([wvid])], ["FTS5 row", () => deleteFtsRow(env.DB_FTS, wvid)], @@ -2411,15 +2422,21 @@ export async function pollWiki( ), ], ] as Array<[string, () => Promise]>) { + // The store record goes last and only when every other surface is gone: + // deletion candidates are read from the store, so dropping it while a row + // survives elsewhere strands that row for good (issue #282). + if (surface === "store record" && surfaceFailed) continue; try { await run(); } catch (err) { + surfaceFailed = true; console.error( `Failed to delete ${surface} for wiki ${repo}/${pageName}:`, err instanceof Error ? err.message : String(err), ); } } + if (surfaceFailed) deleteFailed++; removed++; } @@ -2454,7 +2471,7 @@ export async function pollWiki( `${repo} wiki: ${pages.length} pages, ${visited} visited, ${fetches}/${fetchBudget} fetches, ` + `${subrequests} subrequests, ` + `${embedded} embedded, ${skipped} unchanged, ${failed} failed, ` + - `${inconclusive} inconclusive, ${removed} deleted, ` + + `${inconclusive} inconclusive, ${removed} deleted (${deleteFailed} kept for retry), ` + `${orphansWithheld} reap withheld, ` + `cursor ${startCursor || ""} -> ${nextCursor || ""}, ` + `lap ${lapAnchor || ""}${wrapped ? " complete" : ` -> ${lapFinalSlug}`}` + diff --git a/src/webhook-push-docs.test.ts b/src/webhook-push-docs.test.ts index 38977c6..313ee8f 100644 --- a/src/webhook-push-docs.test.ts +++ b/src/webhook-push-docs.test.ts @@ -102,14 +102,18 @@ describe("webhook: push doc delete fan-out", () => { const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, store.stub); expect(deleteFtsRowMock).toHaveBeenCalledTimes(1); - expect(store.deletes).toEqual(["docs/gone.md"]); + // The store record stays so the cron reap retries the vector (issue #282). + expect(store.deletes).toEqual([]); // `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 () => { + it("keeps the store record when the FTS5 delete fails", async () => { + // Deletion candidates of the cron reap are read from the store. Dropping + // the record here stranded the surviving FTS5 row for good, and the deleted + // doc kept answering searches (issue #282). const store = makeDocStore(); const { env, vectorDeletes } = makeDocEnv(); deleteFtsRowMock.mockRejectedValue(new Error("d1 down")); @@ -117,7 +121,7 @@ describe("webhook: push doc delete fan-out", () => { const res = await handlePushEvent(pushPayload(["docs/gone.md"]), env, store.stub); expect(vectorDeletes).toHaveLength(1); - expect(store.deletes).toEqual(["docs/gone.md"]); + expect(store.deletes).toEqual([]); expect(await docsResult(res)).toMatchObject({ removed: 1, deleted: 0 }); }); @@ -154,7 +158,7 @@ describe("webhook: push doc delete fan-out", () => { store.stub, ); - expect(store.deletes).toEqual(["docs/a.md", "docs/b.md"]); + expect(store.deletes).toEqual(["docs/b.md"]); expect(vectorDeletes).toEqual([await docVectorId(REPO, "docs/b.md")]); expect(await docsResult(res)).toMatchObject({ removed: 2, deleted: 1 }); }); diff --git a/src/webhook.ts b/src/webhook.ts index 66c05e1..94234c3 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -524,6 +524,10 @@ export async function handlePushEvent( ), ], ] as Array<[string, () => Promise]>) { + // The store record goes last and only when every other surface is gone. + // Keeping it leaves the doc on the cron reap's candidate list, which + // retries the surfaces this push could not tear down (issue #282). + if (surface === "store record" && !allSurfacesTornDown) continue; try { await run(); } catch (err) {