Skip to content

refactor: retire robustness supervision and simplify benchmark watchdogs - #1547

Merged
BaoYunkai merged 30 commits into
mainfrom
feature/leixin/simplify-watchdog
Sep 18, 2026
Merged

BaoYunkai merged 30 commits into
mainfrom
feature/leixin/simplify-watchdog

Conversation

@zengleixin-amd

@zengleixin-amd zengleixin-amd commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Scope

Hyperloom-only retirement of redundant robustness supervision and simplification of benchmark lifetime management. No Magpie or pod-tools source changes, dependency pin changes, new monitoring service, TTL fallback, or force-cleanup mechanism are included.

Changes

  • Retire the Robustness agent/RCA, runtime recover action, external Monitor and Hyperloom Supervisor. Keep explicit --resume-from, offline recover-session, and historical SBD v6 readers; KernelForge internal supervision is outside this removal.
  • Apply the 7800-second per-process hard cap and 600-second output-silence rule only after a ready marker is observed in the current round. Warm-reuse hints remain telemetry and do not arm silence.
  • Use measured expected durations for sweep admission rather than charging the hard cap. Share the earliest sweep/session monotonic deadline across boot, retries, reuse, fallback and anchors, preserving stop attribution.
  • Connect build and specialist cooperative cancellation to existing cleanup. Consume one bounded late acknowledgment on the same Ray lease; final unconfirmed cleanup retains ownership.
  • Preserve complete outcomes in task history for diagnosis when cleanup fails, without broadcasting them through delegated_result or permitting promotion/retry. Unregister completion callbacks after confirmed cleanup even if the callback fails. Repeated terminal inline actions reuse their recorded results.
  • Refuse resume before writes/dispatch when persisted ownership is unobservable. Both resume admission and round reconciliation honor the latest explicit cleanup outcome, including evidence appended after a terminal race.
  • Restore safe AITER lock cleanup at actual baseline startup, preserve locks on unknown compiler state, and exclude known zombies. Remove the retired manual sweep timeout option and update its production-call contract tests.
  • Preserve official main grading/roofline fixes. Follow-up changes are split into commits touching at most five files each.

Review findings and retained limits

  • Warm-reuse false silence arming: fixed in d330461 without changing the original Magpie pin. A reuse round without current readiness remains bounded by hard/session/cancellation limits, not the silence timer.
  • Legacy/foreign ownership: diagnosed and rejected, NOT inferred dead after reboot or reclaimed by TTL. Operator verification in the original execution environment is still required.
  • Completed cleanup-unconfirmed outcomes: durable and diagnostic-only. Retaining their lane/GPU ownership and withholding promotion is intentional isolation, not a successful recovery claim.
  • Callback failures after confirmed cleanup and terminal inline duplication: fixed. Unknown executor exit/raw asyncio cancellation still retains ownership; cooperative cancellation is recorded only after cleanup confirmation.
  • Exited roots and pending actors: an empty old process group or missing PID does not prove descendant exit. Unknown cleanup remains blocked. Local specialist natural-exit behavior is unchanged and does not claim to recover every detached orphan.
  • Late Ray acknowledgments: the same-lease follow-up path is exercised through the dispatcher and runner, not only by calling close twice in isolation. No periodic retry service was added.

Legacy ownership recovery follow-up

Commit 0fd82c6 addresses the remaining review blocker in exactly five files. After independently verifying that the entire task process tree, remote workers and Ray actor have stopped, an operator can run inference_optimizer recover-session --session-dir PATH --confirm-stopped TASK_ID --confirmation-reason TEXT.

This explicit, single-task operation requires the real POSIX session lock, accepts only unattributed/empty-scope residual ownership, records an audited cancelled diagnostic outcome, and atomically releases only that task execution/GPU ledger rows. It preserves round/fence/round-lease rows and other tasks, rejects nonempty or mixed scopes and later work, and is idempotent without authorizing future occupancy. It never stops a worker, uploads a report, starts resume, or accepts prior success results. Existing report-only --force behavior is unchanged. Normal resume admission remains read-only and fail-closed.

Local combined resume/reconcile/lock regression: 185 passed, 12 POSIX skips. Earlier targeted CLI regression was 9 failed/1 passed before implementation; core regressions also failed before implementation. Ruff, formatting, Python 3.10 grammar and Bandit passed. Five existing report-recovery tests require Linux because Windows lacks fcntl; no shim or source skip was added to bypass them. No real user session ownership was modified.

ATOM demo instruction follow-up

Commit 2e45aad changes only examples/hyperloom-qwen3-14b-fp8-12h-atom/SKILL.md (+5/-2), removing both remaining mandatory 300-second polling instructions. Status checks now follow the same request-driven persisted-state policy as the other demos; periodic checks require an explicit request and platform scheduling. No runtime code or temporary documents changed.

Validation — current head 2e45aad

  • New-head validation PASSED on 2e45aad: https://github.com/AMD-AGI/Hyperloom/actions/runs/35344287663 . All 12 Linux Python 3.10/3.11 test shards, both strict coverage thresholds, CodeQL and other non-GPU checks succeeded; no checks are pending or failed. Exact two-site content checks and git diff --check passed locally; all other content and list numbering are unchanged.
  • Automatic GPU E2E 35344287556 was cancelled with SHA-specific approval; all job steps were empty.

Prior verified runtime head 0fd82c6

  • Current-head Linux validation PASSED: https://github.com/AMD-AGI/Hyperloom/actions/runs/35332798196 . All 12 Python 3.10/3.11 test shards, both strict coverage thresholds, CodeQL and other non-GPU checks succeeded. The real POSIX lock, CLI confirmation/resume admission, and existing report-recovery cases passed. No checks remain pending or failed; human review is still required and the PR is not merged.
  • Automatic GPU E2E 35332798168 was cancelled with explicit approval for this exact SHA; all job steps were empty.

Prior verified head 2713b1e

  • Final-head validation PASSED on 2713b1e: https://github.com/AMD-AGI/Hyperloom/actions/runs/35310187287 . All 12 Ubuntu Python 3.10/3.11 test shards and both strict coverage thresholds succeeded. CodeQL and all other non-GPU validation checks succeeded; no checks remain pending or failed. Human review is still required; this PR has not been merged.
  • The final commit changes only three side-effecting assertions in one test; no production code changed.
  • Previous head 7d8a1a2 passed all 12 Python 3.10/3.11 shards and both strictly enforced coverage thresholds: https://github.com/AMD-AGI/Hyperloom/actions/runs/35308832684 . Its three CodeQL error annotations were all lease.close calls inside assertions; commit 2713b1e moves those calls before unchanged assertions. Five focused local cases passed.
  • Automatic GPU run 35310187231 was explicitly approved for cancellation and is confirmed cancelled with no executed job steps.
  • Previous run 35307480930 failed on two stale test contracts across three shards; coverage aggregation inherited those test failures. Commit 7d8a1a2 updates only those two tests, retains all timeline/resource assertions, and adds full cleanup-outcome evidence checks. Local SBD wiring plus real budget regression: 56 passed. Those native Linux failures passed in run 35308832684.
  • Automatic GPU run 35308832627 for this head was explicitly approved for cancellation and is confirmed cancelled with no executed job steps.
  • Lint, CodeQL, Packaging, REUSE, Docs, and Secret Scan succeeded for this head. Bandit retains a pre-existing advisory B608 on a bound-parameter SQL placeholder construction; it was not suppressed.
  • Local native tests: resume/reconcile 106 passed with four Windows CLI/POSIX skips; AITER 70 passed; sweep plus manual driver 40 passed; task progress/history 30 passed; round store 14 passed; Ray acknowledgment doubles 12 passed.
  • Additional owner verification: build cancellation 11 passed; specialist teardown 38 passed plus seven existing reaper cases; sweep/grid focused reports 38/33 passed and legacy sweep 113 passed. Six Windows legacy sweep failures were reproduced against the pre-change baseline, not hidden by source skips.
  • Existing Windows AST-isolated dispatcher harness: 42 passed; inline harness: 14 passed. These do not replace native Linux integration or real Ray validation. Two detached-process tests require Linux and were skipped locally.
  • Final 29 changed Python files passed Python 3.10 grammar, Ruff lint/format, and git diff checks before publication.
  • No GPU end-to-end success is claimed. Automatic GPU run 35307480982 was cancelled with explicit user approval; resolve/build/run jobs had no executed steps.

Verification checklist

  • Implement and independently review scoped fixes and regression tests.
  • Preserve conservative cleanup isolation and document unsupported recovery cases.
  • Push reviewed feature-branch commits to this existing PR.
  • Fix the two Linux CPU failures and pass all shards/strict coverage on 7d8a1a2.
  • Complete CPU/CodeQL confirmation on previous head 2713b1e.
  • Complete CPU/CodeQL confirmation on 0fd82c6.
  • Correct both remaining ATOM demo polling instructions.
  • Complete new-head CPU/CodeQL confirmation on 2e45aad.
  • Publish consolidated review responses and explicit limitations.

🤖 Generated with Claude Code

Zeng and others added 7 commits September 17, 2026 14:50
Snapshot the reviewed in-progress implementation before integrating upstream changes. Validation and dependency updates remain pending.

Co-Authored-By: Claude <noreply@anthropic.com>
Preserve ENABLEMENT and Atom updates, upstream removals and schema migrations while retaining the benchmark-only watchdog and explicit recovery boundaries.

Co-Authored-By: Claude <noreply@anthropic.com>
Keep launcher validation scoped to shipped assets and describe the remaining orchestration and critic roles accurately.

Co-Authored-By: Claude <noreply@anthropic.com>
Describe empty turns and retained close-out verdict fields without implying that the retired agent still runs.

Co-Authored-By: Claude <noreply@anthropic.com>
Keep one session bound across server boot, reuse, fallback and anchor measurements. Stop budget exhaustion without retrying it as a server failure.

Co-Authored-By: Claude <noreply@anthropic.com>
Wait for cooperative physical cleanup before closing the database. Keep exactly-once completion with the execution after caller cancellation, and preserve ownership when cleanup is unconfirmed.

Co-Authored-By: Claude <noreply@anthropic.com>
Replace timing guesses with execution completion and assert that unconfirmed GPU cleanup preserves leases and database access.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread src/hyperloom/inference_optimizer/tests/test_grid_runner.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/orchestrator/state/tests/test_round_store.py Fixed
Preserve harness-aware detachment, persisted run environment and authoritative PID checks while keeping the retired monitor and explicit resume boundary.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread src/hyperloom/inference_optimizer/tests/test_grid_runner.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
# Exercise both sides of the synchronous-to-async shutdown regression.
closing = dispatcher.close_db_after_executions()
if inspect.isawaitable(closing):
await closing
# Historical reports retain these terminal reasons after the producer is retired.
DIED_STOP_REASON = "supervisor_coordinator_died"
WEDGED_STOP_REASON = "supervisor_tick_stalled"
SUPERVISOR_RESTART_REASON = "supervisor_restart_requested"
on_complete.assert_awaited_once()
else:
with pytest.raises(ExecutionCleanupUnconfirmed):
await execution
task, gpu_lease=gpu, gpu_specialist_lease=RayLease(), on_complete=on_complete
)
if confirmed:
await execution
assert (await dispatcher.tasks.get(task.task_id)).state == "running"
assert await dispatcher.locks.lane_holders()
finish.set()
await action
Zeng and others added 6 commits September 17, 2026 15:53
…dence

Keep unsettled rounds exclusive after their nominal lease time and test report alerts only when explicitly recorded.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Preserve bounds, evaluation directory, measurement admission and policy recording coverage without relying on retired timeout arguments.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Exercise real dispatch and completion paths with controlled workers, preserving bounded waits and verifying eventual cleanup instead of immediate handle removal.

Co-Authored-By: Claude <noreply@anthropic.com>
Inspect the removed integrate timeout parameter without an invalid call and preserve explicit side effects in deadline and Ray lease tests.

Co-Authored-By: Claude <noreply@anthropic.com>
assert result.state == ("failed" if fails else "succeeded")
else:
with pytest.raises(ExecutionCleanupUnconfirmed, match=task.task_id):
await execution
Comment on lines 709 to +711
"* **RULE F3** — repeated `error_class='subprocess_nonzero'` on `baseline`"
" → stop retrying baseline; send observation 'blocked: …' and let Robustness"
" intervene. Explore variants may be re-proposed; read the failure log first.",
" → stop retrying baseline; send observation 'blocked: …'."
" Explore variants may be re-proposed; read the failure log first.",
Co-Authored-By: Claude <noreply@anthropic.com>
@xiaofei-zheng
xiaofei-zheng marked this pull request as ready for review September 17, 2026 08:40
@xiaofei-zheng

Copy link
Copy Markdown
Collaborator

Review

Checked paths: _subprocess_kill.py (the rewritten wait loop, _StreamCapture, log-scan helpers), task_registry.reclaim_*, resource_lock.holder_is_dead, and the call sites that pass silence_timeout_sec / server_already_ready. Verified against merge-base a4c1bea70. No local test run; CI on 3958bfa71 is green.

The direction matches what was agreed: force unbuffered child output, count both the child's own pipes and the round's server logs as liveness, one silence window plus one hard cap, and drop age-only reclamation in favour of PID-proof reclamation. _StreamCapture._pump moving from readline() to read1() + IncrementalNewlineDecoder, and holder_is_dead gating on owner_scope before trusting a local PID, are both correct and are real improvements over main.

Blocking

1. Warm-reuse rounds arm a 600 s silence gate that has no way to observe the reused server, so healthy work is killed with -911.

_subprocess_kill.py:913ready_at = start if server_log_path and server_already_ready else None. On merge-base, server_ready_since was set only by an observed ready marker (server_already_ready did not set it), so the stall gate never armed on a reuse round. After this PR the gate is armed at t=0 for every reuse round, and _subprocess_kill.py:961 starts counting silence immediately.

In the same change the evidence available to that gate was narrowed:

  • _subprocess_kill.py:589sizes[path] = -1 if candidate.parent != Path(server_log_path).parent else size
  • _subprocess_kill.py:606if prev < 0: continue

Any resolved log that already existed at spawn and sits outside the round's own directory — i.e. the previous round's benchmark_*/server.log, which the reused server keeps appending to — is marked -1 and then skipped for the entire round. On merge-base those files were seeded with their current size, so subsequent appends still counted as grew. The reused server's ongoing output therefore went from "counts as liveness" to "can never count as liveness", in the same change that armed the gate against it.

The trigger is the default configuration, not a constructed one: the conc-sweep and baseline-warmup paths both pass server_already_ready=True with silence_timeout_sec=600 (test_conc_sweep.py:2203, test_baseline_warmup_double_run.py:204-206). With the pinned Magpie e6833b818, which writes its benchmark logs only after the client exits, the one remaining evidence source (the child's pipes) is also silent, so a healthy reuse round reaches 600 s of apparent silence and is reaped.

This is the defect the PR body already documents and reproduces ("A continuously growing warmup log outside the watched round, or buffered client-only output, triggered the silence sentinel -911"). It should be fixed before merge: the purpose of this refactor is to remove false kills, and as it stands it introduces a new one on the default path. The body says this "keeps this PR in Draft" and leaves "Resolve and regression-test the original-Magpie activity-visibility issue" and "Revalidate the resulting implementation" unchecked, but the PR is currently open as ready for review — either move it back to draft or land the fix.

Direction, matching the body's own conclusion: bind the activity source to the execution that actually owns the round — for a reuse round that is the server log the reused server is still writing, not only the new output directory — and do not arm the silence gate on a round whose activity source is not observable.

No other blocking issues found.

@xiaofei-zheng

Copy link
Copy Markdown
Collaborator

Review, part 2 — execution ownership / lease lifecycle

Follow-up after reading the rest of the diff. The changes to dispatcher.py, sub_agent_runner.py and resource_lock.py go well beyond "remove age-only reclamation": the ownership model was rewritten so that nothing is released unless cleanup is positively acknowledged. That direction is right in principle, but with the time-based sweeps gone there is now no path back from any of the unacknowledged cases, and several of them are reachable on the normal path. All four below were checked against merge-base a4c1bea70.

2. After a host reboot, or on a DB written before this change, leases and running rows can never be reclaimed.

reap_expired (SqliteLeaseBackend, SpecialistGpuPool) and TaskRegistry.reclaim_expired_running are both removed, leaving holder_is_dead as the only reclamation judgement:

  • resource_lock.py:363-372 — returns False unless row["owner_scope"] == local_owner_scope()
  • resource_lock.py:80-87local_owner_scope() is boot_id:pid_ns_inode
  • schema.py:241-242 — the migration backfills existing rows with owner_scope = ''

So a lease row whose owner_scope is '' (any DB written before this PR) or whose boot_id no longer matches (node rebooted, pod rescheduled) is judged alive forever. task_registry.py requires all(holder_is_dead(row)), so the corresponding running rows are never failed either. --resume-from after a crash-and-reboot is exactly the case that leaves those rows behind, and nothing in the resume path deletes them: the stale lanes stay at capacity, try_acquire_many never succeeds, and the pump scans the queue every tick without dispatching anything — no exception, no warning.

Either treat a non-matching owner_scope at schema-upgrade / resume time as prior-boot ownership and clear it, or keep one time-based backstop for rows whose scope cannot be compared.

3. raise inside finally discards a completed result and leaks the lane lease.

sub_agent_runner.py:356-369:

finally:
    ...
    released = release_resources is None or await release_resources()
    if not released:
        raise ExecutionCleanupUnconfirmed(...)
    if lease is not None:
        await self.locks.release(lease)

An exception raised in finally replaces the pending return value. release_resources (dispatcher.py:947-955) returns False whenever GpuSpecialistLease.close() does not return True, and close() returns stop(), which is False on a ray.get(..., timeout=CLOSE_STOP_TIMEOUT_SEC) timeout or RayError (_ray_serving.py:658-690) — an ordinary outcome for a specialist whose process tree is slow to tear down.

When that happens the task's terminal row is already written, but execute_and_complete never reaches on_complete, so _reap_dispatched_task does not run: specialist auto-retry, delegated_result, ledger entries, shared-state promotion and fact-write are all skipped, and a successful GPU specialist result is silently dropped. The raise also precedes self.locks.release(lease), so the lane lease is leaked — and by finding 2 it will not be reclaimed while this process lives. On merge-base this path released the lease unconditionally in finally and logged the close failure.

4. _inflight_actions and _executions are only cleaned on the success path.

dispatcher.py:966-967 — both pop and discard sit after a successful run_task, where merge-base had _inflight_actions.pop in a finally. Any execution that raises (finding 3, IllegalTransition, the policy/lane errors in run_task) or is cancelled leaves its entry behind permanently:

  • dispatcher.py:613if not join_in_pump and task.task_id in self._inflight_actions: continue then skips that task id for the rest of the session, so the row can never be re-dispatched.
  • dispatcher.py:171-183unconfirmed is computed over the session-wide _executions set, so one failed or cancelled execution at any point means self.db.close() is never reached again and every subsequent shutdown logs the error.

Retaining ownership for a pending cleanup is the intent; retaining it for the whole session because something failed once is not the same thing.

5. Cancelled in-flight actions get no completion bookkeeping, and the branch that used to handle them is now unreachable.

dispatcher.py:441-459 replaced the per-entry atask.result() / _reap_dispatched_task(...) loop with await asyncio.gather(*done, return_exceptions=True). The only remaining caller of _reap_dispatched_task is on_complete (dispatcher.py:835), invoked solely with a successful SubAgentResult — so dispatcher.py:1213's isinstance(maybe_result, asyncio.CancelledError) branch is dead code.

This is not only a shutdown path: _gate_pump_start cancels in-flight actions on session_time_exhausted and returns False, so the loop keeps running (dispatcher.py:353-359). Combined with the removal of the cancelled_in_flight terminal write (sub_agent_runner.py:328-331), a budget-cancelled action now leaves a running row, a held lane lease, a stale _inflight_actions entry and no delegated_result — inside a session that continues.

Findings 3, 4 and 5 compound: each one leaves state that only findings 2's reclamation could have cleared, and that reclamation no longer fires while the process is alive.

Keep warm-reuse metadata separate from silence evidence so buffered Magpie clients are not reaped without a current ready marker. Preserve hard deadlines, session cancellation and existing telemetry.

Co-Authored-By: Claude <noreply@anthropic.com>
@zengleixin-amd

zengleixin-amd commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the warm-reuse silence blocker in d330461 with a three-line production change, rather than introducing cross-layer log tracking.

server_already_ready still supplies existing telemetry, but no longer arms silence. The existing incremental scanner must observe a ready marker in the current round before the 600-second gate applies. Old nested logs remain excluded, and hard deadlines, session deadlines and cancellation remain unchanged.

The narrowed behavior was explicitly accepted: reuse with no current ready evidence is protected by hard/session/cancel bounds, not by the silence gate. This avoids the original Magpie buffered-output false-kill without modifying Magpie, adding a heartbeat, or scanning unrelated historical logs. It is not a claim that all internally healthy but externally silent workloads can be distinguished from hangs.

Tests: 7 expected failures before the change became 17/17 passes; the related local selection passed 228 tests with POSIX cases left for Linux. Latest-head full CPU CI passed: all 12 Python 3.10/3.11 shards and both coverage gates succeeded. https://github.com/AMD-AGI/Hyperloom/actions/runs/35206770698 The implementation, regression tests and CHANGELOG are the only three changed files in this follow-up.

🤖 Generated with Claude Code

@BaoYunkai

Copy link
Copy Markdown
Collaborator

Re-review against merge-base a4c1bea

What this PR does

Retires the Robustness agent/RCA, the external Monitor and Hyperloom Supervisor, and the runtime recover action, keeping explicit --resume-from, offline recover-session and the historical SBD v6 readers. Benchmark lifetime collapses to two limits: 600 s of output silence after a ready marker is observed in the current round, and a 7800 s per-process hard cap. Age-only task and lease reclamation is replaced by PID-proof reclamation. 309 files, +3786 / -31823.

Finding 1 is fixed

d330461 is correct. _subprocess_kill.py:913-914 now initialises ready_at and last_activity_at to None; the silence check at :961 is gated on ready_at is not None, and ready_at is only set at :937 by scan.saw_ready. server_already_ready still drives the kv_recorder phase note and stamp_server_ready, which is telemetry only. test_kill_spawned_server.py passes 88/88 locally.

Findings 2-5 are unchanged and still blocking

d330461 landed at 09:44Z, after the part-2 review at 09:21Z, but touches only CHANGELOG.md, test_kill_spawned_server.py and _subprocess_kill.py. dispatcher.py, sub_agent_runner.py, resource_lock.py and task_registry.py are untouched since that review. All four were re-checked against merge-base; two were reproduced by running the code.

2. Leases and running rows are unreclaimable after a reboot, or on a DB written before this change.

resource_lock.py:363-372holder_is_dead returns False unless row["owner_scope"] == local_owner_scope(), and local_owner_scope() (:80-87) is boot_id:pid_ns_inode. storage/schema.py:242 backfills existing rows with owner_scope = ''.

Reproduced against a PID that is certainly absent:

owner_scope=''            -> holder_is_dead = False
owner_scope=<old boot_id> -> holder_is_dead = False
owner_scope=<current>     -> holder_is_dead = True

reap_expired (gpu_pool, resource_lock) and reclaim_expired_running (task_registry) are removed in this PR. The per-tick self-heal _reclaim_stale_dispatch_state does not cover the gap: both tasks.reclaim_dead_running (task_registry.py:402, all(holder_is_dead(row) ...)) and locks.reap_dead_holders (resource_lock.py:384) use the same predicate. So a pre-upgrade DB, or any row surviving a node reboot or pod reschedule, is judged alive forever, and the lanes it holds stay at capacity with no exception and no warning.

3. raise inside finally discards a completed result and leaks the lane lease.

sub_agent_runner.py:356-369raise ExecutionCleanupUnconfirmed(...) is inside finally and precedes await self.locks.release(lease). An exception raised in finally replaces the pending return value, and the release never runs; confirmed by executing the same shape.

The trigger is ordinary: dispatcher.py:947-955 release_resources returns False whenever GpuSpecialistLease.close() is not True, and close() returns stop(), which is False on a ray.get(..., timeout=CLOSE_STOP_TIMEOUT_SEC) timeout or RayError. At that point the terminal row is already written, but execute_and_complete never reaches on_complete, so _reap_dispatched_task does not run: specialist auto-retry, delegated_result, ledger entries, shared-state promotion and fact-write are skipped, and a successful GPU specialist result is dropped. Merge-base released the lease unconditionally in finally.

4. _inflight_actions and _executions are cleaned only on the success path.

dispatcher.py:966-967 — both the pop and the discard sit after run_task and on_complete, not in a finally; merge-base had finally: self._inflight_actions.pop(...) at :943-944. This is the only pop in the file — cancel_inflight_actions (:237-305) cancels and gathers but never removes entries.

Two consequences:

  • dispatcher.py:613 — a leftover entry makes a _NOT_JOINED_KINDS task skipped for the rest of the session, so its row can never be re-dispatched.
  • dispatcher.py:171-183unconfirmed is computed over the session-wide _executions. One execution that raised or was cancelled stays in the set with an exception, so every later shutdown returns early and self.db.close() is never reached.

Retaining ownership for a pending cleanup is the intent; retaining it for the whole session because something failed once is not.

5. Cancelled in-flight actions get no completion bookkeeping, and the branch that handled them is unreachable.

dispatcher.py:441-459 replaced the per-entry _reap_dispatched_task loop with await asyncio.gather(*done, return_exceptions=True). The only remaining caller of _reap_dispatched_task is on_complete at :835, invoked at :964-965 solely with a successful SubAgentResult, so the isinstance(maybe_result, asyncio.CancelledError) branch at :1213 is dead code.

This is not only a shutdown path: _gate_pump_start (:345-359) cancels in-flight actions on session_time_exhausted and returns False, so the loop continues. Combined with the removal of the cancelled_in_flight terminal write in sub_agent_runner.py (present at merge-base, absent here), a budget-cancelled action leaves a running row, a held lane lease, a stale _inflight_actions entry and no delegated_result, inside a session that keeps going.

Findings 3, 4 and 5 compound: each leaves state that only finding 2's reclamation could clear, and that reclamation cannot fire while the process is alive.

Also checked, no issue found

Removal is clean: no non-test references remain to supervisor.watch, RobustnessAgent or actions.executors.recover. The remaining robustness references are confined to breakdown/, which is the historical SBD v6 reader the PR states it keeps. dispatcher, sub_agent_runner and resource_lock import cleanly. CHANGELOG carries an entry for this change.

No blocking issues beyond the four above.

Zeng and others added 2 commits September 18, 2026 11:32
Co-Authored-By: Claude <noreply@anthropic.com>
Keep exited-root cleanup unconfirmed and preserve the same specialist actor when stop does not acknowledge teardown. Cover detached descendants with isolated Linux CPU regressions.

Co-Authored-By: Claude <noreply@anthropic.com>
Zeng and others added 8 commits September 18, 2026 12:31
Persist diagnostic outcomes when cleanup remains unconfirmed, unregister only completed execution callbacks, and reuse terminal inline results without duplicate effects. Cover cleanup evidence, cancellation and late worker acknowledgment.

Co-Authored-By: Claude <noreply@anthropic.com>
Listen to the existing cancel scope while waiting for builds and retain outcomes and pending state until physical cleanup is confirmed.

Co-Authored-By: Claude <noreply@anthropic.com>
Wire existing cancellation scopes through pending, running and done-grace paths. Confirm local tree teardown and allow one same-lease follow-up acknowledgment without a background retry mechanism or speculative resource release.

Co-Authored-By: Claude <noreply@anthropic.com>
Inspect persisted ownership read-only before resume writes, report bounded diagnostics for unobservable holders, and consume the latest explicit cleanup outcome when reconciling open rounds.

Co-Authored-By: Claude <noreply@anthropic.com>
Use one earliest sweep/session deadline across boot, retries and reuse. Preserve stop attribution and admit unknown-duration work only while budget remains, rather than charging the hard cap at admission.

Co-Authored-By: Claude <noreply@anthropic.com>
…r state

Keep stale locks when process identity cannot be observed, exclude known zombies, and restore the existing safe sweep at actual baseline startup.

Co-Authored-By: Claude <noreply@anthropic.com>
Remove the retired per-variant timeout argument and CLI option, preserving the intentional unbounded overall manual sweep and production per-process caps.

Co-Authored-By: Claude <noreply@anthropic.com>
Document sweep budgets, cooperative cancellation, durable diagnostic outcomes, explicit resume refusal, bounded same-lease confirmation and safe AITER cleanup. Align recorder field descriptions with measured admission costs.

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed

import pytest

import hyperloom.orchestrator.actions.executors._grid_runner # noqa: F401
asyncio.get_running_loop().call_soon(finish.set)
await _close(dispatcher)
with pytest.raises(RuntimeError, match="completion failed"):
await caller
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Comment thread src/hyperloom/inference_optimizer/tests/test_ray_backend_unit.py Fixed
Zeng and others added 2 commits September 18, 2026 12:52
Replace the stale hard-cap admission assumption with deterministic grid-refusal wiring and verify full terminal outcome evidence for retired recover tasks. Preserve existing resource and timeline assertions.

Co-Authored-By: Claude <noreply@anthropic.com>
Preserve the late-acknowledgment and idempotency checks while ensuring all three close calls execute independently of Python assertion optimization.

Co-Authored-By: Claude <noreply@anthropic.com>
@zengleixin-amd

zengleixin-amd commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Review follow-up and requirement reconciliation

The review fixes are now on this branch in scoped commits (each at most five files), with the two integration-contract regressions corrected in 7d8a1a2. That SHA passed all 12 Ubuntu Python 3.10/3.11 test shards and both strict coverage thresholds: https://github.com/AMD-AGI/Hyperloom/actions/runs/35308832684 . The final 2713b1e changes only three close calls inside one test so they execute outside assertions; its final CI has now PASSED: https://github.com/AMD-AGI/Hyperloom/actions/runs/35310187287 . All 12 final-head test shards, both strict coverage thresholds, and CodeQL succeeded; no checks remain pending or failed. Human review is still required and the PR remains unmerged.

Findings 2–5

  1. Unknown/foreign/legacy ownership is now diagnosed and refused before resume execution, rather than silently stalling. It is not considered dead merely because a boot ID differs or a TTL expired. No clear-all or force-resume path was added.
  2. Completed outcomes survive unconfirmed cleanup in durable task history and on ExecutionCleanupUnconfirmed.result. They are diagnostic-only: ordinary delegated_result replay, promotion, retry and fact-write remain blocked. Both resume and round reconciliation consume the latest explicit cleanup evidence, including terminal-race appends. Retaining unconfirmed capacity is intentional safety, not a leak fixed by unconditional release.
  3. Completion callback failure after a normally returned, cleaned-up runner now unregisters execution tracking. Unknown runner exit and raw asyncio cancellation intentionally retain tracking; normal failed results and cooperative cancellation are not confused with those cases.
  4. Build and specialist executors now observe the existing cancellation scope before spawn and while waiting, including pending/done-grace phases. Confirmed cooperative cancellation completes bookkeeping once. The specialist consumes one bounded follow-up acknowledgment on the same Ray lease, with full dispatcher/runner regression coverage; there is no periodic retry service.

Additional verified fixes

  • Sweep admission uses measured expected cost, not the 7800-second hard cap. All paths use the earlier sweep/session deadline and retain its stop attribution.
  • Exited roots/empty old process groups no longer manufacture a tree-cleanup acknowledgment. An unconfirmed actor remains available for the bounded follow-up.
  • AITER baseline cleanup uses the existing helper, preserves locks on unknown compiler state, and excludes known zombies. The manual flow driver follows the production signature.
  • The original current-round-ready-only silence fix remains unchanged. No Magpie source/pin change, new supervisor, TTL reclamation, global shield removal or forced resource cleanup was added.

Explicit limits

Local specialist natural-exit handling does not claim to discover all detached orphans; unknown Ray/foreign-session cleanup still requires operator verification. Historical SBD readers, explicit recovery entrypoints, and KernelForge internal supervision remain intentionally supported. CPU/AST/mock evidence is not real GPU/Ray E2E evidence. Each automatically triggered GPU E2E for these pushes was explicitly approved for precise cancellation and confirmed cancelled before job steps ran.

The implementation is frozen to this scope. This response does not request merging or changing the PR state.

🤖 Generated with Claude Code

@BaoYunkai

Copy link
Copy Markdown
Collaborator

Re-review of the follow-up commits, against merge-base 2cc382a

What changed since the last review

Upstream main was merged (merge-base moved from a4c1bea70 to 2cc382a83) and eleven commits answer findings 2-5. Execution ownership was reworked so nothing is released without acknowledged cleanup, and a new read-only session/resume_guard.py inspects persisted ownership before a resume writes anything, refusing with bounded diagnostics when a prior execution's exit cannot be established. The rest covers Ray worker cleanup evidence, bounded specialist cleanup acknowledgment, cooperative cancellation in the targeted build executor, and separating measured sweep admission cost from the hard cap.

Findings 3, 4 and 5 are resolved

  • 4dispatcher.py:964-971: _inflight_actions.pop and _executions.discard are now in a finally, so an execution that raises or is cancelled no longer leaves a permanent entry. :173 additionally intersects with self._executions before computing unconfirmed, so shutdown is no longer wedged by a past failure.
  • 3sub_agent_runner.py:372-415: the outcome is carried on ExecutionCleanupUnconfirmed.result and written to task history with cleanup_confirmed and cleanup_error. A result whose cleanup could not be confirmed is now recorded as diagnostic rather than dropped, and the lease is released only on confirmed cleanup, which is the stated contract.
  • 5 — cancelled outcomes now reach _reap_dispatched_task and are branched on result.state == "cancelled" at :1236, :1329 and :1394.

Finding 1's fix survived the merge: _subprocess_kill.py:913-914 still initialises ready_at/last_activity_at to None, and :960 gates silence on ready_at is not None.

Tests run locally, 364 passed: test_dispatcher_execution_shutdown.py, test_session_time_budget.py, test_task_progress_heartbeat.py (186); test_ray_serving_process_cleanup.py, test_build_lifecycle.py, test_specialist_subprocess_kill_unit.py, test_reconcile.py (113); test_resume_guard.py (65).

One blocking issue remains

A session DB written before this PR can never be resumed once this lands, and no supported interface can clear it.

storage/schema.py:242 backfills existing rows with ALTER TABLE leases ADD COLUMN owner_scope TEXT NOT NULL DEFAULT ''. resume_guard.py:81 accepts ownership only when owner_scope and row["owner_scope"] == owner_scope and pid > 0, so '' never qualifies and the row is reported as execution owner scope or PID is unobservable.

Reproduced with a pre-upgrade shaped DB (one owner_scope='' lease row plus a running task) on the same machine, same boot, with no crash:

RESULT: ResumeBlocked
  task='task-1' lane='gpu' holder='holder-1': execution owner scope or PID is unobservable
  task='task-1' lane='gpu' holder='unknown': running execution has no observable local owner

There is no way past it:

  • cli/__init__.py:1672 calls ensure_resume_safe unconditionally, before any other resume gate.
  • --force-resume targets the separate target_reached guard and is only consulted at cli/__init__.py:1878, after the block.
  • recover-session (cli/recover.py:50) only rebuilds the breakdown and flushes Langfuse; it does not touch ownership.
  • resume_guard.py has no override.

The diagnostic tells the operator to verify cleanup in the original execution environment. For owner_scope = '' that environment is the current machine — the column did not exist when the row was written, so what is missing is the evidence, not the process — and there is no interface to express "verified, proceed". Cleanly closed sessions carry no lease rows and are unaffected; the sessions that are blocked are the crashed ones, which are exactly the ones --resume-from exists for.

The CHANGELOG does state "Legacy or foreign execution ownership ... No ownership is cleared", so this is disclosed rather than hidden. The problem is that the disclosed remedy cannot be carried out on this path, which leaves existing sessions unrecoverable.

Either direction resolves it:

  • Distinguish owner_scope == '' (written before the column existed, no evidence either way) from a non-empty scope that does not match (positively another boot or PID namespace). Fall back to the pre-PR PID-liveness judgement for the former and keep refusing the latter.
  • Or give the operator the acknowledgment path the CHANGELOG implies, for example a recover-session action that releases ownership and records who released it.

No other blocking issues found.

Allow an operator to attest that an unattributed task and its workers have stopped, record diagnostic audit evidence, and atomically release only its execution/GPU ledger entries under the session lock. Preserve round ownership, refuse nonempty scopes and later work, and leave normal resume admission unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
@zengleixin-amd

zengleixin-amd commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Resolution of the remaining legacy-resume blocker

The compatibility gap in the latest review was confirmed: a retained task with empty ownership scope could not pass resume, and manual verification alone had no supported way to update that ledger. Clean legacy databases were not affected. We did not adopt automatic PID fallback: an empty scope also occurs when scope discovery fails, and does not establish the original host, boot or PID namespace.

Commit 0fd82c6 adds the explicitly approved, narrow operator acknowledgment path in exactly five files:

inference_optimizer recover-session --session-dir PATH \
  --confirm-stopped TASK_ID \
  --confirmation-reason "Verified the entire task process tree and all remote workers/Ray actors have stopped"

The operator must independently establish physical cleanup first. The command does not stop processes or actors. It acquires the real POSIX session lock and updates only the specified task in one SQLite transaction: record operator/time/reason/resource audit evidence, cancel unfinished work, and release its unattributed execution/GPU ledger rows. It preserves existing terminal state, old outcomes, other tasks, round records/fences and round leases. The ordinary reconciler consumes the new cleanup evidence; normal resume admission remains read-only and strict.

Nonempty or mixed owner scopes remain rejected. Repeating a completed confirmation is a no-op, but later resources/execution evidence cannot be cleared using the old acknowledgment. There is no wildcard/clear-all, TTL fallback, automatic resume, report upload, or result promotion. Existing recover-session --force retains report-only semantics. This is an explicit exception to the previously selected no-manual-clear boundary, not an implicit weakening of automatic reclamation.

Validation: new CLI regressions failed before implementation (9 failed/1 control passed), core regressions also failed first; final local resume/reconcile/session-lock selection passed 185 tests with 12 POSIX-dependent skips. Independent review, Ruff, formatting and Bandit passed. Latest-head Linux validation PASSED on 0fd82c6: https://github.com/AMD-AGI/Hyperloom/actions/runs/35332798196 . All 12 Python 3.10/3.11 test shards, both strict coverage thresholds, and CodeQL succeeded; no checks remain pending or failed. This includes the real POSIX lock/public-CLI path and the existing report recovery tests. Real GPU validation is not claimed; human review of this final fix remains required. The exact automatically triggered GPU E2E was cancelled with explicit approval before any job steps ran.

No actual user session ledger or cluster resource was modified while implementing this fix. Temporary working documents stayed outside the repository and were not committed.

🤖 Generated with Claude Code

@BaoYunkai

Copy link
Copy Markdown
Collaborator

Re-review of 0fd82c6

The previous blocker is resolved

--confirm-stopped TASK_ID with a required --confirmation-reason gives the operator the acknowledgment path that was missing, which is what made a pre-upgrade DB unresumable. Verified end to end against a legacy-shaped DB (one owner_scope='' lease row plus a running task, same machine, same boot):

step 1 -- legacy DB, before any attestation
          ResumeBlocked -- task='task-1' lane='gpu' holder='holder-1': execution owner scope or PID is unobservable
step 2 -- operator attests task-1 has stopped
          confirmed: {'status': 'confirmed', 'task_id': 'task-1', 'released_leases': 1, 'released_gpu_leases': 0}
step 3 -- resume after attestation
          resume ALLOWED

The guard rails read correctly too: both options are required together, they cannot be combined with --force or --backfill-trace, a recorded nonempty owner scope is still refused, and the release is scoped to that task's execution/GPU ledger entries under the session lock. test_resume_guard.py passes 134/134 locally.

One new blocking issue

The atom demo is the only place left telling the agent to poll, after this PR removed that instruction everywhere else.

examples/hyperloom-qwen3-14b-fp8-12h-atom/SKILL.md:384 still reads:

8. Monitor the process every 300 seconds until work is done.

That line is not new — it is present at merge-base 2cc382a83 at the same line. What this PR changed is everything around it:

  • All five demo SKILL.md files carried this instruction at merge-base. The PR deletes it from four (hyperloom-custom-advanced, hyperloom-qwen3-14b-fp8-12h-forge, hyperloom-qwen3-14b-fp8-12h, hyperloom-qwen3-8b-3h) and leaves only the atom one.
  • The replacement prohibition ("Do not start a background watchdog or automatic resume loop", "Read persisted state on requested status checks") is added to eleven files: the three .github/pre-release/prompts/pre-release/demo-*.md, docs/how-to/optimize.md, docs/install/install.md, src/hyperloom/inference_optimizer/SKILL.md, src/hyperloom/inference_optimizer/references/operations.md, and the four demos above. The atom demo is not among them.
  • The PR does edit this file, at lines 375-376 and 385-386 — immediately above and below line 384 — so the sweep reached the file and skipped the line.

The result is a contradiction inside one numbered list: step 9 now says crashes are not automatically resumed and need explicit operator approval, while step 8 directly above still instructs a 300-second monitoring loop. An agent following the atom demo will poll or relaunch in the way the rest of this PR forbids.

Deleting line 384, as the other four demos do, is the whole fix.

No other issues found in the new commit.

Align both progress reporting and launch requirements with request-driven persisted-state checks. Keep recurring checks opt-in through platform scheduling and preserve explicit resume approval.

Co-Authored-By: Claude <noreply@anthropic.com>
@zengleixin-amd

zengleixin-amd commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the ATOM demo polling instruction in 2e45aad. Only examples/hyperloom-qwen3-14b-fp8-12h-atom/SKILL.md changed (+5/-2).

The cited launch requirement now requests persisted-state inspection only on requested status checks. The same file also contained a second instruction at the top of its progress section requiring a summary at every 300-second check; that paragraph now uses the exact request-driven wording already present in the other demos, including platform scheduling only when recurring checks are requested and no background watchdog, blocking polling connection, or automatic resume.

Verified before/after: both obsolete fixed-polling instructions were present before the change and are absent now; the replacement paragraph matches the existing non-ATOM demo, list numbering is unchanged, and all other file contents are unchanged. git diff --check passed. This is a one-file instruction correction, not a runtime or temporary-document change.

The prior runtime head 0fd82c6 passed full Linux CPU validation. New-head validation PASSED on 2e45aad: https://github.com/AMD-AGI/Hyperloom/actions/runs/35344287663 . All 12 Python 3.10/3.11 test shards, both strict coverage thresholds and CodeQL succeeded, with no failed or pending checks. The exact automatic GPU E2E for this SHA was approved for cancellation; no manual GPU run was started. The PR remains unmerged.

🤖 Generated with Claude Code

@BaoYunkai
BaoYunkai merged commit d397fe8 into main Sep 18, 2026
32 of 35 checks passed
@BaoYunkai
BaoYunkai deleted the feature/leixin/simplify-watchdog branch September 18, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants