Conversation
HanXHX
commented
Mar 12, 2026
HanXHX
commented
- Refresh OIDC token when expired while uploading
- Increase HTTP timeouts
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce rare upload failures by (1) refreshing OIDC access tokens during long-running transfers and (2) adjusting HTTP timeout behavior to better accommodate large uploads.
Changes:
- Introduce an
oauth2.TokenSourcewrapper that refreshes expired access tokens (optionally persisting refreshed tokens to disk). - Update the API client to accept a
TokenSource(instead of a static token) and adjust transport-level timeouts while removing the client-wide timeout. - Update the transfer command flow to pass a refreshing token source into the API client.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/auth/oidc.go | Adds a concurrency-safe refreshing TokenSource with optional on-disk persistence. |
| internal/api/client.go | Switches client auth to use a TokenSource and changes timeout strategy for long uploads. |
| cmd/transfer.go | Returns/passes a refreshing TokenSource instead of a single token for transfer operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // No client-level Timeout: that field caps the entire round-trip | ||
| // (including body upload), which would kill large chunk uploads on | ||
| // slow connections. Transport-level timeouts above protect against | ||
| // hung connections and unresponsive servers instead. | ||
| httpClient: &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| Transport: transport, | ||
| }, |
There was a problem hiding this comment.
Removing the http.Client Timeout eliminates an upper bound on the total request time. With callers frequently using context.Background(), a stalled upload/download (e.g., server stops reading request body or response body never completes) can now hang indefinitely; ResponseHeaderTimeout only covers waiting for headers after the request is written. Consider adding a generous but finite default via per-request contexts (or an option/config flag) so large uploads aren’t killed, but truly stuck transfers eventually fail.
| func NewRefreshingTokenSource( | ||
| tok *oauth2.Token, | ||
| cfg config.OIDCConfig, | ||
| httpClient *http.Client, | ||
| persist bool, | ||
| ) oauth2.TokenSource { | ||
| return &RefreshingTokenSource{tok: tok, cfg: cfg, httpClient: httpClient, persist: persist} | ||
| } |
There was a problem hiding this comment.
NewRefreshingTokenSource accepts a *oauth2.Token but neither the constructor nor Token() guard against tok being nil. As written, Token() will panic on s.tok.Valid() if a nil token is passed (this is an exported API, so it’s worth making it robust). Consider validating inputs in NewRefreshingTokenSource (or returning an error) and/or returning a clear error from Token() when s.tok is nil.
| return nil, ErrNoRefreshToken | ||
| } | ||
|
|
||
| newTok, err := Refresh(context.Background(), s.cfg, s.tok.RefreshToken, s.httpClient) |
There was a problem hiding this comment.
RefreshingTokenSource.Token() refreshes using context.Background(), and Refresh currently does not attach the provided context to the HTTP request (it uses httpClient.PostForm). This means refresh can’t be canceled and may hang longer than the calling request’s context. Consider updating Refresh to build an http.Request with context (Do(req)) and in Token() using a bounded context (e.g., WithTimeout) so refresh obeys cancellation/time limits.
| newTok, err := Refresh(context.Background(), s.cfg, s.tok.RefreshToken, s.httpClient) | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
| newTok, err := Refresh(ctx, s.cfg, s.tok.RefreshToken, s.httpClient) |
Prevent token expired in large file upload/download.
