worker: recover File publication idempotently - #59
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis change adds durable file-publication recovery using lease generations, checkpoint and event tables, generation-fenced PostgreSQL procedures, worker interruption injection, and integration/security coverage for recovery, concurrency, isolation, and migration behavior. ChangesFile publication recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: d6d2d67d3f
ℹ️ 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".
| AND fragment.projection_kind = 'body' | ||
| ) | ||
| ) | ||
| AND NOT EXISTS ( |
There was a problem hiding this comment.
If an active revision has any extra exact_phrase_candidate rows, this new no-op validation still succeeds because it only proves each expected phrase from the freshly compiled artifact exists and never compares the total candidate set. The previous v1/v2 no-op checks rejected that case, and accepting it here means a stale or corrupted exact-phrase index can be treated as an unchanged active artifact instead of failing closed.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
engine/persistence/file_imports.py (1)
496-525: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: the concurrent-winner wait is a bounded blocking busy-poll.
_await_concurrent_publicationoccupies the worker thread for up to_CONCURRENT_PUBLICATION_WAIT_SECONDSand opens a freshbegin()/acquiretransaction every 10ms until the winner's recovery row reachescompleted. It's correct and bounded, but under many simultaneous contended imports this both pins worker threads and adds per-poll transaction churn oncontext_worker_acquire_file_publication. If contention volume grows, consider surfacing wait duration/poll count as a metric and/or a small backoff so the hot path is observable and tunable without a code change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/persistence/file_imports.py` around lines 496 - 525, Make the bounded wait in _await_concurrent_publication observable and less transaction-intensive by tracking wait duration or poll count and applying a tunable backoff between context_worker_acquire_file_publication attempts. Preserve the existing deadline, retry behavior, and _rejection(token) outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@engine/persistence/schema_security_manifest.yaml`:
- Line 163: Update the idempotencyBinding entry in the schema security manifest
to remove lease_generation while retaining organization_id, job_id, source_id,
resource_ref, revision_id, content_identity_digest, and
publication_payload_digest; lease_generation must remain available only for
fencing and authorization.
- Around line 149-162: The worker-owned recovery manifest entry must not include
context_worker_issue_file_import_lease, which belongs to the Control phase.
Remove that function from the databaseFunctions list, preserving lease issuance
as an explicit context_engine_control operation before Worker execution.
---
Nitpick comments:
In `@engine/persistence/file_imports.py`:
- Around line 496-525: Make the bounded wait in _await_concurrent_publication
observable and less transaction-intensive by tracking wait duration or poll
count and applying a tunable backoff between
context_worker_acquire_file_publication attempts. Preserve the existing
deadline, retry behavior, and _rejection(token) outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce63c2c4-60c0-45d9-911b-05d2b413575f
📒 Files selected for processing (21)
CONTEXT.mddocs/decisions/0041-recover-file-publication-by-durable-boundary.mddocs/decisions/README.mddocs/design/2026-07-18-context-engine-implementation-design.mdengine/persistence/__init__.pyengine/persistence/file_imports.pyengine/persistence/schema_security_manifest.yamlengine/persistence/worker_jobs.pyengine/supply/jobs.pyeval/catalogs/m0-security-evidence.yamlmigrations/versions/20260723_0015_file_publication_recovery.pyscripts/security_gate/rls.pytests/integration/test_file_import_tracer.pytests/integration/test_m0_security_gate_rls.pytests/integration/test_migrations.pytests/integration/test_zz_file_content_noop.pytests/integration/test_zz_file_publication_recovery.pytests/integration/test_zz_file_revision_replacement.pytests/unit/test_m0_rls_inventory.pytests/unit/test_schema_security_manifest.pytests/unit/test_worker_lease.py
d6d2d67 to
70eed6b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70eed6bc40
ℹ️ 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".
| OR recovery_row.compiler_version <> | ||
| requested_compiler_version | ||
| OR recovery_row.config_version <> requested_config_version | ||
| OR recovery_row.publication_payload_digest <> ({payload_digest}) |
There was a problem hiding this comment.
If a file import reaches a durable recovery checkpoint and then the underlying file is edited or removed before the lease is reclaimed, the resumed worker recompiles the current file and this digest check rejects it instead of resuming the already-recorded publication. Because context_worker_fail_file_import also refuses to fail jobs that have a recovery row, the job remains in running/prepared/ready and the non-completed recovery row keeps later imports for the same resource in the contended path until they time out. Recovery needs to use stored publication data or otherwise complete/fail the durable row without depending on the mutable live file contents.
Useful? React with 👍 / 👎.
1 participant
Closes #27
Summary
Verification
make db-reset && make checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation