Retry transient benchmark infrastructure failures

[rgarcia]

summary

• retry complete isolated trials for transient API timeout, connection, and exec-stream failures
• run comparisons through the trusted benchmark harness while retaining candidate/base build artifacts
• suppress deltas and fail the workflow when any intended trial is infrastructure-failed or ungraded
• expose retry counts in reports

why

A brief control-plane restart can otherwise turn the remaining task queue into infrastructure failures. Per-request retries stay disabled because create/build requests may have succeeded before their responses were lost; the whole trial is the safe retry boundary.

testing

• bun test (277 passed)
• go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 .github/workflows/benchmark-clawbench.yml
• bash -n benchmarks/harbor/clawbench/run.sh
• bunx prettier --check ...
• git diff --check

bunx tsc --noEmit remains blocked by the existing reason field error in src/lib/mcp/tools/auth-connections.test-fixtures.ts on main.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
mcp	Ready	Preview	Sep 1, 2026 8:22pm UTC

[bmsaadat]

Review

Verdict: Request changes (two fixes, one question; direction is right)

The design is sound: trial-level retry is the correct boundary (harbor-hypeman constructs the SDK client with max_retries=0, so the PR body's claim checks out), the strict gate closes a real hole (harbor exits 0 even with infra-errored trials, and the old .scored > 0 floor passed with a single graded trial), and running both arms through one runner removes a real tooling confound. I verified the retry mechanics against the pinned harbor 0.21.0: failed attempts' trial dirs are removed before retry and stats.n_retries matches the ingestion, so the gate and Retries column are wired correctly.

Requested changes

1. The retry list misses the common transient REST classes. The hypeman SDK maps 429 to RateLimitError and any 5xx to InternalServerError (_make_status_error), and neither is in the include list, so a transient 502 on instances.create/wait is still an unretried infra error, which the new strict gate turns into a red multi-hour run. Suggest adding both; harbor's default exclude list still protects grading-semantics errors since exclude takes precedence. (The exec path looks well covered as-is: ConnectionRefusedError matches refused websocket connects, and hypeman/lib/exec.py wraps recv-side stream drops into ExecProtocolError.)

2. The report and the gate now disagree about "complete". The jq gate requires scored == trials, but renderMarkdown's incomplete check only looks at statuses/infra/ungraded, and publish still gates on scored === 0. Two consequences: an arm with a trial dir missing result.json renders unsuppressed deltas with no warning while the workflow fails on the gate, and scored == trials is vacuously true at zero trials where the old .scored > 0 floor failed. Suggest computing one per-arm complete boolean (plus reasons) in report.ts, rendering and gating from that single field, and keeping a nonzero-trials floor.

Question

Instance names are stable across retry attempts (trial_name is minted once and reused; the name is harbor-{session}-{envhash[:8]}), and a create whose response is lost leaves _instance_id unset so cleanup can't delete it. Does Hypeman enforce per-org name uniqueness? If yes, retries after a lost create response hit 409 ConflictError (not in the include list, and not transient) in exactly the restart scenario this PR targets; if no, orphaned instances can accumulate per flaky trial. Either way the README's "the whole trial is the safe retry boundary" line may be worth softening.

Non-blocking notes

• Backoff is effectively a constant 1s (RetryConfig min_wait 1.0, multiplier 1.0), so fast-failing ConnectionRefusedError burns all five retries within seconds of an outage. harbor run --config can set retry.min_wait_sec/wait_multiplier if we want spacing; timeouts (40m single-task, 4h CI) also now bound up to 6 attempts.
• Since the harness runs from main, this PR's own benchmark won't exercise these flags (its run would use main's runner: candidate arm with retries, baseline without), so first real validation is post-merge. Worth noting in the PR body, and worth validating the two .image.env keys right after sourcing; extracting just those keys instead of source-ing arm-generated content would also keep a future extra key from overriding harness pins.
• Retried attempts' cost/tokens are deleted with their attempt dirs, so the Cost column under-reports on flaky runs, and candidate-caused flakiness (e.g. exec wedging) is absorbed silently except for the Retries column. A mention of nonzero retries in the warning area would keep that visible.

[rgarcia]

updated in c413ad2: added RateLimitError and InternalServerError retries, unified report/publisher/workflow validity through per-arm complete reasons with a nonzero trial floor, switched to harbor-hypeman==0.1.2, and made the sole HypemanEnvironment run commands independently from control-plane exec sessions. all 278 Bun tests pass; Actionlint and the focused formatting checks pass. tsc --noEmit remains blocked by the pre-existing reason fixture error on main.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit c413ad2. Configure here.}

Both requested changes are addressed in c413ad2: REST transient classes are included, and report/publish/workflow validity now share a nonzero per-arm complete field with reasons. The 0.1.2 CI pin fix is in a56ca40; CI, security scans, and BugBot are green.

[bmsaadat]

Verdict: Approve. Both requested changes landed cleanly: the retry list now covers the REST 5xx/429 classes (verified the class names against the hypeman version the constraint actually resolves), and the per-arm complete/incompleteReasons unification with the nonzero-trial floor is exactly the single-source-of-truth shape we hoped for, with good edge-case tests.

The harbor-hypeman 0.1.2 detached-exec design deserves a callout: decoupling the running command from the control-plane session, with the 60s reconnect window, solves the restart scenario more fundamentally than retry classes could. Nice.

One open question from last round, non-blocking since the failure mode is a loud red run: after a lost instances.create response, the trial retry reuses the same instance name (trial_name is stable across attempts). If Hypeman enforces name uniqueness, those retries would die on ConflictError, which isn't in the include list. Is that a real path, and if so is ConflictError worth including (or the name worth salting per attempt)?

Two small observations, fine as-is: the 0.1.2 poll raises bare RuntimeError when a job vanishes (e.g. VM death), which the trial retry won't catch; and the unified publish gate now skips Braintrust publication for incomplete runs, so the markdown report is the only artifact for exactly the runs we'd debug. If both are intentional, no changes needed.