fix(worker): recover stale tasks from dead workers after restart#2553
fix(worker): recover stale tasks from dead workers after restart#255376131356 wants to merge 1 commit into
Conversation
Closes vectorize-io#2165 When a Hindsight container is restarted (OOM, docker kill, host crash), operations stuck in 'processing' status are never recovered because only matches the current worker's ID. After restart the new worker gets a different ID (hostname/container-id), so the old tasks remain stuck indefinitely, blocking all new consolidation. This PR adds which finds ALL processing tasks whose is older than 10 minutes (STALE_TASK_TIMEOUT_S=600) and resets them to 'pending'. It runs: - At startup (after recover_own_tasks) - Periodically during the polling loop (every 5 minutes) The 10-minute timeout avoids interfering with legitimately slow tasks (e.g. large consolidation batches) while still recovering promptly after a crash.
koriyoshi2041
left a comment
There was a problem hiding this comment.
I ran a focused worker slice locally on this branch:\n\nuv run pytest tests/test_worker.py -k 'decommission or worker_status or priority or recover'\n\nResult: 24 passed, 2 warnings.\n\nOne edge case I think is worth tightening before merge: recover_stale_tasks() resets any processing row older than 10 minutes, independent of whether the owning worker is actually dead. Since _recover_stale_if_due() runs periodically from other live workers, a legitimate long-running retain/consolidation task can cross the 600s threshold while its original worker is still executing it, get reset to pending, then be claimed again by another worker. That would turn a slow task into duplicate execution rather than just crash recovery.\n\nA safer shape might be to recover only explicitly decommissioned/known-dead worker IDs, or add a heartbeat/lease that is refreshed by active tasks and recover only when that lease is stale. If keeping the timeout-only approach, I’d at least add a regression test with worker A actively processing a task older than the threshold while worker B runs the stale sweep, so the intended behavior is explicit.
benfrank241
left a comment
There was a problem hiding this comment.
thanks for this, and the writeup is excellent — the restart→new-worker-id→orphaned-processing-tasks failure (#2165) is real, and recover_own_tasks() genuinely can't catch it since it keys on worker_id = self. cross-worker stale recovery is the right thing to add.
one correctness concern blocks it though: recovery keys on claimed_at age with no worker-liveness signal, so it can reset tasks a live worker is still legitimately processing. i checked the poller — there's no heartbeat: claimed_at is set once at claim, and updated_at is only touched on terminal transitions (completed/failed/retry), never bumped while a task runs. so a task that a live worker has been working for >10 min looks identical to one from a dead worker. recover_stale_tasks() resets it to pending, a second worker claims it, and now the same operation runs on two workers at once.
for consolidation that's worse than the bug it fixes — concurrent double-processing can produce duplicate observations / corrupt mental-model state, whereas the current failure is "stuck" (recoverable). and large consolidation/retain batches really do exceed 10 min (see #2644 — retains splitting into 100+ sub-batches).
the fix needs a liveness signal rather than a fixed claim-age timeout. two shapes that would work:
- add a periodic heartbeat that bumps
updated_at(or alast_heartbeatcolumn) on in-flight tasks in the polling loop, and key recovery on heartbeat staleness — i.e. "no activity in N minutes", not "claimed N minutes ago". that way a live-but-slow task keeps itself alive and only a truly-dead worker's tasks get reset. - or a worker-lease/registry table where workers renew a lease and recovery targets only tasks whose worker's lease has expired.
also — this needs a test. it's a concurrency/recovery change in the worker path, which has had real churn recently (#2601/#2608/#2614), so a regression test that (a) a stale task past the timeout is recovered and (b) a recently-active/heartbeated task is not would give us confidence it won't reset live work.
happy to help think through the heartbeat approach if useful. direction's right — just want the recovery to distinguish "slow" from "dead" before it can go in.
Problem
When a Hindsight container is restarted (OOM, docker kill, host crash), operations stuck in
processingstatus are never recovered. The existingrecover_own_tasks()only matches tasks whereworker_idequals the current worker's ID. After a restart, the new worker gets a different ID (hostname/container-id changes), so tasks from the previous worker remain stuck indefinitely.This blocks ALL new consolidation work — the worker sees
claimable=1butmy_active= none, withothers: <dead_worker_id>:1in the logs.Reproduced consistently on self-hosted Docker deployment (v0.8.1 through v0.8.4). Related: #2165
Root Cause
The worker_id defaults to
socket.gethostname()(container hostname), which changes on every restart.Fix
Added
recover_stale_tasks()which finds ALLprocessingtasks whoseclaimed_atis older than 10 minutes and resets them topending:Runs at two points:
recover_own_tasks()inrun()The 10-minute timeout avoids interfering with legitimately slow tasks (large consolidation batches) while recovering promptly after a crash.
Testing
pending_consolidationcount drops from 21→0 after recoveryScreenshots
Before fix (worker stuck indefinitely):
After fix (auto-recovered):