fix(kanban): hold reclaim while the worker is still alive - #44909
fix(kanban): hold reclaim while the worker is still alive#44909Sahil-SS9 wants to merge 1 commit into
Conversation
release_stale_claims and detect_stale_running call _terminate_reclaimed_worker and then release the task claim unconditionally, even when the termination did not actually kill the worker. _terminate_reclaimed_worker already reports this via its "terminated" flag, but the callers ignore it. When a worker is parked in uninterruptible (D) state — for example throttled by a cgroup memory.high limit — a pending SIGTERM/SIGKILL cannot be delivered until the throttle lifts, so the kill is a no-op. The dispatcher then frees the claim and spawns a fresh worker beside the still-alive one. Repeated every dispatch tick this accumulates duplicate workers without bound, deepening the memory pressure that caused the throttle in the first place — a self-reinforcing runaway. Fix: gate both automatic reclaim paths on _worker_survived_termination(). When we attempted to kill our own host-local worker and it is still alive, defer the reclaim (_defer_reclaim_for_live_worker extends the claim a short grace and emits a reclaim_deferred event) instead of releasing. This guarantees at most one live worker per task and is self-correcting: not spawning a duplicate is what relieves the pressure so the pending signal lands and the worker dies, and the next tick reclaims cleanly. Non-host-local claims and the operator-driven reclaim_task() path keep their existing force-release behaviour. Related: NousResearch#41448 (concurrent dispatchers amplify this by doubling reclaim frequency); NousResearch#42858 (kill the worker rather than orphan it on archive). Tests: defer-when-worker-survives, reclaim-when-killed, release-when-not-host-local, and the detect_stale_running path.
|
Reviewed the diff — this correctly addresses the worker duplication race condition. When The guard is correctly scoped: CI hasn't started yet. Clean implementation. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Excellent fix with outstanding test coverage. This prevents a kanban worker duplication race condition when a cgroup-throttled worker survives termination attempts.
Looks Good
- Well-documented: detailed comment explaining the cgroup D-state scenario
- New helper functions are focused and well-named: _worker_survived_termination, _defer_reclaim_for_live_worker
- RECLAIM_DEFER_GRACE_SECONDS = 120 provides enough time for throttled workers to die
- The fix is applied in both release_stale_claims and detect_stale_running paths
- Comprehensive tests covering: surviving worker defer, successful termination reclaim, non-host-local bypass, detect_stale interaction
- Uses _append_event with reclaim_deferred kind for observability in hermes kanban tail
Reviewed by Hermes Agent
|
Merged via #49064 — your commit was cherry-picked onto current main with authorship preserved (b9e521d). We added a small follow-up on top: ProcessLookupError from the kill now sets terminated=True (an already-gone process is terminated, not survived) — otherwise the new defer guard would hold a dead worker's claim forever. Thanks! |
Problem
release_stale_claimsanddetect_stale_runningcall_terminate_reclaimed_worker(...)and then release the task claim unconditionally, regardless of whether the kill actually succeeded._terminate_reclaimed_workeralready reports this via itsterminatedflag, but both callers ignore it.When a worker is parked in uninterruptible (D) state — for example throttled by a cgroup
memory.highlimit — a pendingSIGTERM/SIGKILLcannot be delivered until the throttle lifts, so the termination is a no-op. The dispatcher then frees the claim and spawns a fresh worker beside the still-alive one. Repeated every dispatch tick, this accumulates duplicate workers without bound and deepens the memory pressure that caused the throttle in the first place — a self-reinforcing runaway. (Observed in production: a single board grew to 100+ live workers, load average 70+, swap exhausted.)The task-claim CAS is correctly atomic, so this is not a double-claim — it is the reclaim path trusting a kill that never landed.
Fix
Gate both automatic reclaim paths on a new
_worker_survived_termination()helper. When we attempted to kill our own host-local worker and it is still alive, defer the reclaim (_defer_reclaim_for_live_worker()extends the claim a short grace and emits areclaim_deferredevent) instead of releasing.This guarantees at most one live worker per task and is self-correcting: not spawning a duplicate is what relieves the pressure so the pending signal lands and the worker dies, after which the next tick reclaims cleanly.
host_local=False) keep the existing release behaviour — we cannot manage a worker on another host, so stranding the claim would be worse.reclaim_task()path is intentionally not gated: an operator hitting "abort" wants the force-release, and it is a single deliberate action, not the automatic tick loop.Tests
Adds four tests to
tests/hermes_cli/test_kanban_db.py:reclaim_deferredemitted, no duplicate)detect_stale_runningAll existing reclaim/stale tests pass.
Related