fix(test): give the scoped-access-key test a timeout it can actually meet - #1311
Closed
SandyChapman wants to merge 1 commit into
Closed
fix(test): give the scoped-access-key test a timeout it can actually meet#1311SandyChapman wants to merge 1 commit into
SandyChapman wants to merge 1 commit into
Conversation
…meet This test has been failing roughly half of all integration runs, reported as `worker gwN crashed while running ...`. It is not a crash. It is the suite-wide `--timeout=120` firing on a test that legitimately takes about two minutes. Durations recorded on runs where it passed: 89.47s, 108.28s, 117.81s, and 119.92s — the last with 0.08s to spare. Every failing run reports no duration at all, because the test never finished. The distribution straddles the limit, so whether a given run passes is decided by how loaded the runner is. It reads as a crash because of how the kill lands. pytest-timeout's thread method calls `os._exit(1)`, which is not a signal, so faulthandler never runs and writes nothing. Its own stack dump goes to the worker's captured stdout, which xdist does not forward. Both diagnostic channels are dark for different reasons, leaving the controller with only `node down: Not properly terminated`. The cost comes from standing up a full auth-enabled platform in-process and driving the entire access-key lifecycle through it. That is worth doing in an integration test, so raise the ceiling for this test rather than trim what it covers. Verified the marker overrides the command-line value by running under `--timeout=5` and watching it pass. Making the test cheaper is worth doing separately — `bundle_cache_seconds=0` forces a policy reload on every authorization call — but that is a behavioural change to what the test exercises, and this failure should stop now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe scoped access key integration tests now run in the ChangesAuth integration test execution
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change gives a slow integration test enough time to complete without altering production behavior or test assertions, so no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
8 tasks
Contributor
|
Contributor
Author
|
Superseded by the cost fix now in #1290. Both fixes were verified against CI and each confirmed the diagnosis independently — this one by running the test at 129.26s, over the old 120s limit, and passing only because the ceiling was raised. That made it the causal proof, but #1290's approach removes the cost rather than accommodating it. |
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
test_scoped_access_keys.py::test_scoped_access_key_created_by_auth_service_authenticates_platform_requestsfails roughly half of all integration runs, reported asworker gwN crashed while running .... It is not a crash — it is the suite-wide--timeout=120firing on a test that legitimately takes about two minutes.Evidence
Durations from
--durations=25on runs where it passed, against the 120s limit:88698368095b19914c3bef54ed68cbb1Every pass is under 120s; every failure reports no duration. Whether a run passes is decided by runner load.
Why it looked like a crash
pytest-timeout's thread method calls
os._exit(1). That is not a signal, so faulthandler never runs — which is why a per-worker faulthandler dump captured the test-entry trail and no traceback. Its own stack dump goes to the worker's captured stdout, which xdist does not forward. Both channels dark, for different reasons, leaving onlynode down: Not properly terminated.Changes
pytest.mark.timeout(600)on the test, alongside its existingxdist_group. Verified the marker overrides the command-line value by running under--timeout=5and watching it pass.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses — three hooks fail on this machine (Helm Docs,uv lock,UI lint-staged, the last on a missing localpnpmshim). None touch this file.Targeted validation: passes locally in 65.58s under
--timeout=120, and in 18.88s under--timeout=5, confirming the marker takes precedence.Follow-up
Making the test cheaper is worth doing separately —
bundle_cache_seconds=0forces a policy reload on every authorization call. That changes what the test exercises, so it should not ride along with stopping the bleeding.🤖 Generated with Claude Code
Summary by CodeRabbit