You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #287 fixed a deadlock in batch_complete_jobs by inlining the SELECT used by validate_run_id directly against the active transaction (&mut **tx), since the original helper takes a fresh pool connection and would re-deadlock the handler.
That left two near-duplicate copies of the same logic:
src/server/http_server/runtime_support.rs::validate_run_id — used by manage_job_status_change (lifecycle_support.rs:63) and the non-batch apply_job_completion_state path.
An inline equivalent in src/server/http_server/jobs_transport.rs::apply_job_completion_state_tx, written against tx.
This was raised by Copilot on PR #287 (review comment) and deferred from the hotfix to keep the patch's scope minimal during the production incident. Now that the hotfix has shipped, the duplication should be consolidated before the two implementations drift (e.g., a future tweak to error mapping or an extra validation rule lands in only one of them).
Proposed change
Refactor validate_run_id to accept a generic SQLx executor so the same body works for both a borrowed pool and an open transaction:
manage_job_status_change and the non-batch completion path call validate_run_id(&*self.pool, ...).
apply_job_completion_state_tx calls validate_run_id(&mut **tx, ...) and drops its inline copy.
Acceptance criteria
Single implementation of run_id validation; no inlined copy in apply_job_completion_state_tx.
Both pool-based and transaction-based call sites use the shared helper.
Existing tests still pass; tests/test_batch_complete_jobs.rs::test_batch_complete_jobs_does_not_deadlock_in_memory still catches the deadlock regression.
Out of scope
Other places where the same anti-pattern could exist. An audit on PR Fix regression in batch_job_complete #287 confirmed no remaining instances; this issue is purely about the duplication introduced by that PR.
Background
PR #287 fixed a deadlock in
batch_complete_jobsby inlining the SELECT used byvalidate_run_iddirectly against the active transaction (&mut **tx), since the original helper takes a fresh pool connection and would re-deadlock the handler.That left two near-duplicate copies of the same logic:
src/server/http_server/runtime_support.rs::validate_run_id— used bymanage_job_status_change(lifecycle_support.rs:63) and the non-batchapply_job_completion_statepath.src/server/http_server/jobs_transport.rs::apply_job_completion_state_tx, written againsttx.This was raised by Copilot on PR #287 (review comment) and deferred from the hotfix to keep the patch's scope minimal during the production incident. Now that the hotfix has shipped, the duplication should be consolidated before the two implementations drift (e.g., a future tweak to error mapping or an extra validation rule lands in only one of them).
Proposed change
Refactor
validate_run_idto accept a generic SQLx executor so the same body works for both a borrowed pool and an open transaction:Then:
manage_job_status_changeand the non-batch completion path callvalidate_run_id(&*self.pool, ...).apply_job_completion_state_txcallsvalidate_run_id(&mut **tx, ...)and drops its inline copy.Acceptance criteria
apply_job_completion_state_tx.tests/test_batch_complete_jobs.rs::test_batch_complete_jobs_does_not_deadlock_in_memorystill catches the deadlock regression.Out of scope