From dabef0ad5124853c0db20dead8fc85bac85dd5d8 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Tue, 21 Jul 2026 19:29:20 +0000 Subject: [PATCH 1/2] fix(client): classify request timeouts as timeout/exit 4 on all paths 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) --- README.md | 2 +- internal/client/executor.go | 41 +++++++++++++++++++------------- internal/client/executor_test.go | 30 +++++++++++++++++++++++ 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index dc36c4f..ccd5657 100644 --- a/README.md +++ b/README.md @@ -166,7 +166,7 @@ Every command supports `--help`. |--------|----------| | **stdout** | Always JSON. Even `video download` — binary writes to disk; stdout emits `{"asset", "message", "path"}` so you can chain on `.path`. | | **stderr** | Structured envelope on error: `{"error": {"code", "message", "hint", "param", "doc_url", "request_id"}}`. `code`/`message` are always present; `hint`/`param`/`doc_url`/`request_id` appear when applicable (`param`/`doc_url` are surfaced from the API for validation and documented errors). Stable `code` values for programmatic branching. A code prefixed `cli_` is originated by the CLI itself (client/transport/local conditions, e.g. `cli_download_url_expired`). A bare code is either an API code (or a CLI mirror of one) or one of a small frozen set of legacy CLI codes that predate the prefix. The `cli_` prefix is reserved for the CLI, so a new CLI code can never collide with an API code. | -| **Exit codes** | `0` ok · `1` API or network · `2` usage · `3` auth / not permitted · `4` timeout under `--wait` (stdout contains partial resource for resume) | +| **Exit codes** | `0` ok · `1` API or network · `2` usage · `3` auth / not permitted · `4` timeout — a request exceeded its per-operation timeout, or the `--wait` poll window elapsed (stdout contains partial resource for resume) | | **Request bodies** | Flags for simple inputs; `-d` for nested JSON (inline, file path, or `-` for stdin). Flags override matching fields. | | **Async jobs** | `--wait` blocks with exponential backoff; `--timeout` sets max (default 20m). 429s and 5xx retry automatically. | diff --git a/internal/client/executor.go b/internal/client/executor.go index fe95732..32a48c8 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "mime/multipart" + "net" "net/http" "net/url" "os" @@ -207,6 +208,23 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv ExitCode: clierrors.ExitAuth, } } + // A transport-level timeout (the per-request HTTP deadline elapsed + // before the response arrived) is classified as timeout/exit 4 — the + // same code on both the plain and --wait paths, so a slow create or + // upload no longer masquerades as a generic network_error on one path + // and a timeout on the other. net.Error.Timeout() is the robust + // detector: http.Client.Timeout's wrapped error does not reliably + // satisfy errors.Is(context.DeadlineExceeded), which is why the old + // --wait path resorted to string-matching the error message. + var netErr net.Error + if (errors.As(err, &netErr) && netErr.Timeout()) || errors.Is(err, context.DeadlineExceeded) { + return nil, &clierrors.CLIError{ + Code: "timeout", + Message: fmt.Sprintf("request timed out: %v", err), + Hint: "The request took too long and timed out. Retry; large uploads and long-running operations may need another attempt.", + ExitCode: clierrors.ExitTimeout, + } + } // network_error is a grandfathered bare code (not cli_-prefixed): it is // shared with the download-command transport path (cmd/heygen/video_download.go) // and predates the cli_ convention. See the grandfathered set in @@ -455,10 +473,13 @@ func extractJSONPath(raw json.RawMessage, path string) (string, error) { return value, nil } -// translateCreateContextError converts timeout/cancel errors before a resource -// ID is known into user-friendly CLIErrors. Deadline exceeded returns exit 4 -// (timeout) since the operation may have been created server-side. Cancel -// returns exit 1 (general) since the user explicitly interrupted. +// translateCreateContextError maps a canceled/expired poll context to a +// user-friendly CLIError when the create request fails before a resource ID is +// known. A tripped poll deadline returns exit 4 (timeout, the operation may +// exist server-side); an explicit cancel returns exit 1. A transport-level +// request timeout that fires while the poll context is still live is already +// classified as timeout/exit 4 by executeWithContext, so it passes through the +// final return unchanged — no message string-matching required. func translateCreateContextError(ctx context.Context, err error) error { if ctxErr := ctx.Err(); ctxErr != nil { return newCreateContextError(ctxErr) @@ -466,18 +487,6 @@ func translateCreateContextError(ctx context.Context, err error) error { if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return newCreateContextError(err) } - - var cliErr *clierrors.CLIError - if errors.As(err, &cliErr) && cliErr.Code == "network_error" { - msg := strings.ToLower(cliErr.Message) - switch { - case strings.Contains(msg, "context deadline exceeded"): - return newCreateContextError(context.DeadlineExceeded) - case strings.Contains(msg, "context canceled"), strings.Contains(msg, "context cancelled"): - return newCreateContextError(context.Canceled) - } - } - return err } diff --git a/internal/client/executor_test.go b/internal/client/executor_test.go index 9761ddd..19624f0 100644 --- a/internal/client/executor_test.go +++ b/internal/client/executor_test.go @@ -1026,3 +1026,33 @@ func TestExecute_ResponseBodyReadError_NetworkError(t *testing.T) { t.Fatalf("err = %v, want a network_error CLIError", err) } } + +func TestExecute_RequestTimeout_ClassifiedAsTimeout(t *testing.T) { + // Handler sleeps past the client timeout so the request deadline trips + // before any response arrives. The transport timeout must surface as a + // timeout CLIError (exit 4), not a generic network_error (exit 1). + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(200 * time.Millisecond) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{}}`)) + })) + defer srv.Close() + + c := New("key", WithBaseURL(srv.URL), WithHTTPClient(srv.Client()), WithMaxRetries(0)) + c.SetTimeout(50 * time.Millisecond) + + spec := &command.Spec{Endpoint: "/v3/videos", Method: "POST", BodyEncoding: "json"} + inv := &command.Invocation{PathParams: make(map[string]string), Body: map[string]any{"x": 1}} + + _, err := c.Execute(spec, inv) + var cliErr *clierrors.CLIError + if !errors.As(err, &cliErr) { + t.Fatalf("want *CLIError, got %v", err) + } + if cliErr.Code != "timeout" { + t.Errorf("code = %q, want %q", cliErr.Code, "timeout") + } + if cliErr.ExitCode != clierrors.ExitTimeout { + t.Errorf("exit code = %d, want %d (ExitTimeout)", cliErr.ExitCode, clierrors.ExitTimeout) + } +} From a0cdc642c93c3c14c875456f98589f1db7aace43 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Wed, 22 Jul 2026 21:32:56 +0000 Subject: [PATCH 2/2] fix(client): write-safe timeout hint, surface budget, align retry detection 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) --- internal/client/executor.go | 19 ++++++++++++++++-- internal/client/executor_test.go | 33 ++++++++++++++++++++++++++++++++ internal/client/retry.go | 12 +++++++++++- 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/internal/client/executor.go b/internal/client/executor.go index 32a48c8..4b8f052 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -218,10 +218,25 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv // --wait path resorted to string-matching the error message. var netErr net.Error if (errors.As(err, &netErr) && netErr.Timeout()) || errors.Is(err, context.DeadlineExceeded) { + // A GET is safe to retry. A non-GET (create/write) may have been + // committed server-side before the client deadline elapsed, and CLI + // writes carry no idempotency key, so a blind retry can produce a + // duplicate (e.g. a second video create + charge). Steer writes to + // "check status first", mirroring the poll-deadline guidance in + // newCreateContextError. Include the applied budget so support can + // see which per-operation timeout tripped. + hint := "The request took too long and timed out. Retry — a slow endpoint or a stalled network connection can cause this." + if spec.Method != http.MethodGet { + hint = "The request timed out, but the operation may have been submitted. Check with the corresponding get/list command before retrying, to avoid creating a duplicate." + } + msg := "request timed out" + if c.httpClient.Timeout > 0 { + msg = fmt.Sprintf("request timed out after %s", c.httpClient.Timeout) + } return nil, &clierrors.CLIError{ Code: "timeout", - Message: fmt.Sprintf("request timed out: %v", err), - Hint: "The request took too long and timed out. Retry; large uploads and long-running operations may need another attempt.", + Message: fmt.Sprintf("%s: %v", msg, err), + Hint: hint, ExitCode: clierrors.ExitTimeout, } } diff --git a/internal/client/executor_test.go b/internal/client/executor_test.go index 19624f0..2eec256 100644 --- a/internal/client/executor_test.go +++ b/internal/client/executor_test.go @@ -1055,4 +1055,37 @@ func TestExecute_RequestTimeout_ClassifiedAsTimeout(t *testing.T) { if cliErr.ExitCode != clierrors.ExitTimeout { t.Errorf("exit code = %d, want %d (ExitTimeout)", cliErr.ExitCode, clierrors.ExitTimeout) } + // A write (POST) must not advise a blind retry — the create may have been + // committed and CLI writes have no idempotency key. + if !strings.Contains(cliErr.Hint, "before retrying") { + t.Errorf("POST timeout hint should steer to check-status-first, got: %q", cliErr.Hint) + } + // The applied per-operation budget is surfaced for triage. + if !strings.Contains(cliErr.Message, "50ms") { + t.Errorf("timeout message should name the applied budget, got: %q", cliErr.Message) + } +} + +func TestExecute_GETRequestTimeout_SafeToRetryHint(t *testing.T) { + // A read (GET) that times out is safe to retry, so its hint should not carry + // the create-oriented "check before retrying" guidance. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(200 * time.Millisecond) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{}}`)) + })) + defer srv.Close() + + c := New("key", WithBaseURL(srv.URL), WithHTTPClient(srv.Client()), WithMaxRetries(0)) + c.SetTimeout(50 * time.Millisecond) + + _, err := c.Execute(&command.Spec{Endpoint: "/v3/videos", Method: "GET"}, + &command.Invocation{PathParams: make(map[string]string), QueryParams: url.Values{}}) + var cliErr *clierrors.CLIError + if !errors.As(err, &cliErr) || cliErr.Code != "timeout" { + t.Fatalf("want timeout CLIError, got %v", err) + } + if strings.Contains(cliErr.Hint, "duplicate") { + t.Errorf("GET timeout hint should not carry create/duplicate guidance, got: %q", cliErr.Hint) + } } diff --git a/internal/client/retry.go b/internal/client/retry.go index 1f592c2..03e6be0 100644 --- a/internal/client/retry.go +++ b/internal/client/retry.go @@ -6,6 +6,7 @@ import ( "io" "math" "math/rand/v2" + "net" "net/http" "os" "strconv" @@ -88,9 +89,18 @@ func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) { func shouldRetry(req *http.Request, resp *http.Response, err error) bool { if err != nil { + // A tripped client timeout is not worth retrying: http.Client.Timeout is + // a total-Do budget, so the context is already spent. Match on + // net.Error.Timeout() as well as the context sentinels, because + // http.Client.Timeout's wrapped error does not reliably satisfy + // errors.Is(context.DeadlineExceeded) — the same reason the executor's + // classifier uses net.Error.Timeout(). Keeps both layers agreeing on + // what "timeout" means. + var netErr net.Error if req.Context().Err() != nil || errors.Is(err, context.Canceled) || - errors.Is(err, context.DeadlineExceeded) { + errors.Is(err, context.DeadlineExceeded) || + (errors.As(err, &netErr) && netErr.Timeout()) { return false } return isIdempotent(req.Method)