OSAC-3669: Fix CLI hangs when OAuth token/logout endpoints stall - #161
Merged
openshift-merge-bot[bot] merged 4 commits intoAug 7, 2026
Conversation
|
@wgordon17: This pull request references OSAC-3669 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe OAuth token source and logout commands now preserve caller contexts, use bounded HTTP clients, and construct form-encoded requests explicitly. Regression tests verify prompt failure when token, logout, or refresh endpoints block. ChangesOAuth and logout request timeout handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant LogoutCommand
participant HTTPClient
participant LogoutEndpoint
Caller->>LogoutCommand: invoke with deadline context
LogoutCommand->>HTTPClient: send context-bound GET or form POST
HTTPClient->>LogoutEndpoint: execute logout request
LogoutEndpoint-->>HTTPClient: response or context cancellation
HTTPClient-->>LogoutCommand: response or request error
LogoutCommand-->>Caller: logout result or error
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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 `@fulfillment-service/internal/oauth/oauth_token_source_test.go`:
- Around line 1067-1074: Update the test around source.Token to invoke
Token(requestCtx) in a goroutine and select between its result and a five-second
timer, failing the spec if the timer fires before completion. Move
close(release) into deferred cleanup so the handler is always released, while
preserving the existing error assertion for a completed Token call.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fdd7ab86-49c0-4b0b-990f-ba08079ede00
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
fulfillment-service/internal/oauth/oauth_token_source.gofulfillment-service/internal/oauth/oauth_token_source_test.go
wgordon17
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@fulfillment-service/internal/cmd/cli/logout/logout_cmd_test.go`:
- Around line 193-205: Handle the errors returned by both
json.NewEncoder(w).Encode calls in the OpenID configuration and token handlers
within the logout test fixture, checking each result and reporting any encoding
failure so terminateSession reaches the intended logout behavior.
- Around line 229-240: Update the tests at
fulfillment-service/internal/cmd/cli/logout/logout_cmd_test.go:229-240 and
:363-373 to invoke terminateSession and refreshForIdToken in goroutines,
respectively, and await each result through a five-second timeout so the tests
cannot block on the underlying client timeout. Ensure each blocking handler is
released during cleanup, including when the timeout assertion fails, and
preserve the existing error and elapsed-time assertions.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 219cd0d4-6708-41c6-a74d-c96c7e9f271a
📒 Files selected for processing (2)
fulfillment-service/internal/cmd/cli/logout/logout_cmd.gofulfillment-service/internal/cmd/cli/logout/logout_cmd_test.go
sendForm() sent token-endpoint requests with http.Client.PostForm, which builds its request with context.Background() internally and ignores the caller's context entirely. Combined with the client falling back to http.DefaultClient (Timeout: 0) whenever the caller didn't supply one, any stall talking to the token endpoint (a SYN never answered, a stalled TLS handshake, a connection accepted but never written to) blocked forever with no way to cancel it short of killing the process. Build the request explicitly with http.NewRequestWithContext so the caller's context is honored, and stop mutating the shared http.DefaultClient: construct a fresh *http.Client with a bounded 30s timeout when the caller doesn't supply their own via SetHttpClient. Adds a regression test that points the token endpoint at a handler that never responds and asserts Token() fails promptly once the context deadline is reached, instead of hanging. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
logout_cmd.go independently reimplemented the same defect fixed for CLI login in the previous commit: terminateSession() called client.Get() and refreshForIdToken() called client.PostForm(), both of which build their request with context.Background() internally and ignore the caller's context. The client returned by httpClient() also had no Timeout, so a stalled connection to the end_session_endpoint or token endpoint during 'osac logout' would hang forever. Build both requests explicitly with http.NewRequestWithContext, and give httpClient() a bounded 30s timeout as a backstop for a bare context.Background() with no deadline of its own. Adds regression tests for both call sites that point their respective endpoint at a handler that never responds and assert the call fails promptly once the context deadline is reached. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Per CodeRabbit review: the three hang regression tests called Token()/ terminateSession()/refreshForIdToken() synchronously and closed their release channel afterward. If the fix under test ever regressed, the call would hang forever, taking down the whole test binary instead of failing just that spec, and the deferred server.Close() would never even run since close(release) was never reached. Run each call in a goroutine racing a 5-second timer, and defer close(release) so it always runs regardless of which branch of the select executes. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
wgordon17
force-pushed
the
fix/OSAC-3669-oauth-token-source-hang
branch
from
August 6, 2026 13:45
581ae6d to
b5aad74
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fulfillment-service/internal/oauth/oauth_token_source.go (1)
432-438: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet an explicit minimum TLS version.
tls.ConfigleavesMinVersionunset. Set the approved minimum explicitly. Usetls.VersionTLS13if all supported OAuth endpoints permit it. Otherwise set the approved TLS 1.2 baseline and document the compatibility requirement.As per path instructions, this file requires explicit cryptographic-security review.
🤖 Prompt for 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. In `@fulfillment-service/internal/oauth/oauth_token_source.go` around lines 432 - 438, Update the tls.Config initialization in the OAuth token source to set the approved explicit minimum TLS version, using TLS 1.3 when all supported endpoints allow it or the approved TLS 1.2 baseline when compatibility requires it. Document the compatibility requirement if TLS 1.2 is selected, and preserve the existing RootCAs and insecure-mode behavior.Sources: Path instructions, Linters/SAST tools
🤖 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 `@fulfillment-service/internal/oauth/oauth_token_source.go`:
- Around line 439-444: Update the httpClient transport initialization to clone
http.DefaultTransport before applying TLSClientConfig. Configure the cloned
*http.Transport with the existing tlsConfig and retain its default proxy
settings and HTTP/2 behavior, then assign it to cfg.httpClient.Transport.
---
Nitpick comments:
In `@fulfillment-service/internal/oauth/oauth_token_source.go`:
- Around line 432-438: Update the tls.Config initialization in the OAuth token
source to set the approved explicit minimum TLS version, using TLS 1.3 when all
supported endpoints allow it or the approved TLS 1.2 baseline when compatibility
requires it. Document the compatibility requirement if TLS 1.2 is selected, and
preserve the existing RootCAs and insecure-mode behavior.
🪄 Autofix
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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d35b338c-ece2-48a0-9c26-e8d740310a00
📒 Files selected for processing (4)
fulfillment-service/internal/cmd/cli/logout/logout_cmd.gofulfillment-service/internal/cmd/cli/logout/logout_cmd_test.gofulfillment-service/internal/oauth/oauth_token_source.gofulfillment-service/internal/oauth/oauth_token_source_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- fulfillment-service/internal/oauth/oauth_token_source_test.go
- fulfillment-service/internal/cmd/cli/logout/logout_cmd_test.go
- fulfillment-service/internal/cmd/cli/logout/logout_cmd.go
- oauth_token_source.go and logout_cmd.go both built their fallback HTTP client's Transport from a bare struct literal with only TLSClientConfig set, silently dropping http.DefaultTransport's Proxy, dial/keep-alive timeouts, HTTP/2, and connection-pooling defaults. Clone http.DefaultTransport instead and only override TLSClientConfig. CodeRabbit flagged this on oauth_token_source.go; the identical logout_cmd.go instance wasn't re-flagged (marked similar to previous changes) but has the same defect, fixed for consistency. - Both tls.Config values also left MinVersion unset; set it to TLS 1.3, matching this codebase's existing convention (consoleproxy, kubevirt_backend). - logout_cmd_test.go's end-session-endpoint regression test ignored the error from json.NewEncoder(w).Encode() in its fixture handlers; check it, matching the in-handler assertion style already used elsewhere in this file. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Contributor
|
/retest |
|
Re-triggered failed runs:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, wgordon17 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit Aug 7, 2026
87ad589
into
osac-project:main
43 of 56 checks passed
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
4 participants
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
sendForm()(login/token refresh) andlogout_cmd.go'sterminateSession/refreshForIdTokenall sent requests viahttp.Client.PostForm/.Get, which build their request withcontext.Background()internally, so the caller's context could never cancel a stalled request; combined with falling back to an unbounded-timeout client, any stall talking to the OAuth server hung the CLI foreverhttp.NewRequestWithContextso the caller's context is honored, and a bounded 30s timeout is set on every client constructed by this code instead of relying on an unbounded defaultosacmono-repo (osac-operator, osac-csi-driver, bare-metal-fulfillment-operator, and the rest of fulfillment-service) for the same pattern — no other instances found; server-side JWKS/API-client code already builds context-aware requests correctlySummary by CodeRabbit