fix(client): classify request timeouts as timeout/exit 4 on all paths#232
Merged
Conversation
A transport-level HTTP timeout was classified inconsistently: network_error (exit 1) on the plain Execute path, but timeout (exit 4) under --wait (via a fragile string-match on the Go error message). The same slow create/upload therefore reported differently depending on whether --wait was passed. Classify the timeout at the source in executeWithContext using net.Error. Timeout() (robust where errors.Is(context.DeadlineExceeded) is not), so both paths return timeout/exit 4. The now-dead message string-match in translateCreateContextError is removed. Behavior change: a plain (non --wait) request timeout now exits 4 instead of 1. README exit-code doc updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ection Review feedback (#229): - A transport timeout on a create emitted a generic "retry" hint, but CLI writes carry no idempotency key, so a blind retry can duplicate a create (and its charge). Make the hint method-aware: GET -> safe to retry; non-GET -> "check status before retrying", mirroring the poll-deadline guidance. - Surface the applied per-operation budget in the timeout message ("...after 120s") so support can see which timeout tripped. - shouldRetry now also short-circuits on net.Error.Timeout(), matching the executor's classifier, so the retry layer and error classifier agree that a client timeout is not worth retrying (previously a client-timeout on an idempotent GET fell through to a wasted retry attempt). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Scope
Surfaces: API (CLI) | Module: HTTP client / error classification
Supersedes #229 (auto-closed when its base #228 merged and the base branch was removed). Review feedback from #229 is addressed here — see commits on this branch.
Summary
A transport-level HTTP timeout was reported with a different error code and exit code depending on whether
--waitwas passed. It now consistently surfaces astimeout(exit 4) on every path.Root cause
The same event — the per-request HTTP deadline elapsing before a response arrived — was classified two ways: plain
Execute→network_error/ exit 1;--wait→timeout/ exit 4 via a fragile string-match on the Go error message. So a slow create/upload reported inconsistently, and the--waitdetection depended on an error string that could drift.Fix
Classify the timeout at the source in
executeWithContextusingnet.Error.Timeout()— the robust detector, sincehttp.Client.Timeout's wrapped error does not reliably satisfyerrors.Is(err, context.DeadlineExceeded)(exactly why the old path string-matched). Both paths now returntimeout/ exit 4, and the dead string-match intranslateCreateContextErroris removed.Also, from #229 review:
...after 120s) for support triage.shouldRetryalso short-circuits onnet.Error.Timeout(), so the retry layer and the classifier agree a client timeout isn't retryable.Scope of impact: a connection-refused / genuine transport failure still returns
network_error(exit 1); the--waitpoll-window-elapsed path is unchanged.Breaking change
A plain (non-
--wait) request timeout now exits 4 instead of 1. The README exit-code table is updated. Diligence: scannedheygen-comfor consumers branching on the CLI'snetwork_error/ exit 1 — none key on it, so no known internal consumer breaks.Testing
New tests: a request that exceeds the client timeout surfaces
timeout/ exit 4; a POST (write) timeout carries the check-status-first hint and names the budget; a GET timeout does not carry create/duplicate guidance. Existing tests still pass: connection-refused →network_error(TestExecute_NetworkError), and the--waitpoll-deadline test still yieldstimeout/ exit 4.make lintclean.