Add rate-limit spec validation rules RL003-RL008 and Node 24 CI bumps - #62
Merged
Conversation
…ettings (RL003)
A policy name is the unit of limiter registration in RateLimiterOptions, so one
name maps to exactly one limiter and cannot hold two permit limits or two
partition strategies. CollectPolicies is first-wins, so declaring the same name
at several sites with different settings silently discards all but the first.
Reported by a consumer who had two endpoints sharing 'logs-read' and wanted
different partitions for each. They discovered the constraint by reading the
wiki; the build should have told them.
RL003 compares only settings EXPLICITLY declared at each site, not the fully
resolved values. That distinction matters: re-declaring a policy name on a
sub-path without repeating every setting is the idiomatic style across this
repo's specs and samples ("# Inherit accounts policy"), and it contradicts
nothing. Comparing resolved values instead would flag every such site, because
the sub-path inherits document-level defaults the first site overrode - a false
positive on normal authoring. Verified: zero RL003 across all scenarios and
samples in a Release build.
Values parsed case-insensitively (partition, algorithm) are normalized before
comparison so 'user' and 'User' do not read as a conflict. Claim names are
compared verbatim, since claims are case-sensitive.
The message names the policy, the conflicting extension key, each competing
value and its site, and points at both fixes: identical values everywhere, or
split into separate policy names.
Covers permit-limit, window-seconds, queue-limit, algorithm, partition,
partition-claim and emit-retry-after, so it also catches the pre-existing
conflict classes this feature did not introduce.
GitHub is deprecating Node 20 on Actions runners, and node20 actions are already
being force-run on Node 24. Audited every `uses:` across the three workflows
rather than only the two named in the warning.
Bumps (verified node24 via each action.yml `runs.using`, and every input we pass
still exists in the new major):
- actions/checkout v4 -> v7
- actions/setup-dotnet v4 -> v6
- actions/setup-node already v6 (node24), unchanged
Also removes the two remaining non-node24 actions, both of which were pinned to
a moving `master` ref:
- nelonoel/branch-name@v1.0.1 was still **node12**, far past deprecation, and
existed only to set $BRANCH_NAME. Replaced with a workflow-level
`BRANCH_NAME: ${{ github.ref_name }}`. Equivalent here: the action derives the
name from GITHUB_REF for push/workflow_dispatch and only diverges on
pull_request events, which neither of these workflows uses.
- satak/webrequest-action@master was node20. It issued one unauthenticated GET,
so it is now a curl step with --fail and bounded retries/timeout, which also
makes a failed cache invalidation actually fail the job instead of passing
silently.
- dotnet/nbgv is already node24 but was tracking `master`; pinned to v0.5.2.
No `@v4` or `@master` references remain. All three files re-validated as YAML.
Note: checkout v6 changed credential persistence to a separate file. The
merge-to-stable and release jobs push with a PAT via persisted credentials, so
that path is worth watching on the first run - it cannot be verified locally.
…mary The class summary enumerated the rule categories but was never updated when the STREAM and RL categories were introduced.
Brings the Node 24 action bumps onto the RL003 branch so both ship together.
Five diagnostics for x-ratelimit-* configuration that parses cleanly but breaks the build, crashes the app at startup, or is silently ignored at runtime. Each premise was verified against the runtime or the emitter rather than assumed. RL004 (Error) - policy names colliding on the generated constant. ToConstantName splits on '-', '_', ':' and space then PascalCases, so 'logs-read', 'logs_read', 'logs read' and 'LogsRead' all become 'LogsRead'. RateLimitPoliciesExtractor emits one const per policy name with no dedupe, so a collision emits duplicate members and generated code fails with CS0102. Error matches ATC_API_SCH018, which does the same for schema names. RL005 (Warning) - values the limiter constructor rejects. Verified all seven variants throw ArgumentException: permit-limit <= 0, window-seconds <= 0 (fixed/ sliding/token-bucket), queue-limit < 0. These run inside AddApiRateLimiting during DI setup, so the result is not a degraded limit but an app that fails to start. permit-limit: 0 is the common case, so the message points at x-ratelimit-enabled: false instead. RL006 (Warning) - x-ratelimit-enabled outside an operation. It is the one x-ratelimit-* extension that is NOT inherited: ExtractRateLimitConfiguration reads it from the operation only, so declaring it on a path or the document looks like it exempts everything beneath it but changes nothing. RL007 (Info) - algorithm cannot supply a Retry-After value. Verified against System.Threading.RateLimiting: a rejected sliding-window lease advertises the RETRY_AFTER metadata name but TryGetMetadata returns false, and concurrency never lists it; only fixed and token-bucket carry a value. So those two policies return 429 with no header even though the flag defaults to true. Info rather than Warning because the flag defaults on, making this a trade-off to surface rather than an authoring error. RL008 (Info) - window-seconds on a concurrency policy. ConcurrencyLimiterOptions has no time component and the emitter drops the value. Takes precedence over RL005 for a non-positive window, since a value that is never read cannot throw. RL007 walks CollectPolicies rather than resolving per operation. An earlier draft resolved per operation and mislabelled Showcase's 'users' as sliding (it is fixed) and 'file-uploads' as sliding (it is token-bucket), because the reported algorithm came from whichever operation was visited first rather than from the first-wins config the generator actually emits. Cross-checked against the generated Showcase output: the four reported policies are exactly the four sliding/concurrency limiter registrations, and the fix also caught a missed MultipartDemo 'global'. RL004-RL006 and RL008 check each declaration site rather than resolved values, matching RL003: what the author wrote at a site is always actionable, and comparing resolved values produces false positives on the idiomatic inherit-without-repeating style.
Missed by the earlier Node 24 sweep, which only audited .github/workflows/. This file lives in docs/ and is a template users copy into their own repository, so stale action versions propagate to every consumer rather than just to this repo's CI. - actions/checkout v4 -> v7 - actions/setup-dotnet v4 -> v6 - peter-evans/create-pull-request v6 -> v8 create-pull-request needed two majors: v7 is still node20, only v8 is node24. Checked both majors for breaking changes against this template - v7 renamed git-token to branch-token and dropped the deprecated PULL_REQUEST_NUMBER output env var, neither of which this template uses, and all seven inputs it does pass (token, commit-message, title, body, branch, delete-branch, labels) still exist in v8. checkout v7's fork-PR restriction applies to pull_request_target and workflow_run; this template triggers on push. No node20-or-older action references remain anywhere in the repository.
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
1 participant
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.
Summary
x-ratelimit-*misconfiguration at build timeChanges
✨ Features
ATC_API_RL003: one policy name with conflicting settingsATC_API_RL004: policy names colliding on one constantATC_API_RL005: values the limiter rejects at startupATC_API_RL006:x-ratelimit-enabledoutside an operationATC_API_RL007: algorithm cannot supplyRetry-AfterATC_API_RL008:window-secondson aconcurrencypolicyParseAlgorithmso the validator can reuse it👷 CI/CD
actions/checkoutv4 to v7actions/setup-dotnetv4 to v6dotnet/nbgvfrommastertov0.5.2nelonoel/branch-namewithgithub.ref_namesatak/webrequest-actionwith acurlstep📝 Documentation
RLandSTREAMin the rule category summaryNotes
and generated code fails with
CS0102ATC_API_SCH018precedent for schemasAddApiRateLimiting, so the app never startstrade-off to surface rather than an authoring error
fixedandtoken-bucketcarry aRetry-Aftervalue;slidingadvertises the name but never a valuegenerator emits, so it names the real algorithm
@v4or@masteraction references remainpersistence, and two jobs push with a PAT
dependabot.yml, so nothing bumps these actions