fix(client): classify request timeouts as timeout/exit 4 on all paths

[somanshreddy]

Scope

Surfaces: API (CLI) | Module: HTTP client / error classification

Stacked on the per-operation HTTP timeouts PR (the previous PR in the stack).

Summary

A transport-level HTTP timeout was reported with a different error code and exit code depending on whether --wait was passed. It now consistently surfaces as timeout (exit 4) on every path.

Root cause

The same underlying event — the per-request HTTP deadline elapsing before a response arrived — was classified two ways:

• Plain Execute path → network_error / exit 1.
• --wait path → timeout / exit 4, via a fragile string-match on the Go error message text ("context deadline exceeded").

So a slow create or upload reported inconsistently, and the --wait detection depended on an error string that could drift.

Fix

Classify the timeout at the source in executeWithContext using net.Error.Timeout(). This is the robust detector: http.Client.Timeout's wrapped error does not reliably satisfy errors.Is(err, context.DeadlineExceeded), which is exactly why the old --wait path resorted to string-matching. Both paths now return timeout / exit 4, and the now-dead string-match in translateCreateContextError is removed.

Scope of impact: a connection-refused / genuine transport failure still returns network_error (exit 1); the --wait poll-window-elapsed path is unchanged. Behavior change: a plain (non---wait) request timeout now exits 4 instead of 1. The README exit-code table is updated to reflect that exit 4 covers a request timeout as well as the --wait poll window.

Testing

New test asserting a request that exceeds the client timeout surfaces timeout / exit 4. Existing tests still pass: connection-refused stays network_error, and the --wait poll-deadline test still yields timeout / exit 4. make lint clean.

[somanshreddy]

The timeout-classification unification is correct and a genuine improvement. An http.Client.Timeout error (the per-op timeout introduced in #228) does not reliably satisfy errors.Is(err, context.DeadlineExceeded) — so the old string-match path would have mis-bucketed it as network_error/exit 1 on the plain path while the --wait path called it a timeout. net.Error.Timeout() (plus the context.DeadlineExceeded fallback) is the robust detector, and dropping the fragile message string-matching in translateCreateContextError is the right call. Tests cover it well.

One nonblocking safety nit on the create path:

Because executeWithContext now classifies a transport timeout as the generic timeout CLIError before translateCreateContextError runs, a create whose request trips its per-op HTTP budget (120s, from #228) while the --wait poll context (default 20m) is still live returns the generic hint:

"Retry; large uploads and long-running operations may need another attempt."

But CLI creates carry no Idempotency-Key (none in executor.go / client.go), and the server may have committed the resource before the client deadline elapsed. So a blind "retry" can produce a duplicate create. Note the poll-window-elapsed path deliberately gives the safe guidance for exactly this reason — newCreateContextError → "Re-run the corresponding get command to check the current status manually" — and the transport-timeout-on-create case now bypasses it.

Suggestion (keep exit 4 — that's the correct, and now consistent, contract): for a non-idempotent create request, either remap the transport timeout to the create-aware "check status before retrying" message in translateCreateContextError (match on the timeout code, not only ctx.Err() / errors.Is), or soften the generic timeout hint to "check whether it succeeded before retrying." Purely about not advising a blind retry on a possibly-committed create; the exit-code fix itself is good.

— Review by Somu

[james-russo-rames-d-jusso]

Reviewing 0635781 — timeout classification unification (fix(client): classify request timeouts as timeout/exit 4 on all paths).

Scope: source-of-truth transport-timeout classification moves upstream to executeWithContext (internal/client/executor.go:219), using net.Error.Timeout() (via errors.As) with errors.Is(err, context.DeadlineExceeded) as a belt-and-braces fallback. Both plain and --wait paths now yield code timeout / ExitTimeout=4. The old string-match branch on context deadline exceeded / context canceled in translateCreateContextError is deleted (internal/client/executor.go:487 — verified the function body shrunk to just the two context.* guards + passthrough). README exit-code table updated. Pairs with #228 which sets which budget applies.

Verified against HEAD: errors.As(err, &netErr) on an interface target is Go 1.20+ correct and works against *url.Error (the type http.Client.Timeout wraps); net.Error.Timeout() reliably reports true for both http.Client.Timeout trips and net.Dialer / TLS-handshake deadline expirations. ExitTimeout = 4 (internal/errors/errors.go:12). timeout is a grandfathered bare code (internal/errors/codes.go:34), so no namespace-convention issue. strings is still used elsewhere in executor.go (line 267, 377, etc.) — no dead import from the branch removal.

Blockers

None.

Concerns

• 🟠 Behavior change on the plain path is a breaking change with no escape hatch. Any wrapper (shell script, CI job, another bot) that currently branches on if [ $? -eq 1 ] after heygen video create (no --wait) to distinguish "network hiccup, retry" from "user error, don't retry" will see the exit code flip from 1 to 4 on transport timeout. The classification is arguably more correct — and the README is updated — but this deserves an explicit line in the release notes / PR body under a "breaking changes" heading, and ideally a scan of known internal wrappers before it lands. If you're OK with the break, it's OK; but it shouldn't land silently.
• 🟠 net.Error.Timeout() is broader than "http.Client.Timeout tripped." It also fires on TLS-handshake timeouts, dial timeouts, and connection-level read/write deadlines. That's arguably more correct — those genuinely are timeout-shape failures — but the new hint text "The request took too long and timed out. Retry; large uploads and long-running operations may need another attempt." is framed for the "server was slow" case. A user hitting a stalled TLS handshake on a broken corporate proxy gets the same hint and will burn time chasing the wrong hypothesis. Non-blocking; consider a hint that reads "…may be a slow endpoint OR a stalled TLS/DNS handshake; retry, and check network reachability if it persists" or similar.
• 🟠 Retry layer uses the "unreliable" detector; executor uses the "robust" one. internal/client/retry.go:89 shouldRetry short-circuits on errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded). Per this PR's own comment (executor.go:216), the context.DeadlineExceeded check does NOT reliably match on http.Client.Timeout-wrapped errors — which is precisely why the executor path uses net.Error.Timeout(). Consequence: on an idempotent method (GET, PUT, DELETE) that trips the client Timeout, shouldRetry falls through the deadline guard, hits return isIdempotent(req.Method), and returns true. The retry attempt then immediately fails because http.Client.Timeout is a total-Do budget and the context is already canceled — so functionally harmless, just a wasted attempt + no delay (backoff sleeps under a canceled ctx return quickly via waitForRetry). Consider tightening shouldRetry to also check var nerr net.Error; errors.As(err, &nerr) && nerr.Timeout() and short-circuit on that too, so both layers agree on what "timeout" means. One-liner fix; keeps the classifier logic single-sourced. Non-blocking.

Nits

• 🟡 Missing negative-case test. New TestExecute_RequestTimeout_ClassifiedAsTimeout locks the positive path; there's no companion asserting that a non-timeout transport failure (connection refused, DNS NXDOMAIN, immediate RST) still yields network_error / exit 1. Given net.Error.Timeout() is broader than the message text implies, a "refused → network_error" guard would protect against a future refactor accidentally classifying non-timeout transport errors as timeouts. httptest.NewServer + srv.Close() before the request would give you a connection-refused case in a few lines.
• 🟡 The rationale comment at executor.go:211-218 is excellent context — worth linking to the upstream Go stdlib doc / issue explaining why http.Client.Timeout's wrapped error doesn't propagate context.DeadlineExceeded (this is a well-known but non-obvious quirk), so a future reader doesn't have to rederive.
• 🟡 Hint text is a bit verbose — "The request took too long and timed out. Retry; large uploads and long-running operations may need another attempt." could trim to "Retry — large uploads and long-running operations may take a second attempt." for closer alignment to the other hints in the file.

Questions

• Any known internal consumer of heygen-cli (Magi's tooling, Helper, other automation) that branches on network_error / exit 1 to distinguish "transient, retry" from "operation-level timeout, back off"? A quick gh search code "network_error" org:heygen-com before merge would flag any that need updating in lockstep.
• Should translateCreateContextError's remaining logic (internal/client/executor.go:483-491) also apply on the plain path, or is the intentional design "only the --wait path massages context.Canceled → 'canceled' code, plain path leaves it as network_error"? Just want to confirm the asymmetry is deliberate.

What I didn't verify

• The exact net.Error implementer type returned by http.Client.Timeout in Go 1.25.12 (the toolchain this repo just bumped to). I trust the stdlib behavior — well-documented and stable across 1.13+ — but did not run a repro.
• Whether any downstream consumer of the CLI's structured error envelope asserts on the specific code: "network_error" value for a timeout on the plain path. If any do, they'll now see code: "timeout" and need to update. (Same class of concern as the exit-code flip, one layer up.)
• The precise interaction between retryTransport's idempotent-GET retry path and the http.Client.Timeout cancellation, under a real slow-server. I traced the code (retry attempts fail-fast on a canceled context) and it looks harmless, but no runtime confirmation.

Sibling context: #228 sets the per-op budgets (10m upload / 120s create / 30s other); this PR ensures that whichever budget trips, the CLI reports it consistently as timeout / exit 4 on both plain and --wait paths, with the mechanism now sourced from net.Error.Timeout() instead of the old error-message string-match. Together the stack turns a class of ~1% silent truncations into a clean, machine-branchable exit code — assuming the observability gap I flagged on #228 (no budget indicator in the error) gets addressed as a follow-up.

— Review by Rames D Jusso

[somanshreddy]

Thanks both — addressed in a858dd6:

• Duplicate-create risk (Somu): the timeout hint is now method-aware — GET → safe to retry; non-GET (create/write) → "check status before retrying, to avoid a duplicate" (CLI writes carry no idempotency key). Mirrors the poll-deadline guidance.
• Retry/executor detector mismatch (Rames): shouldRetry now also short-circuits on net.Error.Timeout(), so the retry layer and the classifier agree a client timeout isn't retryable.
• Observability (Rames): the timeout message now names the applied budget (...after 120s).
• Broader-than-server-slow hint (Rames): the GET hint now mentions a stalled network connection too.
• Breaking-change diligence (Rames): scanned heygen-com for consumers branching on the CLI's network_error/exit 1 — none key on it, so the exit 1→4 flip on the plain path breaks nothing known. Connection-refused → network_error stays covered by the existing TestExecute_NetworkError.

[somanshreddy]

Superseded by #232 — GitHub auto-closed this PR when its base (#228) merged and the base branch was deleted, and wouldn't let it reopen against the now-gone base. #232 is the same branch retargeted onto main, with all review feedback from this thread addressed (method-aware write-safe hint, budget in message, retry-layer alignment). Review discussion here remains the reference.