fix(kanban): hold claim on iteration-budget exhaustion (#71175)

[JonthanaHanh]

Problem

When a goal-mode Kanban worker exhausts its per-turn iteration budget, _record_task_failure is called with release_claim=True. This releases the claim and closes the run while the worker process is still alive. The dispatcher then sees an unclaimed ready card and spawns a second worker into the same worktree — two workers edit the same files with no coordination.

Fix

Change release_claim=True to release_claim=False in agent/turn_finalizer.py:169. The claim stays held so the dispatcher cannot spawn a duplicate worker. The goal loop continues with a fresh iteration budget until goal_max_turns is exhausted.

Test update

Updated test_pending_response_records_kanban_timeout to expect release_claim=False instead of True.

Fixes #71175

[vadelma-agent]

I verified the current head d9efde6ba43dbe37893d230805ee0aa7f05d0682 on a disposable SQLite board. After claim_task() created run 1, calling _record_task_failure(..., outcome="timed_out", release_claim=False, end_run=True) produced:

task_status:   running
claim_lock:    gateway:123
current_run_id: NULL
run status:     timed_out

So the patch does prevent the immediate ready/unclaimed respawn, but it closes current_run_id while the worker can continue. The remaining safety property depends on the goal loop stopping when the task's current run no longer matches HERMES_KANBAN_RUN_ID; otherwise the held claim can still expire while the old process is alive.

Could the regression coverage assert the run-identity behavior (or add the proposed current_run_id ownership check) in addition to release_claim=False?

[ryangu00]

We hit this same class in production and landed a fix locally, so here's a data point plus one edge case that release_claim=False alone may not cover.

The edge case: not releasing the claim closes the window where the dispatcher hands the task to a replacement worker. But if a replacement does get in — through any other path, or during the window before this fix — the stale worker's later failure mutation still lands on whatever is in the row at that moment. It clears the replacement's claim, closes the replacement's run, and increments the failure counter against work the replacement is still doing. The claim-release is one way in; the underlying issue is that the failure write itself isn't bound to the attempt that produced it.

What we did instead: fence the failure to the exact worker attempt, so a replacement makes the whole thing a no-op.

_record_task_failure takes optional expected_run_id / expected_claim_lock, and bails before any mutation if either doesn't match the current row:

row = conn.execute(
    "SELECT consecutive_failures, status, max_retries, "
    "current_run_id, claim_lock FROM tasks WHERE id = ?", (task_id,),
).fetchone()
if row is None:
    return False
if expected_run_id is not None and row["current_run_id"] != int(expected_run_id):
    return False
if expected_claim_lock is not None and row["claim_lock"] != expected_claim_lock:
    return False

The same fence goes into _end_run, which previously resolved the run from tasks.current_run_id at call time and then cleared the pointer unconditionally. Both halves are now CAS-shaped:

if expected_run_id is None:
    # unchanged legacy path
    ...
else:
    run_id = int(expected_run_id)
...
conn.execute(
    "UPDATE tasks SET current_run_id = NULL "
    "WHERE id = ? AND current_run_id = ?",
    (task_id, run_id),
)

So an accepted failure closes only the run it belongs to and never clears a pointer that has moved on. Both params default to None, which preserves the existing behaviour exactly for every current caller — the fence only engages where the caller actually knows its own run/claim identity (the budget-exhaustion path being the obvious one).

The two approaches aren't exclusive: yours removes the main way a replacement appears, the fence makes the failure write safe if one appears anyway. If it's useful I'm happy to open a follow-up PR for the fencing part on top of this, or leave it — your call, since this is your PR and I don't want to fragment the fix.

Context: this came out of a local hardening pass on a Kanban-heavy deployment; the fenced version has been running with the corresponding tests green. Related to my #73188, which applies the same ownership-proof idea to complete_task.