Differentiate rate limits by tier and bucket per graph - #948
Conversation
Rate limiting worked as burst protection but was undifferentiated by tier,
in four independent ways that each hid the others:
- rate_limiting.py pinned every authenticated user to "ladybug-standard",
so the ladybug-large and ladybug-xlarge tables were unreachable code. The
comment claimed "graph-specific subscriptions are handled at the graph
level"; the only graph-tier-aware limiter is download_limits.py, which
governs monthly backup downloads, not request rate.
- Even reached, those tables held identical values, so differentiating the
lookup alone would have changed nothing.
- api_rate_multiplier (1.0/1.5/2.5) was read in four places and applied in
none, so /v1/graphs/{id}/limits reported 90 and 150 per minute to Large
and XLarge customers who were served 60.
- Buckets were keyed by user, so a customer with ten graphs shared one
budget — per-graph pricing that delivered no per-graph throughput.
The unlock is that LadybugDB writers are not shared. Every customer tier is
databases_per_instance: 1, so query load lands on the tenant's own box. The
config comment asserted the opposite, which is what justified flat limits.
Categories now split by what they actually touch. DEDICATED_RESOURCE_CATEGORIES
hit the tenant's own instance and scale with vCPU (1x/2x/4x for m7g.medium ->
m7g.large -> r7g.xlarge), bucketed per graph. Everything else — OpenSearch,
extensions RDS, the API tier — stays flat and user-bucketed, so buying graphs
cannot multiply one customer's load on a resource others depend on.
Shared repositories are excluded from per-graph bucketing. `sec` is one graph
every tenant queries: keying its bucket by graph id would put all of them in a
single budget and let one heavy user exhaust it for everyone. Those stay
user-keyed, which is also what isolates tenants. Subgraphs count — sec_historical
is as shared as sec.
Tier resolution is Valkey-cached with a 5 minute TTL to keep the limiter's
zero-database-access property on the hot path, and fails closed to the tightest
tier on cache miss, unknown graph, or lookup error. Failing open would hand out
throughput nobody bought.
api_rate_multiplier is gone from graph.yml. The API field remains — it is
required on two public response models — but is now derived from the limits
actually enforced, so it cannot drift from them again. No SDK regen needed.
/limits reports enforced values rather than 60 x multiplier.
Standard's numbers are deliberately not halved after its m7g.large ->
m7g.medium resize: observed load is ~1% CPU, and cutting existing customers'
limits to match a smaller box trades real service for protection that
admission control (85% of instance CPU/memory) already provides.
Note the 1x/2x/4x ratios are anchored to vCPU, not measured. The only load
data is 27-42% peak CPU on a near-idle three-graph fleet.
Two tests updated rather than fixed: one asserted limit == 60 for Large with
the note "1.5x multiplier applied separately", and one pinned the user-keyed
bucket. Both encoded the behaviour being corrected.
…eft open
Follow-ups in the same theme as the parent commit: what we report must be
what we enforce.
- /v1/offering and /v1/graphs/tiers still read api_rate_multiplier off the
writer config, where the key no longer exists, so both advertised 1.0 for
every tier while enforcement gives Large 2x and XLarge 4x — the inverse of
the drift the derived function was added to end. Both now call the derived
function, and the derived function returns 1.0 for any tier without its
own limits table (ladybug-shared, legacy strings), since such graphs are
enforced at the standard fallback.
- The tier resolver created a fresh Valkey client (and connection pool) per
request on the hottest path in the API; in production that is a TCP/TLS
handshake per graph-scoped request. One lazily-created client per process
now, matching RateLimitCache.
- A resolved tier string with no SUBSCRIPTION_RATE_LIMITS entry fell through
to the anonymous "base" table — below the FALLBACK_TIER floor the resolver
promises. Membership is now checked on both the cached and database paths.
No data migration ever normalized graphs.graph_tier after tier renames, so
a legacy value in production would have silently dropped a paying customer
to anonymous limits.
- Subgraphs bucketed by their own id, so an XLarge tenant with 25 subgraphs
held 26 independent budgets against one machine. kg…_dev now draws from
kg…'s bucket: a subgraph lives on the parent instance and is not
separately priced.
- extract_graph_id treated the static /v1/graphs/{tiers,capacity,extensions}
segments as graph ids. Latent — those routes use the general limiter — but
one of them adopting the subscription-aware dependency would have
collapsed every caller into a single shared bucket.
- /v1/graphs/{id}/limits reported the base table (20/min) for shared
repositories while enforcement grants the user-keyed standard tier
(60/min); it now reports the enforced tier, which also covers legacy tier
strings. In the same response, credits was permanently null (the block
read monthly_credit_limit, a column that does not exist, behind a bare
except: pass) and subscription_tier came from a User attribute that also
does not exist, reporting ladybug-standard to everyone.
- /v1/graphs/tiers hid Large and XLarge in production by honouring
deployment.enabled_default, which answers "is the stack deployed by
default" rather than "is this sellable" — the bug 1c25fbb fixed in
/v1/offering — and fabricated $100/$300/$700 fallback prices matching
neither billing nor Stripe. It now mirrors /v1/offering: fetch every tier,
let the billing catalog decide what is offered, omit what it cannot price.
Full suite: 11,413 passed; only failures are the two pre-existing
test_incremental_materialize cases that reproduce on main. New tests pin the
derived multiplier at both catalog and endpoint level, the fallback for
unrecognized tier strings, static-segment extraction, and parent bucketing
for subgraphs.
|
@claude can you review these changes |
|
Claude finished @jfrench9's task in 4m 2s —— View job Reviewing PR #948
SummaryThis is a well-scoped, carefully reasoned fix. I traced the four independent bugs the PR describes back through the code and confirmed each is actually closed:
Fail-closed behavior ( The One gap worth flagging
Nit
I wasn't able to run |
Review follow-up. Three bugs were fixed in this one handler with only unit-level coverage beneath them; these pin the endpoint itself: - a ladybug-large graph reports the enforced 120/min, its own tier as subscription_tier, and credits populated from monthly_allocation — the field that was permanently null behind a bare except - a shared repository reports the user-keyed standard tier (60/min), not the anonymous base table (20/min) - a legacy graph_tier string floors at the standard fallback Also documents why GRAPH_BACKUP, GRAPH_MANAGEMENT, and GRAPH_SYNC stay out of DEDICATED_RESOURCE_CATEGORIES: control-plane operations whose cost lands on shared infrastructure (S3, platform database, external providers via the worker pool), so tier vCPU is not their scaling axis.
1 participant
Rate limiting worked as burst protection but was undifferentiated by tier — in four independent ways, each of which hid the others. A follow-up commit then makes every surface that describes the limits agree with what the first commit enforces.
What was broken
rate_limiting.py:496"ladybug-standard", making theladybug-largeandladybug-xlargetables unreachable codeapi_rate_multiplier/v1/graphs/{id}/limitsreported 90/min to Large and 150/min to XLarge, who were served 60The comment justifying the hardcode said "graph-specific subscriptions are handled at the graph level." The only graph-tier-aware limiter is
download_limits.py, which governs monthly backup downloads.graph_scoped_rate_limit_dependencyturns out to be a pure alias for the same function.What unlocked it
LadybugDB writers are not shared. Every customer tier is
databases_per_instance: 1, so query load lands on the tenant's own box. The config comment asserted the opposite — "protect shared resources (OpenSearch t3.medium, LadybugDB on m7g/r7g)" — and that false premise is what justified flat limits.Categories now split by what they actually touch:
graph_query,graph_read,graph_write,graph_mcp,graph_operator,graph_analyticsgraph_search,graph_import,extensions_*, auth, billinggraph_importstays flat deliberately: LadybugDB ingests sequentially regardless of tier, so extra cores buy nothing.Shared repositories are excluded from per-graph bucketing
secis one graph every tenant queries. Keying its bucket by graph id would put all of them in a single budget and let one heavy user exhaust it for everyone — worse than the bug being fixed. Those stay user-keyed, which is also what isolates tenants from each other. Subgraphs count too:sec_historicalis as shared assec, so the check usesis_shared_repository_or_subgraph.Subgraphs draw from the parent's budget
A subgraph of a user graph is the mirror-image case:
kg…_devlives onkg…'s instance and is not separately priced, so it buckets askg…. Without this, an XLarge tenant with 25 subgraphs would hold 26 independent budgets against one machine.Failing closed
Tier resolution is Valkey-cached (5 min TTL, one lazily-created client per process) to preserve the limiter's zero-database-access property on the hot path. Cache miss, unknown graph, or lookup error all fall back to the tightest tier. A resolved tier string with no
SUBSCRIPTION_RATE_LIMITSentry —ladybug-shared, or a value predating a tier rename — also floors at the fallback tier rather than falling through to the anonymousbasetable: degradation lands a paying customer on the cheapest paid tier, never below it. Failing open would hand out throughput nobody bought.The static segments under
/v1/graphs(tiers,capacity,extensions) are excluded from graph-id extraction. Latent — those routes use the general limiter — but defense in depth against one of them adopting the subscription-aware dependency and collapsing every caller into a single shared bucket.api_rate_multiplierRemoved from
graph.yml. The API field remains — it's required on two public response models — but is now derived from the limits actually enforced. The follow-up commit finishes the job the first started:/v1/offeringand/v1/graphs/tierswere still reading the deleted YAML key and advertising 1.0 for every tier — the inverse of the original drift. Both now call the derived function, and tests pin the derived value at both the catalog level (get_available_tiersfor production and staging) and the endpoint level, so neither surface can flatten again. No SDK regen needed.Reporting surfaces now match enforcement
/v1/graphs/{id}/limitsreports the enforced tier for shared repositories — it was reporting thebasetable (20/min) while enforcement grants the user-keyed standard tier (60/min), the exact "reporting a limit we do not honour" this PR exists to end. The same path covers legacy tier strings. Also in that response:creditswas permanentlynull(the block readmonthly_credit_limit, a column that does not exist, behind a bareexcept: pass), andsubscription_tiercame from aUserattribute that also does not exist, reportingladybug-standardto everyone. It now reports the enforced tier./v1/graphs/tiershid Large and XLarge in production by honouringdeployment.enabled_default, which answers "is the CloudFormation stack deployed by default" rather than "can a customer buy this" — the same bug docs(taxonomy): correct resync module's account of its own callers #946's sibling fix (1c25fbb) closed in/v1/offering— and fabricated $100/$300/$700 fallback prices matching neither billing nor Stripe. It now mirrors/offering: the billing catalog decides what is listed, and a tier it cannot price is omitted rather than invented.Deliberately not changed
Standard's numbers are not halved after its
m7g.large → m7g.mediumresize. Observed production load is ~1% CPU, and cutting existing customers' limits to match a smaller box trades real service for protection admission control (85% of instance CPU/memory) already provides.Caveat worth stating plainly
The 1×/2×/4× ratios are anchored to vCPU, not measured. The only load data is 27–42% peak CPU on a near-idle three-graph fleet. Admission control remains the real overload backstop; these are a product lever.
Testing
Full suite: 11,413 passed,
just test-codeclean. The only failures are the two pre-existingtest_incremental_materializecases that reproduce onmain.Tests are weighted toward the failure modes: fail-closed resolution, that the fallback really is the tightest tier — including for tier strings with no limits table, on both the cache and database paths — the 1×/2×/4× and flat-shared invariants, static-segment extraction, and bucketing pinned directly: two users on
secget distinct buckets, one user on two owned graphs gets distinct buckets, and a subgraph shares its parent's bucket. The derived multiplier is pinned at catalog and endpoint level.Two existing tests were updated rather than fixed: one asserted
limit == 60for Large with the note "1.5x multiplier applied separately", and one pinned the user-keyed bucket. Both encoded the behaviour being corrected.