Skip to content

fix(kanban): reclaim pid-less claims only after claimer death - #121730

Open
Kyzcreig wants to merge 4 commits into
NousResearch:mainfrom
ANG-Ventures:fix/reclaim-dead-claimer-upstream
Open

Kyzcreig wants to merge 4 commits into
NousResearch:mainfrom
ANG-Ventures:fix/reclaim-dead-claimer-upstream

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Sep 24, 2026 •

Copy link
Copy Markdown

Problem

A dispatcher can claim a card and die before stamping its worker PID. A missing PID by itself is ambiguous: the claimer may still be launching a worker. Releasing its claim risks a duplicate; repeatedly deferring an already-dead claimer wedges the card.

Change

Probe the host-local claimer PID in claim_lock when no worker PID was stamped. psutil.Process(pid) is the Windows-safe equivalent of os.kill(pid, 0); NoSuchProcess proves the claimer is gone and lets the normal reclaim record claimer_pid_dead. A live, inaccessible, or malformed claimer holds its claim. This applies to both TTL and manual reclaim; foreign-host release policy and existing worker PID fingerprint semantics are unchanged. The upstream tree split the dispatcher into kanban_db_dispatch.py, so this ports the fork-first fix into that shape.

Fork-first PR: ANG-Ventures#983

Tests

New manual/TTL dead- and live-claimer regressions failed against upstream main before implementation (4 failures, one foreign-host control passed). Targeted upstream run: 60 passed, 1 skipped, 1 deselected. The deselected test_infrastructure_spawn_refusal_never_charges_the_card fails identically on clean upstream/main on this macOS host (verified independently); it is unrelated to this diff. Ruff and git diff --check pass.

On the no-worker-PID branch, probe the local claim owner. Only a dead owner authorizes release; hold a live or unprovable launch through manual and TTL reclaim to prevent duplicate workers. Preserve foreign-host policy and worker-fingerprint handling.
@kiramakes

Copy link
Copy Markdown

🤖 Hermes Agent automated review: PR reviewed. Diff analyzed (189 lines changed). Check CI status and manual review recommended.

@kiramakes

Copy link
Copy Markdown

Summary

Ready to merge. Defends against the pid-less-claim race condition.

Positives

  • The bug: a claim can be stamped with a lock but never get a worker PID (gateway launches worker after claiming). The old code treated "no PID" as "nothing to terminate" and reclaimed immediately — risking a duplicate worker if the claimer was still alive.
  • The fix probes os.kill(pid, 0) for pid-less local claims: if the claimer is dead, reclaim proceeds; if alive, reclaim is deferred. Non-local claims preserve the existing foreign-host release policy.
  • _worker_survived_termination now also returns True for liveness_unprovable, which correctly blocks reclamation.
  • Tests cover dead claimer (reclaim proceeds), live claimer (reclaim deferred), expired vs. non-expired, and foreign-host passthrough.

Suggestions

  • None.

Questions

  • None.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Sep 24, 2026
Apollo added 2 commits September 24, 2026 09:07
Popen precedes the pid stamp, so a dead claimer can leave a detached,
heartbeating worker. Current-run heartbeat/spawned evidence now keeps the
claim held on the manual, TTL and stale paths; prior-run evidence is
ignored. Real-subprocess orphan regression is RED on 739682d.
@Kyzcreig

Copy link
Copy Markdown
Author

Round 2 (addresses review B1: dead claimer != no worker spawned).

The no-pid branch now checks for heartbeat/spawned events bound to the CURRENT run before it probes the claimer. Any such evidence keeps the claim liveness_unprovable, so it stays held and no second worker is spawned. Evidence from a prior run is ignored, so a never-spawned retry still releases.

Class sweep over every caller of _terminate_reclaimed_worker / _worker_survived_termination:

  • reclaim_task, release_stale_claims, detect_stale_running: these can reach the no-pid branch. All three now pass conn/task_id and are covered by the orphan test (manual/ttl/stale).
  • progress-stall reclaim: dismissed. It only iterates rows with worker_pid set.
  • ancestor-reopen invalidation: dismissed. It is a post-commit best-effort kill whose result is never gated.
  • _abort_lost_claim_spawn: dismissed. It is always called with the concrete pid Popen returned.
  • upstream archive and terminal-row reap: dismissed. The pid is known and the claim is already released or terminal.

Tests: the real-subprocess orphan arm is RED on the previous head (3 failed) and GREEN on this head. The incident, alive-claimer and foreign-host arms are still green. Adjacent suites are green.

… context

A dead claimer does not prove no worker exists (Popen precedes the pid
stamp). With no current-run heartbeat/spawned event, hold the pid-less claim
until DEAD_CLAIMER_LAUNCH_BOUND_SECONDS (900 s) after the claim; with such
evidence, release once it is older than DEFAULT_CLAIM_HEARTBEAT_MAX_STALE_SECONDS
so a worker that heartbeated and died cannot hold the card forever. Omitting
conn/task_id now fails closed.
@Kyzcreig

Copy link
Copy Markdown
Author

Update (09bdbd5): a dead claimer is no longer treated as proof that no worker exists, because Popen runs before the pid stamp. A pid-less claim is now held for 900 s after the claim when the current run has no heartbeat or spawned event. When it does have one, the claim is held until the newest event is older than DEFAULT_CLAIM_HEARTBEAT_MAX_STALE_SECONDS. Omitting conn/task_id now fails closed. Note, pre-existing and not changed here: the dashboard _set_status_direct upstream terminates the worker after commit, and nothing gates the release on survival.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants