From 72e1c7c6307a4e1c6fe8d43e5add9ec15c612691 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 21:15:31 -0600 Subject: [PATCH 1/5] spike: evaluate the Core SDK against the dns command group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Throwaway evaluation for #40. internal/sdkspike is not imported by cmd/ and is meant to be deleted once the decision is made either way. Nothing existing is changed; the generated client, the spec, and the preprocessor all stay. The package ports the dns read and read-modify-write paths onto github.com/namedotcom/core-api-go v1.33.2, wired the way the evaluation recommends — the SDK for types, our own transport for rate limiting, retry policy, and the deadline guard from #41 — and the tests answer the three questions the spike was scoped to settle. The headline result is a defect, not a design question. option.WithoutRetries() is silently ignored at client scope: internal/retrier.go's NewRetrier reads only `attempts` out of the options it is handed and discards `disabled`, so a client built with the option still gets the default two attempts. Run() honours `disabled` only from per-call options. Measured: two requests against a 500 with the option set on the client, one with it set per call. That matters because it is a safety setting, and client scope is where a safety setting most needs to hold — the fallback is carrying the option on all ~53 call sites, where anything added later that forgets it silently regains the behaviour. The behaviour it guards is real: the SDK retries a POST on a 5xx (shouldRetry dispatches on status code alone, no method check), which is what idempotent() refuses to do, and the API honours idempotency keys on only five operations. Its backoff also sleeps with a bare time.Sleep — a call with a 100ms deadline took 2s against a Retry-After of 2 — so --timeout would stop being enforceable during backoff. Two claims from the issue body are corrected by the port. The generated-type gotchas do NOT disappear: Record.TTL is a value while the update body's is a pointer, and Record.Type is a *string while the body's is a named enum needing a cast. Both come from the spec, not from oapi-codegen, so both survive. Host is additionally a string on the create body and a *string on the update body. And cmdutil.NextPage does not port unchanged — the SDK reports page numbers as *int where the generated client used *int32, so every paginated walk needs the same re-typing. What does hold: coverage of all 53 operations the CLI calls, auth byte-identical to what client.go builds today, and the http.Client we supply surviving into every request — the last one asserted explicitly, since it is the assumption the whole recommendation rests on and it depends on a fallback in caller.Call that the retry flag did not get. --- go.mod | 1 + go.sum | 2 + internal/sdkspike/spike.go | 165 ++++++++++++++++ internal/sdkspike/spike_test.go | 336 ++++++++++++++++++++++++++++++++ 4 files changed, 504 insertions(+) create mode 100644 internal/sdkspike/spike.go create mode 100644 internal/sdkspike/spike_test.go diff --git a/go.mod b/go.mod index 31fcbb3..9252ead 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/charmbracelet/huh v1.0.0 github.com/charmbracelet/lipgloss v1.1.0 github.com/google/uuid v1.6.0 + github.com/namedotcom/core-api-go v1.33.2 github.com/oapi-codegen/runtime v1.6.0 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 diff --git a/go.sum b/go.sum index 3bb898b..8acc467 100644 --- a/go.sum +++ b/go.sum @@ -124,6 +124,8 @@ github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELU github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= +github.com/namedotcom/core-api-go v1.33.2 h1:FhWPjFATqW37rYaCLtiNiG2WdTHIre3VFAoKggjeJ8k= +github.com/namedotcom/core-api-go v1.33.2/go.mod h1:0rjYfuJejSP5M0a44t4kColJcxPlCWDdE2wsN8K31cg= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE= github.com/nxadm/tail v1.4.8/go.mod h1:+ncqLTQzXmGhMZNUePPaPqPvBxHAIsmXswZKocGu+AU= diff --git a/internal/sdkspike/spike.go b/internal/sdkspike/spike.go new file mode 100644 index 0000000..7052bb4 --- /dev/null +++ b/internal/sdkspike/spike.go @@ -0,0 +1,165 @@ +// Package sdkspike is a throwaway evaluation of github.com/namedotcom/core-api-go +// against the `dns` command group. See issue #40. +// +// It exists to answer three questions with running code rather than argument, +// and it is wired the way the evaluation recommends: the SDK supplies the typed +// client, our own transport keeps supplying rate limiting, retry policy, and +// the deadline guard from #41. +// +// Nothing here is imported by cmd/. Delete the package once the decision on #40 +// is made either way. +package sdkspike + +import ( + "context" + "fmt" + "net/http" + + coreapigo "github.com/namedotcom/core-api-go" + sdk "github.com/namedotcom/core-api-go/client" + "github.com/namedotcom/core-api-go/option" +) + +// New builds an SDK client wired the way the migration would wire it. +// +// httpClient is ours — internal/api builds it with the rate limiter and +// retryTransport inside. WithoutRetries turns the SDK's own retry layer off, +// which is the crux of the recommendation: the SDK's retrier retries POSTs on +// 5xx (it dispatches on status code alone) and sleeps with a bare time.Sleep +// that ignores context. Ours refuses POST retries and returns the response +// rather than sleeping past the deadline. Only one of the two may be live. +func New(baseURL, username, token string, httpClient *http.Client) *sdk.Namecom { + return sdk.NewNamecom( + option.WithBaseURL(baseURL), + option.WithBasicAuth(username, token), + option.WithHTTPClient(httpClient), + option.WithoutRetries(), + ) +} + +// NewWithSDKRetries is the same client with the SDK's retry layer left ON. +// Used only to demonstrate what that layer does; not a supported wiring. +func NewWithSDKRetries(baseURL, username, token string, httpClient *http.Client) *sdk.Namecom { + return sdk.NewNamecom( + option.WithBaseURL(baseURL), + option.WithBasicAuth(username, token), + option.WithHTTPClient(httpClient), + ) +} + +// ListAllRecords walks every page of a zone. +// +// The point of porting this one is the pagination guard from #41. The SDK +// reports nextPage/lastPage as *int where the generated client used *int32, so +// cmdutil.NextPage's signature does not carry over unchanged — this is the +// int-width adaptation the migration needs, isolated so it can be judged. +func ListAllRecords(ctx context.Context, c *sdk.Namecom, domain string) ([]*coreapigo.Record, int, error) { + var ( + all []*coreapigo.Record + page = 1 + requests int + ) + for { + p := page + resp, err := c.DNS.ListRecords(ctx, &coreapigo.ListRecordsRequest{ + DomainName: domain, + Page: &p, + }) + requests++ + if err != nil { + return nil, requests, fmt.Errorf("listing records for %s: %w", domain, err) + } + all = append(all, resp.Records...) + + next, ok := nextPage(page, resp.NextPage, resp.LastPage) + if !ok { + return all, requests, nil + } + page = next + } +} + +// nextPage is cmdutil.NextPage over int rather than int32. +// +// That it had to be re-typed at all is the finding: the guard is not portable +// as written, and every paginated walk would need the same adaptation. The +// logic is identical — the page must advance, and must not run past lastPage. +func nextPage(current int, next, last *int) (int, bool) { + if next == nil || *next == 0 { + return current, false + } + if *next <= current { + return current, false + } + if last != nil && *last > 0 && *next > *last { + return current, false + } + return *next, true +} + +// Changed carries the "was this flag supplied" signal that cmd/dns/dns.go reads +// from cobra, so the read-modify-write merge can be judged without a cobra +// command in the way. A nil field means the caller did not supply that flag and +// the current value must survive. +type Changed struct { + Type *string + Host *string + Answer *string + TTL *int64 + Priority *int64 +} + +// UpdateRecord mirrors runUpdate in cmd/dns/dns.go against the SDK. +func UpdateRecord(ctx context.Context, c *sdk.Namecom, domain string, id int, ch Changed) (*coreapigo.Record, error) { + current, err := c.DNS.GetRecord(ctx, &coreapigo.GetRecordRequest{ + DomainName: domain, + ID: id, + }) + if err != nil { + return nil, fmt.Errorf("reading record %d on %s: %w", id, domain, err) + } + + // The two casts here are the same ones internal/api/gen forces today, and + // the reason is the same: the spec declares Record.type as a plain string + // and the update body's type as an enum, and Record.ttl as a value where + // the body's is a pointer. Both survive the migration unchanged — they come + // from the spec, not from oapi-codegen. + body := &coreapigo.DNSUpdateRecordBody{ + DomainName: domain, + ID: id, + Type: coreapigo.DNSUpdateRecordBodyType(derefStr(current.Type)), + Answer: derefStr(current.Answer), + Host: current.Host, + TTL: ¤t.TTL, + Priority: current.Priority, + } + + if ch.Type != nil { + body.Type = coreapigo.DNSUpdateRecordBodyType(*ch.Type) + } + if ch.Host != nil { + body.Host = ch.Host + } + if ch.Answer != nil { + body.Answer = *ch.Answer + } + if ch.TTL != nil { + body.TTL = ch.TTL + } + if ch.Priority != nil { + body.Priority = ch.Priority + } + + updated, err := c.DNS.UpdateRecord(ctx, body) + if err != nil { + return nil, fmt.Errorf("updating record %d on %s: %w", id, domain, err) + } + return updated, nil +} + +func derefStr(s *string) string { + if s == nil { + return "" + } + return *s +} diff --git a/internal/sdkspike/spike_test.go b/internal/sdkspike/spike_test.go new file mode 100644 index 0000000..e2d30a4 --- /dev/null +++ b/internal/sdkspike/spike_test.go @@ -0,0 +1,336 @@ +package sdkspike + +import ( + "context" + "encoding/base64" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + coreapigo "github.com/namedotcom/core-api-go" + "github.com/namedotcom/core-api-go/option" +) + +// ---- Q1: does WithoutRetries hold at client scope? ------------------------- + +// TestWithoutRetriesHoldsAtClientScope is the question the spike exists to +// answer first, because the fallback is expensive: if the option only takes +// effect per call, every one of the ~53 call sites has to carry it, and any +// site added later that forgets it silently regains POST-on-5xx retries. +func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { + // CONFIRMED SDK DEFECT, not a usage error. internal/retrier.go's NewRetrier + // reads only `attempts` out of the options it is given and discards + // `disabled` entirely, so a client built with WithoutRetries still gets a + // Retrier with the default 2 attempts. Run() honours `disabled` only from + // per-call options. The option is therefore silently ignored at client + // scope — which is exactly where a safety setting most needs to hold. + t.Run("client-scoped WithoutRetries is silently ignored", func(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }) + if err == nil { + t.Fatal("expected the 500 to surface as an error") + } + // Asserted as the defect it is, so the day the SDK fixes it this test + // fails and tells us the per-call workaround can be dropped. + if got := atomic.LoadInt32(&calls); got == 1 { + t.Errorf("client-scoped WithoutRetries now holds (server saw 1 request) — " + + "the SDK has been fixed; drop the per-call workaround and update issue #40") + } else { + t.Logf("server saw %d requests despite client-scoped WithoutRetries — option silently ignored", got) + } + }) + + t.Run("per-call WithoutRetries does suppress it", func(t *testing.T) { + // The fallback, and the reason the client-scoped failure is expensive: + // this form works, so every call site has to carry the option. + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + _, _ = c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }, option.WithoutRetries()) + + if got := atomic.LoadInt32(&calls); got != 1 { + t.Errorf("server saw %d requests, want exactly 1 with a per-call WithoutRetries", got) + } + }) + + t.Run("without it, the SDK retries a 500", func(t *testing.T) { + // The control. If this ever stops retrying, the option above is proving + // nothing and the test has quietly become a tautology. + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(srv.Close) + + c := NewWithSDKRetries(srv.URL, "u", "t", &http.Client{Timeout: 30 * time.Second}) + _, _ = c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }) + if got := atomic.LoadInt32(&calls); got < 2 { + t.Errorf("server saw %d requests, want >1 — the control is not exercising the retrier", got) + } + }) +} + +// TestSDKRetriesPostOn5xx pins the behaviour that makes WithoutRetries +// mandatory rather than merely tidy. +// +// internal/retrier.go dispatches on status code alone, with no method check, so +// a POST is retried on a 5xx. Our transport refuses that deliberately — +// CreateDomain and ProcessRefund are the calls at stake — and the API honours +// idempotency keys on only five operations, of which record creation is not +// one. A retried POST is a second write. +func TestSDKRetriesPostOn5xx(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(srv.Close) + + c := NewWithSDKRetries(srv.URL, "u", "t", &http.Client{Timeout: 30 * time.Second}) + // Host is a plain string on the create body but a *string on the update + // body — one of several asymmetries the SDK inherits from the spec. + _, _ = c.DNS.CreateRecord(context.Background(), &coreapigo.DNSCreateRecordBody{ + DomainName: "example.com", + Type: coreapigo.DNSCreateRecordBodyType("A"), + Host: "spike", + Answer: "1.2.3.4", + }) + + if got := atomic.LoadInt32(&calls); got < 2 { + t.Errorf("POST was sent %d time(s); expected the SDK to retry it on 5xx", got) + } else { + t.Logf("SDK sent the POST %d times on 5xx — this is why WithoutRetries is required, not optional", got) + } +} + +// TestSDKBackoffIgnoresContext pins the second transport problem, which the +// issue did not anticipate: internal/retrier.go sleeps with a bare time.Sleep, +// so a cancelled context does not interrupt the wait. Our own sleep selects on +// ctx.Done(), and #41 additionally declines a wait that cannot fit the +// deadline. Left live, the SDK's retrier makes --timeout unenforceable during +// backoff. +func TestSDKBackoffIgnoresContext(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "2") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"slow down"}`)) + })) + t.Cleanup(srv.Close) + + c := NewWithSDKRetries(srv.URL, "u", "t", &http.Client{Timeout: 30 * time.Second}) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + _, _ = c.DNS.ListRecords(ctx, &coreapigo.ListRecordsRequest{DomainName: "example.com"}) + elapsed := time.Since(start) + + // A context-aware backoff would abandon the wait at ~100ms. The bare + // time.Sleep runs the full Retry-After first. + if elapsed >= 2*time.Second { + t.Logf("call took %s against a 100ms context deadline — the SDK's backoff is not context-aware", elapsed) + } else { + t.Errorf("call took %s; expected the bare time.Sleep to overrun the deadline. "+ + "If the SDK has become context-aware, this finding is stale and the recommendation should be revisited", elapsed) + } +} + +// ---- Q2: does the pagination guard carry over? ------------------------------ + +// TestListAllRecordsPagination checks that the walk terminates on both the +// normal and the pathological case, using the guard re-typed for the SDK's +// *int page fields. +func TestListAllRecordsPagination(t *testing.T) { + t.Run("walks every page", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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,"totalCount":2}`)) + return + } + _, _ = w.Write([]byte(`{"records":[{"id":1,"host":"a","type":"A","answer":"1.1.1.1","ttl":300}],"nextPage":2,"lastPage":2,"totalCount":2}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + records, requests, err := ListAllRecords(context.Background(), c, "example.com") + if err != nil { + t.Fatalf("ListAllRecords: %v", err) + } + if len(records) != 2 { + t.Errorf("got %d records across 2 pages, want 2", len(records)) + } + if requests != 2 { + t.Errorf("made %d requests, want 2", requests) + } + }) + + t.Run("a non-advancing nextPage terminates the walk", func(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if atomic.AddInt32(&calls, 1) > 10 { + t.Error("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(`{"records":[{"id":1,"host":"a","type":"A","answer":"1.1.1.1","ttl":300}],"nextPage":2,"lastPage":99,"totalCount":99}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + _, requests, err := ListAllRecords(context.Background(), c, "example.com") + if err != nil { + t.Fatalf("ListAllRecords: %v", err) + } + if requests != 2 { + t.Errorf("made %d requests, want 2 (page 1 -> 2, then the page stops advancing)", requests) + } + }) +} + +// ---- Q3: read-modify-write ergonomics --------------------------------------- + +// TestUpdateRecordMergesOnlyChangedFields checks the RMW path behaves as +// cmd/dns/dns.go does, and records what the request actually looked like so the +// two can be compared side by side. +func TestUpdateRecordMergesOnlyChangedFields(t *testing.T) { + var body string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"id":7,"host":"www","type":"A","answer":"1.1.1.1","ttl":600,"domainName":"example.com"}`)) + return + } + buf := make([]byte, r.ContentLength) + _, _ = r.Body.Read(buf) + body = string(buf) + _, _ = w.Write([]byte(`{"id":7,"host":"www","type":"A","answer":"9.9.9.9","ttl":600,"domainName":"example.com"}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + answer := "9.9.9.9" + got, err := UpdateRecord(context.Background(), c, "example.com", 7, Changed{Answer: &answer}) + if err != nil { + t.Fatalf("UpdateRecord: %v", err) + } + if derefStr(got.Answer) != "9.9.9.9" { + t.Errorf("answer = %q, want the updated value", derefStr(got.Answer)) + } + // The unchanged fields must survive: the API replaces the whole record, so + // anything the merge drops is silently erased. + for _, want := range []string{`"ttl":600`, `"host":"www"`, `"type":"A"`, `"answer":"9.9.9.9"`} { + if !contains(body, want) { + t.Errorf("request body is missing %s — a full-replacement PUT would erase it:\n%s", want, body) + } + } + t.Logf("update request body: %s", body) +} + +func contains(haystack, needle string) bool { + return len(haystack) >= len(needle) && (func() bool { + for i := 0; i+len(needle) <= len(haystack); i++ { + if haystack[i:i+len(needle)] == needle { + return true + } + } + return false + })() +} + +// countingTransport records how many requests actually pass through it. +type countingTransport struct { + base http.RoundTripper + calls int32 +} + +func (c *countingTransport) RoundTrip(r *http.Request) (*http.Response, error) { + atomic.AddInt32(&c.calls, 1) + return c.base.RoundTrip(r) +} + +// TestOurTransportSurvivesTheWiring is the load-bearing assumption of the whole +// recommendation: that handing the SDK our *http.Client keeps our RoundTripper +// — rate limiter, POST-on-5xx refusal, deadline guard, Retry-After clamp — in +// the path for every call. +// +// It is worth an explicit test because the per-call options are rebuilt from +// scratch on every method (that is what breaks client-scoped WithoutRetries), +// and CallParams carries a Client field alongside DisableRetries. Only a +// fallback in caller.Call — `client := c.client; if params.Client != nil` — +// keeps the client-scoped one alive. If that fallback ever goes the way the +// retry flag went, every request would silently route through +// http.DefaultClient and the rate limiter would be gone with no visible signal. +func TestOurTransportSurvivesTheWiring(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":[],"totalCount":0}`)) + })) + t.Cleanup(srv.Close) + + tr := &countingTransport{base: http.DefaultTransport} + c := New(srv.URL, "u", "t", &http.Client{Transport: tr, Timeout: 5 * time.Second}) + + if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }); err != nil { + t.Fatalf("ListRecords: %v", err) + } + if got := atomic.LoadInt32(&tr.calls); got != 1 { + t.Errorf("our RoundTripper saw %d requests, want 1 — the SDK bypassed the client we supplied", got) + } +} + +// TestBasicAuthMatchesOurHeader pins that the SDK's auth is byte-identical to +// what internal/api/client.go builds today, so the migration is not quietly a +// credential change. +func TestBasicAuthMatchesOurHeader(t *testing.T) { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("Authorization") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"records":[],"totalCount":0}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "alice", "s3cret", &http.Client{Timeout: 5 * time.Second}) + if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }); err != nil { + t.Fatalf("ListRecords: %v", err) + } + + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("alice:s3cret")) + if got != want { + t.Errorf("Authorization = %q, want %q", got, want) + } +} From a0c1533bb2d9bd9886cb8e112ad17dc292fdf670 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 22:01:35 -0600 Subject: [PATCH 2/5] spike: work around both SDK retry defects, and record the reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Saves the two unfiled bug reports under docs/upstream/ so the analysis is not lost between writing it and opening the issues, and makes internal/sdkspike safe to reason about in the meantime. The two defects need two separate workarounds, which a measurement was needed to establish. Disabling retries does NOT skip the sleep: Retrier.run sleeps BEFORE it checks the attempt counter, so a call with retries off still waits out the server's Retry-After and only then declines to retry. Against a 429 carrying "Retry-After: 30" that is thirty seconds of dead time for a single request. Assuming one workaround covered both would have left that in place. Defect 1, the ignored client-scoped WithoutRetries, is worked around by appending the option to every call — and by making that impossible to forget. Client keeps *sdk.Namecom unexported and exposes only wrapped methods, so there is no way to issue a request that skips the option without editing workaround.go. Convention would not survive ~53 call sites; forgetting it silently restores POST-on-5xx retries against endpoints that do not honour idempotency keys. Defect 2, the context-ignoring backoff, is worked around by deleting the Retry-After header from responses on their way back to the SDK. That is only safe because of where it sits: our transport has already read the header, made its retry decision, and either waited on a context-aware timer or declined to wait at all, so the header has no remaining consumer in-process. Removing it caps the SDK's dead sleep at its own minRetryDelay. Measured: 30s to 1s, with *core.APIError still carrying status 429 and the body. The strip is scoped to the client handed to the SDK and does not touch the caller's own, so raw response readers still see what the server sent — asserted, and the assertion fails if the wrapper mutates the client passed in. Residue worth stating plainly: roughly one second of dead sleep remains on every terminal 429 or 5xx, and it cannot be removed from outside the SDK. Each workaround was checked by removing it and confirming its test fails. --- docs/upstream/README.md | 16 ++ .../core-api-go-backoff-ignores-context.md | 134 +++++++++++ .../core-api-go-withoutretries-ignored.md | 210 ++++++++++++++++++ internal/sdkspike/spike.go | 19 +- internal/sdkspike/spike_test.go | 117 +++++++++- internal/sdkspike/workaround.go | 133 +++++++++++ 6 files changed, 613 insertions(+), 16 deletions(-) create mode 100644 docs/upstream/README.md create mode 100644 docs/upstream/core-api-go-backoff-ignores-context.md create mode 100644 docs/upstream/core-api-go-withoutretries-ignored.md create mode 100644 internal/sdkspike/workaround.go diff --git a/docs/upstream/README.md b/docs/upstream/README.md new file mode 100644 index 0000000..5c570bc --- /dev/null +++ b/docs/upstream/README.md @@ -0,0 +1,16 @@ +# Upstream issue drafts + +Bug reports written against dependencies but **not yet filed**. They live here so +the analysis is not lost between the day it is done and the day someone opens +the issue, and so a workaround in this repo can point at the reasoning behind it. + +When one is filed, replace its body with a link to the filed issue and keep the +file — the workaround it justifies will outlive the report. + +| Draft | Against | Status | +|---|---|---| +| [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `namedotcom/core-api-go` v1.33.2 | not filed | +| [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `namedotcom/core-api-go` v1.33.2 | not filed | + +Both are worked around in `internal/sdkspike`; see `workaround.go` for what each +costs and what residue is left over. diff --git a/docs/upstream/core-api-go-backoff-ignores-context.md b/docs/upstream/core-api-go-backoff-ignores-context.md new file mode 100644 index 0000000..fe3bfa6 --- /dev/null +++ b/docs/upstream/core-api-go-backoff-ignores-context.md @@ -0,0 +1,134 @@ +# Retry backoff ignores the request context + +**Version:** v1.33.2 · **Go:** 1.26.6 + +## Summary + +`Retrier.run` waits between attempts with a bare `time.Sleep`. It does not +select on `ctx.Done()`, so a cancelled or expired context does not interrupt the +wait. A call therefore outlives its own deadline, and `context.WithTimeout` — +along with anything built on it, such as a CLI `--timeout` flag — stops bounding +the call once a retry begins. + +## Root cause + +`internal/retrier.go:146`: + +```go +if r.shouldRetry(response) { + defer func() { _ = response.Body.Close() }() + + delay, err := r.retryDelay(response, retryAttempt) + if err != nil { + return nil, err + } + + time.Sleep(delay) // not context-aware + ... +} +``` + +The context is checked before each attempt (`internal/retrier.go:120`), so +cancellation is noticed *eventually* — but only after the full sleep has already +elapsed. With `maxRetryDelay = 60s` and `Retry-After` honoured up to that cap, a +single 429 can hold the call for a minute past its deadline. + +## Reproduction + +```go +package main + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "time" + + coreapigo "github.com/namedotcom/core-api-go" + sdk "github.com/namedotcom/core-api-go/client" + "github.com/namedotcom/core-api-go/option" +) + +func main() { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "2") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"slow down"}`)) + })) + defer srv.Close() + + c := sdk.NewNamecom( + option.WithBaseURL(srv.URL), + option.WithBasicAuth("u", "t"), + ) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := c.DNS.ListRecords(ctx, &coreapigo.ListRecordsRequest{DomainName: "example.com"}) + fmt.Printf("elapsed=%s err=%v\n", time.Since(start).Round(time.Millisecond), err) +} +``` + +### Actual + +``` +elapsed=2.001s err=context deadline exceeded +``` + +### Expected + +Roughly `elapsed=100ms`, the deadline the caller set. + +## Consequences + +- **A deadline stops meaning anything during backoff.** Anything mapping a + user-facing timeout onto `context.WithTimeout` silently loses control of the + call. +- **Cancellation is not honoured promptly.** `ctrl-C` wired to `cancel()` leaves + the process sitting in `time.Sleep` for up to `maxRetryDelay`. +- **The error loses its cause.** The call above reports + `context deadline exceeded` when what actually happened is a 429 the server + already answered clearly. A caller that wants to surface "rate limited, retry + after 2s" cannot, because the useful response was discarded in favour of a + timeout produced by the SDK's own sleep. + +## Suggested fix + +Make the wait cancellable: + +```go +- time.Sleep(delay) ++ timer := time.NewTimer(delay) ++ select { ++ case <-request.Context().Done(): ++ timer.Stop() ++ return nil, request.Context().Err() ++ case <-timer.C: ++ } +``` + +**Verified.** With that patch applied to a local copy of v1.33.2, the +reproduction above prints `elapsed=101ms err=context deadline exceeded` +instead of `elapsed=2.001s`. `go test ./internal/...` still passes against the +patched copy. + +Optionally, and separately: when the remaining time on the deadline is shorter +than `delay`, returning the response rather than sleeping at all preserves the +server's answer instead of converting it into a timeout. That turns the example +above into a 429 the caller can act on. + +## Note on where the fix belongs + +`.fernignore` exempts only `.fern/replay.lock`, `.fern/replay.yml`, and +`.gitattributes`, so `internal/retrier.go` is regenerated and a patch here would +not survive the next Fern run. Flagging it in case this needs to go to the Fern +Go generator template instead. + +## Related + +Filed separately from the `option.WithoutRetries()` issue — different root +cause, different fix — though both are in `internal/retrier.go` and both affect +callers trying to bound or opt out of retry behaviour. diff --git a/docs/upstream/core-api-go-withoutretries-ignored.md b/docs/upstream/core-api-go-withoutretries-ignored.md new file mode 100644 index 0000000..c9c3bae --- /dev/null +++ b/docs/upstream/core-api-go-withoutretries-ignored.md @@ -0,0 +1,210 @@ +# `option.WithoutRetries()` is silently ignored when set on the client + +**Version:** v1.33.2 · **Go:** 1.26.6 + +## Summary + +`option.WithoutRetries()` has no effect when passed to `client.NewNamecom(...)`. +Requests are still retried with the default 2 attempts. The same option passed +per call works correctly. + +This is the option that suppresses retrying non-idempotent requests, so an +option that appears to disable that behaviour and silently does not is worse +than one that does not exist. + +## Root cause + +`internal/retrier.go:58` — `NewRetrier` builds the options struct, reads +`attempts` out of it, and drops `disabled`: + +```go +func NewRetrier(opts ...RetryOption) *Retrier { + options := new(retryOptions) + for _, opt := range opts { + opt(options) + } + attempts := uint(defaultRetryAttempts) + if options.attempts > 0 { + attempts = options.attempts + } + return &Retrier{ + attempts: attempts, // options.disabled is discarded + } +} +``` + +`retryOptions` carries both fields (`internal/retrier.go:265`), but `Retrier` +has only `attempts` (`internal/retrier.go:53`), so there is nowhere to keep it. + +`Run` then honours `disabled` only from the per-call options +(`internal/retrier.go:90`): + +```go +maxRetryAttempts := r.attempts +if options.attempts > 0 { + maxRetryAttempts = options.attempts +} +if options.disabled { + maxRetryAttempts = 1 +} +``` + +Each generated method rebuilds its options from scratch — e.g. +`dns/raw_client.go`: `options := core.NewRequestOptions(opts...)` — and passes +`DisableRetries: options.DisableRetries` into `CallParams`. With no per-call +options that value is `false`, so `buildRetryOptions(0, false)` returns an empty +slice and the client-scoped intent never reaches `Run`. + +Note the asymmetry: `caller.Call` *does* fall back to the client-scoped HTTP +client (`client := c.client; if params.Client != nil { client = params.Client }`), +so `WithHTTPClient` behaves as expected at client scope. Only the retry settings +fail to. + +## Reproduction + +```go +package main + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + + coreapigo "github.com/namedotcom/core-api-go" + sdk "github.com/namedotcom/core-api-go/client" + "github.com/namedotcom/core-api-go/option" +) + +func main() { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + defer srv.Close() + + count := func(label string, opts ...option.RequestOption) { + atomic.StoreInt32(&calls, 0) + c := sdk.NewNamecom(append([]option.RequestOption{ + option.WithBaseURL(srv.URL), + option.WithBasicAuth("u", "t"), + }, opts...)...) + _, _ = c.DNS.ListRecords(context.Background(), + &coreapigo.ListRecordsRequest{DomainName: "example.com"}) + fmt.Printf("%-32s requests=%d\n", label, atomic.LoadInt32(&calls)) + } + + count("no option") + count("client-scoped WithoutRetries", option.WithoutRetries()) + + // Per-call, for contrast. + atomic.StoreInt32(&calls, 0) + c := sdk.NewNamecom(option.WithBaseURL(srv.URL), option.WithBasicAuth("u", "t")) + _, _ = c.DNS.ListRecords(context.Background(), + &coreapigo.ListRecordsRequest{DomainName: "example.com"}, + option.WithoutRetries()) + fmt.Printf("%-32s requests=%d\n", "per-call WithoutRetries", atomic.LoadInt32(&calls)) +} +``` + +### Actual + +``` +no option requests=2 +client-scoped WithoutRetries requests=2 +per-call WithoutRetries requests=1 +``` + +### Expected + +``` +no option requests=2 +client-scoped WithoutRetries requests=1 +per-call WithoutRetries requests=1 +``` + +## Why this matters for this API specifically + +`shouldRetry` (`internal/retrier.go:168`) dispatches on status code alone, with +no method check, so a **POST is retried on a 5xx**: + +```go +func (r *Retrier) shouldRetry(response *http.Response) bool { + return response.StatusCode == http.StatusTooManyRequests || + response.StatusCode == http.StatusRequestTimeout || + response.StatusCode >= http.StatusInternalServerError +} +``` + +Confirmed against a stub: a `CreateRecord` POST was sent twice on a 500. + +On the name.com Core API only five operations declare `X-Idempotency-Key` — +`CreateDomain`, `PurchasePrivacy`, `ProcessRefund`, `VerifyContact`, +`ResendContactVerificationEmail`. Record creation is not among them, and I +verified in sandbox that `POST /core/v1/domains/{domain}/records` does not +deduplicate on the header: two posts under one key produced two records with +distinct IDs, and an identical body under the same key returned +`400 Parameter Value Error - Record already exists` rather than replaying the +original 200. + +So for a caller that wants to opt out of retrying unsafe methods, the +client-scoped option is the natural place to do it, and it is exactly the place +where it does not work. + +## Suggested fix + +Carry `disabled` on the `Retrier` and honour either source in `Run`: + +```go + type Retrier struct { + attempts uint ++ disabled bool + } + + func NewRetrier(opts ...RetryOption) *Retrier { + ... + return &Retrier{ + attempts: attempts, ++ disabled: options.disabled, + } + } + + func (r *Retrier) Run(...) (*http.Response, error) { + ... +- if options.disabled { ++ if options.disabled || r.disabled { + maxRetryAttempts = 1 + } +``` + +`WithMaxAttempts` at client scope already works, since `attempts` is retained — +this brings `disabled` in line with it. + +**Verified.** With that patch applied to a local copy of v1.33.2, the +reproduction above prints the expected output: + +``` +no option requests=2 +client-scoped WithoutRetries requests=1 +per-call WithoutRetries requests=1 +``` + +`go test ./internal/...` still passes against the patched copy. + +## Note on where the fix belongs + +`.fernignore` exempts only `.fern/replay.lock`, `.fern/replay.yml`, and +`.gitattributes`, so `internal/retrier.go` is regenerated. A patch landed here +would be overwritten on the next Fern run unless the file is added to +`.fernignore`. The durable fix is likely in the Fern Go generator template — +happy to open a PR here if you would rather carry it via `.fernignore`, but +flagging it so this does not bounce. + +## Workaround + +Pass `option.WithoutRetries()` on every call. It works, but it has to be +repeated at every call site and anything added later that omits it silently +regains the behaviour. diff --git a/internal/sdkspike/spike.go b/internal/sdkspike/spike.go index 7052bb4..56b9ec4 100644 --- a/internal/sdkspike/spike.go +++ b/internal/sdkspike/spike.go @@ -28,13 +28,8 @@ import ( // 5xx (it dispatches on status code alone) and sleeps with a bare time.Sleep // that ignores context. Ours refuses POST retries and returns the response // rather than sleeping past the deadline. Only one of the two may be live. -func New(baseURL, username, token string, httpClient *http.Client) *sdk.Namecom { - return sdk.NewNamecom( - option.WithBaseURL(baseURL), - option.WithBasicAuth(username, token), - option.WithHTTPClient(httpClient), - option.WithoutRetries(), - ) +func New(baseURL, username, token string, httpClient *http.Client) *Client { + return NewGuarded(baseURL, username, token, httpClient) } // NewWithSDKRetries is the same client with the SDK's retry layer left ON. @@ -53,7 +48,7 @@ func NewWithSDKRetries(baseURL, username, token string, httpClient *http.Client) // reports nextPage/lastPage as *int where the generated client used *int32, so // cmdutil.NextPage's signature does not carry over unchanged — this is the // int-width adaptation the migration needs, isolated so it can be judged. -func ListAllRecords(ctx context.Context, c *sdk.Namecom, domain string) ([]*coreapigo.Record, int, error) { +func ListAllRecords(ctx context.Context, c *Client, domain string) ([]*coreapigo.Record, int, error) { var ( all []*coreapigo.Record page = 1 @@ -61,7 +56,7 @@ func ListAllRecords(ctx context.Context, c *sdk.Namecom, domain string) ([]*core ) for { p := page - resp, err := c.DNS.ListRecords(ctx, &coreapigo.ListRecordsRequest{ + resp, err := c.ListRecords(ctx, &coreapigo.ListRecordsRequest{ DomainName: domain, Page: &p, }) @@ -110,8 +105,8 @@ type Changed struct { } // UpdateRecord mirrors runUpdate in cmd/dns/dns.go against the SDK. -func UpdateRecord(ctx context.Context, c *sdk.Namecom, domain string, id int, ch Changed) (*coreapigo.Record, error) { - current, err := c.DNS.GetRecord(ctx, &coreapigo.GetRecordRequest{ +func UpdateRecord(ctx context.Context, c *Client, domain string, id int, ch Changed) (*coreapigo.Record, error) { + current, err := c.GetRecord(ctx, &coreapigo.GetRecordRequest{ DomainName: domain, ID: id, }) @@ -150,7 +145,7 @@ func UpdateRecord(ctx context.Context, c *sdk.Namecom, domain string, id int, ch body.Priority = ch.Priority } - updated, err := c.DNS.UpdateRecord(ctx, body) + updated, err := c.UpdateRecord(ctx, body) if err != nil { return nil, fmt.Errorf("updating record %d on %s: %w", id, domain, err) } diff --git a/internal/sdkspike/spike_test.go b/internal/sdkspike/spike_test.go index e2d30a4..07dcafe 100644 --- a/internal/sdkspike/spike_test.go +++ b/internal/sdkspike/spike_test.go @@ -3,6 +3,7 @@ package sdkspike import ( "context" "encoding/base64" + "errors" "net/http" "net/http/httptest" "sync/atomic" @@ -10,6 +11,8 @@ import ( "time" coreapigo "github.com/namedotcom/core-api-go" + sdk "github.com/namedotcom/core-api-go/client" + "github.com/namedotcom/core-api-go/core" "github.com/namedotcom/core-api-go/option" ) @@ -35,7 +38,7 @@ func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { })) t.Cleanup(srv.Close) - c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + c := rawClient(srv.URL, option.WithoutRetries()) _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }) @@ -63,7 +66,7 @@ func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { })) t.Cleanup(srv.Close) - c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) + c := rawClient(srv.URL) _, _ = c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }, option.WithoutRetries()) @@ -300,7 +303,7 @@ func TestOurTransportSurvivesTheWiring(t *testing.T) { tr := &countingTransport{base: http.DefaultTransport} c := New(srv.URL, "u", "t", &http.Client{Transport: tr, Timeout: 5 * time.Second}) - if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + if _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }); err != nil { t.Fatalf("ListRecords: %v", err) @@ -323,7 +326,7 @@ func TestBasicAuthMatchesOurHeader(t *testing.T) { t.Cleanup(srv.Close) c := New(srv.URL, "alice", "s3cret", &http.Client{Timeout: 5 * time.Second}) - if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + if _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }); err != nil { t.Fatalf("ListRecords: %v", err) @@ -334,3 +337,109 @@ func TestBasicAuthMatchesOurHeader(t *testing.T) { t.Errorf("Authorization = %q, want %q", got, want) } } + +// rawClient builds an unguarded SDK client, used only to demonstrate the two +// defects. Production wiring goes through NewGuarded, which is why *sdk.Namecom +// is unexported there. +func rawClient(baseURL string, opts ...option.RequestOption) *sdk.Namecom { + return sdk.NewNamecom(append([]option.RequestOption{ + option.WithBaseURL(baseURL), + option.WithBasicAuth("u", "t"), + option.WithHTTPClient(&http.Client{Timeout: 90 * time.Second}), + }, opts...)...) +} + +// ---- the workarounds themselves -------------------------------------------- + +// TestWorkaroundSuppressesDuplicateWrite is the one that matters for safety. +// The guarded client must issue a POST exactly once against a 5xx, without the +// caller having to remember anything. +func TestWorkaroundSuppressesDuplicateWrite(t *testing.T) { + var calls int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + atomic.AddInt32(&calls, 1) + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 30 * time.Second}) + _, err := c.CreateRecord(context.Background(), &coreapigo.DNSCreateRecordBody{ + DomainName: "example.com", + Type: coreapigo.DNSCreateRecordBodyType("A"), + Host: "spike", + Answer: "1.2.3.4", + }) + if err == nil { + t.Fatal("expected the 500 to surface as an error") + } + if got := atomic.LoadInt32(&calls); got != 1 { + t.Errorf("POST was sent %d times, want exactly 1 — the write was duplicated", got) + } +} + +// TestWorkaroundCapsDeadSleep covers the second defect, which disabling retries +// does NOT solve on its own: Retrier.run sleeps before it checks the attempt +// counter, so a call with retries off still waits out the server's Retry-After +// and only then declines to retry. +// +// Stripping the header on the way back to the SDK caps that at its +// minRetryDelay. Measured on the unguarded path: 30s. Guarded: ~1s. +func TestWorkaroundCapsDeadSleep(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "30") + w.WriteHeader(http.StatusTooManyRequests) + _, _ = w.Write([]byte(`{"message":"slow down"}`)) + })) + t.Cleanup(srv.Close) + + c := New(srv.URL, "u", "t", &http.Client{Timeout: 90 * time.Second}) + start := time.Now() + _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + DomainName: "example.com", + }) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected the 429 to surface as an error") + } + if elapsed > 5*time.Second { + t.Errorf("call took %s; the Retry-After strip is not capping the SDK's dead sleep", elapsed) + } + // The status must survive the strip — exit-code mapping depends on it. + var apiErr *core.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error is not a *core.APIError: %v", err) + } + if apiErr.StatusCode != http.StatusTooManyRequests { + t.Errorf("status = %d, want 429 preserved through the strip", apiErr.StatusCode) + } + t.Logf("guarded call against Retry-After: 30 took %s and kept status %d", + elapsed.Round(100*time.Millisecond), apiErr.StatusCode) +} + +// TestStripIsScopedToTheSDKClient pins that the header removal does not leak +// into the caller's own transport. `namecom api` and anything else reading raw +// responses must still see what the server sent. +func TestStripIsScopedToTheSDKClient(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "7") + w.WriteHeader(http.StatusTooManyRequests) + })) + t.Cleanup(srv.Close) + + ours := &http.Client{Timeout: 30 * time.Second} + _ = sdkHTTPClient(ours) // wrapping must not mutate the client passed in + + resp, err := ours.Get(srv.URL) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer func() { _ = resp.Body.Close() }() + if got := resp.Header.Get("Retry-After"); got != "7" { + t.Errorf("Retry-After = %q on our own client, want %q — the strip leaked", got, "7") + } +} diff --git a/internal/sdkspike/workaround.go b/internal/sdkspike/workaround.go new file mode 100644 index 0000000..4f49fa7 --- /dev/null +++ b/internal/sdkspike/workaround.go @@ -0,0 +1,133 @@ +package sdkspike + +import ( + "context" + "net/http" + + coreapigo "github.com/namedotcom/core-api-go" + sdk "github.com/namedotcom/core-api-go/client" + "github.com/namedotcom/core-api-go/option" +) + +// Workarounds for two defects in core-api-go v1.33.2. Both are written up in +// docs/upstream/ and neither is filed yet. +// +// 1. option.WithoutRetries() is silently ignored at client scope, so the SDK +// retries a POST on a 5xx even when the client asked it not to. +// 2. The retry backoff sleeps with a bare time.Sleep, so it ignores the +// request context and can outlast the caller's deadline. +// +// They interact in a way that is not obvious and cost a measurement to find: +// disabling retries does NOT skip the sleep. Retrier.run sleeps BEFORE it +// checks the attempt counter, so a call with retries disabled still waits the +// full Retry-After and only then declines to retry. Against a 429 carrying +// "Retry-After: 30" that is thirty seconds of dead time for one request. +// +// So the two defects need two separate workarounds, and neither substitutes for +// the other. + +// safeCallOptions are appended to every SDK call. +// +// WithoutRetries has to be per-call because the client-scoped form does not +// work — see defect 1. That is the whole reason Client below exists: an option +// that must be repeated at every call site is an option someone will eventually +// forget, and forgetting it silently restores POST-on-5xx retries against +// endpoints that do not honour idempotency keys. +func safeCallOptions() []option.RequestOption { + return []option.RequestOption{option.WithoutRetries()} +} + +// stripRetryAfter removes the Retry-After header from responses on their way +// back to the SDK. +// +// This is the workaround for defect 2, and it is only safe because of where it +// sits. By the time a response leaves our RoundTripper, our own transport has +// already read Retry-After, made its retry decision, and — per #41 — either +// waited on a context-aware timer or declined to wait at all. The header has no +// remaining consumer inside the process. What it still has is the SDK's dead +// sleep downstream, which reads it and blocks for up to maxRetryDelay (60s) +// with no way to interrupt. +// +// Deleting it caps that dead sleep at the SDK's minRetryDelay of one second. +// Measured against a 429 carrying "Retry-After: 30": 30s before, 1s after, with +// *core.APIError still carrying status 429 and the response body in both cases. +// +// It is deliberately NOT applied to the shared transport — only to the client +// handed to the SDK — so that `namecom api` and anything else reading raw +// responses still sees the header the server sent. +type stripRetryAfter struct{ base http.RoundTripper } + +func (s stripRetryAfter) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := s.base.RoundTrip(req) + if resp != nil { + resp.Header.Del("Retry-After") + } + return resp, err +} + +// sdkHTTPClient wraps our own client for handing to the SDK, adding the +// Retry-After strip and nothing else. The rate limiter, the POST-on-5xx +// refusal, and the deadline guard all live in base's transport and are +// untouched. +func sdkHTTPClient(base *http.Client) *http.Client { + wrapped := *base // copy: the caller's client keeps its own transport + inner := base.Transport + if inner == nil { + inner = http.DefaultTransport + } + wrapped.Transport = stripRetryAfter{base: inner} + return &wrapped +} + +// Client is the only supported way to reach the SDK from this package. +// +// It keeps *sdk.Namecom unexported on purpose. The per-call WithoutRetries +// workaround cannot be enforced by convention across ~53 call sites, so it is +// enforced by construction instead: there is no way to issue a request that +// skips safeCallOptions without editing this file. +// +// If defect 1 is fixed upstream, this type collapses to a plain client +// constructor and the methods go away. +type Client struct { + sdk *sdk.Namecom +} + +// NewGuarded builds a client with both workarounds applied. +func NewGuarded(baseURL, username, token string, httpClient *http.Client) *Client { + return &Client{ + sdk: sdk.NewNamecom( + option.WithBaseURL(baseURL), + option.WithBasicAuth(username, token), + option.WithHTTPClient(sdkHTTPClient(httpClient)), + // Kept even though it does not work at client scope: it is the + // declaration of intent, and it starts working the day the SDK is + // fixed. safeCallOptions is what actually holds today. + option.WithoutRetries(), + ), + } +} + +// ListRecords lists one page of DNS records. +func (c *Client) ListRecords(ctx context.Context, req *coreapigo.ListRecordsRequest) (*coreapigo.ListRecordsResponse, error) { + return c.sdk.DNS.ListRecords(ctx, req, safeCallOptions()...) +} + +// GetRecord reads a single DNS record. +func (c *Client) GetRecord(ctx context.Context, req *coreapigo.GetRecordRequest) (*coreapigo.Record, error) { + return c.sdk.DNS.GetRecord(ctx, req, safeCallOptions()...) +} + +// CreateRecord creates a DNS record. +func (c *Client) CreateRecord(ctx context.Context, req *coreapigo.DNSCreateRecordBody) (*coreapigo.Record, error) { + return c.sdk.DNS.CreateRecord(ctx, req, safeCallOptions()...) +} + +// UpdateRecord replaces a DNS record. +func (c *Client) UpdateRecord(ctx context.Context, req *coreapigo.DNSUpdateRecordBody) (*coreapigo.Record, error) { + return c.sdk.DNS.UpdateRecord(ctx, req, safeCallOptions()...) +} + +// DeleteRecord removes a DNS record. +func (c *Client) DeleteRecord(ctx context.Context, req *coreapigo.DeleteRecordRequest) error { + return c.sdk.DNS.DeleteRecord(ctx, req, safeCallOptions()...) +} From 3cc25c423f33a6ec872a0ed171e5dc57c5d02942 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 22:34:16 -0600 Subject: [PATCH 3/5] docs: record the SDK workarounds and how to remove them The workarounds landed with their reasoning in code comments, which is the right place for why a line exists but the wrong place for "here is the whole arrangement and here is how it ends". docs/upstream/workarounds.md collects it: what each one costs, measured; why the two are not one problem; why deleting a response header is defensible here; and the removal steps for each defect. The part worth having written down is that the repo will tell us when upstream fixes this rather than us having to remember to check. Two tests assert the DEFECTS rather than the workarounds, so they fail when the defects go away, with messages that say what to drop. Both were verified against a locally patched copy of v1.33.2 carrying the fixes the drafts suggest: they fire, and the workaround tests keep passing, so a dependency bump surfaces exactly those two and nothing else. Also states the residue plainly, since it is easy to lose in a success story: roughly a second of dead sleep remains on every terminal 429 or 5xx, it is the SDK's own minRetryDelay, and it cannot be removed from outside the package. --- docs/upstream/README.md | 6 +- docs/upstream/workarounds.md | 141 ++++++++++++++++++++++++++++++++ internal/sdkspike/workaround.go | 4 +- 3 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 docs/upstream/workarounds.md diff --git a/docs/upstream/README.md b/docs/upstream/README.md index 5c570bc..8f7e0f0 100644 --- a/docs/upstream/README.md +++ b/docs/upstream/README.md @@ -12,5 +12,7 @@ file — the workaround it justifies will outlive the report. | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `namedotcom/core-api-go` v1.33.2 | not filed | | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `namedotcom/core-api-go` v1.33.2 | not filed | -Both are worked around in `internal/sdkspike`; see `workaround.go` for what each -costs and what residue is left over. +Both are worked around in `internal/sdkspike`. See +[`workarounds.md`](workarounds.md) for what each one costs, why the two are not +one problem, how the test suite will tell you when upstream fixes them, and the +removal steps for each. diff --git a/docs/upstream/workarounds.md b/docs/upstream/workarounds.md new file mode 100644 index 0000000..84cb671 --- /dev/null +++ b/docs/upstream/workarounds.md @@ -0,0 +1,141 @@ +# Workarounds for `namedotcom/core-api-go` + +Two defects in `core-api-go` v1.33.2 are worked around in `internal/sdkspike`. +Neither report is filed yet; both are drafted in this directory. + +**The intent is that these are temporary.** Everything below is arranged so that +the day upstream fixes a defect, the test suite says so and the removal is +mechanical. + +## The two defects + +| # | Defect | Draft | Worked around in | +|---|---|---|---| +| 1 | `option.WithoutRetries()` is silently ignored at client scope | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `safeCallOptions` + the `Client` wrapper | +| 2 | Retry backoff sleeps with a bare `time.Sleep`, ignoring the request context | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `stripRetryAfter` + `sdkHTTPClient` | + +## They are not one problem + +This is the part worth reading before touching either workaround, because the +obvious simplification is wrong. + +Disabling retries does **not** skip the sleep. `Retrier.run` sleeps *before* it +checks the attempt counter: + +```go +if r.shouldRetry(response) { + delay, _ := r.retryDelay(response, retryAttempt) + time.Sleep(delay) // happens first + return r.run(..., retryAttempt+1, ...) // counter checked on entry here +} +``` + +So a call with retries disabled still issues one request, waits out the server's +full `Retry-After`, and only then declines to retry. Measured against a 429 +carrying `Retry-After: 30`: one request, thirty seconds. + +Removing workaround 2 because workaround 1 "already covers it" reintroduces +that, and it will look like a hang rather than a bug. + +## What each one costs + +Measured against a stub; see the tests named below. + +| Scenario | Unguarded | Guarded | +|---|---|---| +| POST against a 5xx | 2 requests | 1 request | +| 429 with `Retry-After: 30` | 30s | ~1s | +| Error typing | `*core.APIError`, status 429 | unchanged | + +### Residue + +**Roughly one second of dead sleep remains on every terminal 429 or 5xx.** That +is the SDK's own `minRetryDelay`, reached after `Retry-After` is stripped, and +it cannot be removed from outside the package. It is latency on the error path +only — no extra request, no lost status — but it is a real cost that arrives +with the SDK and does not exist on the current generated client. + +## Why workaround 2 is safe + +Deleting a response header is the kind of thing that should raise an eyebrow, so +the reasoning is here rather than only in the code. + +`stripRetryAfter` sits on the client handed to the SDK, *above* our own +transport. By the time a response reaches it, `retryTransport` has already read +`Retry-After`, made its retry decision, and either waited on a context-aware +timer or declined to wait at all (see the deadline guard added in #41). The +header has no remaining consumer in-process — except the SDK's dead sleep. + +It is scoped deliberately: + +- Applied to the client passed to `sdk.NewNamecom`. +- **Not** applied to the caller's own `*http.Client`. `sdkHTTPClient` copies the + client before swapping its transport, so `namecom api` and anything else + reading raw responses still sees the header the server sent. + +`TestStripIsScopedToTheSDKClient` fails if that copy is ever removed. + +## Why workaround 1 is a wrapper and not a convention + +`option.WithoutRetries()` has to be repeated on every call. An option that must +be remembered at ~53 call sites is one that will eventually be forgotten, and +forgetting it silently restores POST-on-5xx retries against endpoints that do +not honour idempotency keys — `CreateDomain`, `RenewDomain`, `ProcessRefund`. + +So `Client` keeps `*sdk.Namecom` unexported and exposes only wrapped methods. +There is no way to issue a request that skips `safeCallOptions` without editing +`workaround.go`. Adding an operation means adding a method, which is the moment +to notice. + +## How you will find out upstream has fixed it + +Two tests assert the *defects*, not the workarounds, so they fail when the +defects go away: + +| Test | Fires when | +|---|---| +| `TestWithoutRetriesHoldsAtClientScope/client-scoped_WithoutRetries_is_silently_ignored` | defect 1 is fixed | +| `TestSDKBackoffIgnoresContext` | defect 2 is fixed | + +Both were verified against a locally patched copy of v1.33.2 carrying the fixes +suggested in the drafts. They fail with messages that say what to do: + +``` +client-scoped WithoutRetries now holds (server saw 1 request) — +the SDK has been fixed; drop the per-call workaround and update issue #40 +``` + +``` +call took 102ms; expected the bare time.Sleep to overrun the deadline. +If the SDK has become context-aware, this finding is stale and the +recommendation should be revisited +``` + +The workaround tests keep passing under a fixed SDK — a fix makes them +redundant, not broken — so a dependency bump surfaces exactly these two and +nothing else. + +## Removing them + +### Defect 1 fixed upstream + +1. Delete `safeCallOptions` and the wrapped methods on `Client`. +2. Either export the SDK client or collapse `Client` to a constructor; keep + `option.WithoutRetries()` on `NewGuarded`, which now actually works. +3. Delete the `client-scoped ... is silently ignored` subtest. +4. Mark the draft filed-and-fixed in [`README.md`](README.md). + +### Defect 2 fixed upstream + +1. Delete `stripRetryAfter`, `sdkHTTPClient`, and + `TestStripIsScopedToTheSDKClient`. +2. Pass the caller's `*http.Client` straight to `option.WithHTTPClient`. +3. Delete `TestSDKBackoffIgnoresContext` and `TestWorkaroundCapsDeadSleep`. +4. The ~1s residue on terminal errors goes away with it. + +### Neither fixed, and the migration proceeds anyway + +The wrapper generalises: every service package needs the same treatment, so +`Client` grows from 5 methods to roughly 53. That is mechanical but large, and +it is the cost that belongs in the #40 decision — not the workaround itself, +which is small, but the obligation to maintain it across the whole API surface. diff --git a/internal/sdkspike/workaround.go b/internal/sdkspike/workaround.go index 4f49fa7..c37c4da 100644 --- a/internal/sdkspike/workaround.go +++ b/internal/sdkspike/workaround.go @@ -10,7 +10,9 @@ import ( ) // Workarounds for two defects in core-api-go v1.33.2. Both are written up in -// docs/upstream/ and neither is filed yet. +// docs/upstream/ and neither is filed yet. docs/upstream/workarounds.md carries +// the measurements, the residual cost, and the removal steps for each; the two +// tests that fail when upstream fixes a defect are named there too. // // 1. option.WithoutRetries() is silently ignored at client scope, so the SDK // retries a POST on a 5xx even when the client asked it not to. From 879d5d88057888484087efb314168b820155a136 Mon Sep 17 00:00:00 2001 From: patramsey Date: Wed, 19 Aug 2026 22:41:25 -0600 Subject: [PATCH 4/5] docs: drop the SDK workarounds, keep only the findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the client-side workarounds added in a0c1533. The goal is to document the defects well enough to file them upstream, not to carry code that papers over them — a wrapper enforcing a per-call option across ~53 methods is a real maintenance obligation, and taking it on before the reports have even been filed is premature. internal/sdkspike goes back to its state at 72e1c7c: a plain evaluation of the SDK against the dns command group, with tests that demonstrate the defects and fail if either is ever fixed. What the workarounds proved is kept, in docs/upstream/core-api-go-mitigations.md and marked plainly as evaluated-not-adopted. Both mitigations work and the numbers are recorded — per-call WithoutRetries takes a POST against a 5xx from two requests to one; stripping Retry-After takes a 429 carrying "Retry-After: 30" from 30s to about 1s, keeping the status either way. So does the finding that made two mitigations necessary rather than one: Retrier.run sleeps before it checks the attempt counter, so disabling retries does not skip the wait. That belongs on record whichever way #40 goes. If upstream fixes the defects it is history; if the migration proceeds first, mitigation 1 is not optional because it guards the money endpoints, and the cost of it is the thing to weigh. --- docs/upstream/README.md | 12 +- docs/upstream/core-api-go-mitigations.md | 110 ++++++++++++++++++ docs/upstream/workarounds.md | 141 ----------------------- internal/sdkspike/spike.go | 19 +-- internal/sdkspike/spike_test.go | 122 ++------------------ internal/sdkspike/workaround.go | 135 ---------------------- 6 files changed, 135 insertions(+), 404 deletions(-) create mode 100644 docs/upstream/core-api-go-mitigations.md delete mode 100644 docs/upstream/workarounds.md delete mode 100644 internal/sdkspike/workaround.go diff --git a/docs/upstream/README.md b/docs/upstream/README.md index 8f7e0f0..16fc5af 100644 --- a/docs/upstream/README.md +++ b/docs/upstream/README.md @@ -2,17 +2,17 @@ Bug reports written against dependencies but **not yet filed**. They live here so the analysis is not lost between the day it is done and the day someone opens -the issue, and so a workaround in this repo can point at the reasoning behind it. +the issue — the work of reproducing a defect and verifying a fix is worth more +than the few minutes it takes to paste it into a tracker. When one is filed, replace its body with a link to the filed issue and keep the -file — the workaround it justifies will outlive the report. +file, so the reproduction stays with the repository that produced it. | Draft | Against | Status | |---|---|---| | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `namedotcom/core-api-go` v1.33.2 | not filed | | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `namedotcom/core-api-go` v1.33.2 | not filed | -Both are worked around in `internal/sdkspike`. See -[`workarounds.md`](workarounds.md) for what each one costs, why the two are not -one problem, how the test suite will tell you when upstream fixes them, and the -removal steps for each. +Neither is worked around in this repository. What a mitigation would look like, +and what each was measured to cost, is recorded in +[`core-api-go-mitigations.md`](core-api-go-mitigations.md) for the #40 decision. diff --git a/docs/upstream/core-api-go-mitigations.md b/docs/upstream/core-api-go-mitigations.md new file mode 100644 index 0000000..739d816 --- /dev/null +++ b/docs/upstream/core-api-go-mitigations.md @@ -0,0 +1,110 @@ +# Mitigations for the `core-api-go` retry defects — evaluated, not adopted + +**Nothing in this repository works around these defects.** This file records +what a mitigation would look like and what it was measured to cost, so the +information is available to the #40 decision and to the upstream reports +without anyone having to rediscover it. + +The defects themselves are drafted as issues in this directory: + +| # | Defect | Draft | +|---|---|---| +| 1 | `option.WithoutRetries()` is silently ignored at client scope | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | +| 2 | Retry backoff sleeps with a bare `time.Sleep`, ignoring the request context | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | + +## They are not one problem + +Worth recording, because the obvious simplification is wrong and the reasoning +is not visible from the API surface. + +Disabling retries does **not** skip the sleep. `Retrier.run` sleeps *before* it +checks the attempt counter: + +```go +if r.shouldRetry(response) { + delay, _ := r.retryDelay(response, retryAttempt) + time.Sleep(delay) // happens first + return r.run(..., retryAttempt+1, ...) // counter checked on entry here +} +``` + +So a call with retries disabled still issues one request, waits out the server's +full `Retry-After`, and only then declines to retry. Measured against a 429 +carrying `Retry-After: 30`: one request, thirty seconds. + +Any mitigation therefore needs two parts. Treating defect 2 as a consequence of +defect 1 leaves the wait in place, where it presents as a hang rather than +as a bug. + +## Mitigation for defect 1 — pass the option per call + +`option.WithoutRetries()` works per call; only the client-scoped form is +ignored. Measured against a 500: + +| wiring | requests | +|---|---| +| no option | 2 | +| `WithoutRetries()` on the client | 2 | +| `WithoutRetries()` per call | 1 | + +**Cost.** The option has to be repeated at every call site, and forgetting it +silently restores POST-on-5xx retries against endpoints that do not honour +idempotency keys — `CreateDomain`, `RenewDomain`, `ProcessRefund`. Convention +will not hold across ~53 call sites, so it would have to be enforced by +construction: a wrapper type keeping the SDK client unexported and exposing only +methods that append the option. That is roughly 53 hand-written pass-through +methods to maintain, and one more for every operation added upstream. + +This is the real cost to weigh in #40 — not the option itself, which is one line, +but the obligation to carry it across the whole API surface for as long as the +defect stands. + +## Mitigation for defect 2 — strip `Retry-After` before the SDK sees it + +Deleting the header from responses on their way back to the SDK makes +`retryDelay` fall through to its own `minRetryDelay`. Measured against a 429 +carrying `Retry-After: 30`, with retries already disabled per call: + +| response header | elapsed | requests | error | +|---|---|---|---| +| `Retry-After: 30` intact | 30s | 1 | `*core.APIError`, status 429 | +| `Retry-After` stripped | ~1s | 1 | `*core.APIError`, status 429 | + +**Why it would be safe.** By the time a response passes back through our +transport, `retryTransport` has already read `Retry-After`, made its retry +decision, and either waited on a context-aware timer or declined to wait at all +(the deadline guard from #41). The header has no remaining consumer in-process. +It would have to be scoped to the client handed to the SDK and not to the shared +transport, so `namecom api` and anything else reading raw responses still sees +what the server sent. + +**Cost.** Roughly one second of dead sleep would remain on every terminal 429 or +5xx — the SDK's own `minRetryDelay`, unreachable from outside the package. No +extra request and no lost status, but latency on the error path that does not +exist on the current generated client. + +## What this means for #40 + +Both mitigations work and were measured. Neither is adopted, and the preference +is that upstream fixes the defects instead — the suggested patches in both +drafts were verified against a local copy of v1.33.2 and are small. + +If the migration proceeds before a fix lands, mitigation 1 is not optional: it +guards the money endpoints. Mitigation 2 is a latency question and could be +skipped, at the cost of a call occasionally appearing to hang for up to the +SDK's 60s `maxRetryDelay`. + +## Reproducing the measurements + +The two issue drafts each carry a self-contained `main.go` that reproduces the +defect and prints the numbers above. Both were run against v1.33.2; the +suggested fixes were then applied to a local copy and the programs re-run to +confirm the fixes work and that `go test ./internal/...` still passes upstream. + +The defect-demonstration tests in `internal/sdkspike` cover the same ground as +part of the #40 spike, and are written to fail if a defect is ever fixed: + +| Test | Fires when | +|---|---| +| `TestWithoutRetriesHoldsAtClientScope/client-scoped_WithoutRetries_is_silently_ignored` | defect 1 is fixed | +| `TestSDKBackoffIgnoresContext` | defect 2 is fixed | diff --git a/docs/upstream/workarounds.md b/docs/upstream/workarounds.md deleted file mode 100644 index 84cb671..0000000 --- a/docs/upstream/workarounds.md +++ /dev/null @@ -1,141 +0,0 @@ -# Workarounds for `namedotcom/core-api-go` - -Two defects in `core-api-go` v1.33.2 are worked around in `internal/sdkspike`. -Neither report is filed yet; both are drafted in this directory. - -**The intent is that these are temporary.** Everything below is arranged so that -the day upstream fixes a defect, the test suite says so and the removal is -mechanical. - -## The two defects - -| # | Defect | Draft | Worked around in | -|---|---|---|---| -| 1 | `option.WithoutRetries()` is silently ignored at client scope | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `safeCallOptions` + the `Client` wrapper | -| 2 | Retry backoff sleeps with a bare `time.Sleep`, ignoring the request context | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `stripRetryAfter` + `sdkHTTPClient` | - -## They are not one problem - -This is the part worth reading before touching either workaround, because the -obvious simplification is wrong. - -Disabling retries does **not** skip the sleep. `Retrier.run` sleeps *before* it -checks the attempt counter: - -```go -if r.shouldRetry(response) { - delay, _ := r.retryDelay(response, retryAttempt) - time.Sleep(delay) // happens first - return r.run(..., retryAttempt+1, ...) // counter checked on entry here -} -``` - -So a call with retries disabled still issues one request, waits out the server's -full `Retry-After`, and only then declines to retry. Measured against a 429 -carrying `Retry-After: 30`: one request, thirty seconds. - -Removing workaround 2 because workaround 1 "already covers it" reintroduces -that, and it will look like a hang rather than a bug. - -## What each one costs - -Measured against a stub; see the tests named below. - -| Scenario | Unguarded | Guarded | -|---|---|---| -| POST against a 5xx | 2 requests | 1 request | -| 429 with `Retry-After: 30` | 30s | ~1s | -| Error typing | `*core.APIError`, status 429 | unchanged | - -### Residue - -**Roughly one second of dead sleep remains on every terminal 429 or 5xx.** That -is the SDK's own `minRetryDelay`, reached after `Retry-After` is stripped, and -it cannot be removed from outside the package. It is latency on the error path -only — no extra request, no lost status — but it is a real cost that arrives -with the SDK and does not exist on the current generated client. - -## Why workaround 2 is safe - -Deleting a response header is the kind of thing that should raise an eyebrow, so -the reasoning is here rather than only in the code. - -`stripRetryAfter` sits on the client handed to the SDK, *above* our own -transport. By the time a response reaches it, `retryTransport` has already read -`Retry-After`, made its retry decision, and either waited on a context-aware -timer or declined to wait at all (see the deadline guard added in #41). The -header has no remaining consumer in-process — except the SDK's dead sleep. - -It is scoped deliberately: - -- Applied to the client passed to `sdk.NewNamecom`. -- **Not** applied to the caller's own `*http.Client`. `sdkHTTPClient` copies the - client before swapping its transport, so `namecom api` and anything else - reading raw responses still sees the header the server sent. - -`TestStripIsScopedToTheSDKClient` fails if that copy is ever removed. - -## Why workaround 1 is a wrapper and not a convention - -`option.WithoutRetries()` has to be repeated on every call. An option that must -be remembered at ~53 call sites is one that will eventually be forgotten, and -forgetting it silently restores POST-on-5xx retries against endpoints that do -not honour idempotency keys — `CreateDomain`, `RenewDomain`, `ProcessRefund`. - -So `Client` keeps `*sdk.Namecom` unexported and exposes only wrapped methods. -There is no way to issue a request that skips `safeCallOptions` without editing -`workaround.go`. Adding an operation means adding a method, which is the moment -to notice. - -## How you will find out upstream has fixed it - -Two tests assert the *defects*, not the workarounds, so they fail when the -defects go away: - -| Test | Fires when | -|---|---| -| `TestWithoutRetriesHoldsAtClientScope/client-scoped_WithoutRetries_is_silently_ignored` | defect 1 is fixed | -| `TestSDKBackoffIgnoresContext` | defect 2 is fixed | - -Both were verified against a locally patched copy of v1.33.2 carrying the fixes -suggested in the drafts. They fail with messages that say what to do: - -``` -client-scoped WithoutRetries now holds (server saw 1 request) — -the SDK has been fixed; drop the per-call workaround and update issue #40 -``` - -``` -call took 102ms; expected the bare time.Sleep to overrun the deadline. -If the SDK has become context-aware, this finding is stale and the -recommendation should be revisited -``` - -The workaround tests keep passing under a fixed SDK — a fix makes them -redundant, not broken — so a dependency bump surfaces exactly these two and -nothing else. - -## Removing them - -### Defect 1 fixed upstream - -1. Delete `safeCallOptions` and the wrapped methods on `Client`. -2. Either export the SDK client or collapse `Client` to a constructor; keep - `option.WithoutRetries()` on `NewGuarded`, which now actually works. -3. Delete the `client-scoped ... is silently ignored` subtest. -4. Mark the draft filed-and-fixed in [`README.md`](README.md). - -### Defect 2 fixed upstream - -1. Delete `stripRetryAfter`, `sdkHTTPClient`, and - `TestStripIsScopedToTheSDKClient`. -2. Pass the caller's `*http.Client` straight to `option.WithHTTPClient`. -3. Delete `TestSDKBackoffIgnoresContext` and `TestWorkaroundCapsDeadSleep`. -4. The ~1s residue on terminal errors goes away with it. - -### Neither fixed, and the migration proceeds anyway - -The wrapper generalises: every service package needs the same treatment, so -`Client` grows from 5 methods to roughly 53. That is mechanical but large, and -it is the cost that belongs in the #40 decision — not the workaround itself, -which is small, but the obligation to maintain it across the whole API surface. diff --git a/internal/sdkspike/spike.go b/internal/sdkspike/spike.go index 56b9ec4..7052bb4 100644 --- a/internal/sdkspike/spike.go +++ b/internal/sdkspike/spike.go @@ -28,8 +28,13 @@ import ( // 5xx (it dispatches on status code alone) and sleeps with a bare time.Sleep // that ignores context. Ours refuses POST retries and returns the response // rather than sleeping past the deadline. Only one of the two may be live. -func New(baseURL, username, token string, httpClient *http.Client) *Client { - return NewGuarded(baseURL, username, token, httpClient) +func New(baseURL, username, token string, httpClient *http.Client) *sdk.Namecom { + return sdk.NewNamecom( + option.WithBaseURL(baseURL), + option.WithBasicAuth(username, token), + option.WithHTTPClient(httpClient), + option.WithoutRetries(), + ) } // NewWithSDKRetries is the same client with the SDK's retry layer left ON. @@ -48,7 +53,7 @@ func NewWithSDKRetries(baseURL, username, token string, httpClient *http.Client) // reports nextPage/lastPage as *int where the generated client used *int32, so // cmdutil.NextPage's signature does not carry over unchanged — this is the // int-width adaptation the migration needs, isolated so it can be judged. -func ListAllRecords(ctx context.Context, c *Client, domain string) ([]*coreapigo.Record, int, error) { +func ListAllRecords(ctx context.Context, c *sdk.Namecom, domain string) ([]*coreapigo.Record, int, error) { var ( all []*coreapigo.Record page = 1 @@ -56,7 +61,7 @@ func ListAllRecords(ctx context.Context, c *Client, domain string) ([]*coreapigo ) for { p := page - resp, err := c.ListRecords(ctx, &coreapigo.ListRecordsRequest{ + resp, err := c.DNS.ListRecords(ctx, &coreapigo.ListRecordsRequest{ DomainName: domain, Page: &p, }) @@ -105,8 +110,8 @@ type Changed struct { } // UpdateRecord mirrors runUpdate in cmd/dns/dns.go against the SDK. -func UpdateRecord(ctx context.Context, c *Client, domain string, id int, ch Changed) (*coreapigo.Record, error) { - current, err := c.GetRecord(ctx, &coreapigo.GetRecordRequest{ +func UpdateRecord(ctx context.Context, c *sdk.Namecom, domain string, id int, ch Changed) (*coreapigo.Record, error) { + current, err := c.DNS.GetRecord(ctx, &coreapigo.GetRecordRequest{ DomainName: domain, ID: id, }) @@ -145,7 +150,7 @@ func UpdateRecord(ctx context.Context, c *Client, domain string, id int, ch Chan body.Priority = ch.Priority } - updated, err := c.UpdateRecord(ctx, body) + updated, err := c.DNS.UpdateRecord(ctx, body) if err != nil { return nil, fmt.Errorf("updating record %d on %s: %w", id, domain, err) } diff --git a/internal/sdkspike/spike_test.go b/internal/sdkspike/spike_test.go index 07dcafe..f79c4cc 100644 --- a/internal/sdkspike/spike_test.go +++ b/internal/sdkspike/spike_test.go @@ -3,7 +3,6 @@ package sdkspike import ( "context" "encoding/base64" - "errors" "net/http" "net/http/httptest" "sync/atomic" @@ -11,8 +10,6 @@ import ( "time" coreapigo "github.com/namedotcom/core-api-go" - sdk "github.com/namedotcom/core-api-go/client" - "github.com/namedotcom/core-api-go/core" "github.com/namedotcom/core-api-go/option" ) @@ -38,7 +35,7 @@ func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { })) t.Cleanup(srv.Close) - c := rawClient(srv.URL, option.WithoutRetries()) + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }) @@ -46,10 +43,11 @@ func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { t.Fatal("expected the 500 to surface as an error") } // Asserted as the defect it is, so the day the SDK fixes it this test - // fails and tells us the per-call workaround can be dropped. + // fails and says so. Nothing here mitigates it — see + // docs/upstream/core-api-go-mitigations.md for what that would cost. if got := atomic.LoadInt32(&calls); got == 1 { t.Errorf("client-scoped WithoutRetries now holds (server saw 1 request) — " + - "the SDK has been fixed; drop the per-call workaround and update issue #40") + "the SDK has been fixed; update the draft in docs/upstream/ and issue #40") } else { t.Logf("server saw %d requests despite client-scoped WithoutRetries — option silently ignored", got) } @@ -66,7 +64,7 @@ func TestWithoutRetriesHoldsAtClientScope(t *testing.T) { })) t.Cleanup(srv.Close) - c := rawClient(srv.URL) + c := New(srv.URL, "u", "t", &http.Client{Timeout: 5 * time.Second}) _, _ = c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }, option.WithoutRetries()) @@ -303,7 +301,7 @@ func TestOurTransportSurvivesTheWiring(t *testing.T) { tr := &countingTransport{base: http.DefaultTransport} c := New(srv.URL, "u", "t", &http.Client{Transport: tr, Timeout: 5 * time.Second}) - if _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }); err != nil { t.Fatalf("ListRecords: %v", err) @@ -326,7 +324,7 @@ func TestBasicAuthMatchesOurHeader(t *testing.T) { t.Cleanup(srv.Close) c := New(srv.URL, "alice", "s3cret", &http.Client{Timeout: 5 * time.Second}) - if _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ + if _, err := c.DNS.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ DomainName: "example.com", }); err != nil { t.Fatalf("ListRecords: %v", err) @@ -337,109 +335,3 @@ func TestBasicAuthMatchesOurHeader(t *testing.T) { t.Errorf("Authorization = %q, want %q", got, want) } } - -// rawClient builds an unguarded SDK client, used only to demonstrate the two -// defects. Production wiring goes through NewGuarded, which is why *sdk.Namecom -// is unexported there. -func rawClient(baseURL string, opts ...option.RequestOption) *sdk.Namecom { - return sdk.NewNamecom(append([]option.RequestOption{ - option.WithBaseURL(baseURL), - option.WithBasicAuth("u", "t"), - option.WithHTTPClient(&http.Client{Timeout: 90 * time.Second}), - }, opts...)...) -} - -// ---- the workarounds themselves -------------------------------------------- - -// TestWorkaroundSuppressesDuplicateWrite is the one that matters for safety. -// The guarded client must issue a POST exactly once against a 5xx, without the -// caller having to remember anything. -func TestWorkaroundSuppressesDuplicateWrite(t *testing.T) { - var calls int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost { - t.Errorf("expected POST, got %s", r.Method) - } - atomic.AddInt32(&calls, 1) - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte(`{"message":"boom"}`)) - })) - t.Cleanup(srv.Close) - - c := New(srv.URL, "u", "t", &http.Client{Timeout: 30 * time.Second}) - _, err := c.CreateRecord(context.Background(), &coreapigo.DNSCreateRecordBody{ - DomainName: "example.com", - Type: coreapigo.DNSCreateRecordBodyType("A"), - Host: "spike", - Answer: "1.2.3.4", - }) - if err == nil { - t.Fatal("expected the 500 to surface as an error") - } - if got := atomic.LoadInt32(&calls); got != 1 { - t.Errorf("POST was sent %d times, want exactly 1 — the write was duplicated", got) - } -} - -// TestWorkaroundCapsDeadSleep covers the second defect, which disabling retries -// does NOT solve on its own: Retrier.run sleeps before it checks the attempt -// counter, so a call with retries off still waits out the server's Retry-After -// and only then declines to retry. -// -// Stripping the header on the way back to the SDK caps that at its -// minRetryDelay. Measured on the unguarded path: 30s. Guarded: ~1s. -func TestWorkaroundCapsDeadSleep(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Retry-After", "30") - w.WriteHeader(http.StatusTooManyRequests) - _, _ = w.Write([]byte(`{"message":"slow down"}`)) - })) - t.Cleanup(srv.Close) - - c := New(srv.URL, "u", "t", &http.Client{Timeout: 90 * time.Second}) - start := time.Now() - _, err := c.ListRecords(context.Background(), &coreapigo.ListRecordsRequest{ - DomainName: "example.com", - }) - elapsed := time.Since(start) - - if err == nil { - t.Fatal("expected the 429 to surface as an error") - } - if elapsed > 5*time.Second { - t.Errorf("call took %s; the Retry-After strip is not capping the SDK's dead sleep", elapsed) - } - // The status must survive the strip — exit-code mapping depends on it. - var apiErr *core.APIError - if !errors.As(err, &apiErr) { - t.Fatalf("error is not a *core.APIError: %v", err) - } - if apiErr.StatusCode != http.StatusTooManyRequests { - t.Errorf("status = %d, want 429 preserved through the strip", apiErr.StatusCode) - } - t.Logf("guarded call against Retry-After: 30 took %s and kept status %d", - elapsed.Round(100*time.Millisecond), apiErr.StatusCode) -} - -// TestStripIsScopedToTheSDKClient pins that the header removal does not leak -// into the caller's own transport. `namecom api` and anything else reading raw -// responses must still see what the server sent. -func TestStripIsScopedToTheSDKClient(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Retry-After", "7") - w.WriteHeader(http.StatusTooManyRequests) - })) - t.Cleanup(srv.Close) - - ours := &http.Client{Timeout: 30 * time.Second} - _ = sdkHTTPClient(ours) // wrapping must not mutate the client passed in - - resp, err := ours.Get(srv.URL) - if err != nil { - t.Fatalf("Get: %v", err) - } - defer func() { _ = resp.Body.Close() }() - if got := resp.Header.Get("Retry-After"); got != "7" { - t.Errorf("Retry-After = %q on our own client, want %q — the strip leaked", got, "7") - } -} diff --git a/internal/sdkspike/workaround.go b/internal/sdkspike/workaround.go deleted file mode 100644 index c37c4da..0000000 --- a/internal/sdkspike/workaround.go +++ /dev/null @@ -1,135 +0,0 @@ -package sdkspike - -import ( - "context" - "net/http" - - coreapigo "github.com/namedotcom/core-api-go" - sdk "github.com/namedotcom/core-api-go/client" - "github.com/namedotcom/core-api-go/option" -) - -// Workarounds for two defects in core-api-go v1.33.2. Both are written up in -// docs/upstream/ and neither is filed yet. docs/upstream/workarounds.md carries -// the measurements, the residual cost, and the removal steps for each; the two -// tests that fail when upstream fixes a defect are named there too. -// -// 1. option.WithoutRetries() is silently ignored at client scope, so the SDK -// retries a POST on a 5xx even when the client asked it not to. -// 2. The retry backoff sleeps with a bare time.Sleep, so it ignores the -// request context and can outlast the caller's deadline. -// -// They interact in a way that is not obvious and cost a measurement to find: -// disabling retries does NOT skip the sleep. Retrier.run sleeps BEFORE it -// checks the attempt counter, so a call with retries disabled still waits the -// full Retry-After and only then declines to retry. Against a 429 carrying -// "Retry-After: 30" that is thirty seconds of dead time for one request. -// -// So the two defects need two separate workarounds, and neither substitutes for -// the other. - -// safeCallOptions are appended to every SDK call. -// -// WithoutRetries has to be per-call because the client-scoped form does not -// work — see defect 1. That is the whole reason Client below exists: an option -// that must be repeated at every call site is an option someone will eventually -// forget, and forgetting it silently restores POST-on-5xx retries against -// endpoints that do not honour idempotency keys. -func safeCallOptions() []option.RequestOption { - return []option.RequestOption{option.WithoutRetries()} -} - -// stripRetryAfter removes the Retry-After header from responses on their way -// back to the SDK. -// -// This is the workaround for defect 2, and it is only safe because of where it -// sits. By the time a response leaves our RoundTripper, our own transport has -// already read Retry-After, made its retry decision, and — per #41 — either -// waited on a context-aware timer or declined to wait at all. The header has no -// remaining consumer inside the process. What it still has is the SDK's dead -// sleep downstream, which reads it and blocks for up to maxRetryDelay (60s) -// with no way to interrupt. -// -// Deleting it caps that dead sleep at the SDK's minRetryDelay of one second. -// Measured against a 429 carrying "Retry-After: 30": 30s before, 1s after, with -// *core.APIError still carrying status 429 and the response body in both cases. -// -// It is deliberately NOT applied to the shared transport — only to the client -// handed to the SDK — so that `namecom api` and anything else reading raw -// responses still sees the header the server sent. -type stripRetryAfter struct{ base http.RoundTripper } - -func (s stripRetryAfter) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := s.base.RoundTrip(req) - if resp != nil { - resp.Header.Del("Retry-After") - } - return resp, err -} - -// sdkHTTPClient wraps our own client for handing to the SDK, adding the -// Retry-After strip and nothing else. The rate limiter, the POST-on-5xx -// refusal, and the deadline guard all live in base's transport and are -// untouched. -func sdkHTTPClient(base *http.Client) *http.Client { - wrapped := *base // copy: the caller's client keeps its own transport - inner := base.Transport - if inner == nil { - inner = http.DefaultTransport - } - wrapped.Transport = stripRetryAfter{base: inner} - return &wrapped -} - -// Client is the only supported way to reach the SDK from this package. -// -// It keeps *sdk.Namecom unexported on purpose. The per-call WithoutRetries -// workaround cannot be enforced by convention across ~53 call sites, so it is -// enforced by construction instead: there is no way to issue a request that -// skips safeCallOptions without editing this file. -// -// If defect 1 is fixed upstream, this type collapses to a plain client -// constructor and the methods go away. -type Client struct { - sdk *sdk.Namecom -} - -// NewGuarded builds a client with both workarounds applied. -func NewGuarded(baseURL, username, token string, httpClient *http.Client) *Client { - return &Client{ - sdk: sdk.NewNamecom( - option.WithBaseURL(baseURL), - option.WithBasicAuth(username, token), - option.WithHTTPClient(sdkHTTPClient(httpClient)), - // Kept even though it does not work at client scope: it is the - // declaration of intent, and it starts working the day the SDK is - // fixed. safeCallOptions is what actually holds today. - option.WithoutRetries(), - ), - } -} - -// ListRecords lists one page of DNS records. -func (c *Client) ListRecords(ctx context.Context, req *coreapigo.ListRecordsRequest) (*coreapigo.ListRecordsResponse, error) { - return c.sdk.DNS.ListRecords(ctx, req, safeCallOptions()...) -} - -// GetRecord reads a single DNS record. -func (c *Client) GetRecord(ctx context.Context, req *coreapigo.GetRecordRequest) (*coreapigo.Record, error) { - return c.sdk.DNS.GetRecord(ctx, req, safeCallOptions()...) -} - -// CreateRecord creates a DNS record. -func (c *Client) CreateRecord(ctx context.Context, req *coreapigo.DNSCreateRecordBody) (*coreapigo.Record, error) { - return c.sdk.DNS.CreateRecord(ctx, req, safeCallOptions()...) -} - -// UpdateRecord replaces a DNS record. -func (c *Client) UpdateRecord(ctx context.Context, req *coreapigo.DNSUpdateRecordBody) (*coreapigo.Record, error) { - return c.sdk.DNS.UpdateRecord(ctx, req, safeCallOptions()...) -} - -// DeleteRecord removes a DNS record. -func (c *Client) DeleteRecord(ctx context.Context, req *coreapigo.DeleteRecordRequest) error { - return c.sdk.DNS.DeleteRecord(ctx, req, safeCallOptions()...) -} From f00db0412eabd5875402eb9ad819fa082b930b9f Mon Sep 17 00:00:00 2001 From: patramsey Date: Thu, 20 Aug 2026 11:13:20 -0600 Subject: [PATCH 5/5] docs: record the upstream issues as filed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both defects are now open against namedotcom/core-api-go: #3 for the ignored client-scoped WithoutRetries, #4 for the context-ignoring backoff. Line numbers were re-checked against upstream main before filing — v1.33.2 is still the latest tag and every reference still matches. The drafts keep their bodies rather than being reduced to links. The reproduction programs and the verified fixes are the expensive part of this work, and they should survive the tracker rather than depend on it. --- docs/upstream/README.md | 11 ++++++----- docs/upstream/core-api-go-backoff-ignores-context.md | 3 +++ docs/upstream/core-api-go-mitigations.md | 12 +++++++----- docs/upstream/core-api-go-withoutretries-ignored.md | 3 +++ 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/docs/upstream/README.md b/docs/upstream/README.md index 16fc5af..80e55c0 100644 --- a/docs/upstream/README.md +++ b/docs/upstream/README.md @@ -1,17 +1,18 @@ # Upstream issue drafts -Bug reports written against dependencies but **not yet filed**. They live here so +Bug reports written against dependencies. They live here so the analysis is not lost between the day it is done and the day someone opens the issue — the work of reproducing a defect and verifying a fix is worth more than the few minutes it takes to paste it into a tracker. -When one is filed, replace its body with a link to the filed issue and keep the -file, so the reproduction stays with the repository that produced it. +Each carries a banner naming the filed issue once it is filed. The body stays +put rather than being replaced by a link — the reproduction and the verified fix +are the expensive part, and they should survive the tracker. | Draft | Against | Status | |---|---|---| -| [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `namedotcom/core-api-go` v1.33.2 | not filed | -| [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `namedotcom/core-api-go` v1.33.2 | not filed | +| [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | `namedotcom/core-api-go` v1.33.2 | [filed as #3](https://github.com/namedotcom/core-api-go/issues/3) | +| [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | `namedotcom/core-api-go` v1.33.2 | [filed as #4](https://github.com/namedotcom/core-api-go/issues/4) | Neither is worked around in this repository. What a mitigation would look like, and what each was measured to cost, is recorded in diff --git a/docs/upstream/core-api-go-backoff-ignores-context.md b/docs/upstream/core-api-go-backoff-ignores-context.md index fe3bfa6..14c9fb5 100644 --- a/docs/upstream/core-api-go-backoff-ignores-context.md +++ b/docs/upstream/core-api-go-backoff-ignores-context.md @@ -1,5 +1,8 @@ # Retry backoff ignores the request context +**Filed 2026-08-20 as [namedotcom/core-api-go#4](https://github.com/namedotcom/core-api-go/issues/4).** Kept here so the reproduction stays with the repository that produced it. + + **Version:** v1.33.2 · **Go:** 1.26.6 ## Summary diff --git a/docs/upstream/core-api-go-mitigations.md b/docs/upstream/core-api-go-mitigations.md index 739d816..04ff403 100644 --- a/docs/upstream/core-api-go-mitigations.md +++ b/docs/upstream/core-api-go-mitigations.md @@ -5,12 +5,12 @@ what a mitigation would look like and what it was measured to cost, so the information is available to the #40 decision and to the upstream reports without anyone having to rediscover it. -The defects themselves are drafted as issues in this directory: +The defects themselves are filed upstream and drafted in this directory: -| # | Defect | Draft | +| # | Defect | Upstream | |---|---|---| -| 1 | `option.WithoutRetries()` is silently ignored at client scope | [`core-api-go-withoutretries-ignored.md`](core-api-go-withoutretries-ignored.md) | -| 2 | Retry backoff sleeps with a bare `time.Sleep`, ignoring the request context | [`core-api-go-backoff-ignores-context.md`](core-api-go-backoff-ignores-context.md) | +| 1 | `option.WithoutRetries()` is silently ignored at client scope | [#3](https://github.com/namedotcom/core-api-go/issues/3) · [draft](core-api-go-withoutretries-ignored.md) | +| 2 | Retry backoff sleeps with a bare `time.Sleep`, ignoring the request context | [#4](https://github.com/namedotcom/core-api-go/issues/4) · [draft](core-api-go-backoff-ignores-context.md) | ## They are not one problem @@ -87,7 +87,9 @@ exist on the current generated client. Both mitigations work and were measured. Neither is adopted, and the preference is that upstream fixes the defects instead — the suggested patches in both -drafts were verified against a local copy of v1.33.2 and are small. +drafts were verified against a local copy of v1.33.2 and are small. Both are now +filed as [#3](https://github.com/namedotcom/core-api-go/issues/3) and +[#4](https://github.com/namedotcom/core-api-go/issues/4). If the migration proceeds before a fix lands, mitigation 1 is not optional: it guards the money endpoints. Mitigation 2 is a latency question and could be diff --git a/docs/upstream/core-api-go-withoutretries-ignored.md b/docs/upstream/core-api-go-withoutretries-ignored.md index c9c3bae..6d6c6f0 100644 --- a/docs/upstream/core-api-go-withoutretries-ignored.md +++ b/docs/upstream/core-api-go-withoutretries-ignored.md @@ -1,5 +1,8 @@ # `option.WithoutRetries()` is silently ignored when set on the client +**Filed 2026-08-20 as [namedotcom/core-api-go#3](https://github.com/namedotcom/core-api-go/issues/3).** Kept here so the reproduction stays with the repository that produced it. + + **Version:** v1.33.2 · **Go:** 1.26.6 ## Summary