fix(googleapi): replace Client.Timeout with transport-level ResponseHeaderTimeout (#425)
* feat(drive): include shortcutDetails in drive get fields Add shortcutDetails to the Drive Get API fields to enable resolving shortcut target file IDs and MIME types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(googleapi): replace Client.Timeout with transport-level ResponseHeaderTimeout The global http.Client.Timeout (30s) applied to the entire request lifecycle, causing large Drive file downloads (videos, backups, etc.) to time out. Replace it with http.Transport.ResponseHeaderTimeout which only limits the time waiting for the server to begin responding. Once response headers arrive and the body starts streaming, there is no hard cap — large transfers complete naturally. - Set ResponseHeaderTimeout=30s on the base transport - Remove http.Client.Timeout from the API client - Keep a dedicated tokenExchangeTimeout=30s for OAuth2 token refreshes - Add tests verifying the new transport configuration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
4403350a18
commit
449beff88b
@ -251,7 +251,7 @@ func (c *DriveGetCmd) Run(ctx context.Context, flags *RootFlags) error {
|
||||
|
||||
f, err := svc.Files.Get(fileID).
|
||||
SupportsAllDrives(true).
|
||||
Fields("id, name, mimeType, size, modifiedTime, createdTime, parents, webViewLink, description, starred").
|
||||
Fields("id, name, mimeType, size, modifiedTime, createdTime, parents, webViewLink, description, starred, shortcutDetails").
|
||||
Context(ctx).
|
||||
Do()
|
||||
if err != nil {
|
||||
|
||||
@ -20,7 +20,19 @@ import (
|
||||
"github.com/steipete/gogcli/internal/secrets"
|
||||
)
|
||||
|
||||
const defaultHTTPTimeout = 30 * time.Second
|
||||
const (
|
||||
// responseHeaderTimeout limits the time waiting for the server to begin
|
||||
// responding (send response headers). Once headers arrive and the body
|
||||
// starts streaming, there is no hard cap — large file downloads are not
|
||||
// cut short. This replaces the former http.Client.Timeout which applied
|
||||
// to the entire request lifecycle and caused timeouts on large Drive
|
||||
// file downloads.
|
||||
responseHeaderTimeout = 30 * time.Second
|
||||
|
||||
// tokenExchangeTimeout is applied to the short-lived HTTP client used
|
||||
// for OAuth2 token refresh exchanges, which should always be fast.
|
||||
tokenExchangeTimeout = 30 * time.Second
|
||||
)
|
||||
|
||||
var (
|
||||
readClientCredentials = config.ReadClientCredentialsFor
|
||||
@ -78,7 +90,7 @@ func tokenSourceForAccountScopes(ctx context.Context, serviceLabel string, email
|
||||
}
|
||||
|
||||
// Ensure refresh-token exchanges don't hang forever.
|
||||
ctx = context.WithValue(ctx, oauth2.HTTPClient, &http.Client{Timeout: defaultHTTPTimeout})
|
||||
ctx = context.WithValue(ctx, oauth2.HTTPClient, &http.Client{Timeout: tokenExchangeTimeout})
|
||||
|
||||
return cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: tok.RefreshToken}), nil
|
||||
}
|
||||
@ -130,7 +142,9 @@ func optionsForAccountScopes(ctx context.Context, serviceLabel string, email str
|
||||
})
|
||||
c := &http.Client{
|
||||
Transport: retryTransport,
|
||||
Timeout: defaultHTTPTimeout,
|
||||
// No Timeout set: large file downloads (Drive videos, etc.) must not
|
||||
// be cut short. Server responsiveness is guarded by the transport's
|
||||
// ResponseHeaderTimeout instead.
|
||||
}
|
||||
|
||||
slog.Debug("client options with custom scopes created successfully", "serviceLabel", serviceLabel, "email", email)
|
||||
@ -146,11 +160,14 @@ func newBaseTransport() *http.Transport {
|
||||
TLSClientConfig: &tls.Config{
|
||||
MinVersion: tls.VersionTLS12,
|
||||
},
|
||||
ResponseHeaderTimeout: responseHeaderTimeout,
|
||||
}
|
||||
}
|
||||
|
||||
// Clone() deep-copies TLSClientConfig, so no additional clone needed.
|
||||
transport := defaultTransport.Clone()
|
||||
transport.ResponseHeaderTimeout = responseHeaderTimeout
|
||||
|
||||
if transport.TLSClientConfig == nil {
|
||||
transport.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12}
|
||||
return transport
|
||||
|
||||
@ -296,3 +296,48 @@ func TestNewBaseTransport_RespectsProxyAndTLSMinimum(t *testing.T) {
|
||||
t.Fatalf("expected HTTPS proxy to be honored, got: %v", proxyURL)
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewBaseTransport_SetsResponseHeaderTimeout(t *testing.T) {
|
||||
transport := newBaseTransport()
|
||||
if transport.ResponseHeaderTimeout != responseHeaderTimeout {
|
||||
t.Fatalf("expected ResponseHeaderTimeout=%v, got %v", responseHeaderTimeout, transport.ResponseHeaderTimeout)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOptionsForAccountScopes_NoClientTimeout(t *testing.T) {
|
||||
origRead := readClientCredentials
|
||||
origOpen := openSecretsStore
|
||||
|
||||
t.Cleanup(func() {
|
||||
readClientCredentials = origRead
|
||||
openSecretsStore = origOpen
|
||||
})
|
||||
|
||||
readClientCredentials = func(string) (config.ClientCredentials, error) {
|
||||
return config.ClientCredentials{ClientID: "id", ClientSecret: "secret"}, nil
|
||||
}
|
||||
openSecretsStore = func() (secrets.Store, error) {
|
||||
return &stubStore{tok: secrets.Token{Email: "a@b.com", RefreshToken: "rt"}}, nil
|
||||
}
|
||||
|
||||
opts, err := optionsForAccountScopes(context.Background(), "svc", "a@b.com", []string{"s1"})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected err: %v", err)
|
||||
}
|
||||
|
||||
if len(opts) == 0 {
|
||||
t.Fatalf("expected client options")
|
||||
}
|
||||
|
||||
// The http.Client returned by optionsForAccountScopes must not set a
|
||||
// hard Timeout so that large file downloads (Drive videos, etc.) are
|
||||
// not interrupted. Server responsiveness is instead guarded by the
|
||||
// transport-level ResponseHeaderTimeout.
|
||||
//
|
||||
// We cannot easily extract the http.Client from option.ClientOption,
|
||||
// so we verify the transport layer instead.
|
||||
transport := newBaseTransport()
|
||||
if transport.ResponseHeaderTimeout == 0 {
|
||||
t.Fatalf("expected ResponseHeaderTimeout to be set on transport")
|
||||
}
|
||||
}
|
||||
|
||||
@ -20,7 +20,7 @@ var newServiceAccountTokenSource = func(ctx context.Context, keyJSON []byte, sub
|
||||
cfg.Subject = subject
|
||||
|
||||
// Ensure token exchanges don't hang forever.
|
||||
ctx = context.WithValue(ctx, oauth2.HTTPClient, &http.Client{Timeout: defaultHTTPTimeout})
|
||||
ctx = context.WithValue(ctx, oauth2.HTTPClient, &http.Client{Timeout: tokenExchangeTimeout})
|
||||
|
||||
return cfg.TokenSource(ctx), nil
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user