Skip to content

fix(sandbox): use the verifier-setup budget for hardening execs - #1062

Merged
bingran-you merged 4 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-verifier-setup-timeouts
Sep 2, 2026
Merged

fix(sandbox): use the verifier-setup budget for hardening execs#1062
bingran-you merged 4 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-verifier-setup-timeouts

Conversation

@Benjamin-eecs

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

Copy link
Copy Markdown
Contributor

Description

Apply the canonical VERIFIER_SETUP_TIMEOUT_SEC budget to every normal verifier hardening/setup probe: trusted PATH and PYTHONPATH, pytest-plugin discovery, distro detection, both sandbox-user process-kill passes, and the existing clear/prepare/freeze/cleanup commands.

Motivation and Context

These helpers run once per rollout on the scoring path. Daytona dispatch latency can exceed the previous 5/10/15-second literals even for trivial commands; some helpers then silently fell back to an incomplete verifier environment, while another process-kill pass could wait for the transport hard cap. One owned 180-second ceiling avoids both failure modes without adding retries or changing normal-path latency.

Closes #1058.

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation

Implemented Tasks

  • Use the shared budget for trusted environment discovery
  • Use it for plugin and distro probes that previously failed soft after 15s/5s
  • Bound both sandbox-user process-kill passes
  • Add a PR-named regression test that audits the complete default hardening sequence

Validation

  • tests/test_sandbox_hardening.py: 100 passed
  • tests/: 5933 passed, 48 skipped, 7 deselected
  • Ruff format/lint and ty check src/
  • Real gemini-3.1-pro-preview Docker and Daytona E2E: reward 1.0 on both backends
  • Artifact validator: Docker 1/1 healthy and Daytona 1/1 healthy, with complete ACP + provider trajectories and training-ready rows

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: 1 flag

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

Devin Review

@JeremyJC67

Copy link
Copy Markdown

This fixes the right thing in the right place: after this change no 10-second literal remains in lockdown.py (main had exactly five, all on the harden-before-verify path, all converted here), and reusing VERIFIER_SETUP_TIMEOUT_SEC keeps one owned budget instead of adding a knob. tests/test_sandbox_hardening.py passes for me too (99 passed), and ruff is clean.

  1. The same Daytona latency now fails soft — and silently — two calls later. _build_verifier_env runs on the identical scoring path and still contains _discover_pytest_plugin_flags at timeout_sec=15 (lockdown.py:607-612) and _distro_pip_env at timeout_sec=5 (lockdown.py:685-688). Both swallow the timeout and fall back: plugin discovery degrades to task-declared plugins while PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 stays pinned, and distro detection returns {}, dropping PIP_PREFIX on Fedora-likes. Under the >10s dispatch latency you reported in [BUG] Verifier hardening execs keep a 10s timeout on the scoring path and record rollouts as verifier crashed #1058 these don't crash the rollout — they quietly change the verifier env and can turn a genuine pass into a recorded failure, which is arguably worse than rewards: null because it lands as real signal. Suggest raising both to VERIFIER_SETUP_TIMEOUT_SEC in this PR; the fail-soft handling can stay for genuine command failures.

  2. The second pass of _kill_sandbox_user_procs has no budget at all (lockdown.py:1061-1066). On Daytona that means the _DAYTONA_EXEC_HARD_CAP_SEC = 3600 safety net — a wedged pgrep/pkill burns up to an hour on the path you just budgeted at 180s one statement above. timeout_sec=VERIFIER_SETUP_TIMEOUT_SEC there makes the helper internally consistent; fine as a follow-up if you'd rather keep this PR literal-for-constant only.

  3. Nothing pins the new budgets. test_pr_942_tree_hardening_uses_shared_setup_budget (tests/test_sandbox_hardening.py:237-269) already asserts per-command timeout_sec == VERIFIER_SETUP_TIMEOUT_SEC, but runs with sandbox_user=None and doesn't cover printenv PATH / printenv PYTHONPATH / the extras commands / the pkill first pass. Extending expected_commands (plus one variant with a sandbox_user set) is ~10 lines and gives this PR a test that would fail red on main — right now it has none.

  4. Wall-clock, since "budget + retry" came up: the budget is a ceiling, not a wait — the pkill sequence still completes in ~1s — so the normal path is unchanged. Worst case, the five converted execs add ~14 min to a harden phase that already carried a ~18 min ceiling across its other 180s execs, and which runs outside the verifier's own asyncio.wait_for (rollout/_setup.py:472-480). I think budget-only is the correct minimal fix: all five commands are idempotent, so if 180s ever proves insufficient in the field a retry wrapper is safe to add then — today it would be speculative.

No interaction with my #1046 (feat/ablate-cli): I don't touch lockdown.py and a trial merge in both directions is clean, so landing order between these two is free — happy to rebase whichever lands second.

@Benjamin-eecs

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough verification.

@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 63a2ea9. All verifier-environment and hardening probes now use the owned setup budget, including plugin discovery, distro detection, and both process-kill passes; the 30-second best-effort cache reclaim remains intentionally separate. Exact-head focused tests are 100/100 and every required GitHub check is green. Full local suite, Ruff, ty, plus real-key gemini-3.1-pro-preview Docker and Daytona runs passed at reward 1.0; both trajectory artifacts validated healthy and training-ready. Ready to squash merge.

@bingran-you
bingran-you merged commit 86f8c6a 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