Repository navigation
fix(worker): re-check document and allowlist scope after claim_ingestion_jobs row lock - #3633
Conversation
…ion_jobs row lock Follow-up to #3628. The scoped claim picked candidates from a snapshot and then locked the job row, but 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 check and sibling-lease check, and could sit outside the worker's document allowlist. New migration 20261018053558 replaces the five-argument claim_ingestion_jobs in place (same signature, grants and behaviour) and, after the job row lock, skips the job unless it still belongs to the picked document and is still inside p_allowed_document_ids. A skipped job stays pending with no lease and no consumed attempt. tests/sql/ingestion-claim-recheck-after-lock.sql wraps the authority lock to re-point the job between pick and lock. It fails against the #3628 function (all three re-point cases are leased) and passes with this one. schema.sql mirrors the migration; the hash is sealed with npm run migrations:seal (append-only). The drift manifest follows from the Migration replay artifact.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
BigSimmo has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: rechecking document and allowlist scope after the claim job row lock. |
| Description check | |
The description follows the required template. It includes the summary, areas touched, verification evidence, risk, rollback, provider effects, migration rollout restrictions, and notes. It also ident… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description follows the required template. It includes the summary, areas touched, verification evidence, risk, rollback, provider effects, migration rollout restrictions, and notes. It also identifies the reported stale drift-manifest test result and the required database-change window.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ast-grep (0.45.3)
-
supabase/schema.sql
-
ast-grep skipped this file: it is too large to scan (17910207 bytes)
-
Comment @coderabbitai help to get the list of available commands.
|
Updates to Preview Branch (fix/claim-ingestion-recheck-after-lock-1333) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
codeant-ai
Bot
added
the
size:L
PR Summary by QodoRecheck ingestion job scope after locking its row
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
Code Review by Qodo
No findings for this group No findings for this group No findings for this group Resolved findings 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c9b7c374e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CodeAnt PR Risk: Low Risk
Assessed commit: |
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
|
Auto-fix isn't included in your plan ↗ |
Byte-identical to the drift-manifest-39101323185 artifact that scripts/generate-drift-manifest.ts (npm run drift:manifest) produced in the Migration replay job of run 39101323185 for 0c9b7c3. Only claim_ingestion_jobs(text,integer,integer,uuid[],text[]) def_hash and the schema sha change; no hand edits.
Code Review ✅ Approved🔴 High risk · The migration changes ingestion-job document-scope authorization; a faulty recheck could let workers lease jobs outside their allowlist or under stale authority. Adds a post-lock re-check to the VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Approved For more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
|
Code review by qodo was updated up to the latest commit 63467e0 |
…row lock (#3636) * fix(worker): re-check document after the legacy claim_ingestion_jobs row lock Follow-up to #3633 for the three-argument claim_ingestion_jobs(text, integer, integer). It picked candidates from a snapshot, locked the picked document's authority and sibling leases, then locked the job row without checking that the row still belonged to the picked document, so a job re-pointed in that window was leased under the old document's authority. New migration 20261018062736 replaces the three-argument function in place (same signature, defaults, grants and behaviour) and, after the job row lock, skips the job unless it still belongs to the picked document. A skipped job stays pending with no lease and no consumed attempt. tests/sql/ingestion-legacy-claim-recheck-after-lock.sql wraps the authority lock to re-point the job between pick and lock. It fails against the 20261013075944 function and passes with this one. schema.sql mirrors the migration; the hash is sealed with npm run migrations:seal (append-only). The drift manifest follows from the Migration replay artifact. * chore(db): regenerate drift manifest for legacy claim_ingestion_jobs recheck Byte-identical to the drift-manifest-39104250354 artifact that scripts/generate-drift-manifest.ts (npm run drift:manifest) produced in the Migration replay job of run 39104250354 for be0b278. Only claim_ingestion_jobs(text,integer,integer) def_hash and the schema sha change; no hand edits. --------- Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
User description
Summary
claim_ingestion_jobs(text, integer, integer, uuid[], text[])picks candidates from a snapshot, takes the document authority lock, then locks the job row withFOR 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.supabase/migrations/20261018053558_recheck_ingestion_claim_scope_after_job_lock.sqlreplaces the five-argument function in place. After the job row lock it skips the job unlessv_job.document_idstill equals the picked document and is still insidep_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.tests/sql/ingestion-claim-recheck-after-lock.sql(run by the Migration replay job with everytests/sql/*.sql). It wrapslock_ingestion_document_authorityinside 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.supabase/schema.sqlmirrors the migration (same pattern as fix(rag,worker): telemetry, document attribution, comparator parity, and atomic claim scope #3628),supabase/applied-migration-hashes.jsonvianpm run migrations:seal(append-only, no existing entry changed).supabase/drift-manifest.jsonis taken byte-for-byte from this PR's Migration replay artifact (npm run drift:manifestneeds Docker, which isn't available locally).database.types.tsis unchanged because the signature is unchanged.Areas touched: Data platform and access
Verification
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.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.lock_ingestion_document_authority.Risk and rollout
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.Notes
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_jobswhere 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.tests/sql/ingestion-claim-recheck-after-lock.sqlthat 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.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
Written for commit 63467e0. Summary will update on new commits.
CodeAnt-AI Description
Prevent ingestion jobs from being claimed under a stale document
What Changed
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:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
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:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
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