basecamp / basecamp/basecamp-sdk

Generated Go client's retry loop converts Retry-After to a Duration without clamping

Closed
#798 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
49
Forks
12
Avg merge
20h 47m
Merged PRs (30d)
89

Description

`go/templates/client.tmpl:457` — emitted at `go/pkg/generated/client.gen.go:5810` — does:

```go
if seconds, err := strconv.Atoi(retryAfter); err == nil && seconds > 0 {
retryDelay = time.Duration(seconds) * time.Second
}
```

On a 64-bit build `Retry-After: 9223372036854775807` passes `strconv.Atoi`, and the product wraps to `-1s`. The loop then waits `time.After(retryDelay + jitter)`, which for a negative `retryDelay` is already expired, so a typed operation burns its whole attempt budget back to back against a server that asked it to wait.

This is the same defect two reviewers found on #796 in the hand-written raw path, where it is now fixed: `parseRetryAfter` and `ErrRateLimit` saturate at `math.MaxInt64 / time.Second` (~292 years), a representability bound sanctioned by SPEC §7 note 4 rather than a policy cap (#793). The generated loop keeps a fourth, independent copy of the algorithm and was left alone there because fixing it means a template change plus a regeneration, which #796 deliberately does not do.

Also in the same emitted file, `checkResponse`'s 429 arm (`client.tmpl` ~1328) builds `NewRateLimitError(retryAfter)` from an unchecked `strconv.Atoi`, so the generated error type can carry an over-range value for a caller to multiply.

Related to the two other pre-existing divergences #796 reports in the same loop: it parses delta-seconds only (SPEC §6 step 2's HTTP-date form is dropped, the shape #781 closed in Kotlin and TypeScript), and it adds jitter *on top of* a server-directed delay where the raw path and `downloadURL` replace the curve outright. All three are worth settling in one pass over `client.tmpl`.

Contributor guide

Open the contributing guide

Research direction

Start in go/templates/client.tmpl at the retry loop around line 457 and the 429 handling around line 1328, then compare the raw path fixed in #796 and SPEC §6 and §7 note 4. Regenerate go/pkg/generated/client.gen.go and verify retry-after values cannot overflow, HTTP-date handling and jitter match the intended behavior, and generated 429 errors remain safe for callers.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.