Spend a rate-limit token on every request, not every operation

[robzolkos]

What

Every HTTP request the CLI's SDK client sends now takes a token from the shared, cross-process token bucket in internal/resilience (50 burst, 10/s refill, unchanged). Until now the bucket was charged once per SDK operation, in OnOperationGate. The operation's token now pays for its first request, so a one-request command costs one token, the same as before.

A RoundTripper (internal/resilience/transport.go) wraps the transport the SDK client already uses. When the bucket is empty it waits for a refill and does not fail. It stops if the caller cancels or the request's own timeout expires. The operation gate, the bulkhead, the circuit breaker and the Retry-After block are all unchanged.

Why

The bucket's own docs say it limits requests (TokensPerRequest, "client limit 10/s"). In practice, these requests never reached it:

• Pages 2..N of every --all walk. The SDK follows the Link header with raw doRequestURL calls that never pass through the operation gate (client.go:640 @ SDK 35f3d72d).
• basecamp api get (api.go:55) and the /projects.json walk that --in <id> makes on every invocation (resolver.go:432). The SDK's raw Get/GetAll aren't gated operations (client.go:355).
• Retries.

So a page walk ran as fast as the server answered, and parallel api get loops had no limit at all. That matches the traffic shape behind the 15 October recordings.json load (an old CLI build, OAuth client 19503): about 12.8 requests/s with about 6 in flight on average, walking projects × types × pages.

Measured against a local mock server

All runs used real CLI binaries against a synthetic HTTP server with a fresh state directory each time.

Scenario	main	this branch
1-page command, 80 ms server	2 requests, 0.17 s	2 requests, 0.16 s
200-page --all, 80 ms server	16.4 s, 12.2/s	16.3 s, 12.4/s (fits in burst + refill)
200-page --all, 0 ms server	0.23 s, ~877/s	15.1 s, 13.3/s mean (burst then 10/s)
8 parallel api get loops × 40 pages, 470 ms server	15.8/s, 160 per 10 s	10.8/s, 149 per 10 s
8 parallel api get loops × 60 pages, 0 ms server	145/s	11.2/s

Normal commands don't change. Bulk walks and agent loops now hold to the bucket's 10/s across every basecamp process sharing a cache directory. That means one OS user's processes on one machine, across all of that user's profiles.

Testing

• bin/ci is green locally, including e2e.
• New tests in internal/resilience/transport_test.go run a real SDK client against httptest and count tokens spent:
  • a 5-page walk spends 5
  • 3 raw Gets spend 3
  • a 503-then-200 retry spends 2
  • a single-page operation spends 1
• Red/green: with Transport returning the bare base transport, the walk, raw and retry tests fail (spent 1, 0 and 0). If the prepaid marker is removed, the walk and single-page tests fail (spent 6 and 2), which shows nothing is double-charged.
• Pace waits for a refill on a held clock (2 waits, 1 s for 3 tokens at 2/s) and returns context.Canceled when the caller cancels mid-wait.

Trade-offs and what this does not do

• Numbers unchanged on purpose. At the incident's ~470 ms per request, 10/s is still about 47 s of server time per 10 s, against the 20 s budget in basecamp/bc3#13873. Matching the documented 50 per 10 s (5/s), or capping in-flight requests (by Little's law, server-seconds per second = requests in flight), would need a person's call. This PR is not that call.
• Under extreme contention (hundreds of waiters), a paced request can reach the SDK's 30 s client timeout. It then fails as a network error, not as the gate's named "client limit" error.
• The transport does not apply the shared Retry-After block. The operation gate still does, and the SDK's retry loop still honours each response's Retry-After. Raw api get during a block is now paced, but not blocked.
• api get and name lookups still don't take a bulkhead slot. The bulkhead counts processes, not requests.
• --in <numeric id> still walks /projects.json on every invocation. That's a separate, cheap follow-up.
• Already-installed old CLIs are unaffected. Only server-side enforcement covers them.

---

Summary by cubic

Spends one rate-limit token on every HTTP request the CLI's SDK client sends, instead of once per SDK operation. Requests that previously bypassed the shared cross-process token bucket — pages 2..N of --all walks, raw basecamp api get calls, and retries — are now paced at the documented 10/s across every process sharing a cache directory.

Behavior notes

• A new pacedTransport wraps the SDK client's existing transport and waits for a refill when the bucket is empty instead of failing; it stops on caller cancellation or the request timeout, closing the request body and sending nothing when canceled mid-wait.
• The operation gate's token now pays for that operation's first request, so nothing is charged twice; retries of a typed operation spend a token per attempt. The gate, bulkhead, circuit breaker, and Retry-After block are unchanged.
• api get is paced but still not subject to the shared Retry-After block or the bulkhead; under heavy contention a paced request can reach the SDK's 30 s timeout and fail as a network error.
• Already-installed CLI versions are unaffected.

^{Written for commit 15065d0. Summary will update on new commits.}

[Copilot]

🟡 Changes recommended

Cancellation during pacing leaves request bodies unclosed, violating the transport’s cleanup contract.

1 open finding

• Close request body when Pace is canceled or times out · New

What changed in this PR

Extends the CLI’s shared rate limiter to charge each SDK HTTP request, including pagination, raw calls, and retries.

Changes:

• Adds transport-level pacing with cancellation support.
• Reuses the operation’s token for its first request.
• Adds token-accounting and pacing tests.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File	Description
internal/​resilience/​transport.go	Adds pacing and prepaid-token handling.
internal/​resilience/​transport_test.go	Tests request accounting, refill waits, and cancellation.
internal/​resilience/​rate_limiter.go	Adds cancellable token pacing.
internal/​resilience/​hooks.go	Marks the first request as prepaid.
internal/​appctx/​context.go	Wraps the SDK transport with pacing.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approved

No unresolved blocking issues remain, and the previous request-body cleanup issue is fixed with regression coverage.

0 open findings

1 resolved since last review

• Close request body when Pace is canceled or times out

🧠 Review effort: Balanced

[robzolkos]

🤖 Review summary for head 15065d0:

• Copilot. Asked for the request body to be closed when pacing fails. Fixed in 15065d0, the thread is resolved, and Copilot approved this head.
• Pi (gpt-6.1-sol, xhigh), first head. Found the same body-close issue (fixed), and asked for a test that retries a typed operation; TestARetriedOperationSpendsATokenPerAttempt is added. Two findings declined:
  • It noted that basecamp connect clients aren't paced. Declined: they're separate long-running loops with their own backoff, and this PR covers the app's SDK client only.
  • It asked for exhausted-bucket HTTP timeout coverage. Declined beyond the cancelled-wait test: the SDK's 30 s client timeout cancels the same context Pace already honours.
• Pi, final head. No findings.

Open for a person, not this PR: whether to lower the defaults (50 burst, 10/s) or to cap in-flight requests. The PR body has the numbers.