fix(client)!: apply no response-header timeout by default - #614
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The change is correct and well-reasoned. DefaultClientResponseHeaderTimeout was causing real failures for legitimate slow-start interactions (store unlock, auth prompts, plugin input — referenced in wield#109). Setting it to 0 aligns with the existing DefaultClientRequestTimeout = 0 design, matches what the docker pass CLI already explicitly does, and correctly reflects the secrets-engine domain where blocking on user interaction is expected.
What was checked:
- The behavioral change from
time.Second→time.Duration(0)is intentional and correctly implemented viahttp.Transport.ResponseHeaderTimeout - Both
RequestTimeoutandResponseHeaderTimeoutnow default to0, consistent with each other and the PR rationale - Updated doc comments in
WithResponseTimeoutand theNew()inline comment accurately describe the new default and correctly point callers toWithResponseTimeoutor per-call context deadlines as opt-in bounds - No compilation issues; API contract for
WithResponseTimeout(rejects negatives, 0 = no limit) is unchanged - The concern about non-interactive callers losing an implicit 1s safety net was evaluated and found to be a pre-existing design tradeoff the PR explicitly and correctly addresses — not a regression
No actionable issues found.
The client defaulted ResponseHeaderTimeout to 1 second, so any request where the daemon took more than a second to start responding failed at the transport layer. A request can legitimately block far longer than that on user interaction daemon-side: a store unlock, an authorization prompt, or a plugin waiting on input. The request timeout already defaults to 0 (no limit) for exactly this reason, and the docker pass CLI already overrides both timeouts to 0. Make that the default for every consumer: DefaultClientResponseHeaderTimeout is now 0 as well. Callers that need a hard bound can still set one with WithResponseTimeout or a per-call context deadline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
89243a5 to
418f69e
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR makes three coherent, self-consistent changes:
x/api/defaults.go—DefaultClientResponseHeaderTimeoutchanged fromtime.Secondtotime.Duration(0)(no limit).client/client.go— Doc comments updated to reflect the new default; no logic changes.plugins/pass/commands/run.go— ExplicitWithResponseTimeout(0)override removed (now redundant given the new default); corresponding test updated.
No logic errors, security issues, resource leaks, or concurrency problems were found in the introduced changes. The change correctly propagates through the call chain, and the removal of the now-redundant override in the pass plugin is safe.
Problem
The Go client defaults
ResponseHeaderTimeoutto 1 second (DefaultClientResponseHeaderTimeout). Any request where the daemon takes more than a second to start responding fails at the transport layer — and a request can legitimately block far longer than that on user interaction daemon-side: a store unlock, an authorization prompt, or a plugin waiting on input.The request timeout already defaults to 0 (no limit) for exactly this reason, and the docker pass CLI already overrides both timeouts to 0 when it builds its client. Every other consumer of the SDK silently inherits the 1s cliff unless they know to call
WithResponseTimeout(0)— an internal SDK consumer just hit this.Change
DefaultClientResponseHeaderTimeoutis now0(no limit), matching the request-timeout default. Doc comments onWithResponseTimeoutand the transport construction updated to match.Consumers that pass explicit timeouts are unaffected;
WithTimeout/WithResponseTimeoutstill apply a hard bound, as does a per-call context deadline.Testing
go build ./...andgo test ./...in thex,client, andpluginmodules — all pass.🤖 Generated with Claude Code