fix(engine): isolate operation completion from best-effort side-effects - #2823
Merged
Conversation
execute_task completes an operation via _mark_operation_completed / _mark_operation_completed_and_fire_webhook, both of which wrapped the status='completed' commit in one transaction with fallible side-effects (webhook outbox insert, parent aggregation) and swallowed every exception. A hiccup in either rolled the completion back and dropped the error, leaving the operation stuck in 'processing' forever while the log already said the work was done (#2601). PR #2608 added a poller-side backstop that unstuck the row but silently lost the consolidation webhook. - On failure of the atomic outbox transaction, fall back to a completion-only commit and fire the consolidation webhook best-effort (non-transactional) instead of losing both. Happy path keeps the transactional-outbox guarantee; the failure path degrades to completed + delivered rather than stuck + lost. The best-effort fire only runs when the fallback actually transitioned the row, so there is no duplicate delivery. - Guard every completion UPDATE on `status NOT IN ('completed','failed', 'cancelled')` so an already-terminal row is never re-terminalized: keeps the engine idempotent with the poller backstop (#2608) and avoids double parent aggregation, while still completing pending/processing rows. Adds fast DB-free regression tests (fake connections) covering the happy path (no double-fire), the webhook-failure fallback, and the terminal-row no-op guard.
nicoloboschi
force-pushed
the
fix/worker-completion-decouple
branch
from
July 20, 2026 12:15
d0c6494 to
3a038e7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #2608 that fixes the root cause of #2601 in the engine, rather than only backstopping the symptom in the poller.
execute_taskmarks an operation completed via_mark_operation_completed(most task types) or_mark_operation_completed_and_fire_webhook(consolidation). Both wrapped thestatus = 'completed'commit in one transaction with fallible side-effects — the webhook outbox insert and parent aggregation — and then swallowed every exception. So any hiccup in those side-effects rolled the completion back with them and dropped the error, leaving the operation stuck inprocessingforever while the log already said the work was done.#2608 added a poller-side completion backstop that unsticks the row after the executor returns, but it fires no webhook — so on that path the consolidation webhook was silently lost, and the engine's own completion
UPDATEhad nostatusguard, so the two completion writers could both run parent aggregation.Changes
UPDATEin the engine is now guarded onstatus = 'processing'(matching fix(worker): complete successful operations #2608's poller). Whichever writer runs second sees a non-processingrow, updates nothing, and does not re-run parent aggregation._fire_consolidation_webhook. The happy path keeps the full transactional-outbox at-least-once guarantee; the failure path degrades to "completed + delivered" instead of "stuck + lost". The best-effort fire only runs when the fallback actually transitioned the row, so there is no duplicate delivery.Testing
tests/test_operation_completion.py— fast, DB-free unit tests (fake asyncpg-style connections, same style as fix(worker): complete successful operations #2608'stest_worker.py):uv run pytest tests/test_operation_completion.py— 3 passeduv run ruff check/ruff format --check/ty check— clean on changed filesFixes #2601.