fix(worker): re-check document and allowlist scope after claim_ingestion_jobs row lock

Merged
BI
1mo ago
+239-6

User description

Summary

  • Follow-up to #3628. claim_ingestion_jobs(text, integer, integer, uuid[], text[]) picks candidates from a snapshot, takes the document authority lock, then locks the job row with FOR UPDATE SKIP LOCKED, but it never checked that the locked row still belonged to the picked document. A job re-pointed at another document in that window was leased under the picked document's authority, file-type recheck and sibling-lease check, and could sit outside the worker's document allowlist.
  • New migration supabase/migrations/20261018053558_recheck_ingestion_claim_scope_after_job_lock.sql replaces the five-argument function in place. After the job row lock it skips the job unless v_job.document_id still equals the picked document and is still inside p_allowed_document_ids. A skipped job stays pending with no lease and no consumed attempt, the same as every other skip in the loop. Signature, grants, return shape and every other branch are unchanged. The three-argument legacy function is not touched.
  • New tests/sql/ingestion-claim-recheck-after-lock.sql (run by the Migration replay job with every tests/sql/*.sql). It wraps lock_ingestion_document_authority inside its rolled-back transaction so the job is re-pointed between the pick and the job row lock, which makes the interleaving deterministic. It covers a document-allowlist worker, a file-type-allowlist worker and an unscoped worker (new document in scope but never authority-locked), plus a control and a follow-up claim that leases the job under its new document.
  • Companion files: supabase/schema.sql mirrors the migration (same pattern as #3628), supabase/applied-migration-hashes.json via npm run migrations:seal (append-only, no existing entry changed). supabase/drift-manifest.json is taken byte-for-byte from this PR's Migration replay artifact (npm run drift:manifest needs Docker, which isn't available locally). database.types.ts is unchanged because the signature is unchanged.

Areas touched: Data platform and access

Verification

  • Checks run:
    • Offline harness (PGlite with a minimal stub of the touched tables and authority lock): against the #3628 function the new test fails on all three re-point cases (a job re-pointed outside the document allowlist after the pick must not be claimed, ...disallowed file type..., ...must not be leased under the old document's authority) and the control passes. With the new migration applied, the whole file passes.
    • node scripts/check-migration-immutability.mjs: Migration immutability guard passed: 320 shipped migrations unchanged.
    • check:migration-role: Hosted migration-role guard passed. check:function-grants: OK — all 130 SECURITY DEFINER public function(s) are revoked from PUBLIC.
    • Vitest supabase-schema, migration-sql, migration-immutability, migration-history-guards, hosted-migration-role-guard, worker-row-contract, drift-detection: 300/301 passed. The one failure was the expected stale drift manifest before the CI artifact landed.
    • Hosted proof: the Migration replay job runs the full chain and the new SQL test against the real lock_ingestion_document_authority.

Risk and rollout

  • Risk: Low. One extra predicate on the locked row inside an existing SECURITY DEFINER function. It can only make the claim skip a job that it previously leased by mistake.
  • Rollback: a later migration restoring the #3628 body. Any live database change needs its own owner approval.
  • Merge gate: this PR adds a migration under supabase/migrations/**. Merging to main applies it to the live clinical database within seconds and deploys. Merge only inside the owner's specifically approved database-change window. Auto-merge must stay off for this PR.
  • Provider or production effects: None. No live database, provider or deployment action was taken.

Notes

  • The legacy three-argument claim_ingestion_jobs(text, integer, integer) has the same document-identity gap (no allowlist, but it can still lease a re-pointed job under the old document's authority). It is out of scope here and could get a matching follow-up if wanted.

Summary by cubic

Fixes a race in claim_ingestion_jobs where a job re-pointed at a different document between the candidate pick and the job row lock could be leased under the wrong document's authority, bypassing file-type and document-allowlist checks.

  • Adds a migration that, after locking the job row, skips the claim unless the row still belongs to the picked document and remains inside the document allowlist. Skipped jobs stay pending with no lease and no consumed attempt.
  • Adds tests/sql/ingestion-claim-recheck-after-lock.sql that deterministically re-points a job mid-claim and covers document-allowlist, file-type-allowlist, and unscoped workers, plus a follow-up claim under the new document.
  • Mirrors the migration in supabase/schema.sql, seals the new migration hash, and regenerates the drift manifest from the Migration replay artifact. The legacy three-argument function is untouched.

Migration

  • Merging applies the new migration to the live clinical database; merge only inside the owner's approved database-change window.

CodeAnt-AI Description

Prevent ingestion jobs from being claimed under a stale document

What Changed

  • Workers now skip a job if its document changes after the job is selected, or if the new document is outside the worker’s allowlist.
  • Skipped jobs remain pending without a lease or a consumed attempt, so a later claim can process them under the correct document.

Impact

✅ Jobs stay within document allowlists
✅ No leases under stale document authority
✅ Skipped jobs remain available for later claims

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • Bug Fixes
    • Ingestion jobs are now rechecked against their current document and file type before being claimed. Jobs that no longer match remain pending without a lease or consumed attempt; eligible jobs can be claimed under their current document.
  • Tests
    • Added regression coverage for jobs whose document changes during claim processing, along with a control case for unchanged jobs.