audit fixes (P14): stop one failure stranding a whole document or job batch in the worker - #2625
Conversation
… path (L3)
worker/python/analyze_assertions.py's malformed-chunk warning serialised the
whole chunk dict (which carries the clinical `text` field) with
`f"skipped malformed chunk: {chunk!r:.120}"`, and
worker/assertion-tagging.ts prints every warning verbatim with `console.warn`,
bypassing the worker's safe-logging boundary (safeErrorLogDetails /
safeIngestionJobLog) that every other worker log line goes through. Guideline
text could reach container logs / Railway log drains.
Fix: log the chunk's positional index only ("skipped malformed chunk at index
N"), never the chunk object.
Test: worker/python/test_analyze_assertions.py
(test_warning_omits_chunk_text_and_names_the_index) builds a malformed chunk
carrying a distinctive clinical sentence, runs analyze_assertions.run(), and
asserts the emitted warning contains neither the sentence nor any fragment of
it, only "index 1". Confirmed the test fails against the pre-fix code (the
sentence appeared verbatim in the warning) before the fix, and passes after.
Ran the full worker/python pytest suite (16 passed) to confirm no regression.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FSPY4VSqg7WVukCvmHQP9t
…ngs (L13) The per-job catch in the claimed-batch loop called `markJobFailure` unguarded. markJobFailure -> updateAgentJobStatus throws whenever the status RPC returns ok:false or the database errors, so that throw escaped to the request-level catch and returned 500 with every not-yet-processed job in the batch still `processing` under its lock — invisible until the 45-minute stale reclaim, and each one attempt closer to the terminal `failed` state that is never re-queued. Two smaller holes on the same handler: `limit` was parsed with a bare `Number(...)`, so `?limit=abc` reached claim_indexing_v3_agent_jobs as `NaN::integer` and failed the Postgres cast before a single job was claimed; and GET was accepted on an endpoint whose only job is to claim and mutate rows. The one real caller (invoke_indexing_v3_agent, schema.sql) uses net.http_post. Fix: `runClaimedJobBatch` in behavior.ts isolates both the per-job work and the per-job failure recording, reporting a failed recording as `failure_record_error` rather than aborting the batch; `parseAgentClaimLimit` mirrors the ingestion-worker function's Number.isFinite guard; `isAllowedAgentMethod` accepts POST only. Test: tests/indexing-v3-agent.test.ts "indexing-v3-agent claimed-batch request loop (L13)". Red before the fix (4 failed | 13 passed — "runClaimedJobBatch is not a function", "parseAgentClaimLimit is not a function", "isAllowedAgentMethod is not a function"); green after (17 passed). The stranding case asserts siblings b and c are still processed after job a's markJobFailure throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Two halves of the same failure.
Python: for every embedded image the extractor wrote PyMuPDF's raw stream bytes
as-is and derived the MIME type from the filter extension
(`f"image/{ext}"`), which yields image/jpx, image/jb2, image/tiff, image/bmp
and image/pnm for JPEG2000/JBIG2/other filters — types the vision provider
rejects. There was also no per-image byte ceiling; only the aggregate
maxArtifactBytes budget applied, so one huge stream could pass. Now
`normalize_embedded_image` forwards only png/jpg/jpeg/gif/webp untouched,
re-encodes everything else to PNG via `fitz.Pixmap(document, xref)` under the
existing maxRenderPixels budget (converting CMYK to RGB first), and skips —
with a warning that never quotes the bytes — an image beyond the pixel budget,
one that cannot be re-encoded, or one above the 20 MB per-image cap.
TypeScript: the per-task vision call had no error isolation, so a provider
rejection of one image escaped the caption batch and failed the document with
an OpenAI error string. `visionImageRejectionSkipReason` recognises only
non-retryable per-image rejections (openai_invalid_request,
openai_content_filtered, and unsupported/invalid/corrupt-image messages) and
returns a content-free skip reason; main.ts records that reason on the image
and continues. It is deliberately an allowlist: rate limits, timeouts and
provider outages still propagate, because silently skipping every image during
an outage would complete a document with no visual evidence at all.
Tests:
- worker/python/test_extract_pdf_assets_embedded_images.py — red before
(6 failed, "module 'extract_pdf_assets' has no attribute
'normalize_embedded_image'"), green after; full worker/python suite 22 passed.
- tests/worker-behavior.test.ts "vision per-image error isolation (L12)" — red
before (4 failed | 5 passed, "visionImageRejectionSkipReason is not a
function" plus the main.ts wiring window), green after (9 passed).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…im a job (L24) The recovered ingestion-worker Edge Function is still deployable (`verify_jwt = true` in supabase/config.toml) and public.invoke_ingestion_worker is a live SECURITY DEFINER RPC that POSTs to it. It claims real rows from the same claim_ingestion_jobs queue the container worker uses, but performs no extraction: it reads existing chunks, builds a heuristic summary, and embeds with a 384-dimension model whose output cannot go into vector(1536). Every claimed job therefore lands in the catch, where fail_or_retry_ingestion_job is called with a hardcoded "indexed" document status — so a newly uploaded guideline can read as indexed with nothing retrievable, while the container worker's retry budget for that document is spent on a path that cannot succeed. Guard: the handler returns 410 with code `ingestion_worker_retired` as its first statement, before the method check, the authorization check and the claim. No claim, no lease, no status stamp. Deliberately NOT included: dropping the function, its config.toml block, or invoke_ingestion_worker(integer). That is a migration, and this package ships none; the 410 closes the reachable path in the meantime. Test: tests/ingestion-edge-function-auth.test.ts "ingestion-worker Edge Function retirement guard (L24)". Red before the fix (module missing, then "expected -1 to be greater than -1" for the guard in the request handler); green after (4 passed). Verified red again by reverting index.ts alone with the final assertions in place: "expected ... to contain INGESTION_WORKER_RETIRED". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
… SQL (L11)
`isCommittedGenerationMetadata` returned true whenever the document carried no
committed generation, so a document whose `metadata.index_generation_id` was
absent accepted rows from ANY generation. The SQL predicate
`public.is_committed_document_generation` does the opposite: `row_generation =
document_generation` against a NULL document generation yields NULL, so
`search_document_chunks` excludes those rows.
That divergence is visible during the first atomic reindex of a legacy
(never-stamped) indexed document: `loadAuthorizedDocumentDetail` filters
chunks, pages, images and table facts through this predicate, so the viewer
could interleave staged, uncommitted rows with the live ones — duplicate or
half-built evidence (two copies of a dosing table, a staged table fact without
its committed image) that search would never return, and cited page text that
differs from what search returns.
This is a deliberate behaviour change, not a silent one: the previous
behaviour was pinned by tests/reindex-pipeline.test.ts, and that expectation is
flipped here with the SQL it now mirrors quoted alongside it. A row carrying no
generation of its own stays visible either way, matching the SQL's
unconditional `row_generation is null` arm.
Test: tests/reindex-pipeline.test.ts "compares generated artifacts against the
committed document generation". Red before the fix (expected false, received
true for `{ index_generation_id: "legacy-generation" }` with a null committed
generation); green after — 8 passed, plus the consumers
(deep-memory, worker-row-contract, table-facts, document-detail, signed-url,
retrieval-hydration) 47 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…to write under it (L21) RAG impact: no retrieval behaviour change — cache keys and eviction order only; no ranking, ordering or selection change `cacheIndexingVersion` is the corpus-staleness guard for both the local and shared answer/search caches. A PostgREST `error` on the `documents` stamp query was folded into the same constant as a genuinely empty corpus (`no-indexed-documents`), cached for 5 s, then used as the `indexing_version` for writes and reads. Unlike the thrown-exception path, the error-object path was indistinguishable from "nothing is indexed", and neither path refused to write. Two stamp failures bracketing a worker-side reindex — which cannot call this process's in-process invalidation — therefore let a pre-reindex answer be served with `answer_cache_hit`, bypassing the guard that exists precisely to prevent that. A stale clinical answer with a clean-looking routing reason. Fix: an `error` now yields the distinct `index-stamp-unavailable` stamp, and `setCachedAnswer`/`setCachedSearch` return early when the stamp is unavailable — a stamp that cannot vouch for the corpus must not be paired with a cached answer that a later failed read would match. Only cache keys and write admission change here. Ranking, ordering, selection and the retrieval RPCs are untouched; docs/rag-behaviour/safeguards.md read first. Test: tests/rag-cache-indexing-stamp.test.ts. Red before the fix (2 failed: "expected 'rag-deep-memory-v1:no-indexed-documen…' not to be 'rag-deep-memory-v1:no-indexed-documen…'", and the answer written during the outage being served back with "routingReason: test; answer_cache_hit"); green after (2 passed). rag-cache-invalidation, rag-cache-utils, rag-tail-latency, rag-unsupported-short-circuit-cache, rag-imputation-contract, rag-fast-path-ordering and retrieval-selection all stay green (59 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…L134) RAG impact: no retrieval behaviour change — cache keys and eviction order only; no ranking, ordering or selection change `answerCache.set(key, …)` without a preceding `delete` leaves a re-cached key at its original Map position, and `getCachedAnswer` never bumped recency, so eviction dropped the oldest-INSERTED entry rather than the least-recently-used one. On a ward-round pattern — the same handful of questions repeated — a hot answer aged out ahead of answers nobody had asked for since, giving a lower hit rate than the cache size suggests and under-reporting effectiveness in `cacheMetricsSnapshot`. owner-catalogue-cache, signed-url-cache and bounded-ttl-cache already implement the delete-then-set bump for the same shape; this was the one cache in the lane that did not. Fix: delete-before-set on write, and a read-side re-insert on a hit. The same delete-before-set is applied to the search cache, which has the identical shape. Eviction order only — no key format, ranking, ordering or selection change; docs/rag-behaviour/safeguards.md read first. Test: tests/rag-cache-lru.test.ts. Red before the fix (2 failed: "expected null not to be null" for the answer that had just been read, and "expected undefined to be 'answer a refreshed'" for the overwrite case); green after (2 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
`tableFromImage` took `metadata.table_rows` / `metadata.table_columns` and kept them only when `typeof === "number"`. worker/python/extract_pdf_assets.py emits those two keys as ARRAYS — `table_rows` is a list of row lists, `table_columns` a list of header cells — and puts the counts in `row_count` / `column_count`. Both fields were therefore always null in the raw legacy record. No measurement impact: score.py scores table cells from `markdown` only (markdown_to_cells), so Gate B numbers are unchanged. This only stops out/raw misleading anyone inspecting it. The projection moved to eval/docling/harness/legacy-tables.ts so it can be tested without executing run-legacy.ts's top-level `main()`; run-legacy.ts now imports it and is otherwise unchanged. Test: tests/docling-legacy-table-counts.test.ts. Red before the fix (extracted verbatim first: "rows: null, cols: null" against the expected "rows: 2, cols: 2" for a crop carrying the real extractor metadata shape); green after (3 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…L11) The P14 gate fails on "allows legacy image signed URLs when parent document generation metadata is missing" (404 vs 200). Investigation says that fixture does not model a reachable production state, so no source change is made and no existing test is touched; this commit only adds the missing sibling case that proves the real legacy shape still works. Evidence that the failing fixture is artificial: it gives the image `metadata.index_generation_id = "generation-a"` while the parent document has none. The worker only began stamping document_images metadata in 5cd1426 (2026-06-28), the same commit that introduced commit_document_index_generation, which stamps the parent document in the same transaction; the pre-2026-06-28 batch repair (20260712173000_add_legacy_index_health_batch_repair.sql) also stamps the document alongside every row. So a stamped image on an unstamped document is not "legacy" — it is a staged or abandoned generation, which cleanup_abandoned_document_index_generations (20260629000000_abandoned_reindex_generation_recovery.sql) classifies as garbage to delete, and which the SQL readers already exclude (is_committed_artifact_generation against a NULL document generation yields NULL). A genuinely legacy image carries no generation at all and passes the predicate's unconditional first arm. Trade-off taken: the L11 fail-closed behaviour is kept, so the pre-existing test stays red pending an owner decision on its fixture, rather than reopening the TS/SQL divergence at the signed-URL gate to make the gate green. The distinction between "never stamped" and "mid-reindex staged" is not expressible from the predicate's inputs — a legacy document's rows carry no generation, so the two states are only separable by an ingestion_jobs lookup, which the SQL cleanup RPC does and a per-image access gate must not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
…s (L11) The case named 'allows legacy image signed URLs when parent document generation metadata is missing' was not a legacy image. Its fixture gives the IMAGE a generation while its parent document has none, and asserted 200. supabase/schema.sql is_committed_artifact_generation reads row_generation is null or row_generation = document_generation and 'generation-a' = NULL is NULL, not true — so the database excludes that exact row. The old expectation therefore pinned the TypeScript predicate being MORE permissive than the database, which is the defect L11 exists to remove. Coverage is not reduced. The genuinely legacy shape (an image carrying no generation, written before the worker began stamping) is pinned by the sibling case added in 99a8722 and still returns 200, so no legacy document loses its images. This case now pins the staged/abandoned shape, renamed to say so. tests/private-access-routes.test.ts: Tests 147 passed (147). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e59c96b1-8047-4a2e-838b-71283740198b) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9962a77b-6cd7-4134-985c-fb655741fae8) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_726f8be8-2c69-4f1e-9f4d-2617b3226148) |
Summary
Audit remediation package P14 — Worker, ingestion and caches, from
docs/audit/full-repository-audit-2026-09-02.md(PR #2573). Findings:L3,L11,L12,L13,L21,L24,L117,L134. Each fix landed test-first as its own commit.The two that change real behaviour under failure are
2c3ee8510and6848a3058: previously, one image the vision model rejected failed the entire document, and one job's failure could leave its claimed siblings stranded. Both now degrade to the single failing unit.On the two L11 test commits — worth a reviewer's attention, because the first pass looked like a regression and was not.
33ca79256renames and inverts a test whose fixture described a state the database cannot produce. The SQL predicate insupabase/schema.sqlisrow_generation is null or row_generation = document_generation. The old fixture gave an image a generation while its parent document had none;'generation-a' = NULLevaluates to NULL, so the database excludes the row, and the test's expectation of 200 was wrong about the schema rather than about the code. It now expects 404 and is named for what it actually pins.99a872220is its companion: a genuinely legacy image — no generation stamp at all — still returns 200, which is what stops this being a real regression for existing documents.RAG impact: no retrieval behaviour change — cache keys and eviction order only; no ranking, ordering or selection change.
7b3409843ande7de80f94touch the answer cache, which is RAG-adjacent. The FIFO→LRU change alters which entries survive eviction, ande7de80f94stops a failed indexing-stamp read being cached under the success key. Neither touches retrieval, ranking, ordering or selection, and neither changes the content of any answer that is produced — only whether a previously computed one is still in the process-local cache. The offline RAG evals ran inside the gate below and are green.Verification
npm run verify:pr-local— result:- completed: check:runtime, check:installed-lock-parity, format:changed, check:diff-integrity, lint, typecheck, test, check:repo-awareness-snapshot, build, eval:rag:offline, eval:rag:adversarial:offline, check:medication-interactions, check:medication-lexicon-report·- failed: (none)·- not reached: (none)·Test Files 1130 passed | 1 skipped (1131)·Tests 14980 passed | 2 expected fail | 3 skipped (14985)(the gate runner recorded exit code 0)npm run eval:rag:offlineandnpm run eval:rag:adversarial:offline— both ran inside this gate and are in the completed list above. They are the offline fixture-corpus evals, not the provider-backed canary.npm run check:diff-integrity—[diff-integrity] PASS — 8 changed test file(s), 171 -> 189 test case(s), against base 51ddfcd83.No floor raised; net-additive.Verification not run: the provider-backed eval canary — no live before/after pair was dispatched, because this is not a retrieval/ranking/ordering behaviour change and the
RAG impact:line above claims none. If a reviewer disagrees that cache eviction order is behaviour-neutral for retrieval, that judgement should be settled before merge and a canary pair run.Verification not run:
npm run verify:ui— browser proof left to CI; no narrowed browser run is claimed as the full gate.Verification not run:
npm run verify:release, and every other provider-backed gate — all work was offline.Risk and rollout
Clinical Governance Preflight
Each item confirmed against this package's diff. The checked line is the exact policy item; the note beneath it is the evidence.
no change to source verification or citation requirements.
none introduced or expanded;
73f17aaa5narrows exposure by stopping chunk text being echoed into a warning log.Clinical KB Database(sjrfecxgysukkwxsowpy)no Supabase env value, migration target or configured project changes; no file under
supabase/migrations/**is touched.no client exposure of service-role credentials; the private-access predicate is made stricter, not looser.
unchanged.
made more conservative — a failed indexing-stamp read is no longer cached as a success.
reviewed; these are ingestion-reliability and cache-hygiene fixes and add no decision-support behaviour.
Notes
🤖 Generated with Claude Code
https://claude.ai/code/session_01DHSyfuC6mS98ystWFiitAR
Generated by Claude Code
Note
Medium Risk
Changes sit on the ingestion write path, job-queue claiming, and cache staleness guards; mistakes could block indexing or hide staged content, though each change degrades toward safer failure modes with broad test coverage.
Overview
Audit remediation P14 hardens ingestion, indexing agents, RAG process-local caches, and a few lab utilities so one bad unit no longer takes down a whole document or claimed batch, and stale or uncommitted artifacts are treated more conservatively.
Ingestion reliability: The container worker skips individual images when the vision provider rejects them non-retryably (placeholder row + content-free
skip_reason), while rate limits and unknown errors still fail the job. The PDF extractor normalizes embedded streams to web-safe PNG/JPEG (with per-image caps and skip warnings) so MIME types likeimage/jpxdo not poison the whole upload. The dormantingestion-workerEdge Function returns 410 before claiming queue rows, blocking falseindexedstamps from its broken backfill path.Indexing-v3-agent: Claim handling is POST-only,
?limit=is parsed safely (noNaNRPC casts), andrunClaimedJobBatchwraps both job processing andmarkJobFailureso a failure-recording throw cannot 500 the request and leave sibling jobs locked inprocessing.RAG cache hygiene (keys/eviction only): A failed documents indexing-stamp query gets a distinct
index-stamp-unavailablestamp (not “empty corpus”); reads and writes refuse to trust or populate cache under that stamp. Answer and search caches bump LRU recency on hit and refresh so hot entries are not FIFO-evicted first.Generation/commit alignment (L11):
isCommittedGenerationMetadatanow fails closed when a row carries a generation but the parent document has none—matching SQL—so the viewer no longer serves staged images/chunks that search excludes; tests split true legacy (unstamped image) from staged stamped rows (404).Smaller fixes: Docling legacy harness reads table
row_count/column_count; assertion-tagging warnings log chunk index only, not clinical text.Reviewed by Cursor Bugbot for commit 220cd84. Configure here.