refactor(jobs): extract admission operations#2611
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
6ec98f2 to
6d263f1
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the Jobs create/admission transaction path by extracting it from JobManager.create_job into backend-specific SQLite and Postgres operation modules, while keeping JobManager as the public facade. It also introduces durable-event support for idempotent existing results, direct SQLite operation tests, and shared parity coverage. The review feedback highlights two issues with request and trace ID propagation: first, in the SQLite idempotent replay branch, in-process event listeners receive stale IDs because the current replay context is not overridden; second, in the non-idempotent Postgres path, using row.get for request and trace IDs instead of the command values directly could lead to missing context if the row is empty or mocked.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| else: | ||
| with contextlib.suppress(_JOB_NONCRITICAL_EXCEPTIONS): | ||
| emit_job_event("job.created", job=row, attrs=attrs) |
There was a problem hiding this comment.
In the SQLite idempotent replay branch, when outbox_enabled is False, emit_job_event is called with job=row instead of overriding the request_id and trace_id with the current replay context. This causes in-process event listeners to receive the stale/original request and trace IDs rather than the current ones, which is inconsistent with the audit event emission on line 1188 and the Postgres implementation.
| else: | |
| with contextlib.suppress(_JOB_NONCRITICAL_EXCEPTIONS): | |
| emit_job_event("job.created", job=row, attrs=attrs) | |
| else: | |
| with contextlib.suppress(_JOB_NONCRITICAL_EXCEPTIONS): | |
| emit_job_event( | |
| "job.created", | |
| job={**row, "request_id": request_id, "trace_id": trace_id}, | |
| attrs=attrs, | |
| ) |
There was a problem hiding this comment.
Fixed in e8d7ae2. The SQLite idempotent replay path now builds the in-process emitted job from the returned row plus the durable event's current request_id/trace_id, so outbox-disabled fanout observes the replay context while the returned row still preserves the original idempotent job context. Added test_idempotent_replay_in_process_event_uses_current_context_sqlite and reran the focused Jobs latest-dev validation.
| request_id=row.get("request_id"), | ||
| trace_id=row.get("trace_id"), |
There was a problem hiding this comment.
In the non-idempotent Postgres admission path, the transactional job.created event is created using row.get("request_id") and row.get("trace_id"). If row is empty/mocked (e.g., if _insert_job returns None), these fields will be None instead of falling back to the current context. It is safer and more consistent with the SQLite implementation to use command.request_id and command.trace_id directly.
| request_id=row.get("request_id"), | |
| trace_id=row.get("trace_id"), | |
| request_id=command.request_id, | |
| trace_id=command.trace_id, |
There was a problem hiding this comment.
Fixed in e8d7ae2. The Postgres non-idempotent admission path now writes the durable job.created event with CreateJobCommand.request_id and trace_id rather than row fields, which keeps fake/fallback rows from dropping event context. Added test_pg_non_idempotent_create_uses_command_context_for_job_created_event and reran the focused Jobs latest-dev validation.
1056464 to
57607ba
Compare
57607ba to
1f8f848
Compare
Change summary
Human-written Change summary required before this draft PR is merge-ready under
Docs/superpowers/AI_GENERATED_PR_CHANGE_SUMMARY_POLICY_2026_04_17.md.What changed
Extracts Jobs admission/create behavior behind backend-specific operation modules while keeping
JobManager.create_jobas the public facade.Backlog tracking:
TASK-12147.Latest dev refresh
devat6b727b221e55646eba663a03571e38302f7fafc2.codex/jobs-admission-operations-extractionat1f8f8483e6b401784e0347d927bdd3fc472a95b4.6b727b221e55646eba663a03571e38302f7fafc2.Validation
# nosec B608warnings only (/tmp/bandit_jobs_admission_origin_dev_6b727b.json).git diff --check HEAD~1..HEAD-> clean.