Skip to content

fix(kanban): claim guard sees open prior runs and every spawn of a run - #946

Merged
Kyzcreig merged 2 commits into
mainfrom
fix/t_3a06ba8f-r5-open-run-guard
Sep 24, 2026
Merged

Kyzcreig merged 2 commits into
mainfrom
fix/t_3a06ba8f-r5-open-run-guard

Conversation

@Kyzcreig

@Kyzcreig Kyzcreig commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #921 (merged at 250b521 while Argus's round-4 review of t_3a06ba8f was still FAIL/HOLD). This carries the round-4 fix, which #921 does not include.

Defects (Argus round 4, t_3a06ba8f)

  • B-1: _prior_worker_still_alive only scanned runs with ended_at IS NOT NULL. A ready or review card with a leaked open current_run_id passed the guard. claim_task's invariant recovery then closed that run and started a second worker next to a live owner. Repro: dispatch_once spawned a second worker while the original /bin/sleep PID was still running.
  • Cardinality: only the newest spawned event of a run was probed, so a dead newer PID vouched for a live older one.
  • C-1 (test gap): the TTL-expiry host-local NULL-PID branch had no test. Replacing _worker_survived_termination(...) with termination_attempted and not terminated stayed green.

Change

  • Scan every prior run, whether open or ended.
  • Probe every spawned PID in the run's claim interval.
  • _set_worker_pid is the only worker_pid writer and always appends spawned in the same transaction, so an open run's row PID is already covered by the event record. A separate row-PID branch was tried; its mutant survived because the branch was redundant, so it was dropped.

Tests

New tests in test_kanban_second_claim_class.py. They go through the real dispatch_once spawn boundary and each negative case has a proven-dead control:

  • leaked open run with a real /bin/sleep owner (no spawn, run stays open), plus a dead-owner control (recovered and dispatched)
  • the same case at the review door
  • double-stamped run with the older PID alive or the newer PID alive, plus an all-dead control
  • TTL expiry with NULL PID: hold, no signals, no second spawn; plus a dead-PID requeue control

Verification

  • New tests on 250b521: 3 failed / 23 passed. The TTL test passes there because that code was already correct; the test closes a coverage gap.
  • Mutants, all RED: ended-only scan (2 failed), newest-spawn-only (1 failed), TTL survivor predicate (1 failed). Source restored, hash verified.
  • Argus probe_gaps.py: ENDED_CONTROL, ACTIVE_UNENDED and TWO_SPAWNS all show second_spawned=false.
  • Focused kanban suites on current main plus this commit: 257 passed, 1 skipped.
  • Sharded kanban test glob (121 files): 1569 passed, 5 failed, 3 skipped. The 5 failures are test_kanban_notify artifact delivery and 4× test_kanban_review_surfaces. They fail the same way on 250b521, so they are not caused by this change.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@Kyzcreig
Kyzcreig force-pushed the fix/t_3a06ba8f-r5-open-run-guard branch from db31dd0 to e4212cc Compare September 24, 2026 11:42
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

🤖 merged-by: apollo · lane: owner · gate: BYPASS: FR paused by Ace 2026-09-22 · why: t_3a06ba8f argus r6 PASS-WITH-CAVEATS @e4212cc: claim guard sees open prior runs and every spawn of a run (reclaim prev_pid=null class)

@Kyzcreig
Kyzcreig enabled auto-merge September 24, 2026 14:57
@Kyzcreig
Kyzcreig force-pushed the fix/t_3a06ba8f-r5-open-run-guard branch from e4212cc to caaf1af Compare September 24, 2026 18:19
Apollo added 2 commits September 24, 2026 12:20
Round-4 review (t_3a06ba8f) found two prior-owner shapes the second-claim
guard could not see:

- a ready/review card with a leaked OPEN current_run_id: the scan filtered
  ended_at IS NOT NULL, then claim_task's invariant recovery closed that run
  and minted a second worker beside the still-live owner;
- a run stamped twice: only the newest spawned event was probed, so a dead
  later PID vouched for a live earlier one.

_prior_worker_still_alive now scans every prior run (open or ended) and
_spawned_owner_alive probes every spawned PID in the run's claim interval.
_set_worker_pid is the only worker_pid writer and appends `spawned` in the
same txn, so the event record covers an open run's row pid.

Tests (dispatch boundary, proven-dead controls): leaked open run with a real
/bin/sleep owner, review-door variant, double-stamp older/newer alive, and a
TTL-expiry NULL-PID hold with a dead-PID requeue control.

Verified: new tests RED on 250b521 (3 failed); mutants RED:
ended-only scan (2), newest-spawn-only (1), TTL survivor predicate replaced
with `termination_attempted and not terminated` (1). Focused kanban suites
249 passed, 1 skipped.
Round-6 of t_3a06ba8f (Argus r5 B-1, B-2).

B-2: _spawned_owner_alive treated any live process at a recorded PID as the
prior owner, so a recycled PID stranded a ready/review card forever (live
via #921; regression vs pre-card base). A live PID now counts as the owner
only if its psutil create_time lies in the run's causal window
[claimed_at - 1 s, spawned_at + 2 s] (claimed_at falls back to the run's
started_at). The window is one-sided around the spawn on purpose: the
spawned event is written after Popen and can lag the real worker by up to
the DB busy timeout, so a symmetric +/-N window would call genuine workers
dead. Unreadable create_time fails CLOSED ("unverified"). Refusals carry
owner_identity.

Surfacing: new diagnostics rule claim_refused_live_owner fires for READY
and REVIEW cards refused for prior_worker_still_alive past a 5 min grace
(error, critical at 30 min), clears on the next claimed event. Visible via
`hermes kanban diagnostics` and the dashboard diagnostics surface.

B-1: test_model_reread_on_retry_spawn returned PID 2 as its "crashed"
worker; on Linux that is kthreadd (always alive), so the open-run guard
refused the retry and CI slice 14/16 went red. The stub now returns the
PID of an exited, reaped child.

Verified: second_claim_class + model_override 54 passed; diagnostics rule
7 passed. Old fixture FAILS under Linux PID-2 emulation, new passes.
Mutants RED: identity check removed (both recycled arms), symmetric +/-3 s
window (lock-lagged arm), fail-open on unreadable ctime, rule unregistered,
rule ready-only (review arms). Broad kanban glob on main df43599 + this
branch: 1860 passed, 5 failed, 3 skipped; the same 5 (4 review_surfaces,
1 notify artifact) fail on unmodified main 03de88f.
@Kyzcreig
Kyzcreig force-pushed the fix/t_3a06ba8f-r5-open-run-guard branch from caaf1af to e00d74e Compare September 24, 2026 19:44
@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

Rebased onto main 9f857c4 (head e00d74e). Not a pure mechanical rebase, so please read this before merging.

While this PR was DIRTY, main landed #956 ("reject recycled PIDs as prior worker owners"). #956 fixes the same B-2 class as this PR's r6 commit, but independently. It adds _real_pid_started_in_claim / the _pid_started_in_claim seam (psutil, then a ps -o lstart fallback, with a symmetric window [claimed-2s, spawned+2s] that fails OPEN to "not owner" when the start time can't be read), plus a max_runtime_seconds shortcut.

How the two were merged into one guard:

Verification on e00d74e:

  • Focused 5 files (second_claim_class, model_override, diagnostics, reclaim_unprovable_liveness, watchers_mixin): 137/137.
  • Broad kanban glob, 148 files: 2239 passed, 0 failed, 4 skipped.
  • ruff is clean.
  • Argus r7 probe battery, normalised diff against Argus's r7 logs:
    • Every head log matches r7: gaps, levers, livepid, modelov, original, r1-timeout, r3, recycled, recycled-review, lag8, lag20.
    • r3ui differs only in task ids.
    • Control: main now un-strands RECYCLED at both doors (which is fix(kanban): reject recycled PIDs as prior worker owners #956). The merged tree does too, and it still refuses ORIGINAL.
  • Mutants (Argus's r6 set re-anchored, plus 3 new ones):
    • 12/15 KILLED, including ended-only scan, newest-spawn-only, fail-open, symmetric window, and all 3 diagnostics mutants.
    • SURVIVED: M_vi (LEAD widened to 60 s) and M_viii (no started_at fallback). These are the known r6 residual gaps and they fail closed.
    • Also SURVIVED: the new M_xii (drop the ps fallback). psutil is always present in the test env, so the fallback is untested there, same as on main.

@Kyzcreig

Copy link
Copy Markdown
Collaborator Author

🤖 merged-by: apollo · lane: kanban-merge-pass · gate: BYPASS: FleetReview paused by Ace 2026-09-22 (state/fleetreview-pause marker present) · why: t_d5113765: reclaim with prev_pid=null, rebased over #956, CI 33 pass; Argus removed from card review (Ace 13:08), CI green, mergeable

@Kyzcreig
Kyzcreig added this pull request to the merge queue Sep 24, 2026
Merged via the queue into main with commit c83be65 Sep 24, 2026
57 checks passed
@Kyzcreig
Kyzcreig deleted the fix/t_3a06ba8f-r5-open-run-guard branch September 24, 2026 21:52
@Kyzcreig Kyzcreig added the fleetreview:post-merge Ask FleetReview to review this MERGED pull (merge commit vs first parent) label Sep 24, 2026
Kyzcreig pushed a commit that referenced this pull request Sep 25, 2026
…ecycled PID (t_0ae83825)

Rebased onto main after #946/#956/t_21dfa673. The owner window now carries
the spawned event's start_token, so termination and liveness paths use the
same clock-step-immune identity as the claim guard; legacy rows keep the
causal window. enforce_max_runtime passes conn/task_id per main's run-context
class guard.
@ang-prism

ang-prism Bot commented Sep 27, 2026

Copy link
Copy Markdown

FleetReview

Review: post-merge · head c83be6578e5f · duration 4m 19s
Profile: light (merit: default light: lines 691<1000000, files 5<1000000, hunks 11<1000000, no hot path) · policy: changed-lines>400
Roster: B-assert-ctx → gpt-6-sol (openai), B-state → gpt-6-sol (openai), F → gpt-6-sol (openai)

PARTIAL — ensemble escalated: family floor: too few distinct model families completed

This review did not reach a trusted verdict, so it is not a gate pass and the findings below may be incomplete. They are posted so they can be read rather than lost in a terminal record.

Post-merge review (fleetreview:post-merge override): this reviewed the merge commit against its first parent — the bytes that already shipped. It is not a pre-merge gate pass.

Reviewed with 1 of 2 model families — xai unavailable.

profile: light (rule: default light: lines 691<1000000, files 5<1000000, hunks 11<1000000, no hot path) · round 0 · members: B-assert-ctx, B-state, F, G · families: openai

Confidence: 1/5

Findings

  • P1 hermes_cli/kanban_diagnostics.py:1158 — Stale alert · agreed: B-assert-ctx,B-state,F (openai)

FleetReview provenance · models: B=gpt-6-sol, D=grok-4.6, F=gpt-6-sol · cost: $1.27 (estimated) · duration: 6m 35s · rounds: 1 · files examined: 5

@ang-prism

ang-prism Bot commented Sep 28, 2026

Copy link
Copy Markdown

FleetReview

FleetReview's daily member-call budget is spent (60/600 for 2026-09-28 UTC); review skipped.


FleetReview · reviewKind: skipped-budget

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

Labels

fleetreview:post-merge Ask FleetReview to review this MERGED pull (merge commit vs first parent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant