[Klaud] [AMD][Power] fix: wait for AMD telemetry to cover the benchmark window end before stopping the monitor / 修复 AMD 遥测在基准窗口结束前被截断导致功耗校验失败的问题

[edwingao28]

Closes gap G10 (PLAN-05). Fixes the benchmark_window_not_bracketed power-validation failures on AMD single-node AgentX runs (e.g. run 32433563482, kimik3-fp4-mi355x-atom-agentic-mtp, 3 of 4 points failed).

Problem

On run 32433563482 conc1 the amd-smi stream's last CSV tick is 1787277605 (integer epoch second) while the aiperf window end is 1787277609.157 — telemetry stops ~4 s before the formal window end. Three interacting mechanisms, none bounded by the old fixed sleep interval+2 before killing the pipeline:

1. Consumer-side data loss at kill — GPU_MONITOR_PID is the awk consumer, not amd-smi; rows sitting in the OS pipe (awk's per-row fflush() can block on NFS-backed $result_dir) are discarded at kill.
2. Producer staleness — one amd-smi metric -p -c -t -u iteration over 8 GPUs takes non-trivial time, so the newest emitted tick can trail wall clock by seconds.
3. Degenerate teardown rows — SMI can emit N/A or 0 W power cells near termination; an N/A row in the ±3 s ingest band flipped validity via invalid_power_sample, and a 0 W row past the window end could silently fake end bracketing while corrupting the boundary interpolation.

Solution

benchmarks/benchmark_lib.sh — deterministic poll-until-covered stop.
stop_gpu_monitor's AMD branch now polls $GPU_METRICS_CSV until every observed GPU has a usable tick (numeric epoch timestamp, numeric power > 0) stamped ≥ floor(stop-entry epoch)+1, then kills. The window end is always ≤ the stop-entry wall clock and amd-smi stamps integer seconds, so that tick strictly covers any fractional window end — no aiperf knowledge needed in the shell. Details:

• Bounded by AMD_MONITOR_STOP_TIMEOUT_S (default 30 s, env-overridable, 0 skips the wait; a non-integer value warns and falls back to 30 instead of aborting the stop). On timeout or early monitor death: warn and proceed — aggregation then flags the point exactly as today (fail-safe, never fail-silent).
• Non-epoch timestamps (older amd-smi emitting ISO strings) fall back to the legacy interval+2 fixed tail once; millisecond epochs (>1e12) are normalized to seconds, mirroring _parse_timestamp in utils/aggregate_power.py.
• The awk coverage helper is POSIX-only (no interval regex, no gawk extensions; works under mawk/busybox awk) and neutralizes amd-smi's quoted list cells (embedded commas), CR line endings, and blank lines between tick groups — all verified against the real run-32433563482 CSV.
• AgentX INT/TERM/EXIT traps stop in abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0): a cancelled run's power validity is moot and teardown stays fast. The explicit post-replay stop performs the full wait. Idempotency via agentx_monitor_stopped preserved.
• NVIDIA branch is behaviorally unchanged. This also fixes every AMD single-node consumer of stop_gpu_monitor (all benchmarks/single_node/fixed_seq_len/* recipes), not just AgentX.

utils/aggregate_power.py — explicit boundary-degenerate handling.
integrate_power now skips-and-counts rows inside the ±max_sample_gap_s ingest band but outside the formal window whose power is unparseable, non-finite, or ≤ 0, into a new per-GPU sidecar field boundary_degenerate_rows ("unknown" buckets rows without a GPU identity). They are never used for bracketing and never poison validity. In-window semantics are byte-identical: in-window N/A still ⇒ invalid_power_sample; in-window 0 W still integrates.

Contract compatibility (PLAN-06 / PLAN-07)

• No new top-level reason codes; benchmark_window_not_bracketed keeps meaning "a real usable sample does not bracket the boundary".
• Aggregate-row schema untouched (power_valid still numeric; no power_invalid_reasons on aggregates).
• Sidecar schema_version stays 1; boundary_degenerate_rows is additive and readers tolerate its absence.

Validity flips on re-aggregated legacy artifacts (intentional)

Only when explicitly re-running aggregation on old raw artifacts (e.g. the recover-failed-ingest path):

• streams previously failed only by an outside-window N/A row now validate (strict improvement);
• streams whose only end bracket was a degenerate 0 W row now fail benchmark_window_not_bracketed (they were silently corrupt before).
  Already-ingested aggregate rows are untouched.

Cost

Normal case ~1–2 s at stop (first whole second after stop entry). Pathological hang: up to 30 s (was 3 s), bounded, logged, env-tunable.

Tests

• bash -n benchmarks/benchmark_lib.sh clean.
• utils/test_aggregate_power.py + utils/agentic/aggregation/test_power_lifecycle.py: 74 passed (57 + 17).
  • 5 new aggregation tests — N/A-outside-window skipped-and-counted; 0 W tail no longer fakes bracketing (flips a silently-corrupt legacy case to explicit-invalid); in-window 0 W semantics frozen; sidecar key present-and-empty on clean streams; regression fixture encoding the run-32433563482 conc1 shape (real 21-column MI355X watch header, integer ticks ending 4 s before fractional end ...609.157497, real power values, quoted list cells, CR endings, blank inter-tick lines, plus the documented synthetic N/A / 0 W teardown shapes) asserting benchmark_window_not_bracketed attribution.
  • 6 new shell-contract tests running the real stop_gpu_monitor against a scripted producer (coverage wait for all GPUs; degenerate rows never satisfy coverage → timeout warning; per-GPU min semantics; ISO-timestamp legacy fallback; non-integer timeout survival; millisecond-epoch normalization), plus the signal test now pins abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0 at trap-driven stop) and the new trap strings.
• utils/test_process_result.py: the AMD stop test now pins the new contract (covered tick ⇒ no legacy sleep, energy snapshot still written); truncated-row repair test runs with the wait skipped.
• Broader power suites (test_aggregate_power*, test_process_result, utils/agentic/aggregation/): 224 passed.
• Full utils/ suite: 804 passed; the only 2 failures (utils/evals/test_run_eval_dispatch.py) reproduce identically on clean origin/main in this local environment (macOS temp-path issue), unrelated to power.
• Shell-contract tests re-run 3× locally, no flakes (0.2 s producer cadence vs ≥1 s coverage threshold ⇒ ≥5× margin).

Review

Two independent review passes; every finding verified and resolved:

1. CONFIRMED (minor): a non-integer AMD_MONITOR_STOP_TIMEOUT_S (e.g. "30s") aborted the whole stop_gpu_monitor call via a bash arithmetic error under set -e — leaking the monitor process and skipping tail repair and the energy sidecar. Fixed: the timeout is sanitized once before any arithmetic (warn + fall back to 30); the timeout warning now always prints a real number. Pinned by test_amd_stop_survives_non_integer_timeout.
2. PLAUSIBLE (nit, raised by both reviewers): a hypothetical amd-smi build stamping millisecond epochs would trivially satisfy coverage (~1.8e12 ≥ any second-scale target) and skip the tail wait entirely. No such build is known, and the failure mode was fail-safe, but the hardening is cheap. Fixed: the awk helper normalizes >1e12 timestamps to seconds, mirroring _parse_timestamp. Pinned by test_amd_stop_normalizes_millisecond_epoch_timestamps.
3. CONFIRMED (minor): the PR-opening step was deferred out of the implement phase per workflow rules. Resolved: this PR.

Evidence note

The retrieved run-32433563482 conc1 artifact shows the trailing rows all carry valid socket_power (254–264 W) with N/A activity/voltage cells — the observed failure is purely mechanisms (1)+(2) (the stream stops 4 s early). The degenerate-row handling hardens against mechanism (3) using the plan's documented synthetic shapes.

Hardware smoke (for PLAN-14; not a merge gate for this PR, but the gap-closure gate)

Re-sweep kimik3-fp4-mi355x-atom-agentic-mtp (configs/amd-master.yaml:656) from this PR branch on the MI355X ATOM cluster. For each point (conc 1, 4, 8, 10) in the raw-results artifact:

• agg_*.json has power_valid == 1 and finite avg_power_w / total_gpu_energy_j;
• results/**/power_validation.json has reasons == [], and for every GPU the newest usable gpu_metrics.csv tick ≥ benchmark_window.end_time_unix;
• boundary_degenerate_rows (if non-empty) shows teardown rows were counted, not integrated;
• accumulator_check.within_tolerance == true where the sidecar snapshots exist;
• the job log shows no never covered the stop request warning, and per-point wall time increased by ≲ a few seconds versus the failing run.

If any point still fails with benchmark_window_not_bracketed, attach its CSV tail + power_validation.json to this PR before iterating (per-GPU sampling_gap_exceeded would instead indicate the separate cadence issue listed under PLAN-05 follow-ups).

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes benchmark teardown timing and power-validation semantics for AMD telemetry and boundary rows; affects AgentX power metrics but is bounded, logged, and covered by contract tests.

Overview
Fixes AMD AgentX benchmark_window_not_bracketed failures when amd-smi integer-second ticks trail the fractional benchmark window end or pipe-buffered rows are lost at kill.

stop_gpu_monitor (AMD) no longer uses a fixed sleep interval+2 before killing the watch pipeline. It polls gpu_metrics.csv until every GPU has a usable sample (numeric epoch timestamp, power > 0) at or after the first whole second past stop entry, bounded by AMD_MONITOR_STOP_TIMEOUT_S (default 30s; 0 skips the wait). ISO timestamps fall back to the legacy tail sleep; ms epochs are normalized like aggregate_power. AgentX INT/TERM/EXIT traps call stop in abort mode (AMD_MONITOR_STOP_TIMEOUT_S=0); the normal post-replay stop keeps the full wait.

integrate_power skips N/A, non-finite, or ≤0 W rows that sit in the ingest band but outside the formal window, counting them in sidecar boundary_degenerate_rows instead of invalidating power or faking end bracketing (0 W tails now fail bracketing explicitly). Tests cover the shell poll, signal abort path, and aggregation regressions including the MI355X run shape.

^{Reviewed by Cursor Bugbot for commit 3c08814. Bugbot is set up for automated code reviews on this repo. Configure here.}

[github-actions]

Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs

---

感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have。

如需进行 PR 验证，请为此 PR 添加 full-sweep-fail-fast 标签（强烈推荐）— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled。

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

[claude]

🟡 _wait_for_amd_stop_coverage's timeout sanitization regex ^-?[0-9]+$ (line 299) accepts leading-zero digit strings like "08"/"09", but bash arithmetic treats a leading-0 numeral as octal; digits 8/9 make it an invalid octal literal. The very next arithmetic uses of $timeout_s -- [[ "$timeout_s" -le 0 ]] (line 303) and deadline=$(( target + timeout_s )) (line 307) -- both perform bash arithmetic evaluation and hit "value too great for base", which is a fatal bash expansion error that aborts the shell outright (not just a non-zero return), independent of set -e.

Extended reasoning...

Set AMD_MONITOR_STOP_TIMEOUT_S=08 (or any leading-zero value containing an 8/9 digit, e.g. "09", "018"). The regex guard added by this PR to catch non-integer timeouts (and specifically tested by test_amd_stop_survives_non_integer_timeout for "30s") lets "08" through as "valid", so no fallback-to-30 warning fires. The subsequent -le 0 test / deadline=$((...)) arithmetic then throws bash's octal "value too great for base" error, which is a fatal, non-interactive-shell-terminating error class -- reproducing (and likely worsening, since it can kill the whole caller script rather than just stop_gpu_monitor) the exact leaked-monitor/skipped-tail-repair/skipped-energy-sidecar failure this PR's sanitization was specifically added to prevent. Fix: strip/reject leading zeros (or force base-10 with 10#$timeout_s) before using $timeout_s in any arithmetic context.

Verification: Severity: nit. The regex at benchmark_lib.sh:299 (^-?[0-9]+$) accepts leading-zero strings like "08"/"09"/"018", so the non-integer fallback (line 300-301) does not fire. Bash arithmetic then treats a leading-0 numeral as octal, and digits 8/9 make it an invalid octal literal. Line 303 [[ "$timeout_s" -le 0 ]] is an if condition (set -e exempt: it prints the error but continues),…

[claude]

🟣 The explicit post-replay stop (_stop_agentx_power_monitor at line 2385) runs stop_gpu_monitor's now-up-to-30s AMD coverage wait (_wait_for_amd_stop_coverage, AMD_MONITOR_STOP_TIMEOUT_S default 30) while the INT/TERM traps installed at lines 2370-2372 are still active (they're only removed at line 2386, after the call returns). If SIGINT/SIGTERM arrives during that wait, the trap fires _stop_agentx_power_monitor abort; exit N; since agentx_monitor_stopped was already set to 1 at function entry (line 2317, before calling stop_gpu_monitor), the trap's guarded body is skipped but the trailing exit 130/exit 143 still runs unconditionally, killing the subshell immediately.

Extended reasoning...

A user Ctrl-C's (or a scheduler SIGTERMs) the benchmark while the explicit post-replay stop is mid-poll waiting for AMD telemetry coverage. The process exits via the trap's exit 130/143 before stop_gpu_monitor reaches kill/wait on GPU_MONITOR_PID, tail repair, or the end-of-run amd-smi energy sidecar — leaking the orphaned awk/amd-smi consumer process and dropping the energy_end.csv artifact. This same trap-during-explicit-stop race existed pre-diff too, but was bounded to the old fixed ~3s sleep window; this diff widens the exposed window roughly 10x (up to 30s by default) by design, making the previously-negligible race a realistically triggerable teardown gap. A correct fix must make the explicit stop itself not re-enter abort-truncation (e.g. temporarily disable/replace the INT/TERM traps for the duration of the explicit stop, or have stop_gpu_monitor ignore/defer signals during its wait) rather than leaving the pre-existing unconditional exit reachable mid-wait.

Verification: pre-existing. The race is real and reachable: line 2385 calls _stop_agentx_power_monitor (full-wait mode) while the INT/TERM traps at lines 2371-2372 are still armed (removed only at 2386). _stop_agentx_power_monitor sets agentx_monitor_stopped=1 before calling stop_gpu_monitor, which on the AMD path enters _wait_for_amd_stop_coverage (a while :; ... sleep 1 loop bounded by…

[edwingao28]

Superseded by #2767 (branch renamed to fix/amd-agentx-window-bracketing); identical diff.