fix(rollout): route verifier timeout cleanup by service - #1095
Open
zhiheng-yang wants to merge 1 commit into
Open
fix(rollout): route verifier timeout cleanup by service#1095zhiheng-yang wants to merge 1 commit into
zhiheng-yang wants to merge 1 commit into
Conversation
zhiheng-yang
marked this pull request as ready for review
September 4, 2026 13:21
Contributor
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| "suspected; retrying verifier once" | ||
| ) | ||
| await _kill_orphan_verifier(env) | ||
| await _kill_orphan_verifier(env, service=verifier_service) |
Contributor
There was a problem hiding this comment.
🟡 Timed-out verifier retries can overlap
When a legacy or custom verifier silently times out, _kill_orphan_verifier misses its process. The original can run beside the retry and corrupt its score.
Prompt for agents
The silent-timeout retry in src/benchflow/rollout/_setup.py calls _kill_orphan_verifier before starting the second attempt, but that helper only runs pkill against /verifier/test.sh. Verifier.verify supports legacy /tests/test.sh, verifier.md script strategies with arbitrary commands, and reward-kit runners, so those timed-out processes survive and can race the retry's output files. Make timeout cleanup target the actual command or process group started by Verifier, while preserving service routing and avoiding unrelated processes. Add coverage using a real legacy or custom verifier command rather than only a mocked verify coroutine.
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
[verifier].servicecontrols which Compose service runstest.sh.The main verifier path respects that setting, but the timeout recovery path did not.
For a verifier configured on
scorer, the flow was effectively:test.shonscorer;test-stdout.txton the default service;pkillon the default service.Those services do not share the same processes or necessarily the same files, so the recovery code could inspect the wrong output and leave the timed-out verifier running.
I reproduced the timeout path with
verifier.service = "scorer".The two recovery calls were routed as follows:
main:[None, None]['scorer', 'scorer']Change
Pass the configured verifier service to both the silent-timeout probe and the orphan cleanup helper.
The timeout and retry policy are unchanged.
src/benchflow/rollout/_setup.pykeeps recovery on the verifier service.tests/test_verifier_wedge_retry.pycovers bothmainandscorer, plus the timeout-with-output path.Tests
uv run pytest -q tests/test_verifier_wedge_retry.pyResult:
4 passed.