fix(ingestion): phase-3 concurrency fixes — R11, R15/R16, R22, R24d#346
Merged
Conversation
State-machine finding R11 [DATA-LOSS, DET]: the DELETE route creates the
storage_cleanup_jobs ledger row with the LIVE document's source-PDF + image
paths before the point of no return. Every abort path (late re-check 409,
trace-cleanup failure, DB-delete failure) only flipped the row to `failed`
and left the paths populated. The storage janitor drains rows in status
('pending','failed') without checking the document still exists, so one
transient error plus one routine janitor run permanently deletes a live
document's PDF and images.
Clear document_paths/image_paths on every abort path (via a new pure
buildStorageCleanupJobUpdate helper) so the janitor has nothing to remove for
a document that survived the abort. Genuine post-delete storage failures keep
their paths so the janitor can finish removing the orphaned objects.
The narrower concurrent-janitor-vs-pending window still needs the companion
janitor guard in scripts/cleanup-storage.ts (skip rows whose document_id still
resolves) — out of this branch's file surface, flagged as follow-up.
Regression test: tests/ingestion.test.ts (storage cleanup ledger update).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… index (R15/R16) State-machine findings R15 [DATA-LOSS, DET] and R16. R15: the retry route unconditionally set documents.status='queued'. For an already-indexed document that write alone forces the worker's non-atomic path, whose reset_document_index deletes the entire live committed index at job start — hours before any replacement commit, and a second failure leaves the document `failed` with zero index. Retry now keeps indexed documents on the atomic reindex path (status untouched, old generation stays live until the new commit swaps it); only non-indexed documents are re-queued (retryDocumentQueueUpdate). R16: the guard accepted `completed` jobs. Resetting a completed job re-pends terminal work that a worker then re-claims and rebuilds against the live index (zombie re-ingest). Completed jobs are now rejected with 409; rebuild goes through the reindex route, which enqueues a fresh job. Note: this file (src/app/api/ingestion/jobs/[id]/retry/route.ts) sits just outside the branch's stated file surface but is the concrete site of R15/R16, one of the two headline DATA-LOSS findings — flagged in the PR. Regression test: tests/ingestion.test.ts (ingestion job retry guards). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…an indexed doc (R22) State-machine finding R22 [OPS-CHURN, DET]: buildIngestionRecoveryPlan's rule `indexed && chunk_count>0 && status!='completed'` superseded plain `pending` jobs, so a routine `recover:ingestion --apply` marked legitimately-queued reindex jobs "completed / superseded by successful index" — the reindex never ran and nothing reported it. Routing such a pending job through the retry branch instead would reset the live index (R19). A pending job on an already-indexed document is now left untouched for the worker's atomic reindex path (which keeps the old generation live until the new commit swaps it). Failed / stale-processing jobs on indexed documents are still superseded as before. Regression test: tests/ingestion-recovery.test.ts (R22 cases). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
State-machine finding R24d [DATA-LOSS/SILENT-CORRUPTION]: reindex `mode:'enrichment'` runs deep-memory/document-enrichment in-route with no job row, and checkIngestionMutationSafety reads only ingestion_jobs — so it runs freely against a live indexing-v3 agent pass. The two writers delete-then- insert the same artifact families with no shared lock; interleaved deletes can leave a strict-gate-"completed/good" document with ZERO enrichment artifacts, for which no repair path is wired up. Add hasActiveAgentEnrichmentJob (+ the pure isActiveAgentEnrichmentJob predicate) and refuse route enrichment with 409 while a genuinely-live agent pass holds the document. A stale/abandoned agent lease (no heartbeat) does not block; steady-state pending/completed agent jobs never block. Scoping deep-memory's unscoped deletes to its own generated_by (the other half of R24d, and R24c for the worker-inline path) is left as follow-up — it needs live verification of the generated_by data model. Regression test: tests/ingestion-mutation-safety.test.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
BigSimmo
marked this pull request as ready for review
July 7, 2026 14:22
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3 of the ingestion-concurrency/scale review: implements the smallest safe fix for four confirmed state-machine violations from
docs/ingestion-state-machine.md§6 (analysis merged to main; phase-2 scale review indocs/scale-readiness-review.md). One commit per finding, each with a regression test.5e60952storage_cleanup_jobsledger's storage paths on every abort path, so the janitor can't destroy a live document's PDF/images after a transient delete failure.95b1aacindexeddocument toqueued(which forced the worker's destructive non-atomic reset of the live index); completed jobs are rejected (no zombie re-ingest).3ca2870pendingreindex of an already-indexed document.c0808b9mode:'enrichment'refuses to run while a live indexing-v3 agent pass holds the document (prevents the interleaved-delete "completed/good doc with zero artifacts" end state).Per-finding detail
failedand left paths populated, and the janitor (scripts/cleanup-storage.ts) removes('pending','failed')rows without checking the doc still exists. Now cleared on abort; genuine post-delete storage failures keep their paths so the janitor can finish. New pure helperbuildStorageCleanupJobUpdate.retryDocumentQueueUpdatekeeps indexed docs on the atomic reindex path (status untouched → old generation stays live until the new commit);ingestionJobRetryRejectionReasonrejectscompletedjobs with 409.pendingjob on an indexed doc is a legitimately-queued reindex, not an abandoned leftover; it is now left for the worker's atomic path. Failed/stale-processing jobs on indexed docs are still superseded.hasActiveAgentEnrichmentJob(+ pureisActiveAgentEnrichmentJob); a stale/abandoned agent lease does not block (no heartbeat), steady-state pending/completed never block.Scope notes / deliberate deferrals
src/app/api/ingestion/jobs/[id]/retry/route.ts, just outside the branch's stated file surface, but it is the concrete site of a headline DATA-LOSS finding. Flagging explicitly.pendingledger row window needs the janitor-side guard inscripts/cleanup-storage.ts(skip rows whosedocument_idstill resolves — FK ison delete set null, so a non-null id proves the doc is alive). Not in this branch's surface → follow-up.deep-memorydeletes to its owngenerated_by(and the worker-inline vs agent overlap, R24c) is left as follow-up — it needs live verification of thegenerated_bydata model.locked_byfences), R5, R7, R9, R17, R23, R24e drift. These require editing live RPC bodies via migrations. schema.sql is known to diverge from live in load-bearing places (R24e), so authoringcreate or replace functionmigrations from schema.sql risks writing the drift into live. They need the live function source + a live-verification window and per the branch rules must not be applied blind. Recommend a dedicated follow-up with live Supabase access.Verification
npm run verify:cheap— green (lint + typecheck + 1253 tests passed, 1 skipped)npm run verify:ui— N/A (no UI/routing/styling change)npm run verify:release— not run (no release/handoff claim)npm run format:check— prettier clean on changed filesnpm run eval:retrieval:quality— N/A: no retrieval/ranking/selection/chunking/scoring change (queue-state + delete/enrichment concurrency guards only)npm run eval:rag/eval:quality— N/A (no answer-generation change)npm run check:production-readiness— could not run here: requires live Supabase creds (this env is demo mode percheck:supabase-project). Please run with live keys.Domain gates
npm run check:indexingandnpm run reindex:health(requested per-fix gates) both require live Supabase creds and exit 1 in this demo-mode environment (NEXT_PUBLIC_SUPABASE_URL/SUPABASE_SERVICE_ROLE_KEYunset). They need a live run by a maintainer.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy) — no env/config changeNotes
tests/ingestion.test.ts,tests/ingestion-recovery.test.ts,tests/ingestion-mutation-safety.test.ts).🤖 Generated with Claude Code