Repository navigation
test: scale the SSH retry-budget test's process deadline with its attempt count - #14068
Closed
teamleaderleo wants to merge 2 commits into
Closed
teamleaderleo wants to merge 2 commits into
teamleaderleo wants to merge 2 commits into
Conversation
foregroundAuthenticatedAttachUsesConfiguredRetryBudget runs the full 20-attempt fallback budget through fake ssh/cmux/sleep, which takes about 4.5 s per case on an idle runner, against a fixed 5 s process deadline. Under shard load the harness SIGTERMed the script after 19 sleeps (status 143). The deadline now scales with the attempts the case expects; the assertions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe retry-budget test now sets a timeout based on the expected sleep count. The ChangesSSH reattach test timeout
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This test change does not establish a material risk that should block merging. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Collaborator
Author
|
Status on
No bot review threads are open yet. — Ibex g1 🌿 |
2 tasks done
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Collaborator
Author
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
1 participant
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.
Summary
SSHDeepSleepReattachTests.foregroundAuthenticatedAttachUsesConfiguredRetryBudgetfails on main's app-host shard 3 whenever the shard runs slowly. The test runs the generated attach script through the whole 20-attempt fallback retry budget. Each attempt spawnsuuidgen, the fakessh,cmuxandsleep. The harness'srunProcessgave the script a fixed 5 s deadline, and a full budget takes about 3.8 s per case even on an idle runner. Under shard load the harness SIGTERMs the script before it finishes: status 143 after 19 of 20 sleeps, as in runs 35843579677 (09:32Z) and 35882157190 (15:31Z). The 12:41Z run passed in 8.97 s for the two cases, just under the limit.runProcessnow takes atimeoutparameter, which defaults to the old 5 s. This test passesmax(5, (expectedSleepCount + 1) * 2), which is 42 s for 21 attempts. The deadline only limits how long a hang can run: a passing script returns when it exits, so a healthy run takes no longer. Every assertion is unchanged, including the full 20-sleep budget and the backoff sequence. No other caller changes.This is timing, not logic. The test passes when run alone on main.
Testing
Blacksmith
test-depotdispatches, one fresh process each, suiteSSHDeepSleepReattachTests:72490a9deb(run 35934027300):Test run with 10 tests in 1 suite passed. The target took 7.641 s for its 2 cases, about 3.8 s each against the 5 s deadline. The same run's other selector failed (a separate test, see below).d2126d9891(run 35934029692):Test run with 10 tests in 1 suite passed, target 7.664 s,** TEST SUCCEEDED **.Not verified: a loaded shard run on this branch. The failure needs shard load, which an isolated dispatch doesn't reproduce. So the evidence is the margin (about 3.8 s used of a 42 s deadline), not a red-to-green flip.
#13752 also fixes this test, differently: it cuts the loop to a 4-attempt budget and adds a budget-resolution probe. That PR is 35 files and red on all app-host shards. This change is only the harness deadline, so it can land alone.
Checklist
— Ibex g1 🌿 (subagent)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Scales the SSH retry-budget test's process deadline with the expected attempt count so the script isn't SIGTERMed under shard load.
The full 20-attempt budget takes about 4.5 s per case on an idle runner against the fixed 5 s deadline, causing status 143 after 19 sleeps on slow shards.
runProcessnow accepts atimeoutparameter defaulting to 5 s; the test passesmax(5, (expectedSleepCount + 1) * 2), 42 s for 21 attempts. The deadline only bounds hangs, so assertions and the full budget are unchanged and no other callers change.Written for commit b7ea132. Summary will update on new commits.
Summary by CodeRabbit