PR #796 - basecamp/basecamp-sdk

Closes #795. go/pkg/basecamp/client.go:694 read its server-specified retry delay off a *retryableError. Nothing in the tree ever built one.

$ rg -n "retryableError" go/
go/pkg/basecamp/http.go:115:// retryableError wraps an error with retry metadata.
go/pkg/basecamp/http.go:117:type retryableError struct {
go/pkg/basecamp/http.go:122:func (r *retryableError) Error() string {
go/pkg/basecamp/http.go:126:func (r *retryableError) Unwrap() error {
go/pkg/basecamp/client.go:694:		if re, ok := err.(*retryableError); ok {

The type, its two methods, one type assertion. No &retryableError{. So the branch was unreachable, every 429 landed on the else if apiErr arm, and the loop backed off locally against a server that had named a time. The number was parsed — client.go:847 calls parseRetryAfter — and then spent on a hint string, because Error had nowhere to put it.

Scope: the raw path only. Typed service methods go through the generated client's loop, which reads the header inline and honours it. downloadURL and the rate-limiter hook honour it too. This was Client.Get/GetAll and the escape hatch, which is why conformance never saw it.

Shape: `Error.RetryAfter`, and `retryableError` deleted

Of the two candidates in #775's remedy table, only one of them removes a mechanism. Constructing retryableError at the 429 arm keeps two error types flowing through one loop and changes what singleRequest returns to every other caller — including the mutation path's err.(*Error) at client.go:664. Putting the value on Error makes the *Error arm the only arm, and the dead type goes with the branch that was its only reader.

Nothing is lost by deleting it. Unwrap is load-bearing for errors.Is/errors.As only through a value that reaches a caller, and no value ever existed — apidiff confirms it was never public surface either.

The field is populated at both 429 construction sites — ErrRateLimit for the raw path, checkResponse for the generated service layer — so a caller reading err.RetryAfter gets the same answer whichever door it came through. Half-populating it would have been worse than not having it.

What did NOT change: which statuses carry a value

Only the two 429 sites set the field, matching downloadURL and the generated loop. #775 decides whether 503 and the rest join them, and its five-SDK convergence is not this PR's. The code says so at the branch. No ceiling is applied either — #793 is settling that in SPEC.

Red proof

Reverting only the honour hunk — the loop back to delay = c.backoffDelay(attempt), everything else including the tests unchanged — and running go test ./pkg/basecamp/ -run RetryAfter:

    client_retry_after_test.go:127: computed a 1.42974ms retry delay, want 2s from the Retry-After header (the backoff curve here is ~1ms, so this is the backoff, not the server's number)
--- FAIL: TestClient_RetryAfterReplacesBackoff (0.00s)
    client_retry_after_test.go:147: computed a 1.639404ms retry delay for an HTTP-date 90s out, want ~90s
--- FAIL: TestClient_RetryAfterHTTPDateReplacesBackoff (0.00s)
--- PASS: TestClient_RetryAfterAbsentOrUnusableKeepsBackoff (0.00s)
FAIL	github.com/basecamp/basecamp-sdk/go/pkg/basecamp	1.342s
REAL_EXIT=1

Five mutants, five kills. Files restored by cp and verified with diff -q, never git checkout --.

Mutation	Killed by
loop back to `backoffDelay` unconditionally	`RetryAfterReplacesBackoff`, `…HTTPDate…` — 1.4ms vs 2s
`if apiErr.RetryAfter > 0` → `if true`	`…AbsentOrUnusableKeepsBackoff` — `[0s 0s]`, backoff collapsed
retry wait's `select` → plain `time.Sleep`	both probe tests — 2 delays computed where a cancelled request must compute 1
`checkResponse` drops the field	`CheckResponse_CarriesRetryAfter/seconds` — `0, want 17`
`ErrRateLimit` drops the field	`…ErrorCarriesSeconds` — `0, want 42`; and `…ReplacesBackoff`

The tests spend no wall clock and add no seam

The first version of this branch gave Client a retrySleep func(...) field so tests could read the computed delay without sleeping it. The api-compat job from #776 rejects that, and correctly:

Incompatible changes:
- ./pkg/basecamp.Client: old is comparable, new is not

A func field makes the struct non-comparable. Rather than work around the gate, the seam is gone: the loop already logs the computed delay and then fires OnRetry, both before it sleeps. Cancelling from OnRetry returns the wait instantly however long it was, and the delay is read off the "retrying request" log record's delay attribute — an exact value, from observables the SDK already emits, at zero production cost. apidiff on the branch as it stands:

Compatible changes:
- ./pkg/basecamp.Error.RetryAfter: added

No test in this file measures elapsed time as an assertion (#783). The one timing check in retryAfterProbe is a 5s tripwire whose only job is to turn "the wait stopped observing cancellation" into a message instead of a package timeout, and it is one-sided: load can only push a ~2ms measurement toward a bound three orders of magnitude away.

Interruptibility: yes, and now proven

The wait is a select on ctx.Done() and time.After(delay), so a caller can abandon a long server-directed delay — which matters precisely because no ceiling is applied to it. The select is unchanged by this PR; what changed is that it can now be reached holding a five-minute delay instead of a one-second one.

It was previously untested. The obvious test does not work: signalling from the handler and cancelling from a goroutine cancels while the response is still in flight, so the loop takes the network-error path, backs off ~1ms, and returns a wrapped context.Canceled that satisfies every assertion — that version passes against a plain uninterruptible time.Sleep, which is how the mutant survived the first round here. Cancelling from OnRetry is what pins it, and now every case in the file carries that guard.

download.go:205 and the generated loop are the same select; all three honour cancellation.

Generated path: not affected, but not identical either

client.gen.go:5805-5813 reads and honours Retry-After on 429 inline. It never had this defect, go/templates/client.tmpl is untouched, and nothing was regenerated.

Two pre-existing differences, reported rather than fixed:

It parses delta-seconds only (strconv.Atoi), so SPEC §6 step 2's HTTP-date form is dropped — the same shape #781 just closed in Kotlin and TypeScript, in a fourth copy of the algorithm that PR did not reach. The raw path resolves both forms; a test here covers it.
It adds jitter on top of a server-directed delay. The raw path and downloadURL replace the curve outright.

Neither belongs in a bug fix for a dead branch; both are worth an issue.

SPEC

§5's BasecampError RECORD already listed retry_after, with a Go divergence note saying Go omits it. That note is now false and is rewritten. RequestResult.retry_after is untouched — it stays the hook-facing copy, no longer the only one.

Left alone deliberately: §6's "Go on 429 and 503" in the status-set paragraph. It measures http.go:203, which feeds hooks and the rate limiter, not the sleep — and that paragraph is explicitly #775's.

Verification

LC_ALL=C on all of it: make go-check 0 · go-check-drift 0 · go-check-wrapper-drift 0 · doc-constants-check 0 · conformance-go 0 (all six Retry-After cases PASS) · check-retry-metadata-parity 0 · apidiff -m against origin/main compatible.

Review follow-up: the conversion is clamped

Two reviewers independently found the same defect in the honour hunk, and it is real: Retry-After: 9223372036854775807 parses cleanly on a 64-bit build and time.Duration(n) * time.Second wraps to -1s, so time.After fires at once and the loop spends its whole attempt budget back to back — the newly honoured header turned into a tight retry loop.

Both doors onto Error.RetryAfter now normalize through clampRetryAfterSeconds: parseRetryAfter, which also feeds checkResponse, downloadURL and RequestResult, and the exported ErrRateLimit, which takes a bare int and could therefore also carry a negative value the field's own doc calls invalid.

Over-range that the parser can hold saturates rather than falling back to absent — falling back would compute the ~1ms backoff curve and hammer the peer, the same tight loop by another route — while a digit string the parser's own int64 cannot hold is malformed and falls through to backoff (bf45888bc; the tiers are #793's, the cross-SDK decision #799's). This amends "no ceiling is applied either" above: what is applied is a representability bound, not the policy cap #793 declined. At 2147483647s (~68 years) it rejects nothing a server could sensibly ask for, and SPEC §7 note 4 already carves out bounding against host limits (Swift's UInt64 trap is its worked example). This PR no longer touches that sentence; #793 is the lane that adds Go to it.

Four mutants, four kills: the clamp out of parseRetryAfter (CheckResponse_CarriesRetryAfter/beyond_duration_range), out of ErrRateLimit (ErrRateLimit_NormalizesRetryAfter/{negative, beyond duration range}), out of both — the reported state — (RetryAfterSaturatesAtDurationCeiling, which reports 2 delays ([-1s 2.802726ms]), the tight loop itself), and saturate replaced by treat-as-absent (same test, 1.964215ms, want 2562047h47m16s). Neither single-guard mutant is caught by the end-to-end case, so each clamp carries its own killing test.

The generated client keeps the identical unclamped conversion at client.gen.go:5810 (go/templates/client.tmpl:457), plus an unchecked Atoi feeding NewRateLimitError. Not fixed here — it needs a template change and a regeneration this PR deliberately does not do — and filed as #798 alongside the two other divergences in that loop reported above.

Summary by cubic

Honor Retry-After in Go’s raw GET retry loop and make waits cancellation-first. Previously 429s ignored Retry-After and used millisecond backoff; now the loop sleeps the server’s delay (delta-seconds or HTTP-date), rounds HTTP-date up to whole seconds, clamps to 2,147,483,647s, and checks context before waiting.

Add basecamp.Error.RetryAfter (seconds). ErrRateLimit and checkResponse set it; the raw loop uses it when > 0. Remove dead retryableError.
Parse delta-seconds as digits-only into int64 and clamp via a shared clampRetryAfterSeconds; HTTP-date converts with integer math and rounds up. Shared by downloadURL and the rate-limiter hook.
Cancellation-first: both GET and download retry waits check ctx.Err() before the select.
Generated clients unchanged (seconds-only parse, jitter, no clamp). SPEC now states step-2 “round up” and clears pending markers.

Migration

Replace any unkeyed basecamp.Error{...} literals with keyed fields; RetryAfter was inserted between Retryable and RequestID.
When rescheduling from a returned error, use errors.As to extract *basecamp.Error and read RetryAfter. 429 waits can be long; prefer context cancellation or reschedule using RetryAfter.