From 225de0fb1393b26da60193e871bce571bfd7f3ec Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 19:10:31 -0600 Subject: [PATCH 1/3] fix: idempotency key scoping, profile resolution, and unbounded paging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five bugs found by probing the binary against a local stub rather than by reading. Each fix has a regression test, and each test was checked by reverting the fix and confirming it fails. The idempotency one is the reason this branch exists. A key identifies an operation, but root.go minted one per invocation and pinned it on the context, so every write in a command shared it. `dns import` posts once per record; a three-record file went out as three different bodies under one key. If the API enforces keys on that endpoint as its own docs describe, record 1 is created and records 2..N return record 1's response, while the CLI counts each as a success and prints "Imported N record(s)" — silent data loss reported as completion. The editor now mints a key per outgoing write. Retries are unaffected: the editor runs once where the request is built and retryTransport replays that same request with its headers already set. Profile resolution fell through to the empty string when a config file had no top-level `default:` key, so f.Profiles[""] returned the zero value and the CLI reported "no credentials configured" — recommending `auth login`, which would have overwritten the working profile that `config list-profiles` was printing one command earlier. auth login writes the key, so only hand-edited files were affected, which is the only way to configure token_cmd. Paginated walks trusted the server to stop saying "there is more". Against one that kept answering nextPage:2, `dns list --all` never returned — 20 seconds of hammering at the full rate limit before it was killed. domain list was immune because it bounds on lastPage; nothing else did, including record-ID completion, where the symptom is a hung shell. cmdutil.NextPage now gates all ten walks on the page advancing and staying within lastPage. A 429 with Retry-After: 600 was slept on until the client timeout fired, converting an answer the server had already given into "context deadline exceeded ... while awaiting headers" at exit 1. The response is now returned when the wait cannot fit the budget, so it stays a 429 at exit 5, and APIError carries Retry-After so the hint can name it instead of advising the user to "wait a moment". Non-JSON error bodies were the error message verbatim, bounded only by the 1 MiB read limit: an nginx 502 page rendered as one 20 KB line. Also corrects CLAUDE.md, which claimed POSTs are retried when an idempotency key is set. They are not, deliberately, and transport_test.go pins it. --- CHANGELOG.md | 45 +++++++ cmd/cmdutil/args.go | 27 +++++ cmd/cmdutil/args_test.go | 58 +++++++++ cmd/cmdutil/complete.go | 5 +- cmd/contact/contact.go | 5 +- cmd/dns/dns.go | 5 +- cmd/email/email.go | 5 +- cmd/order/order.go | 5 +- cmd/root.go | 24 ++-- cmd/status.go | 14 ++- cmd/transfer/transfer.go | 5 +- cmd/url/url.go | 5 +- cmd/vanity/vanity.go | 5 +- internal/api/apierror.go | 44 ++++++- internal/api/apierror_test.go | 68 +++++++++++ internal/api/client.go | 38 ++++-- internal/api/idempotency_test.go | 106 +++++++++++++++++ internal/api/transport.go | 36 +++++- internal/api/transport_test.go | 197 +++++++++++++++++++++++++++---- internal/config/config.go | 40 ++++++- internal/config/config_test.go | 84 +++++++++++++ 21 files changed, 754 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd023a..2b4d114 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,51 @@ Releases before `0.2.0` predate this file. Their notes are on the ## [Unreleased] +### Fixed +- `dns import` sent the **same idempotency key on every record**. The key was + minted once per invocation, but an invocation can perform many operations — + import posts once per record — so a 50-record zone file went out under one + key. An API honouring keys as documented ("reusing the same key returns the + original result instead of repeating the operation") would create the first + record, echo it back for the other 49, and let the CLI report the whole file + as imported. Each write now gets its own key. `--idempotency-key` still pins + every write in an invocation to one value, which is what makes re-running a + failed command collapse onto the original. +- Credentials that existed were reported as missing. A config file with no + top-level `default:` key resolved to no profile at all, so `auth status` said + "no credentials configured — run 'namecom auth login'" (which would have + overwritten them) while `config list-profiles` printed the profile it was + refusing to use. A profile named `default` is now used without the key, as is + a lone profile under any name; two or more with no default is an error that + names them and suggests `--profile`. +- Seven list commands could page forever. Only `domain list` bounded its walk + with `lastPage`; the rest trusted the server to stop saying "there is more", + so one that kept answering `nextPage: 2` made `dns list --all` run + indefinitely at the full client rate limit. All paginated walks — including + record-ID shell completion and `namecom status` — now stop unless the page + number advances and stays within `lastPage`. +- A 429 carrying a long `Retry-After` was swallowed. The CLI slept on it until + the request deadline expired and then reported `context deadline exceeded + (Client.Timeout exceeded while awaiting headers)` with exit 1 — a transport + error, hiding the rate-limit answer the server had already given. A wait that + cannot fit the remaining budget is no longer taken: the 429 is returned as-is, + with exit 5 and a hint naming the wait the API asked for. A server-supplied + wait is also capped at 30s, matching the cap computed backoff always had. +- Error bodies that are not the API's JSON envelope are summarized instead of + echoed. A 502 HTML page from a proxy became a single 20 KB error message; it + is now collapsed to one line and truncated to 400 characters with the dropped + byte count disclosed. + +### Changed +- `--timeout` is described as the total budget for one API call including + retries, which is what it has always been (`http.Client.Timeout`), rather + than "per-request timeout". + +### Documentation +- `CLAUDE.md` claimed `transport.go` retries a POST when `X-Idempotency-Key` is + set. It never has: `idempotent()` covers GET/HEAD/PUT/DELETE only, and + `transport_test.go` pins that a key does not make a POST retryable on 5xx. + ### Added - `--wide` keeps every table column even when the table is wider than the terminal. diff --git a/cmd/cmdutil/args.go b/cmd/cmdutil/args.go index 7f31c1e..b5e9dfa 100644 --- a/cmd/cmdutil/args.go +++ b/cmd/cmdutil/args.go @@ -126,3 +126,30 @@ func suggestionHint(suggestions []string) string { } return b.String() } + +// NextPage decides whether a paginated walk continues, given the page just +// fetched and the nextPage/lastPage the API reported alongside it. +// +// The only stopping condition used to be `nextPage == nil || *nextPage == 0`, +// which trusts the server to eventually stop saying "there is more". A server +// that keeps answering `nextPage: 2` — a caching bug, a filter interaction, a +// proxy replaying a response — walked forever at the client's full rate limit. +// `domain list` escaped it by bounding on lastPage; the other seven list +// commands and record-ID completion did not, and `dns list --all` against such +// a server never returned. +// +// Two guards, both cheap: the page number must advance, and it must not run +// past lastPage when the API reports one. +func NextPage(current int32, nextPage, lastPage *int32) (int32, bool) { + if nextPage == nil || *nextPage == 0 { + return current, false + } + next := *nextPage + if next <= current { + return current, false + } + if lastPage != nil && *lastPage > 0 && next > *lastPage { + return current, false + } + return next, true +} diff --git a/cmd/cmdutil/args_test.go b/cmd/cmdutil/args_test.go index b243d6f..1a2e7c0 100644 --- a/cmd/cmdutil/args_test.go +++ b/cmd/cmdutil/args_test.go @@ -227,3 +227,61 @@ func TestGroupCmd(t *testing.T) { } }) } + +// TestNextPage guards the paginated-walk stopping conditions. +// +// The old condition was `nextPage == nil || *nextPage == 0` alone, which trusts +// the server to eventually stop saying "there is more". Against one that kept +// answering nextPage:2, `dns list --all` walked forever at the full client rate +// limit — confirmed by running it: still going after 20 seconds. domain list +// escaped only because it bounded on lastPage; the other seven list commands +// and record-ID completion did not. +func TestNextPage(t *testing.T) { + p := func(v int32) *int32 { return &v } + + tests := []struct { + name string + current int32 + next *int32 + last *int32 + wantPage int32 + wantOK bool + }{ + {"advances normally", 1, p(2), p(9), 2, true}, + {"nil next ends the walk", 3, nil, p(9), 3, false}, + {"zero next ends the walk", 3, p(0), p(9), 3, false}, + {"a next that repeats the current page ends the walk", 2, p(2), p(99), 2, false}, + {"a next that goes backwards ends the walk", 5, p(3), p(99), 5, false}, + {"a next beyond lastPage ends the walk", 9, p(10), p(9), 9, false}, + {"reaching lastPage exactly is allowed", 8, p(9), p(9), 9, true}, + {"an unknown lastPage still allows advancing", 1, p(2), nil, 2, true}, + {"a zero lastPage is treated as unknown", 1, p(2), p(0), 2, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotPage, gotOK := NextPage(tt.current, tt.next, tt.last) + if gotPage != tt.wantPage || gotOK != tt.wantOK { + t.Errorf("NextPage(%d, %v, %v) = (%d, %v), want (%d, %v)", + tt.current, tt.next, tt.last, gotPage, gotOK, tt.wantPage, tt.wantOK) + } + }) + } + + t.Run("a stuck server terminates the walk", func(t *testing.T) { + // The exact shape that hung: nextPage pinned at 2 forever. + page, steps := int32(1), 0 + for { + next, ok := NextPage(page, p(2), p(99)) + if !ok { + break + } + page = next + if steps++; steps > 100 { + t.Fatal("walk did not terminate against a non-advancing nextPage") + } + } + if steps != 1 { + t.Errorf("walk took %d steps, want 1 (page 1 -> 2, then stop)", steps) + } + }) +} diff --git a/cmd/cmdutil/complete.go b/cmd/cmdutil/complete.go index c4e8afb..b2ac1ad 100644 --- a/cmd/cmdutil/complete.go +++ b/cmd/cmdutil/complete.go @@ -67,10 +67,11 @@ func CompleteRecordIDs(cmd *cobra.Command, domain string) ([]string, cobra.Shell // "12345\tA @ → 1.2.3.4" — tab separates value from description in zsh/fish completions = append(completions, fmt.Sprintf("%s\t%s %s → %s", id, typ, host, answer)) } - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := NextPage(page, result.NextPage, result.LastPage) + if !ok { break } - page = *result.NextPage + page = next } return completions, cobra.ShellCompDirectiveNoFileComp } diff --git a/cmd/contact/contact.go b/cmd/contact/contact.go index 68178e9..2d5adba 100644 --- a/cmd/contact/contact.go +++ b/cmd/contact/contact.go @@ -110,7 +110,8 @@ func runUnverified(cmd *cobra.Command, _ []string) error { } contacts = append(contacts, result.UnverifiedContacts...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, &result.LastPage) + if !ok { break } // --quiet is for scripting, and the "showing first page" hint lives in @@ -121,7 +122,7 @@ func runUnverified(cmd *cobra.Command, _ []string) error { hasMore = true break } - page = *result.NextPage + page = next } spin.Stop() diff --git a/cmd/dns/dns.go b/cmd/dns/dns.go index 0554bba..f8a47ed 100644 --- a/cmd/dns/dns.go +++ b/cmd/dns/dns.go @@ -619,14 +619,15 @@ func fetchAllRecords(cmd *cobra.Command, domain string, all bool) (records []gen records = append(records, result.Records...) lastNextPage = result.NextPage - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !all { hasMore = true break } - page = *result.NextPage + page = next } if hasMore { nextPage = lastNextPage diff --git a/cmd/email/email.go b/cmd/email/email.go index 22c3173..d8f25f9 100644 --- a/cmd/email/email.go +++ b/cmd/email/email.go @@ -116,14 +116,15 @@ func runList(cmd *cobra.Command, args []string) error { } all = append(all, result.EmailForwarding...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !listAll { hasMore = true break } - page = *result.NextPage + page = next spin.Update(fmt.Sprintf("Fetching email forwardings… (page %d, %d so far)", page, len(all))) } spin.Stop() diff --git a/cmd/order/order.go b/cmd/order/order.go index 112b1f0..18b944b 100644 --- a/cmd/order/order.go +++ b/cmd/order/order.go @@ -136,14 +136,15 @@ func runList(cmd *cobra.Command, _ []string) error { } orders = append(orders, result.Orders...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !autoPage { hasMore = true break } - page = *result.NextPage + page = next spin.Update(fmt.Sprintf("Fetching orders… (page %d, %d so far)", page, len(orders))) } spin.Stop() diff --git a/cmd/root.go b/cmd/root.go index 72ad7e6..3ead81d 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -10,7 +10,6 @@ import ( "strings" "time" - "github.com/google/uuid" "github.com/patramsey/namecom-cli/cmd/apicmd" "github.com/patramsey/namecom-cli/cmd/cmdutil" configcmd "github.com/patramsey/namecom-cli/cmd/config" @@ -192,12 +191,12 @@ func init() { pf.BoolVar(&gf.noHeader, "no-header", false, "omit header row from table output") pf.BoolVar(&gf.wide, "wide", false, "keep every table column even if it overflows the terminal") pf.StringVar(&gf.color, "color", "auto", "colorize output: auto, always, never (env: NO_COLOR, CLICOLOR_FORCE)") - pf.DurationVar(&gf.timeout, "timeout", 30*time.Second, "per-request timeout") + pf.DurationVar(&gf.timeout, "timeout", 30*time.Second, "total time budget for one API call, retries included") pf.BoolVar(&gf.debug, "debug", false, "log HTTP requests/responses to stderr (token redacted)") pf.StringVar(&gf.debugFile, "debug-file", "", "log HTTP requests/responses to this file instead of stderr") pf.BoolVarP(&gf.yes, "yes", "y", false, "skip confirmation prompts") pf.BoolVar(&gf.dryRun, "dry-run", false, "for write operations, print the request instead of sending it (reads are unaffected)") - pf.StringVar(&gf.idempKey, "idempotency-key", "", "idempotency key for write operations (auto-generated per invocation if not set)") + pf.StringVar(&gf.idempKey, "idempotency-key", "", "pin every write in this invocation to one idempotency key (default: a fresh key per write)") pf.StringVar(&gf.baseURL, "base-url", "", "override the API base URL (for local stubs and proxies; credentials are sent to whatever you name)") // Apply styled help to every command in the tree. @@ -307,6 +306,15 @@ func initContext(cmd *cobra.Command) error { creds, err := config.Resolve(cfgFile, ov) if err != nil { if errors.Is(err, config.ErrNoCredentials) { + // Resolve adds context to this error when it can say something more + // specific than "nothing is configured" — several profiles exist + // but none is the default, say. Substituting the generic text threw + // that away and pointed the user at `auth login`, which overwrites. + //nolint:errorlint // identity, not chain: the bare sentinel means + // Resolve had nothing to add, so the friendlier text below applies. + if err != config.ErrNoCredentials { + return cmdutil.NewAuthError(err) + } if output.IsInteractive() { return cmdutil.NewAuthError(fmt.Errorf("no credentials configured — run 'namecom auth login' to set them up")) } @@ -358,11 +366,13 @@ func initContext(cmd *cobra.Command) error { // Stash everything on the context so subcommands can retrieve them via // the helpers below without threading parameters through every call. ctx := cmd.Context() - idempKey := gf.idempKey - if idempKey == "" { - idempKey = uuid.New().String() + // Only pin a key when the user named one. Left unpinned, the API client + // mints a fresh key per write, because one invocation can perform many + // operations — `dns import` posts once per record — and a shared key makes + // an API that honours it collapse them all onto the first. + if gf.idempKey != "" { + ctx = api.ContextWithIdempotencyKey(ctx, gf.idempKey) } - ctx = api.ContextWithIdempotencyKey(ctx, idempKey) ctx = context.WithValue(ctx, cmdutil.KeyClient, apiClient) ctx = context.WithValue(ctx, cmdutil.KeyConfig, cfgFile) ctx = context.WithValue(ctx, cmdutil.KeyOverrides, ov) diff --git a/cmd/status.go b/cmd/status.go index 790c3e7..23b0abc 100644 --- a/cmd/status.go +++ b/cmd/status.go @@ -120,10 +120,11 @@ func runStatus(cmd *cobra.Command, _ []string) error { return err } expiringDomains = append(expiringDomains, result.Domains...) - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(p, result.NextPage, result.LastPage) + if !ok { return nil } - p = *result.NextPage + p = next } }) @@ -158,11 +159,16 @@ func runStatus(cmd *cobra.Command, _ []string) error { return nil } transfers = append(transfers, tResult.Transfers...) - if tResult.NextPage == nil || *tResult.NextPage == 0 { + cur := int32(0) + if tPage != nil { + cur = *tPage + } + next, ok := cmdutil.NextPage(cur, tResult.NextPage, tResult.LastPage) + if !ok { transfersOK = true return nil } - tPage = tResult.NextPage + tPage = &next } }) diff --git a/cmd/transfer/transfer.go b/cmd/transfer/transfer.go index 86e31e1..89240cb 100644 --- a/cmd/transfer/transfer.go +++ b/cmd/transfer/transfer.go @@ -145,14 +145,15 @@ func runList(cmd *cobra.Command, _ []string) error { } transfers = append(transfers, result.Transfers...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !listAll { hasMore = true break } - page = *result.NextPage + page = next spin.Update(fmt.Sprintf("Fetching transfers… (page %d, %d so far)", page, len(transfers))) } spin.Stop() diff --git a/cmd/url/url.go b/cmd/url/url.go index cf0e938..c397762 100644 --- a/cmd/url/url.go +++ b/cmd/url/url.go @@ -133,14 +133,15 @@ func runList(cmd *cobra.Command, args []string) error { } all = append(all, result.UrlForwarding...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !listAll { hasMore = true break } - page = *result.NextPage + page = next spin.Update(fmt.Sprintf("Fetching URL forwardings… (page %d, %d so far)", page, len(all))) } spin.Stop() diff --git a/cmd/vanity/vanity.go b/cmd/vanity/vanity.go index fb6aff5..d6437cb 100644 --- a/cmd/vanity/vanity.go +++ b/cmd/vanity/vanity.go @@ -119,14 +119,15 @@ func runList(cmd *cobra.Command, args []string) error { } all = append(all, result.VanityNameservers...) lastResult = result - if result.NextPage == nil || *result.NextPage == 0 { + next, ok := cmdutil.NextPage(page, result.NextPage, result.LastPage) + if !ok { break } if !listAll { hasMore = true break } - page = *result.NextPage + page = next spin.Update(fmt.Sprintf("Fetching vanity nameservers… (page %d, %d so far)", page, len(all))) } spin.Stop() diff --git a/internal/api/apierror.go b/internal/api/apierror.go index dfdd76f..e85bb9b 100644 --- a/internal/api/apierror.go +++ b/internal/api/apierror.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "strings" + "time" ) // APIError is a normalized name.com API error. The API returns a consistent @@ -16,6 +17,9 @@ type APIError struct { StatusCode int Message string Details string + // RetryAfter carries the Retry-After header from a 429, when the server + // sent one. Zero means it did not. + RetryAfter time.Duration } func (e *APIError) Error() string { @@ -36,6 +40,11 @@ func (e *APIError) UserHint() string { case 404: return "the requested resource was not found — check the domain name or ID" case 429: + // Say how long when the server said so: "wait a moment" is misleading + // advice next to a Retry-After of ten minutes. + if e.RetryAfter > 0 { + return fmt.Sprintf("rate limited — the API asked to wait %s before retrying", e.RetryAfter.Round(time.Second)) + } return "rate limited — wait a moment and try again" } if e.StatusCode >= 500 { @@ -61,10 +70,7 @@ func ErrorFromResponse(statusCode int, body []byte) *APIError { e.Message = env.Message e.Details = env.Details } else { - e.Message = strings.TrimSpace(string(body)) - if e.Message == "" { - e.Message = http.StatusText(statusCode) - } + e.Message = summarizeBody(body, statusCode) } if statusCode == http.StatusUnauthorized { // Error() already renders details as "message (details)", so the note @@ -81,10 +87,38 @@ func ErrorFromResponse(statusCode int, body []byte) *APIError { return e } +// maxFallbackMessage bounds how much of a non-JSON error body becomes the +// error message. +const maxFallbackMessage = 400 + +// summarizeBody turns a body that is not the API's JSON envelope into a usable +// one-line message. +// +// It used to be passed through verbatim, capped only by the 1 MiB read limit. +// A proxy answering with an HTML error page therefore became the error text: a +// 502 from nginx rendered as a single 20 KB line of markup, in the terminal and +// inside the JSON error envelope alike. Collapse the whitespace, keep the +// front of it, and say how much was dropped. +func summarizeBody(body []byte, statusCode int) string { + msg := strings.Join(strings.Fields(string(body)), " ") + if msg == "" { + return http.StatusText(statusCode) + } + if len(msg) > maxFallbackMessage { + return fmt.Sprintf("%s… (%d bytes of non-JSON body truncated)", + strings.TrimSpace(msg[:maxFallbackMessage]), len(body)) + } + return msg +} + // parseError builds an APIError from a non-2xx response, reading and closing // the body. The caller should only invoke this for non-2xx responses. func parseError(resp *http.Response) *APIError { body, _ := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) _ = resp.Body.Close() - return ErrorFromResponse(resp.StatusCode, body) + e := ErrorFromResponse(resp.StatusCode, body) + if ra := parseRetryAfter(resp.Header.Get("Retry-After")); ra != nil { + e.RetryAfter = *ra + } + return e } diff --git a/internal/api/apierror_test.go b/internal/api/apierror_test.go index 9687868..7ce1d75 100644 --- a/internal/api/apierror_test.go +++ b/internal/api/apierror_test.go @@ -5,6 +5,7 @@ import ( "net/http" "strings" "testing" + "time" ) func makeResp(status int, body string) *http.Response { @@ -131,3 +132,70 @@ func TestAPIError_UnauthorizedNoteFormatting(t *testing.T) { } }) } + +// TestSummarizeBody covers error bodies that are not the API's JSON envelope. +// +// They used to become the error message verbatim, bounded only by parseError's +// 1 MiB read limit. A 502 HTML page from a proxy rendered as a single 20 KB +// line — in the terminal and inside the JSON error envelope alike. +func TestSummarizeBody(t *testing.T) { + t.Run("a long non-JSON body is truncated and counted", func(t *testing.T) { + html := "" + strings.Repeat("

nginx error page

", 800) + "" + e := ErrorFromResponse(502, []byte(html)) + if len(e.Message) > maxFallbackMessage+80 { + t.Errorf("message is %d chars, want it bounded near %d", len(e.Message), maxFallbackMessage) + } + if !strings.Contains(e.Message, "truncated") { + t.Errorf("truncation is not disclosed: %q", e.Message) + } + if !strings.Contains(e.Message, "") { + t.Errorf("the front of the body was dropped: %q", e.Message) + } + }) + + t.Run("newlines are collapsed so the message stays one line", func(t *testing.T) { + e := ErrorFromResponse(500, []byte("upstream\n connect\n\terror")) + if strings.ContainsAny(e.Message, "\n\t") { + t.Errorf("message spans lines: %q", e.Message) + } + if e.Message != "upstream connect error" { + t.Errorf("message = %q, want %q", e.Message, "upstream connect error") + } + }) + + t.Run("a short body is passed through", func(t *testing.T) { + if got := ErrorFromResponse(503, []byte("upstream down")).Message; got != "upstream down" { + t.Errorf("message = %q, want %q", got, "upstream down") + } + }) + + t.Run("an empty body falls back to the status text", func(t *testing.T) { + want := http.StatusText(http.StatusServiceUnavailable) + if got := ErrorFromResponse(http.StatusServiceUnavailable, nil).Message; got != want { + t.Errorf("message = %q, want %q", got, want) + } + }) + + t.Run("a proper JSON envelope is untouched", func(t *testing.T) { + e := ErrorFromResponse(422, []byte(`{"message":"bad ttl","details":"minimum is 300"}`)) + if e.Message != "bad ttl" || e.Details != "minimum is 300" { + t.Errorf("got %+v, want the envelope decoded", e) + } + }) +} + +// TestRetryAfterHint checks that a 429's hint reflects what the server asked +// for. "wait a moment and try again" is misleading next to a ten-minute +// Retry-After, and that combination is exactly what used to be swallowed +// entirely — slept on until the client timeout fired and reported as a +// transport error. +func TestRetryAfterHint(t *testing.T) { + long := &APIError{StatusCode: 429, Message: "slow down", RetryAfter: 10 * time.Minute} + if hint := long.UserHint(); !strings.Contains(hint, "10m") { + t.Errorf("hint = %q, want it to name the wait", hint) + } + bare := &APIError{StatusCode: 429, Message: "slow down"} + if hint := bare.UserHint(); !strings.Contains(hint, "wait a moment") { + t.Errorf("hint = %q, want the generic wording when no header was sent", hint) + } +} diff --git a/internal/api/client.go b/internal/api/client.go index b0f74f6..c77fb8b 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -14,21 +14,46 @@ import ( "net/http" "time" + "github.com/google/uuid" "github.com/patramsey/namecom-cli/internal/api/gen" "github.com/patramsey/namecom-cli/internal/config" "golang.org/x/time/rate" ) -// idempKeyCtxKey is the private context key for per-request idempotency keys. +// idempKeyCtxKey is the private context key for an explicitly-supplied +// idempotency key. type idempKeyCtxKey struct{} -// ContextWithIdempotencyKey attaches key to ctx; the Client's request editor -// will set X-Idempotency-Key on all write requests (POST/PUT/DELETE) that -// use this context. +// ContextWithIdempotencyKey pins every write request made with ctx to key. +// +// This is for --idempotency-key, where the user is naming a specific key to +// reuse — typically to make a retried invocation collapse onto an earlier one. +// Leave it unset and each write gets its own generated key instead; see +// idempotencyKeyFor. func ContextWithIdempotencyKey(ctx context.Context, key string) context.Context { return context.WithValue(ctx, idempKeyCtxKey{}, key) } +// idempotencyKeyFor returns the key to stamp on one outgoing write request. +// +// A pinned key from --idempotency-key wins. Otherwise every write gets a fresh +// one, because a key identifies an OPERATION, not an invocation. It used to be +// minted once per process and reused: `dns import` therefore sent one key +// across every record's POST, and an API honouring keys as documented — +// "reusing the same key returns the original result instead of repeating the +// operation" — would create the first record, echo it back for all the rest, +// and let the CLI report the whole file as imported. +// +// Retries stay safe. The editor runs once, where the generated client builds +// the request; retryTransport replays that same *http.Request with its headers +// already set, so every attempt at one operation carries one key. +func idempotencyKeyFor(ctx context.Context) string { + if pinned, _ := ctx.Value(idempKeyCtxKey{}).(string); pinned != "" { + return pinned + } + return uuid.NewString() +} + const ( prodBaseURL = "https://api.name.com" sandboxBaseURL = "https://api.dev.name.com" @@ -145,10 +170,9 @@ func New(opts Options) (*Client, error) { // Set() here silently overwrote a key the user passed explicitly — // meaning a retried write sent a different key each attempt and could // double-charge, which is precisely what the key exists to prevent. - if key, _ := ctx.Value(idempKeyCtxKey{}).(string); key != "" && - req.Header.Get("X-Idempotency-Key") == "" && + if req.Header.Get("X-Idempotency-Key") == "" && (req.Method == http.MethodPost || req.Method == http.MethodPut || req.Method == http.MethodDelete) { - req.Header.Set("X-Idempotency-Key", key) + req.Header.Set("X-Idempotency-Key", idempotencyKeyFor(ctx)) } return nil } diff --git a/internal/api/idempotency_test.go b/internal/api/idempotency_test.go index f2c8e54..bf685c4 100644 --- a/internal/api/idempotency_test.go +++ b/internal/api/idempotency_test.go @@ -73,3 +73,109 @@ func TestIdempotencyKeyFallsBackToContext(t *testing.T) { t.Errorf("context idempotency key should apply when caller supplies none, got %q", got) } } + +// TestIdempotencyKeyIsPerOperation guards the scoping of the generated key. +// +// The key was minted once per process and pinned on the context, so every +// write in one invocation carried the same one. `dns import` posts once per +// record, so a 50-record file went out under a single key — and an API +// honouring keys as documented ("reusing the same key returns the original +// result instead of repeating the operation") would create record 1, echo it +// back for the other 49, and let the CLI report the file as fully imported. +// +// A key identifies an OPERATION. Retries of one operation still share theirs: +// the editor runs once where the request is built, and retryTransport replays +// that same *http.Request with its headers already set. +func TestIdempotencyKeyIsPerOperation(t *testing.T) { + var keys []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + keys = append(keys, r.Header.Get("X-Idempotency-Key")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c, err := New(Options{BaseURL: srv.URL}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + + // Three record creations, the shape `dns import` produces. + ctx := context.Background() + for _, host := range []string{"a", "b", "c"} { + if _, err := c.Gen().CreateRecord(ctx, "example.com", gen.CreateRecordJSONRequestBody{ + Type: "A", Host: host, Answer: "1.2.3.4", + }); err != nil { + t.Fatalf("CreateRecord(%s): %v", host, err) + } + } + + if len(keys) != 3 { + t.Fatalf("server saw %d writes, want 3", len(keys)) + } + seen := map[string]bool{} + for i, k := range keys { + if k == "" { + t.Fatalf("write %d carried no idempotency key", i+1) + } + if seen[k] { + t.Errorf("key %q reused across separate operations: %v", k, keys) + } + seen[k] = true + } +} + +// TestIdempotencyKeyPinnedAcrossOperations pins --idempotency-key's behaviour: +// naming a key deliberately still applies it to every write, which is what +// makes a re-run of a failed invocation collapse onto the original. +func TestIdempotencyKeyPinnedAcrossOperations(t *testing.T) { + var keys []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + keys = append(keys, r.Header.Get("X-Idempotency-Key")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c, err := New(Options{BaseURL: srv.URL}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + + ctx := ContextWithIdempotencyKey(context.Background(), "PINNED-123") + for _, host := range []string{"a", "b"} { + if _, err := c.Gen().CreateRecord(ctx, "example.com", gen.CreateRecordJSONRequestBody{ + Type: "A", Host: host, Answer: "1.2.3.4", + }); err != nil { + t.Fatalf("CreateRecord(%s): %v", host, err) + } + } + for _, k := range keys { + if k != "PINNED-123" { + t.Errorf("key = %q, want the pinned value", k) + } + } +} + +// TestIdempotencyKeyAbsentOnReads pins that GETs stay clean — the header is a +// write-path concern and stamping it on reads would be noise. +func TestIdempotencyKeyAbsentOnReads(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("X-Idempotency-Key") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"records":[]}`)) + })) + defer srv.Close() + + c, err := New(Options{BaseURL: srv.URL}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + if _, err := c.Gen().ListRecords(context.Background(), "example.com", &gen.ListRecordsParams{}); err != nil { + t.Fatalf("ListRecords: %v", err) + } + if got != "" { + t.Errorf("a read carried an idempotency key: %q", got) + } +} diff --git a/internal/api/transport.go b/internal/api/transport.go index 04e3865..bbce749 100644 --- a/internal/api/transport.go +++ b/internal/api/transport.go @@ -145,6 +145,9 @@ func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) { if err != nil { if attempt < t.maxRetries && idempotent(req) && transientErr(err) { delay := t.backoffDelay(attempt, nil) + if !fitsDeadline(ctx, delay) { + return nil, err + } if t.onRetry != nil { t.onRetry(attempt+1, delay) } @@ -163,10 +166,20 @@ func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) { // 429 is always retryable; 5xx only for idempotent requests. if resp.StatusCode == http.StatusTooManyRequests || idempotent(req) { retryAfter := parseRetryAfter(resp.Header.Get("Retry-After")) + delay := t.backoffDelay(attempt, retryAfter) + // Hand back the response rather than sleeping past the + // deadline. A 429 carrying "Retry-After: 600" used to be slept + // on until the client timeout fired, turning a rate-limit + // answer the server had already given into "context deadline + // exceeded (Client.Timeout exceeded while awaiting headers)" — + // exit 1, a transport error, with the real cause discarded. + // Returning it keeps the status, the body, and exit code 5. + if !fitsDeadline(ctx, delay) { + return resp, nil + } // Drain and close so the connection can be reused. _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<20)) _ = resp.Body.Close() - delay := t.backoffDelay(attempt, retryAfter) if t.onRetry != nil { t.onRetry(attempt+1, delay) } @@ -185,13 +198,17 @@ func (t *retryTransport) RoundTrip(req *http.Request) (*http.Response, error) { // is non-nil it is honored; otherwise exponential backoff with jitter is used. func (t *retryTransport) backoffDelay(attempt int, retryAfter *time.Duration) time.Duration { if retryAfter != nil { - return *retryAfter + // Clamp it. Computed backoff has always been capped at maxBackoff, but + // a server-supplied value went through unbounded, so one header could + // park the process for as long as it liked. Where a deadline exists the + // caller stops before sleeping at all (see fitsDeadline); this bounds + // the case where there is none. + return min(*retryAfter, maxBackoff) } unit := t.baseDelay if unit == 0 { unit = time.Second } - const maxBackoff = 30 * time.Second base := min(unit< d +} + // sleep waits for d, returning false if ctx is cancelled first. func (t *retryTransport) sleep(ctx context.Context, d time.Duration) bool { timer := time.NewTimer(d) diff --git a/internal/api/transport_test.go b/internal/api/transport_test.go index 2c443b4..4fddce3 100644 --- a/internal/api/transport_test.go +++ b/internal/api/transport_test.go @@ -108,32 +108,77 @@ func TestNoRetryOn4xx(t *testing.T) { } func TestContextCancelStopsRetry(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Retry-After", "30") // long, so we cancel during backoff - w.WriteHeader(http.StatusTooManyRequests) - })) - defer srv.Close() + // Two ways a wait can be cut short, and they now end differently. + // + // When the wait plainly cannot fit the deadline, the transport no longer + // sleeps into cancellation — it hands back the response it already has, so + // a 429 stays a 429 instead of becoming "context deadline exceeded". When + // the wait does fit and the context is cancelled underneath it, the sleep + // must still abandon the backoff rather than run it to completion. + + t.Run("a wait that cannot fit returns the response, not a context error", func(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer srv.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL, nil) + start := time.Now() + resp, err := newTestClient(http.DefaultTransport).Do(req) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("got a transport error instead of the 429: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusTooManyRequests { + t.Errorf("status = %d, want the 429 preserved", resp.StatusCode) + } + if elapsed > 2*time.Second { + t.Errorf("did not short-circuit the backoff: took %v", elapsed) + } + if got := atomic.LoadInt32(&calls); got != 1 { + t.Errorf("calls = %d, want 1 — no retry fits in the budget", got) + } + }) - ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) - defer cancel() - req, _ := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL, nil) - start := time.Now() - _, err := newTestClient(http.DefaultTransport).Do(req) - elapsed := time.Since(start) - if err == nil { - t.Fatal("expected error from cancelled context") - } + t.Run("cancelling during an eligible backoff abandons it", func(t *testing.T) { + // No deadline, so the wait is eligible and the transport commits to + // sleeping. Checking only err != nil would pass even if sleep() ignored + // ctx entirely and blocked for the full schedule — verified: replacing + // the ctx.Done() select with a bare <-timer.C left this green while the + // call took 30s instead of 0.1s. The elapsed-time bound is the assertion + // that actually holds sleep() to account. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + })) + defer srv.Close() - // Assert the cancellation actually short-circuited the backoff. Checking - // only err != nil passes even if sleep() ignores ctx entirely and blocks for - // the full retry schedule — verified: replacing the ctx.Done() select with a - // bare <-timer.C left this green while the call took 30s instead of 0.1s. - if elapsed > 2*time.Second { - t.Errorf("cancellation did not interrupt the backoff: took %v", elapsed) - } - if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, context.Canceled) { - t.Errorf("expected a context error, got %v", err) - } + ctx, cancel := context.WithCancel(context.Background()) + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL, nil) + time.AfterFunc(100*time.Millisecond, cancel) + defer cancel() + + start := time.Now() + _, err := newTestClient(http.DefaultTransport).Do(req) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error from the cancelled context") + } + if elapsed > 2*time.Second { + t.Errorf("cancellation did not interrupt the backoff: took %v", elapsed) + } + if !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected a context error, got %v", err) + } + }) } func TestIdempotent(t *testing.T) { @@ -491,3 +536,107 @@ func TestRateLimiterPacesRequests(t *testing.T) { n, elapsed, minExpected) } } + +// TestRetryAfterDoesNotOutliveDeadline guards the 429 path against burning the +// caller's whole time budget on a wait the server asked for. +// +// A 429 carrying "Retry-After: 600" used to be slept on until the client +// timeout fired. The user waited the full 30s and got "context deadline +// exceeded (Client.Timeout exceeded while awaiting headers)" — exit 1, a +// transport error, with the rate-limit answer the server had already given +// thrown away. Handing the response back keeps the status, the body, and the +// exit code 5 that scripts branch on. +func TestRetryAfterDoesNotOutliveDeadline(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Retry-After", "600") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"slow down"}`)) + })) + defer srv.Close() + + tr := &retryTransport{ + base: http.DefaultTransport, + limiter: rate.NewLimiter(rate.Inf, 1), + maxRetries: 3, + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL, nil) + + start := time.Now() + resp, err := tr.RoundTrip(req) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("RoundTrip returned an error instead of the 429: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusTooManyRequests { + t.Errorf("status = %d, want 429 preserved", resp.StatusCode) + } + if elapsed > time.Second { + t.Errorf("took %s; it should return immediately rather than sleep into the deadline", elapsed) + } + if calls != 1 { + t.Errorf("server saw %d calls, want 1 — no retry fits in the budget", calls) + } + body, _ := io.ReadAll(resp.Body) + if !strings.Contains(string(body), "slow down") { + t.Errorf("response body was consumed: %q", body) + } +} + +// TestRetryAfterHonouredWhenItFits pins the other side: a short Retry-After +// inside the budget is still respected, so this fix did not disable the header. +func TestRetryAfterHonouredWhenItFits(t *testing.T) { + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + if calls == 1 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + tr := &retryTransport{ + base: http.DefaultTransport, + limiter: rate.NewLimiter(rate.Inf, 1), + maxRetries: 3, + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, srv.URL, nil) + + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d, want 200 after the retry", resp.StatusCode) + } + if calls != 2 { + t.Errorf("server saw %d calls, want 2", calls) + } +} + +// TestRetryAfterClamped pins the cap on a server-supplied wait for the case +// with no deadline at all. Computed backoff was always bounded by maxBackoff; +// the header went through unbounded, so one response could park the process +// for as long as it liked. +func TestRetryAfterClamped(t *testing.T) { + tr := &retryTransport{} + huge := 12 * time.Hour + if got := tr.backoffDelay(0, &huge); got != maxBackoff { + t.Errorf("backoffDelay with Retry-After 12h = %s, want it clamped to %s", got, maxBackoff) + } + short := 2 * time.Second + if got := tr.backoffDelay(0, &short); got != short { + t.Errorf("backoffDelay with Retry-After 2s = %s, want it honoured", got) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 02b2b13..8303181 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,7 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strconv" "strings" "time" @@ -216,7 +217,7 @@ func Resolve(f *File, ov Overrides) (Credentials, error) { } // Select the active profile name. - profileName := firstNonEmpty(ov.Profile, os.Getenv("NAMECOM_PROFILE"), f.Default) + profileName := firstNonEmpty(ov.Profile, os.Getenv("NAMECOM_PROFILE"), f.Default, impliedDefault(f)) prof := f.Profiles[profileName] // zero Profile if absent creds := Credentials{Profile: profileName} @@ -245,11 +246,48 @@ func Resolve(f *File, ov Overrides) (Credentials, error) { } if creds.Username == "" || creds.Token == "" { + // Distinguish "nothing is configured" from "several profiles exist and + // none is marked default" — the second needs a different fix, and the + // generic message sent people to `auth login`, which overwrites. + if profileName == "" && len(f.Profiles) > 1 { + names := make([]string, 0, len(f.Profiles)) + for n := range f.Profiles { + names = append(names, n) + } + sort.Strings(names) + return Credentials{}, fmt.Errorf( + "%w: %d profiles exist (%s) but none is the default — pass --profile, set NAMECOM_PROFILE, or run 'namecom config use '", + ErrNoCredentials, len(names), strings.Join(names, ", ")) + } return Credentials{}, ErrNoCredentials } return creds, nil } +// impliedDefault names the profile to use when nothing selected one: no +// --profile, no NAMECOM_PROFILE, and no top-level `default:` key in the file. +// +// Without it the name resolved to "", f.Profiles[""] returned the zero Profile, +// and the CLI reported "no credentials configured — run 'namecom auth login'" +// while sitting on a perfectly good profile that `config list-profiles` was +// happily printing. `auth login` writes the `default:` key, so this only bit +// hand-edited files — which is exactly how token_cmd has to be configured. +// +// A profile actually named "default" wins; failing that, a lone profile is +// unambiguous enough to use. Two or more unnamed candidates stay an error, +// because guessing between them is worse than saying so. +func impliedDefault(f *File) string { + if _, ok := f.Profiles["default"]; ok { + return "default" + } + if len(f.Profiles) == 1 { + for name := range f.Profiles { + return name + } + } + return "" +} + // tokenCmdTimeout bounds how long a credential helper may run. Generous enough // for an interactive unlock (biometric prompt, hardware key touch) but finite: // unbounded, a helper blocked on a locked vault or a prompt with no TTY hung diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 1bbd8fe..baa4e77 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "os" "path/filepath" "strings" @@ -505,3 +506,86 @@ profiles: } } } + +// TestResolveImpliedDefault covers config files that name no default profile. +// +// firstNonEmpty(flag, env, f.Default) resolved to "" when a file carried no +// top-level `default:` key, f.Profiles[""] returned the zero Profile, and the +// CLI reported "no credentials configured — run 'namecom auth login'" while +// `config list-profiles` was printing the profile it refused to use. auth login +// writes the key, so this only ever bit hand-edited files — which is the only +// way to configure token_cmd. +func TestResolveImpliedDefault(t *testing.T) { + t.Setenv("NAMECOM_PROFILE", "") + t.Setenv("NAMECOM_USERNAME", "") + t.Setenv("NAMECOM_TOKEN", "") + + t.Run("a profile named default is used without the default key", func(t *testing.T) { + f := &File{Profiles: map[string]Profile{ + "default": {Username: "alice", Token: "aaa"}, + }} + creds, err := Resolve(f, Overrides{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if creds.Username != "alice" || creds.Profile != "default" { + t.Errorf("got %+v, want alice via the 'default' profile", creds) + } + }) + + t.Run("a lone profile is used whatever it is called", func(t *testing.T) { + f := &File{Profiles: map[string]Profile{ + "work": {Username: "bob", Token: "bbb"}, + }} + creds, err := Resolve(f, Overrides{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if creds.Username != "bob" || creds.Profile != "work" { + t.Errorf("got %+v, want bob via the 'work' profile", creds) + } + }) + + t.Run("the default key still wins over the implied one", func(t *testing.T) { + f := &File{Default: "work", Profiles: map[string]Profile{ + "default": {Username: "alice", Token: "aaa"}, + "work": {Username: "bob", Token: "bbb"}, + }} + creds, err := Resolve(f, Overrides{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if creds.Profile != "work" { + t.Errorf("profile = %q, want work", creds.Profile) + } + }) + + t.Run("ambiguity is an error that names the candidates", func(t *testing.T) { + f := &File{Profiles: map[string]Profile{ + "work": {Username: "bob", Token: "bbb"}, + "home": {Username: "eve", Token: "eee"}, + }} + _, err := Resolve(f, Overrides{}) + if err == nil { + t.Fatal("two profiles and no default resolved to credentials") + } + if !errors.Is(err, ErrNoCredentials) { + t.Errorf("error does not wrap ErrNoCredentials: %v", err) + } + for _, want := range []string{"home", "work", "--profile"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error does not mention %q: %v", want, err) + } + } + }) + + t.Run("an empty file is still the plain sentinel", func(t *testing.T) { + // root.go swaps in friendlier wording for exactly this case, keyed on + // the error being the bare sentinel rather than a wrapped one. + _, err := Resolve(&File{Profiles: map[string]Profile{}}, Overrides{}) + //nolint:errorlint // identity is the property under test + if err != ErrNoCredentials { + t.Errorf("got %v, want the bare ErrNoCredentials sentinel", err) + } + }) +} From a0e61bbb3a0767cfc145c2174e43ad1c182c7272 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 19:17:35 -0600 Subject: [PATCH 2/3] test: cover Retry-After capture in parseError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseError was the one piece of new logic in this branch without direct coverage. Nothing else populates APIError.RetryAfter, and UserHint falls back to the generic "wait a moment" wording when it is zero — so a regression here would produce a hint that still reads correctly while having lost the number the API supplied. Covers the delta-seconds form, an absent header, and the HTTP-date form the parser deliberately does not accept. --- internal/api/apierror_test.go | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/internal/api/apierror_test.go b/internal/api/apierror_test.go index 7ce1d75..d69b971 100644 --- a/internal/api/apierror_test.go +++ b/internal/api/apierror_test.go @@ -199,3 +199,44 @@ func TestRetryAfterHint(t *testing.T) { t.Errorf("hint = %q, want the generic wording when no header was sent", hint) } } + +// TestParseErrorCapturesRetryAfter pins that the header survives the trip from +// response to APIError. UserHint reads RetryAfter to say how long the API asked +// for, and nothing else populates it — a hint that silently fell back to "wait +// a moment" would look correct while having lost the number. +func TestParseErrorCapturesRetryAfter(t *testing.T) { + withHeader := func(status int, body, retryAfter string) *http.Response { + resp := makeResp(status, body) + if resp.Header == nil { + resp.Header = http.Header{} + } + if retryAfter != "" { + resp.Header.Set("Retry-After", retryAfter) + } + return resp + } + + t.Run("a delta-seconds header is captured", func(t *testing.T) { + e := parseError(withHeader(http.StatusTooManyRequests, `{"message":"slow down"}`, "600")) + if e.RetryAfter != 10*time.Minute { + t.Errorf("RetryAfter = %s, want 10m", e.RetryAfter) + } + if !strings.Contains(e.UserHint(), "10m") { + t.Errorf("hint = %q, want it to name the wait", e.UserHint()) + } + }) + + t.Run("no header leaves it zero", func(t *testing.T) { + e := parseError(withHeader(http.StatusTooManyRequests, `{"message":"slow down"}`, "")) + if e.RetryAfter != 0 { + t.Errorf("RetryAfter = %s, want zero", e.RetryAfter) + } + }) + + t.Run("an unparseable header leaves it zero", func(t *testing.T) { + e := parseError(withHeader(http.StatusTooManyRequests, `{"message":"slow down"}`, "Wed, 21 Oct 2026 07:28:00 GMT")) + if e.RetryAfter != 0 { + t.Errorf("RetryAfter = %s, want zero for the HTTP-date form", e.RetryAfter) + } + }) +} From 9e1fcf52d495a0c8293a3e44959d24575f904f45 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 19:41:27 -0600 Subject: [PATCH 3/3] test: cover the rewritten pagination call sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codecov flagged the patch, and most of what it listed is call-site plumbing not worth a harness. Four entries were not: email, transfer, and vanity-ns had no multi-page test at all, and record-ID completion had no test at all. That matters here specifically. Ten pagination loops were rewritten to route through cmdutil.NextPage, and nine of them by a regex. The continuation line is exactly what a mechanical rewrite gets wrong, and with no multi-page fixture a loop that stopped after page 1 — or never advanced — would have looked green. Verified by mutation: pinning `page = 1` in each of the four now fails its test. Each gets both halves of the behaviour: a two-page walk asserting every item arrives and that the pages were not aliased, and a server whose nextPage never advances, asserting the walk stops after two requests. For completion the second case is the interesting one, since the symptom there is a shell hung mid-tab with nothing on screen to explain it. Also covers the deadline check on the transport's network-error path, the sibling of the one already covered on the status path. One test case here was wrong on the first pass and is worth noting rather than quietly fixing: it called CompleteRecordIDs with a bare &cobra.Command{} to check the no-client path, which panicked on a nil context. Cobra never produces that — ExecuteC always sets one — so the panic was the test's fault, not a bug. The case now uses a context with no client on it, which is the state root.go actually leaves for __complete when credentials are absent. --- cmd/cmdutil/complete_test.go | 121 +++++++++++++++++++++++++++++++++ cmd/email/email_test.go | 94 +++++++++++++++++++++++++ cmd/transfer/transfer_test.go | 94 +++++++++++++++++++++++++ cmd/vanity/vanity_test.go | 96 ++++++++++++++++++++++++++ internal/api/transport_test.go | 37 ++++++++++ 5 files changed, 442 insertions(+) create mode 100644 cmd/cmdutil/complete_test.go diff --git a/cmd/cmdutil/complete_test.go b/cmd/cmdutil/complete_test.go new file mode 100644 index 0000000..d0e74c3 --- /dev/null +++ b/cmd/cmdutil/complete_test.go @@ -0,0 +1,121 @@ +package cmdutil + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/patramsey/namecom-cli/internal/api" + "github.com/spf13/cobra" +) + +// cmdWithClient returns a command carrying an API client pointed at srv, the +// shape completion functions expect to find on the context. +func cmdWithClient(t *testing.T, srv *httptest.Server) *cobra.Command { + t.Helper() + client, err := api.New(api.Options{BaseURL: srv.URL}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + cmd := &cobra.Command{} + cmd.SetContext(context.WithValue(context.Background(), KeyClient, client)) + return cmd +} + +// TestCompleteRecordIDs covers record-ID completion, which had no test at all. +// +// It pages, so it carried the same unbounded-walk bug as the list commands — +// and here the symptom is worse than a hung command: a server whose nextPage +// never advances hangs the user's shell mid-tab-completion, with no output to +// explain why and nothing obvious to interrupt. +func TestCompleteRecordIDs(t *testing.T) { + t.Run("returns ids with a descriptive suffix", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"records":[{"id":42,"host":"www","type":"A","answer":"1.2.3.4","ttl":300}]}`)) + })) + t.Cleanup(srv.Close) + + got, directive := CompleteRecordIDs(cmdWithClient(t, srv), "example.com") + if directive != cobra.ShellCompDirectiveNoFileComp { + t.Errorf("directive = %v, want NoFileComp", directive) + } + if len(got) != 1 { + t.Fatalf("got %d completions, want 1: %v", len(got), got) + } + // zsh and fish split value from description on the tab, so the ID must + // come first and the context after it. + id, desc, found := strings.Cut(got[0], "\t") + if !found { + t.Fatalf("completion %q has no tab separator", got[0]) + } + if id != "42" { + t.Errorf("completion value = %q, want the bare record ID %q", id, "42") + } + for _, want := range []string{"A", "www", "1.2.3.4"} { + if !strings.Contains(desc, want) { + t.Errorf("description %q does not mention %q", desc, want) + } + } + }) + + t.Run("walks every page", func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`{"records":[{"id":2,"host":"b","type":"A","answer":"2.2.2.2","ttl":300}],"lastPage":2}`)) + return + } + _, _ = w.Write([]byte(`{"records":[{"id":1,"host":"a","type":"A","answer":"1.1.1.1","ttl":300}],"nextPage":2,"lastPage":2}`)) + })) + t.Cleanup(srv.Close) + + got, _ := CompleteRecordIDs(cmdWithClient(t, srv), "example.com") + if len(got) != 2 { + t.Fatalf("got %d completions across 2 pages, want 2: %v", len(got), got) + } + if requests != 2 { + t.Errorf("made %d page requests, want exactly 2", requests) + } + }) + + t.Run("a non-advancing nextPage does not hang the shell", func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 10 { + t.Errorf("completion did not terminate against a non-advancing nextPage") + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"records":[{"id":1,"host":"a","type":"A","answer":"1.1.1.1","ttl":300}],"nextPage":2,"lastPage":99}`)) + })) + t.Cleanup(srv.Close) + + CompleteRecordIDs(cmdWithClient(t, srv), "example.com") + if requests != 2 { + t.Errorf("made %d requests, want 2 (page 1 -> 2, then the page stops advancing)", requests) + } + }) + + t.Run("no client on the context degrades quietly", func(t *testing.T) { + // Completion runs before credentials necessarily exist: root.go lets + // initContext fail silently for __complete rather than break the shell, + // which leaves a context with no client on it. Offering nothing is the + // correct answer; erroring would surface in the middle of a tab. + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + got, directive := CompleteRecordIDs(cmd, "example.com") + if got != nil { + t.Errorf("got %v, want no completions", got) + } + if directive != cobra.ShellCompDirectiveNoFileComp { + t.Errorf("directive = %v, want NoFileComp", directive) + } + }) +} diff --git a/cmd/email/email_test.go b/cmd/email/email_test.go index 4305a42..5122154 100644 --- a/cmd/email/email_test.go +++ b/cmd/email/email_test.go @@ -398,3 +398,97 @@ func TestEmailDelete_DeletesEntry(t *testing.T) { t.Errorf("unexpected delete path: %q", deletePath) } } + +// TestEmailList_PagesToTheEnd covers the --all walk across more than one page. +// +// There was no multi-page test here at all, which mattered when every list +// loop was rewritten to go through cmdutil.NextPage: the continuation line is +// the one a mechanical rewrite gets wrong, and nothing would have noticed a +// loop that stopped after page 1 or one that never advanced. +func TestEmailList_PagesToTheEnd(t *testing.T) { + const maxRequests = 8 // a correct implementation needs exactly 2 + var requests int + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > maxRequests { + t.Errorf("pagination did not terminate: %d requests", requests) + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + // Final page: nextPage omitted, as the spec describes. + _, _ = w.Write([]byte(`{"emailForwarding":[{"domainName":"example.com","emailBox":"bbb","emailTo":"b@example.com"}],"lastPage":2}`)) + return + } + _, _ = w.Write([]byte(`{"emailForwarding":[{"domainName":"example.com","emailBox":"aaa","emailTo":"a@example.com"}],"nextPage":2,"lastPage":2}`)) + })) + t.Cleanup(srv.Close) + + cmd := cmdForEmailList(t, srv) + out := cmdutil.Out(cmd) + out.Format = output.FormatJSON + listAll = true + + if err := runList(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d page requests, want exactly 2", requests) + } + + buf, ok := out.Writer.(*bytes.Buffer) + if !ok { + t.Fatal("output writer is not a *bytes.Buffer") + } + var env struct { + Data []struct { + EmailBox string `json:"emailBox"` + EmailTo string `json:"emailTo"` + } `json:"data"` + } + if err := json.Unmarshal(buf.Bytes(), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, buf.String()) + } + if len(env.Data) != 2 { + t.Fatalf("got %d forwardings across 2 pages, want 2: %s", len(env.Data), buf.String()) + } + // Pairing box to target catches page 2 overwriting page 1's backing array, + // which is the aliasing bug this shape of loop has produced before. + want := map[string]string{"aaa": "a@example.com", "bbb": "b@example.com"} + for _, e := range env.Data { + if want[e.EmailBox] != e.EmailTo { + t.Errorf("box %q forwards to %q, want %q — pages were aliased", e.EmailBox, e.EmailTo, want[e.EmailBox]) + } + } +} + +// TestEmailList_StuckNextPageTerminates covers the guard itself: a server that +// keeps answering nextPage:2 used to make --all walk forever at the full client +// rate limit. Confirmed against a stub before the fix — still running after 20s. +func TestEmailList_StuckNextPageTerminates(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 10 { + t.Errorf("walk did not terminate against a non-advancing nextPage") + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"emailForwarding":[{"domainName":"example.com","emailBox":"aaa","emailTo":"a@example.com"}],"nextPage":2,"lastPage":99}`)) + })) + t.Cleanup(srv.Close) + + cmd := cmdForEmailList(t, srv) + cmdutil.Out(cmd).Format = output.FormatJSON + listAll = true + + if err := runList(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d requests, want 2 (page 1 -> 2, then the page stops advancing)", requests) + } +} diff --git a/cmd/transfer/transfer_test.go b/cmd/transfer/transfer_test.go index 74eec25..c97e229 100644 --- a/cmd/transfer/transfer_test.go +++ b/cmd/transfer/transfer_test.go @@ -3,6 +3,7 @@ package transfer import ( "bytes" "context" + "encoding/json" "io" "net/http" "net/http/httptest" @@ -839,3 +840,96 @@ func TestTransferEligibility_APIError(t *testing.T) { t.Errorf("error should surface the API message, got: %v", err) } } + +// TestTransferList_PagesToTheEnd covers the --all walk across more than one +// page, and the guard that stops it against a server whose nextPage never +// advances. +// +// There was no multi-page test here, which mattered when every list loop was +// rewritten to route through cmdutil.NextPage: the continuation line is what a +// mechanical rewrite gets wrong, and nothing would have caught a loop that +// stopped after page 1 or one that never advanced at all. +func TestTransferList_PagesToTheEnd(t *testing.T) { + t.Run("walks every page", func(t *testing.T) { + const maxRequests = 8 // a correct implementation needs exactly 2 + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > maxRequests { + t.Errorf("pagination did not terminate: %d requests", requests) + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`{"transfers":[{"domainName":"two.com","status":"completed"}],"lastPage":2}`)) + return + } + _, _ = w.Write([]byte(`{"transfers":[{"domainName":"one.com","status":"pending"}],"nextPage":2,"lastPage":2}`)) + })) + t.Cleanup(srv.Close) + + cmd := cmdForTransferList(t, srv) + out := cmdutil.Out(cmd) + out.Format = output.FormatJSON + listAll = true + + if err := runList(cmd, nil); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d page requests, want exactly 2", requests) + } + + buf, ok := out.Writer.(*bytes.Buffer) + if !ok { + t.Fatal("output writer is not a *bytes.Buffer") + } + var env struct { + Data []struct { + DomainName string `json:"domainName"` + Status string `json:"status"` + } `json:"data"` + } + if err := json.Unmarshal(buf.Bytes(), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, buf.String()) + } + if len(env.Data) != 2 { + t.Fatalf("got %d transfers across 2 pages, want 2: %s", len(env.Data), buf.String()) + } + // Pairing name to status catches page 2 overwriting page 1's backing + // array, the aliasing failure this loop shape has produced before. + want := map[string]string{"one.com": "pending", "two.com": "completed"} + for _, e := range env.Data { + if want[e.DomainName] != e.Status { + t.Errorf("%s has status %q, want %q — pages were aliased", e.DomainName, e.Status, want[e.DomainName]) + } + } + }) + + t.Run("a non-advancing nextPage terminates the walk", func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 10 { + t.Errorf("walk did not terminate against a non-advancing nextPage") + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"transfers":[{"domainName":"one.com","status":"pending"}],"nextPage":2,"lastPage":99}`)) + })) + t.Cleanup(srv.Close) + + cmd := cmdForTransferList(t, srv) + cmdutil.Out(cmd).Format = output.FormatJSON + listAll = true + + if err := runList(cmd, nil); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d requests, want 2 (page 1 -> 2, then the page stops advancing)", requests) + } + }) +} diff --git a/cmd/vanity/vanity_test.go b/cmd/vanity/vanity_test.go index aae7639..c27ca4d 100644 --- a/cmd/vanity/vanity_test.go +++ b/cmd/vanity/vanity_test.go @@ -346,3 +346,99 @@ func TestSplitIPs(t *testing.T) { }) } } + +// TestVanityList_PagesToTheEnd covers the --all walk across more than one page, +// and the guard that stops it when nextPage never advances. +// +// There was no multi-page test here, which mattered when every list loop was +// rewritten to route through cmdutil.NextPage: the continuation line is what a +// mechanical rewrite gets wrong, and nothing would have caught a loop that +// stopped after page 1 or one that never advanced at all. +func TestVanityList_PagesToTheEnd(t *testing.T) { + t.Run("walks every page", func(t *testing.T) { + const maxRequests = 8 // a correct implementation needs exactly 2 + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > maxRequests { + t.Errorf("pagination did not terminate: %d requests", requests) + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`{"vanityNameservers":[{"domainName":"example.com","hostname":"ns2.example.com","ips":["2.2.2.2"]}],"lastPage":2}`)) + return + } + _, _ = w.Write([]byte(`{"vanityNameservers":[{"domainName":"example.com","hostname":"ns1.example.com","ips":["1.1.1.1"]}],"nextPage":2,"lastPage":2}`)) + })) + t.Cleanup(srv.Close) + + cmd := baseCmd(t, srv) + cmd.Flags().BoolVar(&listAll, "all", false, "") + t.Cleanup(func() { listAll = false }) + listAll = true + out := cmdutil.Out(cmd) + out.Format = output.FormatJSON + + if err := runList(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d page requests, want exactly 2", requests) + } + + buf, ok := out.Writer.(*bytes.Buffer) + if !ok { + t.Fatal("output writer is not a *bytes.Buffer") + } + var env struct { + Data []struct { + Hostname string `json:"hostname"` + Ips []string `json:"ips"` + } `json:"data"` + } + if err := json.Unmarshal(buf.Bytes(), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, buf.String()) + } + if len(env.Data) != 2 { + t.Fatalf("got %d nameservers across 2 pages, want 2: %s", len(env.Data), buf.String()) + } + // Pairing hostname to its glue IP catches page 2 overwriting page 1's + // backing array, the aliasing failure this loop shape has produced before. + want := map[string]string{"ns1.example.com": "1.1.1.1", "ns2.example.com": "2.2.2.2"} + for _, e := range env.Data { + if len(e.Ips) != 1 || want[e.Hostname] != e.Ips[0] { + t.Errorf("%s has ips %v, want [%s] — pages were aliased", e.Hostname, e.Ips, want[e.Hostname]) + } + } + }) + + t.Run("a non-advancing nextPage terminates the walk", func(t *testing.T) { + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + if requests > 10 { + t.Errorf("walk did not terminate against a non-advancing nextPage") + http.Error(w, "loop", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"vanityNameservers":[{"domainName":"example.com","hostname":"ns1.example.com","ips":["1.1.1.1"]}],"nextPage":2,"lastPage":99}`)) + })) + t.Cleanup(srv.Close) + + cmd := baseCmd(t, srv) + cmd.Flags().BoolVar(&listAll, "all", false, "") + t.Cleanup(func() { listAll = false }) + listAll = true + cmdutil.Out(cmd).Format = output.FormatJSON + + if err := runList(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runList: %v", err) + } + if requests != 2 { + t.Errorf("made %d requests, want 2 (page 1 -> 2, then the page stops advancing)", requests) + } + }) +} diff --git a/internal/api/transport_test.go b/internal/api/transport_test.go index 4fddce3..91f5c1d 100644 --- a/internal/api/transport_test.go +++ b/internal/api/transport_test.go @@ -640,3 +640,40 @@ func TestRetryAfterClamped(t *testing.T) { t.Errorf("backoffDelay with Retry-After 2s = %s, want it honoured", got) } } + +// TestNetworkRetryRespectsDeadline covers the deadline check on the +// network-error path, the sibling of the one on the status path. +// +// Retrying a connection failure is only worth doing if there is time left to +// make the attempt. Sleeping through the remaining budget first turns a +// concrete "connection refused" into a context error that says nothing about +// why the call failed. +func TestNetworkRetryRespectsDeadline(t *testing.T) { + // A port with nothing listening: RoundTrip fails immediately and the error + // is transient by the transport's classification, so a retry is eligible. + tr := &retryTransport{ + base: http.DefaultTransport, + limiter: rate.NewLimiter(rate.Inf, 1), + maxRetries: 3, + baseDelay: 10 * time.Second, // far longer than the budget below + } + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, "http://127.0.0.1:1", nil) + + start := time.Now() + _, err := tr.RoundTrip(req) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected the connection error to be returned") + } + if elapsed > time.Second { + t.Errorf("took %s; the wait does not fit the deadline and should not be taken", elapsed) + } + // The underlying failure must survive rather than being replaced by a + // context error produced by our own sleep. + if errors.Is(err, context.DeadlineExceeded) { + t.Errorf("connection error was masked by our own backoff: %v", err) + } +}