Skip to content

fix(eval): re-run infra-retryable verifier-errored tasks on resume - #1063

Merged
bingran-you merged 5 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-resume-rerun-infra-verifier-errors
Sep 2, 2026
Merged

fix(eval): re-run infra-retryable verifier-errored tasks on resume#1063
bingran-you merged 5 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-resume-rerun-infra-verifier-errors

Conversation

@Benjamin-eecs

@Benjamin-eecs Benjamin-eecs commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Description

On resume, re-run scoreless verifier failures that the existing retry taxonomy classifies as infrastructure, while preserving valid scored artifacts and keeping sequential-shared learning jobs on their original ordering contract.

Motivation and Context

A finished agent rollout can lose its score when Daytona verifier setup/transport fails. Reusing that scoreless artifact forever defeats resume. The resume path now shares the same canonical verifier-error taxonomy as within-run retries. Historical Failed to execute session command and bounded Command timed out after ... artifacts classify as verifier infra, while contract failures such as a missing reward remain terminal.

When multiple retry artifacts exist, any scored artifact outranks a newer scoreless verifier error; within the same tier, recency and path provide deterministic selection.

Closes #1058.

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation

Implemented Tasks

  • Re-run scoreless infra-retryable verifier errors on independent-job resume
  • Preserve sequential-shared resume behavior
  • Classify the exact historical Daytona exec errors as verifier infra
  • Prefer durable scored artifacts over later scoreless retry artifacts
  • Add PR-named regression coverage

Validation

  • Focused retry/resume/usage suite: 127 passed
  • tests/: 5936 passed, 48 skipped, 7 deselected
  • Ruff format/lint and ty check src/
  • Real gemini-3.1-pro-preview Docker resume E2E: seeded historical scoreless infra result was detected, a second real rollout ran, and reward 1.0 was restored
  • Replacement rollout artifact validator: 1/1 healthy with complete ACP + provider trajectory and a training-ready result row

Copilot AI lite review requested due to automatic review settings August 29, 2026 07:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment thread src/benchflow/evaluation.py Outdated
Comment on lines +1119 to +1126
if r.get("rewards") is None and (
self._config.retry.should_retry_verifier_error(r["verifier_error"])
):
logger.info(
f"Re-running verifier-errored task on resume: {task} "
f"({truncate_end(r['verifier_error'], 80)})"
)
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Resumed learning tasks run out of order

In sequential-shared resumes, _get_completed_tasks reruns an earlier errored task after later tasks advanced the persisted learner state. The reordered task consumes future skills and corrupts the learning curve.

Prompt for agents
The new resume filtering in src/benchflow/evaluation.py::_get_completed_tasks is safe for parallel-independent jobs but breaks sequential-shared ordering. A sequential run can continue after task B has a retryable verifier error, then complete task C and persist C's learner generation. On resume, B is the only remaining task and runs against C's later learner state. Preserve sequence semantics by either rewinding the learner store and invalidating/rerunning the errored task plus every later task, or by keeping retryable verifier failures terminal for sequential-shared mode. Add a regression test with completed A, retryable-error B, completed C, and a persisted learner snapshot.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed: re-run is gated to parallel-independent; sequential-shared keeps the reuse semantics. Regression test added.

@JeremyJC67

Copy link
Copy Markdown

The boundary is drawn where it should be, and the tests prove it: I verified test_infra_verifier_errored_reruns fails red against main's evaluation.py and passes with the change, verifier crashed: No reward file found still classifies verifier_failure and is reused, and routing the decision through should_retry_verifier_error means resume and within-run retry share one taxonomy — including the retry_on_verifier_infra=false opt-out. tests/test_verify.py (58), tests/test_job.py (53), and tests/test_usage_tracking.py (13) all pass here, ruff clean.

  1. No cap across resumes. The within-run loop already spent max_retries + 1 attempts before that scoreless infra result was written (evaluation.py:1346-1391); every resume now grants a fresh set. A deterministically sick task — say a genuinely slow verifier that hits verifier timed out every run, which the string taxonomy can't distinguish from the exec-layer wedge the way rollout/_setup.py's timeout-with-output check can — re-runs on every resume, forever. That also silently changes resume for existing users: it flips from "idempotent, regenerates the summary for free" to "re-spends agent rollouts". The bound is already in hand: the rglob loop enumerates every scoreless-infra result.json for the task, so counting them and reusing after N attempts (with a "giving up after N" log line) caps the spend. retry_on_verifier_infra=false works as an escape hatch but also kills the within-run retry, so a count (or separate knob) is cleaner.

  2. The motivating Daytona failure doesn't classify as infra. The [BUG] Verifier hardening execs keep a 10s timeout on the scoring path and record rollouts as verifier crashed #1058 failure mode records verifier crashed: Command timed out after 10 seconds (Daytona's bounded-exec message, wrapped at rollout/_setup.py:519), and classify_verifier_error returns verifier_failure for it — I probed the realistic strings and only transport/setup/download markers and verifier timed out make the retryable set. So resume still pins exactly the artifact class that motivated your pair of PRs, including any post-fix(sandbox): use the verifier-setup budget for hardening execs #1062 hang that exceeds 180s. Consider adding a "command timed out after" marker to _looks_like_verifier_infra_error (scoring.py:231-241): a slow verifier surfaces as verifier timed out, never this exec-layer phrasing, so the false-positive risk looks low — and it would let resume repair result.json files damaged before fix(sandbox): use the verifier-setup budget for hardening execs #1062 lands.

  3. An older scored artifact loses to a newer scoreless-infra artifact. Reproduced locally: rollout-1/result.json with reward: 1.0 plus a newer retry artifact with a scoreless verifier timed out_get_completed_tasks returns nothing and the whole task re-runs, despite the description's "keep reusing … any result that carries rewards". Newest-wins is pre-existing and the normal flow protects you (the retry loop stops on a score), but mtime is fragile across job-dir copies (rsync/scp without -a). Preferring any rewards-bearing artifact during collection would make the description literally true; a comment documenting the mtime assumption is the cheaper alternative.

One layout check for the record: matrix trials each get their own job dir (root/alias/trial-NN), so the per-task_name collapse only merges retry artifacts within one job — the filter's granularity is right for the matrix path.

On ordering vs my #1046 (feat/ablate-cli): a trial merge in both directions is clean — my evaluation.py changes (the task_rollout_config extraction around L1203-1295 plus config fields) don't touch _get_completed_tasks, and your filter handles my branch's artifacts correctly: unscored branch-children that raise instead of reporting 0 write neither rewards nor verifier_error, so they simply re-run, same as agent-errored tasks today. Landing order is free — happy to rebase whichever lands second.

@Benjamin-eecs

Copy link
Copy Markdown
Contributor Author

Thanks for verifying. Also addressed the sequential-shared concern from the automated review: the re-run is now gated to parallel-independent mode, with a regression test.

@bingran-you bingran-you left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated at exact head d4f72b5. Resume now shares the canonical retry taxonomy, covers the historical Daytona session-exec and bounded-command timeout strings, preserves sequential-shared ordering, and ranks durable scored artifacts above later scoreless retries. Exact-head focused tests are 127/127 and every GitHub check is green. A real-key gemini-3.1-pro-preview Docker job was converted to the historical scoreless infra artifact; the public eval run resumed it, performed a new rollout, restored reward 1.0, and the replacement trajectory validated healthy/training-ready. Ready to squash merge.

@bingran-you
bingran-you merged commit 268964b into benchflow-ai:main Sep 2, 2026
11 checks passed
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.

[BUG] Verifier hardening execs keep a 10s timeout on the scoring path and record rollouts as verifier crashed

4 participants