Conversation
alt-glitch
added
type/bug
Contributor
Author
|
CI is currently blocked at the fork-approval gate ( |
reefmind
force-pushed
the
fix/kanban-process-retry-safety
branch
2 times, most recently
from
August 5, 2026 15:31
c2b6811 to
a3359d8
Compare
… before a task is claimable again Two production incidents, one lifecycle contract: a task must not be handed to a new worker while the previous decision about it is still binding. A) t_27263082 recorded gave_up with effective_limit=2/limit_source=dispatcher after two spawn failures, then a later recompute_ready promoted it and the dispatcher retried it. The NousResearch#35072 guard re-resolved its OWN effective limit (per-task max_retries → caller failure_limit → DEFAULT_FAILURE_LIMIT), so any caller that passed a laxer limit — a restarted gateway falling back to the default, tools/dashboard callers that don't thread kanban.failure_limit through, or a raised config value — decided the task was still under budget. The trip is now recorded on the task (tasks.breaker_limit, mirroring the gave_up payload) and its PRESENCE binds every promotion caller. Presence, not a counter comparison: force_trip callers (systemic-error fingerprints, the protocol-violation streak) record gave_up with failures below the limit, and comparing let exactly those tasks promote themselves on the next tick. The stamp rides the same status-guarded UPDATE as the blocked transition, so a task that completes under the reaper is neither transitioned nor stamped. Legacy rows are backfilled from the event log on migration, with the same fallback at read time, keyed off a single list of reset events that mirrors the code paths clearing the column. Deliberate fresh starts still clear it: kanban_unblock, manual promote, operator reclaim, successful completion. Tasks that never tripped keep NousResearch#35072's lenient-config recovery, and worker-initiated sticky blocks (NousResearch#28712) are untouched. B) t_80a3542a run 141 hit its budget while worker PID 98725 stayed alive, reparented to PID 1 with live Claude children. The runtime cap signalled the PID only, never confirmed the process group was gone, flipped the task to ready anyway, and the dispatcher claimed run 152 and spawned PID 57286 against the same task and workspace. Termination now targets the worker's process group (workers already get their own session, so the group is the whole tree) with zombie-aware, group-wide liveness. Every requeue path — runtime cap, TTL expiry, heartbeat staleness, crash reaping, operator reclaim — reaches the same decision: if the worker LEADER survived termination, hold the claim (it is throttled or wedged, and the next tick retries); if only its children still hold the group, requeue but record a durable worker_fence (pid, pgid, identity, run id, claim lock), and claim_task rejects at the single choke point every spawn path goes through until that group exits. The fence lives on the row, so it survives a gateway restart. A PID is a recycled number, so nothing destructive or binding rests on one alone. Each worker's leader identity (start time) is captured at spawn: group signals are sent only while that identity still verifies — no proof means pid-only signalling, and a proven mismatch means our worker is already gone — and a fence resolves the moment its identity changes. Only a positive identity match earns an unbounded hold; every weaker signal (a live pid with no identity token, a group whose leader we cannot verify) is evidence rather than proof and expires after WORKER_FENCE_MAX_SECONDS rather than stranding the task. A fence is also pinned to the host that recorded it: a local pid number says nothing about a remote worker, and we would never see that worker exit. The fence is part of the claim CAS, not a separate look: the gate probes the OS and so cannot run under the write lock, which leaves a window where a reaper commits ready + a fresh fence while a claimer sits between gate and CAS. Both claim gates (ready and review) now carry `worker_fence IS NULL` in the compare-and-swap itself. For the same reason the crash reaper does its liveness and process-group probing BEFORE opening its transaction, then re-reads the row under the lock and acts only if status, pid and claim lock are unchanged — a `ps` scan inside BEGIN IMMEDIATE stalls every other writer on the board. Clearing a fence is itself a compare-and-swap against the exact serialized fence the caller evaluated, so a claim gate or reclaim that decided one fence was releasable can never delete a newer one recorded while it probed. In reclaim that CAS is bound into the same UPDATE as the release, so a refusal writes nothing at all rather than leaving the task released with no reclaimed event and an open run row still pointed at by current_run_id. And a pgid is persisted only when the OS confirms the worker leads that group (pgid == pid, which start_new_session guarantees for real workers; a failed getpgid records nothing rather than assuming leadership): a worker that merely joined a group would otherwise record the gateway's own, and signalling that would take down the dispatcher and every sibling worker. Operators keep explicit exits throughout. unblock and promote release a fence outright; reclaim applies to a task fenced while sitting in ready and re-probes it first — reading pid/pgid/identity from the fence payload, since a requeued row deliberately carries none of them, and committing the clear-or-keep decision in the same transaction as the release so no dispatcher tick can claim through a half-applied reclaim. An operator reclaim is therefore never more permissive than an ordinary claim: a live orphan group stays fenced. Platforms without process groups or an identity probe (Windows) fail closed: no group signalling, no group-based liveness, pre-existing PID-only behaviour. Also fixes a test that stubbed _pid_alive on a re-imported module: with an earlier test evicting hermes_cli from sys.modules the stub missed, leaving the real probe running against pytest's own pid.
The board is meant to stay continuously automated, but the worker guidance told every coding worker to park its own card in `blocked` with a `review-required:` reason and wait for a person to approve+unblock. That made `blocked` the default terminal state for the most common task type, so an unattended board stalled on the first code change. Reviewable code now fans out as cards instead of stalling: - The implementation worker creates a `ready` review card assigned to a real installed reviewer profile (normally `pr-reviewer`), carrying exact workspace / repo / base SHA / head SHA / changed files / tests / acceptance evidence, then completes its own card listing the review card in `created_cards`. - Reviewer PASS completes the review card. Reviewer BLOCK creates a remediation card for the original implementation profile rather than blocking — a rejected review is normal, not a stuck board. - The remediation worker fixes the findings and creates a fresh review card at the new exact head SHA; reviews bind to one SHA and are never re-opened. - If no valid reviewer profile is installed, the worker surfaces `needs_input` instead of inventing an assignee. The rule that unknown assignees must be discovered and validated is unchanged. `blocked` keeps its narrow meaning and is now stated explicitly: irretrievable human input, a hard capability/access wall, or a truly transient failure after retries. It is never a dispatcher fence, a status update, a review queue, an ordinary dependency wait, or a substitute for child tasks — dependencies are DAG edges via `parents=[...]`. No safety block or DB sticky-block semantics are weakened; this is guidance text only. Covered by prompt contract tests asserting the automated review DAG is present and the old mandatory review-required block instruction is gone.
Failing closed on an unverifiable PID had been applied to the pid as well as the process group. A running worker whose row carried no worker_identity (legacy row, or one written before the token landed) was therefore never signalled on Linux/macOS: the reclaim paths recorded a fence and moved on. A fence with no identity token is weak evidence by design and expires after WORKER_FENCE_MAX_SECONDS, so the worker aged out of its own fence and the claim gate handed the task to a replacement — the t_80a3542a duplicate-writer state the fence exists to prevent. Narrow the fail-closed rule to what it was actually protecting. A recorded identity that does NOT match a live PID still signals nothing (the number was recycled; our worker is gone). No recorded identity at all now signals the bare PID — the pre-existing behaviour, and the only handle we ever had on that worker — bounded to SIGTERM then SIGKILL with the existing wall-clock/probe caps. The process group still requires a proven identity, so the blast radius never widens without proof. Survival is judged on the same basis we signalled: with no identity, leader_alive reads the bare PID, so a survivor is HELD by _worker_survived_termination (claim retained, next tick retries the kill) instead of falling through to a bounded fence. reclaim_task is the one path that fences a live leader rather than holding it, because the operator asked for the release. It now pins the identity observed during termination, turning that hold on a recyclable number into a provable one: unbounded while it matches, self-releasing as worker_identity_changed once the PID is recycled. Also corrects the _terminate_reclaimed_worker docstring, which described the bare-PID fallback that the code no longer took. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…t in triage `block_task` routed a repeated same-cause block (`block_recurrences >= BLOCK_RECURRENCE_LIMIT`) to `status='triage'` and emitted `block_loop_detected`. But triage is the auto-decomposer's *input queue*, not a human hold: with the default `kanban.auto_decompose: true` the dispatcher treats every triage row as a decompose/specify candidate. On the live default board `t_bdf1001a` was escalated to triage at event 2955 / run 233, sat there ~30s, then moved to `todo` with no lifecycle event and would have gone `ready` — the exact blocked-task resurrection this branch exists to stop. `t_80a3542a` hit the same class. The escalation now stays in `blocked` and the `block_loop_detected` event makes that block sticky: * `_has_sticky_block` treats `block_loop_detected` like `blocked`, so `recompute_ready`, claim, restart/migration and the dispatcher tick all leave it alone. An explicit `unblock_task` still resumes it (it emits `unblocked`, which clears the predicate). * New `has_block_loop_escalation()` is defense in depth on the orchestration paths: `specify_triage_task`, `decompose_triage_task`, `decompose_task` and `list_triage_ids` all refuse a row carrying an escalation no matter which column it sits in. Ordinary triage cards keep auto-decomposing. * `_migrate_block_loop_escalations_out_of_triage` lifts legacy rows already parked in triage (block_kind set + latest block-lifecycle event is `block_loop_detected`) into the sticky blocked state on init, so activating this build doesn't auto-decompose them. Narrow by construction: plain triage cards and already-unblocked rows are untouched. * Notifier ping and docs/comments reworded off the old triage routing. Regression: tests/hermes_cli/test_kanban_block_loop_sticky.py (7 tests) — red before the fix on the real block -> unblock -> re-block -> dispatcher path, green after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ns are stale Review BLOCK on a0cfd412b: `_migrate_block_loop_escalations_out_of_triage` ran *before* `_rebuild_drifted_tables`. The escalation migration decides whether a row is still escalated by reading the newest block-lifecycle event with `ORDER BY id DESC`, but on a pre-NousResearch#35096 board `task_events.id` is still TEXT — so that ordering is lexicographic and `ev-9` outranks `ev-10`. A legacy triage card whose real history ends `block_loop_detected (ev-9)`, `unblocked (ev-10)` therefore read as still escalated: the migration flipped it triage -> blocked, the rebuild then renumbered the ids to chronological INTEGERs, `_has_sticky_block` read the true tail (`unblocked`) and returned False, and the next `recompute_ready` promoted the card to `ready` for a worker to claim — the same resurrection this branch exists to stop, reached through a different door. Fix is the ordering: rebuild the drifted tables first, then run the escalation migration over correct ids. Genuinely-still-escalated legacy rows continue to become sticky blocked; ordinary triage cards are still untouched. Regression: test_migration_reads_event_order_after_text_id_drift_is_repaired builds a realistic drifted TEXT-id board (>= 10 events per row, the hazard pinned explicitly before init) and asserts the cleared card stays in triage and never becomes ready/claimable. Red before this commit (`assert 'blocked' == 'triage'`, with the migration logging "moved 2 escalation(s)" ahead of "rebuilding task_events"), green after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
reefmind
force-pushed
the
fix/kanban-process-retry-safety
branch
from
August 5, 2026 15:34
a3359d8 to
3a5bcf3
Compare
This branch has not been deployed
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
3 participants
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
Incidents closed
gave_uptask could be auto-promoted and redispatched with contradictory notificationsCloses #71175.
Relationship to existing work
current_run_idwhile the old worker continues. This PR instead makes every failure/finalization path attempt-bound and refuses replacement claims until the prior verified process group is gone.Verification
98 passed, 1 skipped338 passed, 0 failed43 passedgit diff --check: cleanA broader one-process Kanban selection retains the same pre-existing cross-file pollution failure set on
origin/main; no branch-specific failures were added.Safety / rollout