Update dependency to github.com/cli/browser and survive failed browser open#943
Update dependency to github.com/cli/browser and survive failed browser open#943mikelolasagasti wants to merge 1 commit intoopenshift-online:mainfrom
Conversation
openshift-ci
bot
added
the
needs-rebase
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSwapped the browser-launch dependency from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
mikelolasagasti
force-pushed
the
switch-to-cli-browser
branch
from
77912b4 to
0735bcc
Compare
openshift-ci
bot
removed
the
needs-rebase
![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 the current code and only fix it if needed.
Inline comments:
In `@authentication/auth.go`:
- Around line 106-109: The browser.OpenURL(url) error handling currently returns
early and skips shutting down the HTTP server; instead, catch the error from
browser.OpenURL in the auth flow (authentication/auth.go), log the failure and
the URL so the user can open it manually, and continue to the existing wait loop
rather than returning; ensure the existing shutdown(server) invocation
(shutdown(server)) still executes in all exit paths (success, manual completion,
or timeout) so the server is not leaked—modify the block around
browser.OpenURL(url) in the function that drives the auth flow to log and
continue rather than return on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e928d580-c4ee-4a53-bcf8-762d82b115f8
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
authentication/auth.goexamples/go.modgo.mod
mikelolasagasti
force-pushed
the
switch-to-cli-browser
branch
from
0735bcc to
98a85ec
Compare
mikelolasagasti
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
authentication/auth.go (1)
22-23:⚠️ Potential issue | 🔴 CriticalSynchronize
authTokenaccess and replace busy-loop with blocking wait.Concurrent writes and reads of
authTokenwithout synchronization create a data race: the callback handler (running in HTTP server's goroutine) writes toauthTokenat line 45, whileInitiateAuthCodereads it at lines 114 and 116 without locks. Additionally, the wait loop at lines 113–122 busy-spins with only CPU-bound condition checks, consuming resources until the timeout.Use a mutex to guard
authTokenaccess and replace the tight polling loop with a blockingselectstatement usingtime.Timerandtime.Ticker.Proposed fix
var ( conf *oauth2.Config ctx context.Context verifier string authToken string + authMu sync.RWMutex ) @@ - authToken = tok.RefreshToken + authMu.Lock() + authToken = tok.RefreshToken + authMu.Unlock() @@ - fiveMinTimer := time.Now().Local().Add(time.Minute * 5) + timeout := time.NewTimer(5 * time.Minute) + defer timeout.Stop() + ticker := time.NewTicker(100 * time.Millisecond) + defer ticker.Stop() @@ - for { - if authToken != "" { - shutdown(server) - return authToken, nil - } - if time.Now().After(fiveMinTimer) { - shutdown(server) - return authToken, fmt.Errorf("time expired") - } - } + for { + select { + case <-timeout.C: + shutdown(server) + return "", fmt.Errorf("time expired") + case <-ticker.C: + authMu.RLock() + token := authToken + authMu.RUnlock() + if token != "" { + shutdown(server) + return token, nil + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@authentication/auth.go` around lines 22 - 23, Protect the shared variable authToken with a sync.Mutex (or sync.RWMutex): add a mutex (e.g., authMu) and use authMu.Lock()/Unlock() around every write to authToken in the HTTP callback handler and use authMu.RLock()/RUnlock() around every read in InitiateAuthCode to eliminate the race. Replace the tight polling loop in InitiateAuthCode with a blocking select: create a time.Timer for the overall timeout and a time.Ticker (or a channel signaled by the callback) to wait for authToken to be set; on each tick (or when the callback signals) acquire the read lock and check authToken, and return when set or when the timer fires. Ensure the callback signals the waiting goroutine (if using a channel) after writing authToken while holding the mutex to avoid races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@authentication/auth.go`:
- Around line 22-23: Protect the shared variable authToken with a sync.Mutex (or
sync.RWMutex): add a mutex (e.g., authMu) and use authMu.Lock()/Unlock() around
every write to authToken in the HTTP callback handler and use
authMu.RLock()/RUnlock() around every read in InitiateAuthCode to eliminate the
race. Replace the tight polling loop in InitiateAuthCode with a blocking select:
create a time.Timer for the overall timeout and a time.Ticker (or a channel
signaled by the callback) to wait for authToken to be set; on each tick (or when
the callback signals) acquire the read lock and check authToken, and return when
set or when the timer fires. Ensure the callback signals the waiting goroutine
(if using a channel) after writing authToken while holding the mutex to avoid
races.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a0b7849-4031-4036-9154-5867c5366951
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
authentication/auth.goexamples/go.modgo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/go.mod
openshift-ci
bot
added
the
needs-rebase
Replace github.com/skratchdot/open-golang with github.com/cli/browser. On OpenURL failure (headless, SSH, WSL, etc.), log the auth URL and keep waiting for the callback instead of returning early. That avoids leaving the local HTTP server running and still allows manual login. Signed-off-by: Mikel Olasagasti Uranga <mikel@olasagasti.info>
mikelolasagasti
force-pushed
the
switch-to-cli-browser
branch
from
98a85ec to
3c12c46
Compare
openshift-ci
bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mikelolasagasti 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 |
1 participant
Module
github.com/skratchdot/open-golanghasn't had any activity since 2020. Modulegithub.com/cli/browseris more maintained, includes better support for Windows or WSL and is being used by GitHub's CLI or OpenTofu.On OpenURL failure (headless, SSH, WSL, etc.), log the auth URL and keep waiting for the callback instead of returning early. That avoids leaving the local HTTP server running and still allows manual login.