feat(ops): warn when the tier outgrows the rate limiter's single-instance assumption - #94
Merged
Conversation
…t the caller's Rate limiting partitioned on the leftmost X-Forwarded-For entry. A proxy appends to that header rather than replacing it, so a caller sending "X-Forwarded-For: 9.9.9.9" arrives as "9.9.9.9, <real client>" and position zero is a value they chose. Varying it per request minted a fresh partition every time, which is throttling defeated -- with TrustForwardedFor correctly set to true, on the deployed app. TrustForwardedFor was the setting that got all the attention, and it turns out to answer only half the question. Whether to read the header is one decision; which entry of it a proxy actually wrote is the other, and only the second is a trust boundary. A unit test asserted the wrong semantics by name -- The_leftmost_entry_in_a_ forwarded_chain_is_the_client -- so the defect was pinned in place by a passing test that described the code accurately and the protocol incorrectly. Read from the trusted end instead. With TrustedProxyHops proxies in front the client sits at count - hops; everything left of it is caller-supplied and ignored. A chain shorter than the hop count falls back to the connection address rather than trusting an entry nearer the caller. Entries are normalised through IPAddress/IPEndPoint, which drops the ":port" App Service appends. That mattered: the source port is ephemeral, so keeping it would have partitioned per connection instead of per caller and reopened the same hole from the other side. ProxyConfigurationCheck gained a third warning, for a hop count higher than the chain arriving -- that misconfiguration reproduces the global-cap outage while the setting that explains it reads as correct. Verified: the pre-fix code returns "9.9.9.9" for the forged chain and the new code returns the real client, both exercised directly. 530 tests pass, format clean, no warnings under -warnaserror. Not verified: the live spoof test against App Service. This sandbox's network policy blocks azurewebsites.net (403 on CONNECT), so whether the platform normalises the header before the app sees it is still unconfirmed from here. The fix is correct either way -- it stops depending on that question having a favourable answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
…ance assumption Throttling counters live in the serving process's memory. That is correct on the F1 tier, which runs one instance, and wrong the moment a second one exists: each keeps its own counters and enforces the configured budget separately, so two instances allow twice the configured limit and four allow four times. Nothing about that surfaces. No error, no rejected request, no log line -- the numbers in appsettings.json simply stop being the numbers in effect. It is the same silent-failure shape as the proxy misconfiguration, and it deserves the same treatment: a check, not a comment. ScaleOutCheck warns once at startup when WEBSITE_SKU names a tier that can run more than one instance. It reports capability rather than live instance count, because no instance can see how many siblings it has -- and on an autoscaling plan the second one can arrive at any moment, with nothing marking the transition. Absent off App Service, where there is no platform claim to check, so local runs stay quiet. Worth noting the sharp edge this guards: the provisioning script takes the tier as a parameter and defaults to F1. Passing a scale-capable SKU is a one-word change that silently multiplies every limit. The handbook now states the constraint outright, including why the database is not a substitute for a shared counter store: counters are written on every request, rejected ones included, so a serverless database would be billed for compute in proportion to the abuse the limiter exists to absorb. Written as a static method taking the SKU as an argument rather than reading the environment itself, so it is reachable by tests -- the lesson from the defect fixed in the previous commit, where unreachable code was exactly how a bug survived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The assumption
Throttling counters live in the serving process's memory. That is correct on F1, which runs one instance — one counter table, and it is the whole picture.
It stops being correct the moment a second instance exists. Each keeps its own counters and enforces the configured budget separately, so two instances allow twice the configured limit and four allow four times.
Nothing about that surfaces on its own. No error, no rejected request, no log line — the numbers in
appsettings.jsonsimply stop being the numbers in effect. Same silent-failure shape as the proxy misconfigurationProxyConfigurationCheckalready exists for, and it deserves the same treatment: a check, not a comment.The sharp edge this guards
Provision.ps1:42takes the tier as a parameter, defaulting toF1. Passing-Sku S1is a one-word change that silently multiplies every limit by the instance count.What it does
ScaleOutCheckwarns once at startup whenWEBSITE_SKUnames a tier that can run more than one instance — anything other thanFreeorShared.Two deliberate choices, both pinned by tests:
Absent off App Service — local runs, containers — where there is no platform claim to check, so it stays quiet rather than training an operator to ignore it.
Testable by construction
It takes the SKU as an argument instead of reading the environment itself. That is the direct lesson from the defect fixed earlier on this branch: logic that reads its own inputs is logic a test cannot drive, and unreachable code is exactly how the forwarded-header bug survived review, CI, and coverage.
Documentation
The config handbook now states the constraint outright, including why the database is not a substitute for a shared counter store: counters are written on every request, rejected ones included, so a serverless database would be billed for compute in proportion to the abuse the limiter exists to absorb.
Verification
WEBSITE_SKU.dotnet format --verify-no-changesclean, 0 warnings under-warnaserror.Generated by Claude Code