Skip to content

NEB-898 Report Xero's rate limits on every response, not only on refusal - #85

Merged
abreckner merged 2 commits into
masterfrom
tonybreckner/neb-898-surface-rate-limit-headers-on-success
Aug 27, 2026
Merged

NEB-898 Report Xero's rate limits on every response, not only on refusal#85
abreckner merged 2 commits into
masterfrom
tonybreckner/neb-898-surface-rate-limit-headers-on-success

Conversation

@abreckner

@abreckner abreckner commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Intent (What)

Report Xero's remaining rate-limit budget on every XPM response, not only on the 429 that refuses one.

XpmRuby::RateLimits parses the header set off any response, and a new module-level callback is handed one whenever a response reports a budget:

XpmRuby.on_rate_limits = ->(limits) do
  MyApp.record(tenant: limits.xero_tenant_id, day_left: limits.daylimit_remaining, problem: limits.problem)
end

This is the gem half of NEB-898 — its item 1. Nothing in the monorepo reads the callback yet; the pre-flight guard that will (items 2–5) is monorepo work and needs a release and a pin bump first.

What XPM actually sends — from the recorded traffic in this repo, which is the evidence the ticket was missing:

header on a success on a 429
x-minlimit-remaining, x-daylimit-remaining, x-appminlimit-remaining yes yes
retry-after, x-rate-limit-problem no yes

A 200 in spec/vcr_cassettes/xpm_ruby/connection/delete.yml carries x-minlimit-remaining: 59, x-daylimit-remaining: 4984, x-appminlimit-remaining: 9999. So the remaining counts were there on the happy path all along and the gem dropped them — that is the gap being closed. Xero names a delay and a cause only when it refuses, so problem and retry_after are legitimately absent on a success and must not be read as "no problem".

Motivation (Why)

XPM has no model of Xero's daily request budget, and this gem is why it cannot have one. The 429 branch was the only place reading the headers, so the gem could only ever report that a budget was already gone — too late for a caller trying to pace itself under the limit.

That daily budget is the limit actually refusing us. sum:ignitionapp.xpm.rate_limit.refused{*} by {problem}, full 14 days to 27 Aug: 61,910 problem:day, and zero minute, concurrent or appminuteday is the only group the metric has ever emitted. Episodic rather than steady state (all of it 18–21 Aug, bulk client imports on two tenants), but any tenant importing in bulk burns its whole day budget and then syncs nothing until the window resets.

It also blocks knowing the cap at all: on a 429 x-daylimit-remaining is always 0, so the number a threshold would be sized against can only come from a successful response. (The cassette's 4984 hints at a ~5,000/day cap, but those recordings are old — size the threshold from fresh production data once this is wired up, not from that.)

Implementation (How)

  • A module-level callback, not a reader on the connection. Every entry point here builds its own Connection and never hands it back, so Client.get callers could never reach an instance reader — the same unreachable-by-construction trap as the respond_to?(:retry_after) guard documented in xpm_integration. The callback also reaches the 429s that raise from inside someone else's rescue, which an exception-carried payload does not.
  • Reported ahead of the status case, so a budget is read off responses that raise as well as those that return. Skipped when a response reports nothing at all — that is not the same as a budget of zero, and a caller should not have to tell the two apart from an object of nils.
  • Absent stays absent. Counts parse via Integer(..., exception: false). to_i would read a missing or unparseable header as 0, which for a remaining-budget count is not "unknown" but "exhausted" — a guard built on it would refuse every request off a response that never mentioned a budget.
  • A raising callback cannot fail the request it measured. A caller whose store is down would otherwise take every XPM call with it. Warned rather than swallowed, because silent is indistinguishable from a callback nobody wired up.

Consequence

RateLimitExceeded#details is unchanged for today's traffic, so the three monorepo consumers (workers/retry_metrics.rb:140, workers/honours_retry_after.rb:52 and :94) keep reading it as they do. The existing 429 spec asserts the same hash it always did.

One fix beyond the ticket, easy to drop if unwanted: details now reads through [] rather than slice. Only [] is case-insensitive on a Faraday::Utils::Headers:

stored keys:      ["Retry-After"]
[] lowercase:     "22"
slice lowercase:  {}      ← what details has always used

So a Retry-After from Xero would have left details empty, and Workers::HonoursRetryAfter would have fallen back to its fixed curve with nothing to show it had. Xero sends these lowercase — HTTP/2 requires it — which is why this has never bitten; the hash is byte-identical on current traffic, key order included, and a spec now pins both casings.

Behaviour is unchanged for any caller that does not set on_rate_limits.

Verification

bundle exec rake — 111 examples, 0 failures (93 before). bundle exec rubocop — 48 files, no offences. CI green on Ruby 3.1.

Version bumped 0.5.0 → 0.6.0. api/Gemfile in the monorepo pins "0.5.0" exactly and its lockfile pins a revision, so nothing there moves until someone bumps it deliberately.

🤖 Generated with Claude Code

abreckner and others added 2 commits August 27, 2026 10:18
Xero returns its rate-limit headers on every XPM response. The 429 branch was
the only place that read them, so the gem could only ever tell a caller that a
budget was already gone -- which is too late to pace against. XPM's daily budget
is the limit that refuses us in production, and a caller that wants to stop
short of it needs the remaining count while its requests are still succeeding.

`XpmRuby::RateLimits` parses the header set off any response, and
`XpmRuby.on_rate_limits` is handed one whenever a response reports a budget.

A module-level callback rather than a reader on the connection: every entry
point here builds its own Connection and never hands it back, so a reader would
be unreachable from a caller that only calls `Client.get`. The callback also
reaches the 429s that raise from inside a rescue somewhere, which an
exception-carried payload does not.

Counts parse through `Integer(..., exception: false)`, so an absent header stays
absent. `to_i` would read it as 0, and for a remaining-budget count that is not
"unknown" but "exhausted" -- a caller gating on it would refuse every request
off a response that never mentioned a budget at all.

A callback that raises cannot fail the request it was measuring: a caller whose
store is down would otherwise take every XPM call with it. The error is warned
rather than swallowed, because silent is indistinguishable from a callback
nobody wired up.

`RateLimitExceeded#details` keeps the same wire-named hash, now read through
`[]` rather than `slice`. Only `[]` is case-insensitive on a
Faraday::Utils::Headers, so a `Retry-After` would have left `details` empty and
a caller reading it would have fallen back silently, as though Xero had never
named a delay. Xero sends these lowercase -- HTTP/2 requires it -- so for
today's traffic the hash is unchanged, down to the key order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The recorded traffic splits the header set in two, and the specs and README were
treating it as one. A 200 carries the three remaining counts and nothing else
(spec/vcr_cassettes/xpm_ruby/connection/delete.yml: min 59, day 4984, appmin
9999). `retry-after` and `x-rate-limit-problem` arrive only on a 429 — Xero
names a delay and a cause only when it refuses.

The success spec asserted `x-rate-limit-problem` on a 200, which no XPM response
sends, so it pinned a shape the API does not produce. It now uses the recorded
values, and documents that `problem` and `retry_after` are absent rather than
"no problem" on a successful call.

No production behaviour changes: the reader already parsed each header
independently, so a 200 without the two 429-only headers was always read as nil.

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

Copy link
Copy Markdown
Contributor Author

/auto-approve

@wizardofosmium wizardofosmium left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 ⏳ 📝

@abreckner
abreckner merged commit 7147fcc into master Aug 27, 2026
1 check passed
@abreckner
abreckner deleted the tonybreckner/neb-898-surface-rate-limit-headers-on-success branch August 27, 2026 04:25
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.

2 participants