Skip to content

Read the rate-limit headers, and hold a 429 once per client - #48

Open
cubehouse wants to merge 4 commits into
mainfrom
feat/rate-limit-awareness
Open

cubehouse wants to merge 4 commits into
mainfrom
feat/rate-limit-awareness

Conversation

@cubehouse

Copy link
Copy Markdown
Member

Same treatment as ThemeParks_Python#27, same reasoning.

The SDK read exactly one header, Retry-After, and only after a 429 had already happened. It could tell you that you had run out, never that you were about to.

Both meters, on the client

import { ThemeParks, isExhausted, secondsUntilReset } from 'themeparks';

const tp = new ThemeParks({ apiKey: KEY });
await tp.entity(parkId).live();

tp.rateLimit.rest.remaining;            // 299
secondsUntilReset(tp.rateLimit.rest);   // 40
tp.rateLimit.history.remaining;         // on a history call

The per-minute REST meter, and the hourly history budget the API started publishing today.

Absence is not zero, and the design turns on it

An unmetered plan advertises no figures, and neither does a publicly cacheable response, because the numbers are per-caller and a shared cache would hand one caller's budget to another. Anonymous calls therefore carry nothing.

So null means the server did not say. isExhausted is true only when it said zero. Reading an unknown as zero would stall every anonymous client permanently, and it is the first thing the tests pin.

reset is a relative countdown frozen when it was read, so secondsUntilReset ages it. Using the raw value later is how a client waits an hour longer than it needs to — the same bug the server had in its cached headers.

It acts on what it reads

A window the server said is spent is waited out rather than walked into: that request is a certain 429 that also spends a unit of budget being refused. retry: { respectRemaining: false } opts out.

A 429 is now held once for the whole client

The wait belongs to the caller, not to whichever request met it. Ten concurrent requests each slept their own Retry-After and then retried at the same instant, re-tripping the limit together.

It goes on a shared gate with a little jitter, taken once, and the retry path no longer pays it a second time. A shorter wait arriving while a longer one is in force no longer brings the gate forward. Past maxRetryAfterMs the gate is deliberately left open: we throw instead, and blocking the caller's next call for most of an hour is the opposite of letting them checkpoint.

Verification

  • 123 tests, 21 new. Mutation-checked: treating an unknown remaining as exhausted fails 3, letting a blank response erase what we knew fails 1, bringing the gate forward on a shorter wait fails 1, removing the jitter fails 1, not ageing the reset fails 2.
  • Run against production: limit=300 remaining=296 reset=40 policy="300;w=60" on a real anonymous call, history meter correctly absent, isExhausted false.
  • tsc, eslint, prettier and the build are clean.

One note: the cache-wrapper bug that Python hit doesn't exist here, because the client holds the inner transport directly. There's a test for it anyway — caching is on by default and every other test turns it off, which is exactly the blind spot that let it through on the Python side.

🤖 Generated with Claude Code

cubehouse and others added 4 commits September 24, 2026 17:29
Same treatment as the Python sibling, same reasoning.

The SDK read exactly one header, Retry-After, and only after a 429 had already
happened. It could tell you that you had run out, never that you were about to.

Both meters are now on the client. The per-minute REST one, and the hourly
history budget the API started publishing today:

    tp.rateLimit.rest.remaining
    tp.rateLimit.history.remaining
    secondsUntilReset(tp.rateLimit.rest)

ABSENCE IS NOT ZERO, and the design turns on it. An unmetered plan advertises
no figures, and neither does a publicly cacheable response, because the numbers
are per-caller and a shared cache would hand one caller's budget to another --
so anonymous calls carry nothing. null means the server did not say.
isExhausted is true only when it said zero. Reading an unknown as zero would
stall every anonymous client permanently, which is the first thing the tests
pin.

reset is a relative countdown frozen when it was read, so secondsUntilReset
ages it. Using the raw value later is how a client waits an hour longer than it
needs to, and it is the same bug the server had in its cached headers.

The client acts on what it reads: a window the server said is spent is waited
out rather than walked into, because that request is a certain 429 that also
costs a unit of budget to refuse. retry: { respectRemaining: false } opts out.

A 429 IS NOW HELD ONCE FOR THE WHOLE CLIENT. The wait belongs to the caller,
not to whichever request met it. Ten concurrent requests each slept their own
Retry-After and then retried at the same instant, re-tripping the limit
together. It goes on a shared gate with a little jitter, taken once, and the
retry path no longer pays it a second time. A shorter wait arriving while a
longer one is in force no longer brings the gate forward. Past maxRetryAfterMs
the gate is deliberately left open: we throw instead, and blocking the next
call for most of an hour is the opposite of letting the caller checkpoint.

The cache wrapper gap that Python hit does not exist here -- the client holds
the inner transport directly -- but there is a test for it either way, because
caching is on by default and every other test turns it off.

Verified against production: reads limit=300 remaining=296 reset=40
policy="300;w=60" on a real anonymous call, with the history meter correctly
absent and isExhausted false.

123 tests, 21 new. Mutation-checked: treating an unknown remaining as exhausted
fails 3, letting a blank response erase what we knew fails 1, bringing the gate
forward fails 1, removing the jitter fails 1, and not ageing the reset fails 2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four review agents, two of them independently found the same blocker.

PAGED HISTORY CRASHED FOR EVERY DEFAULT CALLER. I added #gate and #hold as
ECMAScript private fields on Transport. client.ts wraps the transport in a
Proxy to add caching, and a Proxy forwards methods with `this` bound to the
proxy, so the private brand check fails:

    TypeError: Receiver must be an instance of class Transport

history.days() threw on page two for anyone who had not passed cache: false --
the paid feature, on the default configuration. 130 tests passed because the
paging test helper hardcodes cache: false, so every paging test took the one
path where the bug cannot fire. They are TS-private now, which compiles to a
plain property and forwards fine, and there is a paging test on the default
cache that fails if the # fields come back.

THE GATE TIMED OFF THE WALL CLOCK. closeFor stored Date.now() + ms and waitMs
subtracted Date.now(), so a backward NTP step turned a five-second wait into
however far the clock moved -- an hour, measured -- with nothing bounding it,
because the cap is applied when the gate is armed and never when it is served.
Monotonic now, which is what the Python sibling had from the start.

on429: false DID NOT OPT OUT. It threw the error the caller asked for and then
closed the shared gate anyway, so their NEXT call blocked for the full
Retry-After. An advertised switch that switches nothing is worse than none.

THE PREMISE WAS BACKWARDS. The docs said anonymous calls carry no figures.
Measured against production it is the other way round for the per-minute meter;
the hourly history set is the one withheld from cacheable responses. And a
cached response's figures belong to whoever populated the entry -- age: 9 with
an unmoving remaining: 285, served to everyone -- so a non-zero Age is now
treated as saying nothing. A cache MISS carries no Age and is still recorded.

Also: strict integer parsing, so "0.4" no longer reads as 0 and makes
isExhausted true; fifteen edge cases now agree byte for byte with Python. Two
timing tests made deterministic instead of asserting ranges around the real
clock. Gate, readRateLimits and the UNKNOWN_* sentinels unexported -- Gate had
no route to the client's instance and readRateLimits' parameter type was not
exported, so nobody could name what they were passing. A CHANGELOG Changed
section saying plainly that calls may now block before sending.

131 tests. tsc, eslint, prettier and the build clean. Mutation-checked:
restoring the # fields fails the new paging test, honouring cached figures
fails 1, and both opt-outs are pinned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same per-call budget as the Python sibling, same measurement behind it: the
gate wait and the spent-window wait stack, so a 429 carrying both a
Retry-After and RateLimit-Remaining: 0 blocked for 180 seconds under a 120
second cap. Every leg was under the cap, so the per-leg check never fired.
maxRetryAfterMs is a per-call budget now.

Three tests could not fail, all found by mutation:

- The spent-window assertion was an upper bound only, on the single test
  covering the flagship behaviour. `sleep(left)` in place of
  `sleep(left * 1000)` -- seven milliseconds instead of seven seconds, a
  thousandfold too short -- passed green. Both bounds now.

- The jitter test was sound against Date.now()'s millisecond granularity and
  MY monotonic-clock change gutted it: performance.now() ticks between the 20
  calls, so the set is distinct with or without jitter. It asserted that time
  passes. It runs against a frozen clock now and bounds the spread.

- Nothing proved a sleep was AWAITED rather than merely requested. Dropping
  every await in hold() passed all 135 tests. A fake sleep that resolves on a
  later macrotask and counts itself pending now fails if a request starts
  while a hold is in flight. Honest limit: dropping ONE await is still masked
  by the other, because with one await remaining the hold does still block.

No test in either file had ever sent a 429 carrying rate-limit headers, which
is why none of this surfaced: both docstrings claimed to cover the gate and
every fixture was a 200.

136 tests. tsc, prettier and the build clean. Mutation-checked: removing the
budget fails 1, deleting the jitter fails 2, the thousandfold-short sleep
fails 1, dropping all awaits fails 1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the gate

Same three fixes as the Python sibling, same measurements behind them.

RETRY-AFTER PARSING. Only null reaches the exponential backoff, so a header
parsing to zero meant no wait at all -- and `Retry-After: 0` is legal per RFC
9110, as is a negative and an already-past date. Measured in Python, which had
the identical shape: four requests in 3ms against a server that had just said
429, and 204 a second across ten threads.

Number() was also far too generous for a `delta-seconds = 1*DIGIT` field: it
read '   ' as 0 and spun, and '0x10' as 16 and slept 48 seconds across three
retries where Python correctly took 2. The two parsers in this file now follow
the same rule, which they did not after the last round tightened only one.

THE SPENT-WINDOW PATH WAS A PURE HERD: ten waiters left inside the SAME
MILLISECOND, measured. Every one derived its deadline from the same observedAt
and slept to the same absolute instant with no spread. It runs through the same
jitter as the gate now.

A WAITER THAT WOKE INTO A RE-CLOSED GATE SENT ANYWAY -- measured waking at
584ms with the gate shut for another two seconds. It re-reads now, but only
when the deadline actually MOVED. My first attempt re-read unconditionally and
spun 57 times against a frozen test clock, which is the correct behaviour of a
wrong loop.

A NaN jitter would have made setTimeout fire immediately and silently disable
the gate rather than fail loudly. Guarded.

The new spread test is measured against a frozen clock, because against a live
one `left` varies by itself and the assertion passes with the spread deleted.

137 tests. Mutation-checked: removing either spread fails, and so does
removing the budget.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cubehouse

Copy link
Copy Markdown
Member Author

Held for 28 September. Decided 2026-09-26: this ships with the paid plans going live, not before. Open until then for review — ThemeParks/programme#453 carries the release steps and the deferred items.

Not a blocker on anything: the server side (tpwserver#257) has been live since the 24th and nothing depends on clients reading these headers yet. If review turns something up on the day, dropping this from the launch costs nothing.

What changed since it was opened, and why it is worth a look

Four review agents found eleven defects that the passing suite did not. In rough order of how badly they would have bitten:

Paged history crashed for every default caller #private fields failed their brand check through the caching Proxy; every test passed cache: false (JS)
Retry-After: 0 made the client hammer 4 requests in 3ms, 204/s across ten threads, against a server actively refusing
max_retry_after bounded nothing 180s of blocking inside one call under a 120s cap; a test had encoded the bug
The documented premise was backwards anonymous responses DO carry the per-minute figures, and they come off a CDN — measured age: 9, frozen remaining: 285
Both opt-outs did not opt out you got the error you asked for, then your next call blocked
Two release herds the spent-window path had zero spread: ten waiters left in the same millisecond
Three tests could not fail including one my own monotonic-clock fix gutted

Every fix is mutation-checked and the commit messages carry the measurements. They are written to be read rather than skimmed.

Two things I would want a reviewer to push on

  1. The client now sleeps on its own initiative, up to 120s, silently. Two reviewers independently said that needs at least a log line before it is the default (ThemeParks/programme#442). I left it, because both opt-outs work — but it is the judgement call most worth disagreeing with.
  2. The gate is a real improvement and a partial one. Measured: refused requests halved over a sustained run, 53 → 26. But the release spread is a fixed 0.25s that does not scale, so at N=50 it is cosmetic (ThemeParks/programme#441). The docstring now records the measurement rather than the claim.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant