security/oidcauth: fix TestOIDCAuthorization_RoleGrantAndRevoke timeout flake#167913
Conversation
…ut flake The test was failing with `context deadline exceeded (Client.Timeout exceeded while awaiting headers)` because the HTTP client used the default 10-second timeout from `GetHTTPClient()`. Under the race detector, the OIDC callback handler (which acquires a mutex, performs token exchange, and executes SQL role grant/revoke operations) can exceed this timeout. Other OIDC tests in the same package already set `client.Timeout = 30 * time.Second` with the comment "Set a reasonable timeout for the client to prevent flakiness under stress." This change applies the same 30-second timeout to the three authorization test functions (`TestOIDCAuthorization_TokenPaths`, `TestOIDCAuthorization_UserinfoPaths`, and `TestOIDCAuthorization_RoleGrantAndRevoke`) that were missing it. Additionally, the retry loop in `performOIDCLogin` is removed because it masked the root cause (insufficient timeout) and had correctness issues with reusing `http.Request` objects after failed `Do` calls. With the proper timeout, retries are unnecessary. Resolves: cockroachdb#167912 Epic: none Release note: None Generated by Claude Code Auto-Solver Co-Authored-By: Claude <noreply@anthropic.com>
|
😎 Merged successfully - details. |
souravcrl
left a comment
There was a problem hiding this comment.
Same retry logic worked here, so approving this https://github.com/cockroachdb/cockroach/pull/161381/changes
|
/trunk merge |
|
[autosolve-response] I reviewed the comments but no code changes were necessary. Analysis: |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #167912: branch-release-26.1, branch-release-26.2. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. merge conflict cherry-picking 3691c2d to blathers/backport-release-25.4-167913 Backport to branch 25.4.x failed. See errors above. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. merge conflict cherry-picking 3691c2d to blathers/backport-release-26.1-167913 Backport to branch 26.1.x failed. See errors above. 💡 Consider backporting to the fork repo instead of the main repo. See instructions for more details. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
…ke timeout flake" This reverts commit 3691c2d (PR cockroachdb#167913). PR cockroachdb#167913 was an auto-generated fix that removed the retry logic added by PR cockroachdb#161381 and replaced it with increased client timeouts. However, PR cockroachdb#161381 already properly addressed the flaky test issue (cockroachdb#159262) by adding retry logic with exponential backoff. The auto-solver's changes were unnecessary and removed a valid fix. Epic: none Release note: None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…AndRevoke flake This reverts commit 3691c2d (PR cockroachdb#167913). PR cockroachdb#167913 was an auto-generated fix that removed the retry logic added by PR cockroachdb#161381 and replaced it with increased client timeouts. However, PR cockroachdb#161381 already properly addressed the flaky test issue (cockroachdb#159262) by adding retry logic with exponential backoff. The auto-solver's changes were unnecessary and removed a valid fix. Epic: none Release note: None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test was failing with
context deadline exceeded (Client.Timeout exceeded while awaiting headers)because the HTTP client used thedefault 10-second timeout from
GetHTTPClient(). Under the racedetector, the OIDC callback handler (which acquires a mutex, performs
token exchange, and executes SQL role grant/revoke operations) can
exceed this timeout.
Other OIDC tests in the same package already set `client.Timeout = 30
with the comment "Set a reasonable timeout for the client to prevent flakiness under stress." This change applies the same 30-second timeout to the three authorization test functions (TestOIDCAuthorization_TokenPaths,TestOIDCAuthorization_UserinfoPaths, andTestOIDCAuthorization_RoleGrantAndRevoke`) that were missing it.Additionally, the retry loop in
performOIDCLoginis removed becauseit masked the root cause (insufficient timeout) and had correctness
issues with reusing
http.Requestobjects after failedDocalls.With the proper timeout, retries are unnecessary.
Resolves: #167912
Epic: none
Release note: None
Generated by Claude Code Auto-Solver
Co-Authored-By: Claude noreply@anthropic.com
This PR was auto-generated by issue-autosolve using Claude Code.
Please review carefully before approving.