Fix three orchestrator bugs found during extended framework testing - #1125
Fix three orchestrator bugs found during extended framework testing#1125AMD-melliott wants to merge 6 commits into
Conversation
|
Thanks for chasing these down. The root-causing in the description holds up, and the 1. The new unrelated-transition cleanup drops a legitimate 2. The EXPLORE fix doesn't close the failure it's named for. 3. The 4. When every probe target is down, the branch fires once per target. 5. 6. The sentinel pass-through is silent. 7. The discarded hint is written to an audit field that means the opposite. 8. 9. Four comments now contradict the code. The 10. Two of the defensive reads can't fire. 11. None of the three behaviours has a test. The 105 in the description pass on this head, but they're all pre-existing; nothing exercises the sentinel pass-through, the hint cleanup, or the On the rebase. |
2b50a44 to
29a9468
Compare
…und EXPLORE exit Review feedback on AMD-AGI#1125 found the unrelated-transition hint cleanup added in the prior commit was itself a regression, and that it hadn't actually closed the zero-round EXPLORE bug it was meant to fix: - machine.py's cleanup discarded a skip_to_kernel/skip_to_close hint on ANY phase transition fired for an unrelated reason, including one where the hint was still legitimately in flight toward its only consumer (exit_normal_explore, which only runs once phase == EXPLORE). A hint set during FRAMEWORK_AGENT no longer survived the FRAMEWORK_AGENT -> EXPLORE transition to reach it. Only discard now when the transition target isn't EXPLORE, and log the discard. - exit_normal_explore still honored skip_to_kernel unconditionally, ahead of compute_plateau_explore's own evidence requirement -- so a hint that arrived before EXPLORE had dispatched any specialist round this cycle still exited with explore_search.tested=0 (the direct cause of a prior cumulative_gain_validated=0.00% session). Gate the hint on at least one specialist round having run this macro-cycle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@lishuoshuo-amd Quick status update: pushed a commit (29a9468) addressing the two core issues from the review:
Also picked up a few smaller items from the review while I was in there: extended the source-file sentinel list for TBD/AITER (vendor)/Triton (vendor), threaded I haven't yet touched |
29a9468 to
bfeb300
Compare
…und EXPLORE exit Review feedback on AMD-AGI#1125 found the unrelated-transition hint cleanup added in the prior commit was itself a regression, and that it hadn't actually closed the zero-round EXPLORE bug it was meant to fix: - machine.py's cleanup discarded a skip_to_kernel/skip_to_close hint on ANY phase transition fired for an unrelated reason, including one where the hint was still legitimately in flight toward its only consumer (exit_normal_explore, which only runs once phase == EXPLORE). A hint set during FRAMEWORK_AGENT no longer survived the FRAMEWORK_AGENT -> EXPLORE transition to reach it. Only discard now when the transition target isn't EXPLORE, and log the discard. - exit_normal_explore still honored skip_to_kernel unconditionally, ahead of compute_plateau_explore's own evidence requirement -- so a hint that arrived before EXPLORE had dispatched any specialist round this cycle still exited with explore_search.tested=0 (the direct cause of a prior cumulative_gain_validated=0.00% session). Gate the hint on at least one specialist round having run this macro-cycle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@lishuoshuo-amd Pushed four more commits addressing the remaining review items. Rebased onto current
Full suite: 13,786 passed / 21 failed / 36 skipped — the 21 failures are pre-existing and unrelated (credential/auth-probe tests failing on missing local fixtures), confirmed identical before and after this branch's changes; unchanged by anything here. |
Root-caused by the orchestrator's own self-diagnosis in reports/final.md
after a 9h08m run found zero validated gain despite 95.8% roofline
headroom:
- PolicyGate rejected every specialist delegate this session because a
placeholder string ("Not found") in a source_file field isn't empty,
so it reached the trusted-scope check and was denied identically 15
times. Treat known absent-value sentinels as an omitted field instead
of a bogus path.
- EXPLORE could exit via a stale pending_escalate_hint left over from a
different phase or macro-cycle, before ever dispatching a round
(explore_search.tested=0), which was the direct cause of
cumulative_gain_validated=0.00%. Clear the hint whenever a phase
transition fires for an unrelated reason, and on macro-cycle reload.
- local_server_unreachable had no remediation branch in the action
ladder; its own suggestion text pointed at "server_lifecycle", which
isn't a real dispatchable action. Wire it to the existing recover
action instead, mirroring the gpu_memory_leaked branch.
Also threads a PolicyGate rejection's specific rule through
SubAgentResult into the gap ledger as error_class instead of the
generic "unknown_error", so future policy denials are recognizable
instead of blending into normal retries.
…und EXPLORE exit Review feedback on AMD-AGI#1125 found the unrelated-transition hint cleanup added in the prior commit was itself a regression, and that it hadn't actually closed the zero-round EXPLORE bug it was meant to fix: - machine.py's cleanup discarded a skip_to_kernel/skip_to_close hint on ANY phase transition fired for an unrelated reason, including one where the hint was still legitimately in flight toward its only consumer (exit_normal_explore, which only runs once phase == EXPLORE). A hint set during FRAMEWORK_AGENT no longer survived the FRAMEWORK_AGENT -> EXPLORE transition to reach it. Only discard now when the transition target isn't EXPLORE, and log the discard. - exit_normal_explore still honored skip_to_kernel unconditionally, ahead of compute_plateau_explore's own evidence requirement -- so a hint that arrived before EXPLORE had dispatched any specialist round this cycle still exited with explore_search.tested=0 (the direct cause of a prior cumulative_gain_validated=0.00% session). Gate the hint on at least one specialist round having run this macro-cycle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
aff9db7 to
c064fa6
Compare
|
Update: rebased onto current Also ran a fresh ~24h validation against a different model/framework than the original bug reports (Qwen3.8-27B on vLLM, vs. DeepSeek-V4-Flash on ATOM), specifically to stress-test these three fixes end to end under real, extended load. All three held up:
More importantly, the underlying zero-round Caveat on that last result, for accuracy: reaching that full clean run also needed two fixes outside this PR's scope — a critic completion-token-budget fix (independently found and fixed better in #1214) and a Full suite: 13,807 passed / 7 failed / 38 skipped — the 7 failures are pre-existing and unrelated (confirmed identical on a clean |
…r_lifecycle Review item AMD-AGI#3: local_health.py still constructed suggestion="delegate(server_lifecycle)..." for local_server_unreachable (HIGH) and log_error_pattern (HIGH), even though action_ladder.py already routes local_server_unreachable to a real delegate(recover, force_gpu_cleanup=True). That stale string reaches the orchestration prompt via alert detail.suggestion, telling the model to reach for a non-dispatchable action. Point both suggestions at the real remedy. Review item AMD-AGI#4: action_ladder.py's local_server_unreachable idempotency key only carried the tick. _server_unreachable emits one symptom per unreachable probe target and marks all of them HIGH together, so two dead targets in one tick produced two delegates with the same idempotency_key — the first creates the recovery task, the second comes back as a duplicate-idempotency PolicyDenied that pollutes repeated_policy_denied tracking. Disambiguate the key with a short hash of the target URL. Also updates action_ladder.py's stale module docstring (review item AMD-AGI#9), which still listed delegate(recover) as gpu_memory_leaked-only. Adds direct test coverage for the local_server_unreachable -> recover mapping (review item AMD-AGI#11), none of which existed before.
… ones Review item AMD-AGI#7: machine.py's unrelated-transition cleanup dropped a pending escalate hint through consume_pending_escalate_hint(), which records the hint in last_consumed_escalate_hint -- an audit field documented as meaning "this hint drove a transition." A hint that was thrown away without acting on it is a different event; recording it as consumed told the breakdown the opposite of what happened. Add a sibling discard_pending_escalate_hint() that records into new last_discarded_escalate_hint(_ts) fields instead, wire machine.py's discard branch to it, and register the new fields in both LLM-write-blocked state allowlists (gate.py, robustness envelope.py) so an LLM-authored patch can't forge them. reset_per_cycle_plateau_state() also discarded a hint directly (bypassing both audit paths); route it through the new method too since that clear is semantically the same event. Also fixes review item AMD-AGI#9's remaining stale comments: pending_escalate_hint's field comment ("cleared once acted on"), consume_pending_escalate_hint()'s own docstring, and reset_per_cycle_plateau_state()'s docstring (it resets pending_escalate_hint too, which isn't "plateau and dispatch state"). Review item AMD-AGI#6: gate.py's sentinel pass-through returned silently on a match. Add a log line so a sentinel-driven accept is visible in logs instead of looking identical to a normal accept. Adds direct test coverage (review item AMD-AGI#11) distinguishing the discard and consume paths by their respective audit fields, and covering the sentinel log line, none of which existed before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lass vocabulary Review item AMD-AGI#10: getattr(denied, "rule", ...) at both call sites default a PolicyDenied.rule attribute that __init__ always assigns (gate.py:119) -- the getattr default can never fire. Simplify to direct attribute access; the "or 'denied'" still does real work for the actual None case. Review item AMD-AGI#8: "policy_{rule}" error_class values (e.g. policy_source_file_outside_trusted_scope) don't match any of the field's existing exact-match buckets (crash/oom/hang/detokenizer_stall), and nothing documents that this is an open, not closed, vocabulary. Checked both consumers: writeback._pitfall_severity_for correctly excludes policy_* from crash-severity (a policy denial isn't a runtime crash), and explore._extract_gaps_from_attempts groups it into its own gap key just from the raw string -- neither needs new handling. Document the field's known producers/consumers on SubAgentResult.error_class instead, so a future prefix family is discoverable from one place.
…ure exits Review item AMD-AGI#5: run_task has three return SubAgentResult(state="failed", result={}, ...) sites; only the PolicyDenied one set error_class. The no_executor and executor-exception exits still returned an empty result with no class, so they kept collapsing into unknown_error at the gap key (explore._extract_gaps_from_attempts) -- the same generic bucket this item was meant to get callers out of. no_executor now sets error_class="no_executor" (matches the transition evidence's own "reason" value at the same site). The executor-exception exit sets error_class to the raised exception's own class name, following the same convention already used elsewhere in this codebase (e.g. collective_driver_generator.py, forge_fusion.py) for exception-derived classes -- more specific than a flat string, and free since the exception object is already in hand. Extends SubAgentResult.error_class's docstring (added for item AMD-AGI#8 in the previous commit) with these two producers. Adds direct test coverage for both paths; the no_executor test previously asserted nothing about error_class.
Context
Found while stress-testing Hyperloom's orchestrator loop against
--framework atom, pushing past the vLLM/SGLang-centric path Hyperloom appears to have been primarily validated against. A long (9h+) unattended run produced zero validated gain despite 95.8% roofline headroom; the orchestrator's own self-diagnosis inreports/final.mdroot-caused all three independently of framework choice, so they should reproduce under vLLM/SGLang too.Fixes
PolicyGatefalse-rejects specialist delegates on a placeholder path string. A literal "Not found" in asource_filefield isn't empty, so it reached the trusted-scope check and was denied, identically, 15 times in one session, zeroing out the entire framework-agent lane. Now treats known absent-value sentinels as an omitted field.EXPLOREcan exit with zero variants tested. A stalepending_escalate_hintleft over from an unrelated phase or a prior macro-cycle was honored unconditionally, short-circuitingcompute_plateau_explore()before a single round ran (explore_search.tested=0). The direct cause ofcumulative_gain_validated=0.00%in that run. Now cleared on unrelated phase transitions and macro-cycle reload.local_server_unreachable's own suggestion text pointed atserver_lifecycle, which isn't a real dispatchable action. Wired to the existing recover action, mirroring thegpu_memory_leakedbranch.This PR also threads
PolicyGaterejection's specific rule throughSubAgentResultaserror_classinstead of the genericunknown_error, so future policy denials are recognizable in the gap ledger.Testing
test_policy_gate.py, test_machine_state.py, test_decision_action_ladder.py, test_delegate_denial_loop.py: 105 passed.
Also validated live: a same-day re-run against the same workload showed zero
PolicyDenied hits and EXPLORE completing 3 real variant attempts (previously 0).