fix(worker): release operations when a task is cancelled or its terminal write fails - #3229
Open
russellbrenner wants to merge 1 commit into
Open
Conversation
…nal write fails Three paths let a task stop running while its async_operations row stays 'processing' owned by a live worker, permanently: 1. asyncio.CancelledError derives from BaseException, so it escapes every handler in _execute_task_inner; the done-callback then removes the task from _active_tasks and nothing in-process remembers the row. 2. _mark_failed in the 'except Exception' branch is itself a DB write; if it raises, the exception escapes with the row still claimed. 3. shutdown_graceful cancels remaining tasks after the drain timeout and returns immediately; the event loop may close before any cancelled task could reconcile its own row. recover_own_tasks only runs at startup, and no dead-worker logic applies because the worker is alive, so such rows are unreachable by any existing recovery path and clients polling them wait forever. Fix: a self-scoped release guarded on status='processing' AND worker_id=<self>, applied at all three sites. Only the worker's own local knowledge is used, the same authority recover_own_tasks already exercises. The retry_count increment mirrors recover_own_tasks (vectorize-io#2675/vectorize-io#2834) so an interrupted task still counts against its retry budget. Fixes vectorize-io#3228
There was a problem hiding this comment.
Pull request overview
This PR addresses a production issue where async_operations rows can remain stuck in status='processing' and owned by a live worker when an in-flight task exits early (cancellation) or its terminal DB write fails, causing clients polling the operation to wait indefinitely.
Changes:
- Add guarded “self-release” helpers in
WorkerPollerto return still-claimed operations back topendingwithretry_countincremented. - Ensure
asyncio.CancelledErrorand terminal-write failures release stranded operations rather than leaving them stuck inprocessing. - Add regression tests covering cancellation, terminal-write failure, and graceful shutdown cleanup behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hindsight-api-slim/hindsight_api/worker/poller.py | Adds self-scoped release logic and wires it into cancellation, terminal-write failure handling, and graceful shutdown. |
| hindsight-api-slim/tests/test_worker_task_release.py | Adds regression tests validating operations are returned to pending in the three previously-stranding paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+58
to
+64
| @pytest_asyncio.fixture | ||
| async def clean_operations(pool): | ||
| """Clean up async_operations table before and after tests.""" | ||
| await pool.execute("DELETE FROM async_operations WHERE status = 'pending'") | ||
| yield | ||
| await pool.execute("DELETE FROM async_operations WHERE bank_id LIKE 'test-worker-%'") | ||
|
|
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
When a worker's in-flight task stops running without reaching its terminal-marking code, its
async_operationsrow staysstatus='processing'owned by a live worker, forever. The worker has already forgotten it (_cleanup_taskremoved it from_active_tasksand freed the slot),recover_own_tasksonly runs at startup, and no dead-worker handling applies because the worker is alive. Clients polling the operation wait indefinitely.Observed in production (0.8.6-slim, k8s, single worker): a pod running 39h without restart held two rows
processingfor 38h —retry_count=0, absent from the worker's own[WORKER_TASK]in-flight logging, zero log mentions in 20 minutes, not counted inslots=9/12. Full evidence in #3228.The three paths
asyncio.CancelledErrorescapes_execute_task_inner. The handler chain catches_WallTimeoutExceeded,DeferOperation,RetryTaskAt, andException.CancelledErrorderives fromBaseException, so it escapes all four; there is nofinally._mark_failedis itself a DB write. If it raises inside theexcept Exceptionbranch (pool exhaustion, connection reset, statement timeout), the row is stranded. Likely the cause of the observed rows, since no shutdown occurred on that pod.shutdown_gracefulcancels and returns. After the drain timeout it callsbg_task.cancel()on each in-flight task and returns immediately; the event loop may close before any cancelled task could reconcile its own row. Every rollout with in-flight work over the grace period strands it.The fix
A self-scoped release, applied at all three sites:
_execute_task_inner: newexcept asyncio.CancelledError:branch that releases and re-raises (no metric — cancellation is not a terminal outcome).except Exception:_mark_failedwrapped so its own failure releases instead of stranding.shutdown_graceful: after the cancel loop, a bulk release of everything still claimed by this worker id across all schemas — the same shape asrecover_own_tasks, run at shutdown instead of only at the next startup.Scope note: this deliberately does not touch other workers' rows. I read the history on #992/#1441/#1510 and the line drawn there — no cross-worker coordination, no liveness inference. The guard
worker_id = <self>means every write here uses only the worker's own local knowledge, the same authorityrecover_own_tasksalready exercises at startup; it is a no-op the moment a task lands its own terminal state, so there is no duplicate-work race. It also answers "we need to figure out why workers are stuck" (#992) — these three paths are why.The
retry_countincrement mirrorsrecover_own_tasks(#2675/#2834): a task that reliably kills its worker still exhausts its retry budget rather than being re-claimed forever.Complementary to (not overlapping) #3092, which makes the drain timeout configurable; this PR makes the post-timeout cancellation stop stranding rows regardless of the timeout's value.
Tests
tests/test_worker_task_release.py, three tests against real Postgres (pg0), all failing onmainand passing with the fix:test_cancelled_task_returns_operation_to_pending— claim, block the executor, cancel the trackedbg_task; row must endpending/worker_id NULL/retry_count 1.test_failed_terminal_write_returns_operation_to_pending— executor raises,_mark_failedmonkeypatched to raise; row must endpending, not strandedprocessing.test_shutdown_graceful_releases_inflight_operations—shutdown_graceful(timeout=0.1)with a blocked task in flight; row must endpending.Fixes #3228