worker: autonomously dispatch scheduled File imports through exact leases - #92
Conversation
📝 WalkthroughWalkthroughThe PR adds autonomous scheduled File-import dispatch through a dedicated PostgreSQL scheduler role, atomic exact-lease claiming, one-cycle and long-running worker modes, server-owned root configuration, security catalog updates, and unit/integration/process coverage. ChangesScheduled File dispatch
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
migrations/versions/20260725_0033_autonomous_file_dispatch.py (1)
128-209: 🚀 Performance & Scalability | 🔵 TrivialConsider an index supporting the global oldest-eligible scan.
The selector joins nine relations and orders by
scheduled.accepted_at, scheduled.sequence, ...with no index added by this revision. At low job volumeLIMIT 1is cheap, but oncefile_import_jobaccumulates non-availablehistory, every dispatch cycle pays a growing scan. A partial index onfile_import_job (state, lease_generation)filtered tostate = 'available' AND lease_generation = 0, plus one onfile_source_acquisition_checkpoint (change_kind, accepted_at, sequence), would keep the cycle bounded.🤖 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 `@migrations/versions/20260725_0033_autonomous_file_dispatch.py` around lines 128 - 209, The selector for the global oldest eligible scan lacks supporting indexes and can scan growing historical tables. Add migration indexes for file_import_job covering the available, lease_generation = 0 predicate and for file_source_acquisition_checkpoint covering change_kind, accepted_at, and sequence, while preserving the existing ordering and selection logic.tests/integration/test_postgres_harness.py (1)
180-190: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe new file-dispatch definer is provisioned here but its role posture is never asserted.
file_dispatch_definer_roleis added to the contract, yet unlikeWORKER_LEASE_DEFINER_ROLE,CONTEXT_RUN_READER_DEFINER_ROLE,CITATION_DEFINER_ROLE, andACTION_EXECUTE_DEFINER_ROLE, it is not covered by the drop/recreate list, themissing_rolescount, or thefactsquery that provesrolcanlogin = false, no superuser/bypassrls, and exactly one migrator membership. Since this PR introduces aSECURITY DEFINERclaim authority, that posture is the control worth pinning.Extending the existing
factsassertion (or adding a small dedicated query) to coverFILE_DISPATCH_DEFINER_ROLEwould close the gap. Want me to draft it?🤖 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 `@tests/integration/test_postgres_harness.py` around lines 180 - 190, Extend the role-posture assertions in the PostgreSQL harness to include FILE_DISPATCH_DEFINER_ROLE alongside the existing definer roles: add it to the drop/recreate coverage and missing_roles accounting, then include it in the facts query/assertion verifying no login, no superuser or bypassrls privileges, and exactly one migrator membership.applications/worker.py (2)
219-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSigning-key parsing is duplicated instead of reused.
_worker_signing_key()re-implements the exact hex-decode/length-validation logic that already exists inline in_run_one_file_import(unchanged, ~lines 139-150). Having two independently-maintained copies of security-sensitive signing-key validation risks divergence if one is updated and the other isn't.♻️ Reuse the extracted helper in `_run_one_file_import`
def _run_one_file_import() -> int: """Consume one exact, signed File job in the independent Supply process.""" - signing_key_hex = _required_environment( - "CONTEXT_ENGINE_WORKER_LEASE_SIGNING_KEY_HEX" - ) - if len(signing_key_hex) != 64: - raise ValueError("Supply worker configuration is not available") - try: - signing_key = bytes.fromhex(signing_key_hex) - except ValueError: - raise ValueError("Supply worker configuration is not available") from None - if len(signing_key) != 32: - raise ValueError("Supply worker configuration is not available") + signing_key = _worker_signing_key() configuration = load_database_configuration(DatabasePurpose.SUPPLY_WORKER)🤖 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 `@applications/worker.py` around lines 219 - 232, Update _run_one_file_import to obtain the signing key through the existing _worker_signing_key helper instead of performing its own hex decoding and length validation. Remove the duplicated inline parsing while preserving the current signing behavior.
82-116: 🩺 Stability & Availability | 🔵 Trivial
authority.claim()failures are unhandled and will crash the long-running dispatch loop.
dispatch_one_file_importonly guards theworker_factory(...).run(...)call against(FileImportUnavailable, WorkNotAvailable); the precedingauthority.claim()call is unguarded. SincePostgreSQLFileDispatchAuthority.claim()maps anySQLAlchemyError(e.g. a transient connection blip) toWorkerLeaseAuthorityUnavailable, that exception propagates straight throughdispatch_file_imports_until_stopped's loop and terminates the entire--dispatch-filesprocess rather than backing off and retrying.This may be intentional (fail loud on scheduler-role misconfiguration is arguably correct), and ADR-0059's "Revisit trigger" section already defers retry/backoff to future work, so I'm not blocking on this — but worth confirming this crash-on-transient-failure behavior for the continuous mode is the desired operational posture, since it means a single flaky DB connection kills a process meant to run indefinitely.
🤖 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 `@applications/worker.py` around lines 82 - 116, Confirm the intended continuous-dispatch behavior for authority.claim() failures in dispatch_one_file_import: determine whether WorkerLeaseAuthorityUnavailable should propagate and terminate dispatch_file_imports_until_stopped or be caught and converted into a retryable outcome with the appropriate backoff. If resilience is required, update the claim path and cycle result handling while preserving fail-fast behavior for configuration errors.docs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.md (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd ADR-0055 to the decisions README.
0055-schedule-accepted-file-observations-explicitly.mdexists, butdocs/decisions/README.mdhas no entry for it, so the “Refines” list leaves this referenced ADR invisible from the decision index.🤖 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 `@docs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.md` around lines 11 - 13, Add an entry for ADR-0055 to docs/decisions/README.md, using the existing decision-index format and the title from 0055-schedule-accepted-file-observations-explicitly.md, so the referenced ADR appears in the decisions index.scripts/provision_database_roles.py (1)
365-660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrowing role-provisioning boilerplate could be collapsed into data-driven loops.
provision_security_rolesnow repeats near-identicalALTER ROLE ... WITH LOGIN PASSWORD .../... WITH NOLOGIN .../_revoke_roles_granted_to/_revoke_members_of/GRANT ... TO migrator_roleblocks for 18 roles (two more added by this PR). A table-driven loop over(role, password)pairs for LOGIN roles and a list of NOLOGIN definer roles would remove most of this duplication and reduce the risk of a future role being added inconsistently (as happened here where CONNECT is correctly withheld fromfile_dispatch_definer_rolebut that's easy to get wrong by copy-paste).🤖 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 `@scripts/provision_database_roles.py` around lines 365 - 660, Refactor provision_security_roles to use data-driven collections and loops for the LOGIN role/password ALTER ROLE statements, NOLOGIN definer roles, role cleanup via _revoke_roles_granted_to and _revoke_members_of, and migrator grants. Keep CONNECT grants limited to the intended LOGIN roles, using the role collections to prevent NOLOGIN definers such as file_dispatch_definer_role from receiving CONNECT.
🤖 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`:
- Around line 318-323: Remove the unconditional UPDATE grants for
context_engine_file_dispatch_definer on membership, context_source, and
service_principal. In the migration grant definitions, restrict this role to the
read-only access required by context_scheduler_claim_file_import; if any updates
are necessary, scope them to the selecting organization rather than using USING
(true) WITH CHECK (true), and preserve mutation access only for file_import_job.
In `@eval/catalogs/security-invariants.yaml`:
- Around line 26-27: Add the missing `#91` reconciliation sentence to the
authority.reconciliation string, matching the issueRefs entry and stating that
scheduler-only first-attempt dispatch is active while reclaim, retry/backoff,
dead-letter, provider polling, and delete ordering are NOT_ACTIVE.
In `@migrations/versions/20260725_0033_autonomous_file_dispatch.py`:
- Around line 58-80: Remove the authority-row-level FOR UPDATE locks on source,
audience, and receiver from the SECURITY DEFINER dispatch path, relying on the
existing organization/job source progress lock and lease-redemption recheck to
satisfy the fence. Update the associated query or function while preserving
tenant isolation and existing lock behavior for the progress and lease rows.
In `@tests/integration/test_file_dispatch.py`:
- Around line 1165-1168: Fix the polling loop around pending_claim so it waits
while the future has not completed, up to the deadline, then assert that
pending_claim is still incomplete only after allowing the worker to run. Prefer
the sibling test’s established waiting-observation approach or pg_locks helper
to positively verify the scheduler is blocked on the advisory lock.
- Around line 1516-1533: Update the subprocess assertions around the worker
readiness and completion reads to use deadline-bounded I/O instead of unbounded
process.stdout.readline() calls. Ensure teardown drains stdout and stderr
without pipe-buffer deadlocks, using terminate() followed by
communicate(timeout=...) or equivalent timeout handling, and make timeout or
early-exit conditions fail the test rather than hang CI.
In `@tests/integration/test_migrations.py`:
- Around line 1165-1176: The downgrade privilege assertion in
tests/integration/test_migrations.py#L1165-L1176 must enumerate every public
table and column privilege for context_engine_file_dispatch_definer, including
context_source, source_version, and service_principal, and assert none remain.
In tests/unit/test_schema_security_manifest.py#L916-L919, add the dispatch
role’s expected source_version permission, or explicitly verify that it is
intentionally unnecessary.
---
Nitpick comments:
In `@applications/worker.py`:
- Around line 219-232: Update _run_one_file_import to obtain the signing key
through the existing _worker_signing_key helper instead of performing its own
hex decoding and length validation. Remove the duplicated inline parsing while
preserving the current signing behavior.
- Around line 82-116: Confirm the intended continuous-dispatch behavior for
authority.claim() failures in dispatch_one_file_import: determine whether
WorkerLeaseAuthorityUnavailable should propagate and terminate
dispatch_file_imports_until_stopped or be caught and converted into a retryable
outcome with the appropriate backoff. If resilience is required, update the
claim path and cycle result handling while preserving fail-fast behavior for
configuration errors.
In `@docs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.md`:
- Around line 11-13: Add an entry for ADR-0055 to docs/decisions/README.md,
using the existing decision-index format and the title from
0055-schedule-accepted-file-observations-explicitly.md, so the referenced ADR
appears in the decisions index.
In `@migrations/versions/20260725_0033_autonomous_file_dispatch.py`:
- Around line 128-209: The selector for the global oldest eligible scan lacks
supporting indexes and can scan growing historical tables. Add migration indexes
for file_import_job covering the available, lease_generation = 0 predicate and
for file_source_acquisition_checkpoint covering change_kind, accepted_at, and
sequence, while preserving the existing ordering and selection logic.
In `@scripts/provision_database_roles.py`:
- Around line 365-660: Refactor provision_security_roles to use data-driven
collections and loops for the LOGIN role/password ALTER ROLE statements, NOLOGIN
definer roles, role cleanup via _revoke_roles_granted_to and _revoke_members_of,
and migrator grants. Keep CONNECT grants limited to the intended LOGIN roles,
using the role collections to prevent NOLOGIN definers such as
file_dispatch_definer_role from receiving CONNECT.
In `@tests/integration/test_postgres_harness.py`:
- Around line 180-190: Extend the role-posture assertions in the PostgreSQL
harness to include FILE_DISPATCH_DEFINER_ROLE alongside the existing definer
roles: add it to the drop/recreate coverage and missing_roles accounting, then
include it in the facts query/assertion verifying no login, no superuser or
bypassrls privileges, and exactly one migrator membership.
🪄 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: d3a36b9b-8407-41e6-a0c8-c7a39cf96115
📒 Files selected for processing (35)
README.mdapplications/worker.pycompose.yamldocs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.mddocs/decisions/README.mdengine/persistence/__init__.pyengine/persistence/configuration.pyengine/persistence/role_guard.pyengine/persistence/schema_security_manifest.yamlengine/persistence/worker_jobs.pyeval/catalogs/m0-security-evidence.schema.jsoneval/catalogs/m0-security-evidence.yamleval/catalogs/security-catalog.schema.jsoneval/catalogs/security-invariants.yamlinfra/postgres/init/10-security-roles.shmigrations/versions/20260725_0033_autonomous_file_dispatch.pyscripts/database_harness.shscripts/provision_database_roles.pyscripts/security_gate/runner.pyscripts/validate_security_catalog.pyscripts/wait_for_database.pytests/catalog/test_m0_security_gate.pytests/catalog/test_validate_security_catalog.pytests/integration/conftest.pytests/integration/test_file_dispatch.pytests/integration/test_membership_schema.pytests/integration/test_migrations.pytests/integration/test_postgres_harness.pytests/support/migrations.pytests/unit/test_database_configuration.pytests/unit/test_database_harness_contract.pytests/unit/test_file_dispatch.pytests/unit/test_learning_database_configuration.pytests/unit/test_learning_database_role_provisioning.pytests/unit/test_schema_security_manifest.py
|
Addressed the review pass in 3ea5170. Implemented the actionable fixes: bounded/positive concurrency and process-I/O tests, complete downgrade privilege enumeration, explicit source_version manifest permission, dispatch definer posture coverage, scheduler scan indexes, #91 reconciliation, ADR-0055 index entry, and shared signing-key parsing. Kept two intentionally coupled authority controls: the column-level UPDATE grants and FOR UPDATE authority-row locks prevent Membership/Source/ServicePrincipal revocation races during final validation and lease issuance. Also kept fail-fast authority-unavailable behavior because ADR-0059 explicitly leaves retry/backoff NOT_ACTIVE, and deferred the broad provisioning refactor as unrelated scope. Fresh verification: make db-reset plus make check passed (1290 unit, 131 catalog, 6 process, 426 integration with 4 deselected, and 151 M0 security-gate tests). |
![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: 3ea5170b9e
ℹ️ 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".
|
Follow-up 57f09a3 addresses the post-push P1 review: worker infrastructure unavailability now fails fast after one claim instead of being converted to refused and claiming further jobs. Fresh clean-harness verification passed: make db-reset plus make check (1291 unit, 131 catalog, 6 process, 426 integration with 4 deselected, and 151 M0 security-gate tests). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57f09a3c66
ℹ️ 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".
|
Follow-up a0cc467 closes the post-review database-time issue: immediate autonomous WorkerLease verification now uses the worker PostgreSQL clock rather than host time. Fresh clean-harness DoD is green (1292 unit, 131 catalog, 6 process, 426 integration / 4 deselected, 151 M0 security gate). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0cc4678a5
ℹ️ 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".
|
Follow-up 42ff755 distinguishes terminal per-job File refusal from infrastructure failure. Terminal failures continue the loop only after exact durable failure or current-authority rejection; failure-recording/worker infrastructure outages still fail fast. Fresh clean-harness DoD is green (1293 unit, 131 catalog, 6 process, 426 integration / 4 deselected, 151 M0 security gate). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ff755ee7
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/persistence/file_imports.py (1)
147-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the refusal contract.
FileImportRefusedis also raised when_fail()getsWorkNotAvailable(suppressed at Lines 272-281), so the job is not necessarily durably sealed. Describe both durable sealing and current-authority rejection.Proposed fix
-class FileImportRefused(FileImportUnavailable): - """Content-free refusal after the exact job was durably sealed as failed.""" +class FileImportRefused(FileImportUnavailable): + """Content-free terminal refusal after sealing or authority rejection."""🤖 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 147 - 148, Update the FileImportRefused class docstring to state that it represents a content-free refusal both after the exact job is durably sealed as failed and when current authority rejects the job, including the WorkNotAvailable path handled by _fail().
🤖 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.
Nitpick comments:
In `@engine/persistence/file_imports.py`:
- Around line 147-148: Update the FileImportRefused class docstring to state
that it represents a content-free refusal both after the exact job is durably
sealed as failed and when current authority rejects the job, including the
WorkNotAvailable path handled by _fail().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9013ded9-5fb6-468b-a739-3898430ad2ed
📒 Files selected for processing (9)
README.mdapplications/worker.pydocs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.mdengine/persistence/__init__.pyengine/persistence/file_imports.pyengine/persistence/schema_security_manifest.yamlengine/persistence/worker_jobs.pytests/integration/test_file_dispatch.pytests/unit/test_file_dispatch.py
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- engine/persistence/init.py
- docs/decisions/0059-dispatch-scheduled-file-imports-through-exact-leases.md
- engine/persistence/worker_jobs.py
- tests/integration/test_file_dispatch.py
- engine/persistence/schema_security_manifest.yaml
1 participant
Closes #91
Summary
Security properties
Verification
make db-reset && PATH="/tmp/context-engine-node2212:$PATH" make checkM0 SECURITY PASS)Summary by CodeRabbit
New Features
--dispatch-file-onceand--dispatch-files, including deterministic lease claiming, safe concurrency, and clean SIGTERM/SIGINT shutdown.dispatched,no_work, orrefusedoutcomes.Documentation
Tests