Let a parallel child hand back a verdict it reached on time - #75
Let a parallel child hand back a verdict it reached on time#75jessie1111101 wants to merge 2 commits into
Conversation
A converging objective whose children poll for something that never appears was scored "error", not "fail". The children are handed the same deadline the parent's futures_wait uses, and a child that polls to the very end overshoots it slightly: poll_until clamps its final sleep but then re-checks the predicate once more before giving up. The parent stopped waiting at exactly the deadline, so the two raced, and a child that lost was recorded as never observed even though it had reached a verdict. That inflates the score. rollup drops an errored entry from both sides of the correctness fraction, so an objective the agent genuinely failed is deleted rather than counted. Seen on deploy-hello-app's disruption-and-scaling: the agent created neither a PodDisruptionBudget nor a HorizontalPodAutoscaler, confirmed against the live cluster, and both children errored on the deadline. Scored as an error the run was 0.9170; counting the observation it is 0.8780. The wait now allows a one second handoff window past the deadline. That is wide enough to collect a result that landed on time and far too narrow to rescue a genuinely hung child, which must keep erroring so a stuck kubectl under assert mode never reads as an observed safeguard violation. Signed-off-by: Jessie Liu <jssl@google.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jessie1111101 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @jessie1111101. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/M
📝 WalkthroughWalkthroughParallel convergence waits now allow a one-second child handoff period after the shared deadline. Single-shot waits keep their existing limit. A regression test verifies that child failure verdicts and reasons remain preserved. ChangesParallel deadline handoff
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devops_bench/verification/runner.py`:
- Line 88: Update the module-level constant _CHILD_HANDOFF_GRACE_SEC with an
explicit float type annotation while preserving its current value and behavior.
- Line 546: Update the wait_timeout calculation in _run_parallel() to cap the
handoff at the absolute deadline plus _CHILD_HANDOFF_GRACE_SEC, rather than
adding the grace period after clamping negative remaining time. Ensure late
starts receive only the remaining time until that absolute handoff deadline,
while preserving the one-second grace window.
- Around line 546-547: Update the parallel-child result handling around
futures_wait and _PARALLEL_INCOMPLETE_REASON so children not actually evaluated
by the deadline retain status="error" instead of being overwritten by the
default deadline-exhausted fail result. Preserve fail for checks that genuinely
completed evaluation, and add coverage for workloads exceeding
_MAX_PARALLEL_WORKERS to verify the conjunction remains unevaluated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ba26655-1249-4081-be94-425409cb2bd0
📒 Files selected for processing (2)
devops_bench/verification/runner.pytests/unit/verification/test_runner.py
janetkuo
added
ok-to-test
Per review. Adding the grace to the time remaining meant a group entered after its deadline waited a full grace period from that late start, so the handoff could finish well past deadline plus grace and outlive the parent wait. A nested parallel node reached late is exactly that case. Measuring back from the absolute handoff deadline caps it: a group starting on time still gets its full window, and one entered late gets whatever is left of it, down to nothing. Also annotates the constant as float per the repository typing rule. Signed-off-by: Jessie Liu <jssl@google.com>
2 participants
A converging objective whose children poll for something that never appears is scored
error, notfail.Root cause
_run_parallelhands its children the samedeadlineits ownfutures_waituses. A child that polls to the very end overshoots it slightly —poll_untilclamps its final sleep but then re-checks the predicate once more before giving up. The parent stopped waiting at exactly the deadline, so the two raced, and a child that lost kept the pre-seeded placeholder:Both children had in fact reached a verdict. They just could not hand it back.
Why it matters
rollup.pydrops an errored entry from both the numerator and the denominator, so an objective the agent genuinely failed is deleted rather than counted — and the score goes up.Seen live on
deploy-hello-app. The agent created neither a PodDisruptionBudget nor a HorizontalPodAutoscaler; I confirmed that directly against the cluster mid-run (0 pdb, 0 hpa). Both children errored on the deadline:Same failure shape as #74: an observation the harness did make, discarded as if it had not been.
The change
The converge wait now allows a one second handoff window past the deadline.
The asymmetry is deliberate and preserved. A genuinely hung child must keep erroring, because a stuck
kubectlunder assert mode must never read as an observed safeguard violation — that could zero a run through the catastrophic gate. One second is wide enough to collect a result that landed on time and far too narrow to rescue a hung child; the two existing hung-child tests (a 3.0s sleep against a 1.2s deadline, and the single-shot ceiling path) pass unchanged, which is what pins that boundary.single_shotis untouched.Tests
One new regression test reproducing the real scenario: two children that poll just past a converge deadline and each reach a genuine "not there" verdict. Verified it actually catches the bug — reverting the fix fails exactly this test and nothing else.
The budget must clear
_MIN_LEAF_BUDGET_SECONDSin that test, otherwise the leaves are short-circuited before they run and the race is never exercised. My first attempt made that mistake and passed for the wrong reason.1194 tests pass; ruff and boilerplate clean.
Summary by CodeRabbit
Fixes #67.