Repository navigation
feat(ratelimit): distributed per-account report concurrency cap with local-ceiling fallback (#545) - #559
Conversation
…local-ceiling fallback (#545)
|
@codex please review this PR — distributed per-account report concurrency cap on the shared |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1bdccdb5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await Task.Delay(_renewInterval, _clock, token).ConfigureAwait(false); | ||
| if (token.IsCancellationRequested) | ||
| break; | ||
| RenewOnce(); |
There was a problem hiding this comment.
When a report outlives the 60-second TTL and Redis was unavailable long enough for its key to expire, the first renewal after recovery returns false rather than throwing; this loop discards that result and lets the report continue indefinitely without a lease or even an alarm. Another request can then acquire the slot, so repeated lease losses can exceed the promised per-account/local ceiling; renewal rejection needs to trigger an explicit fail-closed action or otherwise keep the running work accounted for.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real and not yet addressed. The two review-round fixes in 07831a5a (broadened renew/release catch so a non-Redis fault can't bypass release + alarm; renewal lifetime tied to DisposeAsync not RequestAborted) do not cover this: a genuine lease loss (Redis down > TTL, key expires, recovers) still returns false and the loop discards it, so the report continues uncounted and the account can drift above the ceiling during a sustained outage. The correct fix (re-acquire-or-yield on renewal rejection, or an explicit fail-closed cancel) changes a design property the epic signed off (best-effort renewal, report always completes), so it's going to the owner for a decision rather than being patched silently. Will update this thread with the outcome + SHA.
There was a problem hiding this comment.
Addressed in 05471fb. On a renewal REJECTED by a reachable backend (new RenewOutcome.Lost, distinct from Faulted — a Redis fault is not treated as loss, so we don't double-count a slot we still hold), the permit re-grabs any free slot for the account and adopts it, keeping the running report counted. If no slot is free the account is genuinely over its ceiling with this report on top: a distinct SecurityEvents.ReportConcurrencyOverCapacity alarm fires and the report continues (owner decision: never cancel a running report; bounded and self-healing as reports finish). Two new contract tests: re-acquire-keeps-counted, and no-free-slot-alarms. Verified: build clean, 64/64 green; two mutants (disable re-acquire → both tests red; suppress the alarm → the over-capacity test red) re-run and confirmed.
| for (var slot = 0; slot < _permitLimit; slot++) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| var key = $"report-cc:{accountId:N}:{slot}"; | ||
| if (TryAcquirePinned(key, owner, out var backend)) |
There was a problem hiding this comment.
When Redis is unreachable and preceding fallback slots are already occupied, every loop iteration retries the synchronous Redis operation before checking the next local slot. An over-cap request can therefore block for PermitLimit Redis timeouts (about four timeout waits with the default), and a burst of such requests can tie up ASP.NET worker threads precisely during a store outage; latch the fault for this acquisition and scan only the fallback, or replace the scan with one bounded asynchronous/atomic operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 07831a5a. AcquireAsync now latches a per-scan redisFaulted flag: the first RedisException/RedisTimeoutException marks Redis unreachable for that acquisition and TryAcquirePinned skips it (_redis is not null && !redisFaulted) for the remaining slots, so an over-cap request pays at most one Redis fault, not PermitLimit. Verified: build clean, 62/62 green.
…ly + scan fault memo (#545 review)
|
@codex re-review please. Since your last round: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05471fbb7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // and self-healing as reports finish, but a deployment backend should alert if it | ||
| // persists: it means the shared store dropped slots under load or during an | ||
| // outage recovery (#545). | ||
| public const string ReportConcurrencyOverCapacity = "ReportConcurrency.OverCapacity"; |
There was a problem hiding this comment.
This event's contract says a deployment backend should alert on it, but docs/security/log-redaction-policy.md still defines only the five authentication event IDs and explicitly tells operators to alert only on those five. Consequently, the new over-capacity condition can ship without an alert, and the documented field contract for stable security events is incomplete; add this ID, its firing condition and fields, and update the operator alerting requirement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 42cc4dd7. docs/security/log-redaction-policy.md now has an Operational (non-credential) security events subsection documenting both ReportConcurrency.OverCapacity (new, #545) and SharedState.RedisUnavailable (pre-existing from #543/#544, which your finding correctly exposed was never documented) — firing condition and capability field for each — and the operator alerting requirement is updated from 'the five' to also cover these two operational events.
|
Review loop stopped deliberately at the 2-consecutive-zero-product-defect threshold (per this repo's review-response rule), not left unfinished. Tally: the loop confirmed and fixed 4 product findings (exception-unsafe release, renewal-eviction on client disconnect, per-scan Redis re-probe, and sustained-outage over-admission → re-acquire-on-loss + over-capacity alarm), each mutation-verified by the driver. The last two rounds yielded no product defect — an adversarial concurrency pass on the re-acquire commit (all vectors refuted) and one docs-only P2 (now fixed in |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
The review loop is complete, the requested fixes are already included in the current commit, and the working tree is clean. No additional code changes, commit, or pull request were created. Checks
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42cc4dd73d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| try | ||
| { | ||
| if (_redis.TryAcquire(key, owner, _ttl)) |
There was a problem hiding this comment.
When an outage has already admitted PermitLimit reports into the pinned in-process fallback and Redis recovers before they finish, this Redis-first path grants up to another PermitLimit permits because Redis has no record of the fallback holders and the limiter maintains no backend-independent local count. A single replica can therefore run twice its configured report ceiling (and a multi-replica deployment can run all fallback holders plus the shared Redis limit), defeating the capacity protection during the exact outage-recovery window it is meant to bound; acquisition needs to account for locally active permits regardless of which backend grants the distributed slot.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
🤖 I have created a release *beep* *boop* --- ## [0.1.0](v0.0.4...v0.1.0) (2026-09-12) ### ⚠ BREAKING CHANGES * log in by farm code, with per-account email identity ([#532](#532)) (#564) ### Features * **accounts:** add Account.Slug (farm code), suspend/reactivate, list-accounts verb ([#531](#531)) ([3fe9754](3fe9754)) * **accounts:** provision additional farms ([#581](#581)) ([006f298](006f298)) * add Aspire local development AppHost ([#567](#567)) ([2c9e6b9](2c9e6b9)) * add configurable worker sale allocation ([#619](#619)) ([0955095](0955095)) * add searchable entity pickers ([#642](#642)) ([60d2053](60d2053)) * **api:** provision-account takes an optional --timezone at creation ([#603](#603)) ([#694](#694)) ([a0aee39](a0aee39)) * **audit:** show the sales-line audit payload as a readable Details column ([#745](#745)) ([#749](#749)) ([d26d389](d26d389)) * **auth:** add ApplicationUser.StepUpLogoutEpoch column ([#338](#338)) ([#554](#554)) ([18306ee](18306ee)) * certify over-cap simulation fixture bands ([#633](#633)) ([a67b2e1](a67b2e1)), closes [#627](#627) * **cli:** rename-account verb to change a farm code ([#732](#732)) ([#733](#733)) ([4b70559](4b70559)) * **customers:** edit existing customer details ([#625](#625)) ([#626](#626)) ([062a55c](062a55c)) * **jobs:** single-runner leader gate for the durable job worker ([#271](#271)) ([#555](#555)) ([4148f9b](4148f9b)) * let owners change user email addresses ([#605](#605)) ([842347b](842347b)) * log in by farm code, with per-account email identity ([#532](#532)) ([#564](#564)) ([68adb62](68adb62)) * **ratelimit:** distributed IP-keyed auth limiters ([#544](#544)) ([#558](#558)) ([ec14972](ec14972)) * **ratelimit:** distributed per-account report concurrency cap with local-ceiling fallback ([#545](#545)) ([#559](#559)) ([1522e4e](1522e4e)) * **sales:** mark discounted lines, total the discount, and show it in the Orders list ([#723](#723), [#724](#724)) ([#741](#741)) ([1a07441](1a07441)) * **sales:** record list, old and new price in the order-line audit payload ([#722](#722)) ([#742](#742)) ([97c866f](97c866f)) * **sales:** refuse an over-ceiling confirm from a Sales user ([#727](#727)) ([#766](#766)) ([8c0792a](8c0792a)) * **sales:** show what each order still owes, and filter the list to unpaid ([#771](#771)) ([ca59d68](ca59d68)) * **sales:** snapshot the list price on the order line and show the discount ([#734](#734)) ([cffed5e](cffed5e)) * **sales:** snapshot the product name and unit in the order-line audit payload ([#747](#747)) ([#748](#748)) ([0481c06](0481c06)) * scope Worker reads to assigned flocks ([#388](#388)) ([#611](#611)) ([5884a9a](5884a9a)) * shared-state ports with Redis + in-process fallback ([#543](#543)) ([#552](#552)) ([f767fa9](f767fa9)) * suspend-account / reactivate-account operator verbs ([#534](#534)) ([#573](#573)) ([d0be26c](d0be26c)) * **tenancy:** write-side tenant guard + single-assignment TenantContext ([#546](#546)) ([#561](#561)) ([f371f1d](f371f1d)) * **web:** dashboard rework — capture-status tiles, 14-day trend, stock as a stacked bar ([#654](#654)) ([396ba23](396ba23)) * **web:** date-range filters on audit and expenses, and the stock lot filter gets its bounded toolbar ([#666](#666), [#667](#667), [#653](#653)) ([94b188f](94b188f)) * **web:** elevation hierarchy and sentence-case labels ([#651](#651), [#652](#652)) ([#661](#661)) ([28db4c7](28db4c7)) * **web:** Expenses and Audit keep a clear-filters control while rows are still showing ([#679](#679)) ([#697](#697)) ([b859982](b859982)) * **web:** expenses filters by a date range like its sibling screens ([#667](#667)) ([f13858f](f13858f)) * **web:** key the farm brand palette per farm ([#586](#586)) ([#600](#600)) ([7183a43](7183a43)) * **web:** let operators forget remembered farms ([#598](#598)) ([577d94e](577d94e)) * **web:** one-line provenance, bounded date filters, and empty states that invite action ([#653](#653), [#655](#655)) ([#668](#668)) ([80b53f4](80b53f4)) * **web:** prefill the farm code from ?farm= and remember it ([#535](#535)) ([#588](#588)) ([b7f5cc6](b7f5cc6)) * **web:** split authenticated routes into lazy chunks ([#620](#620)) ([5089271](5089271)) * **web:** the audit log filters by a date range, and says which window is empty ([#666](#666)) ([63027e0](63027e0)) * **web:** typeset numbers as numbers and refresh the Help glossary ([#650](#650), [#657](#657)) ([af4fe11](af4fe11)) ### Bug fixes * **api:** order same-instant audit events by a durable monotonic key ([#700](#700)) ([8fcf084](8fcf084)) * **api:** print the farm code from bootstrap-admin ([#589](#589)) ([#594](#594)) ([34032ac](34032ac)) * **audit:** show the price a line sold for, not its list price ([#759](#759)) ([e6b37d0](e6b37d0)) * **audit:** store catalog enums by name and guard the add-item transaction shape ([#751](#751)) ([23609ff](23609ff)) * **auth:** reject invalid account claims ([#622](#622)) ([8d6c7fe](8d6c7fe)) * **auth:** require step-up for durable user access ([#360](#360)) ([#607](#607)) ([f767dce](f767dce)) * **ci:** bound the npm audit calls and give the web job room to finish ([#686](#686)) ([153b7a8](153b7a8)) * **ci:** escalate the audit bound to SIGKILL, so it actually bounds ([#686](#686)) ([a0c8f4e](a0c8f4e)) * **ci:** fail closed on invalid vulnerability config ([#621](#621)) ([1690db8](1690db8)) * **ci:** lockfix covers the two AppHost lock files, derived from the sln ([efb05e6](efb05e6)) * **ci:** lockfix covers the two AppHost lock files, derived from the sln ([8986d77](8986d77)) * **ci:** remove invalid XML comment from nuget.lockfix.config ([#541](#541)) ([5f1bc0a](5f1bc0a)) * **ci:** the advisory vuln gate no longer blocks on an unusable report ([#686](#686)) ([aaf6934](aaf6934)) * **ci:** the advisory vuln gate no longer blocks on an unusable report ([#686](#686)) ([64f1f53](64f1f53)) * **i18n:** tl help text names the saleable flag and unit-system setting what their labels call them ([#688](#688)) ([#696](#696)) ([bfd24d7](bfd24d7)) * **infra:** AccountId must be a non-nullable Guid or both tenant write layers refuse ([#673](#673)) ([#695](#695)) ([2470c4e](2470c4e)) * require step-up for flock scope changes ([#609](#609)) ([4151f89](4151f89)) * **sales:** keep a line's discount markers agreeing while its price is edited ([#752](#752)) ([#753](#753)) ([c159b4b](c159b4b)) * **sales:** say which kind of missing list price a line has ([#774](#774)) ([489180e](489180e)) * scope legacy logout to selected farm ([#624](#624)) ([fae8d82](fae8d82)) * **seed:** drain the daily-entry lock sweep so deep simulation fixtures validate ([#644](#644)) ([730fa23](730fa23)), closes [#638](#638) * **tenancy:** AccountId is a concurrency token, so the database refuses a detached cross-tenant write ([#562](#562)) ([4d1dfa3](4d1dfa3)) * **tenancy:** AspNetUserRoles carries a tenant column, so a role write naming another farm's user is refused ([#670](#670)) ([fc0552a](fc0552a)) * **tests:** bump the image-pin allow-list counts for the AppHost LocalPorts tests ([#593](#593)) ([58d3056](58d3056)) * **tests:** the OTLP collector survives a lost port race and ignores traffic that is not an export ([#672](#672), [#676](#676)) ([#677](#677)) ([965c737](965c737)) * **web:** a scoped audit view filtered to nothing names both the record and the range ([#666](#666)) ([41bbfe1](41bbfe1)) * **web:** an abandoned dialog attempt's success no longer hijacks the replacement on Customers, Daily Entry, Flocks, Grades and Products ([#703](#703)) ([#705](#705)) ([85605db](85605db)) * **web:** an abandoned dialog attempt's success no longer hijacks the replacement on Inventory, Expenses, History and Stock ([#703](#703)) ([#706](#706)) ([60a4997](60a4997)) * **web:** an abandoned edit's success no longer hijacks the dialog that replaced it on Users ([#703](#703)) ([#710](#710)) ([778faab](778faab)) * **web:** an abandoned order attempt's success no longer hijacks the dialog that replaced it ([#702](#702)) ([522c699](522c699)) * **web:** capture screens open on the flock you last used, and assigning one no longer guesses ([#646](#646)) ([#699](#699)) ([7f8f317](7f8f317)) * **web:** constrain dialog session helpers to declared scopes ([#715](#715)) ([389e3c8](389e3c8)) * **web:** date validation gets one boundary table instead of one case per review round ([#666](#666)) ([215f830](215f830)) * **web:** keep a paged window and an item panel on the user's newest intent ([#645](#645)) ([d81bccf](d81bccf)) * **web:** keep Sales order panels closed after pending writes ([#711](#711)) ([f0f7492](f0f7492)) * **web:** keep Sales panels closed after pending Open reads ([#716](#716)) ([620411f](620411f)) * **web:** make login take the cross-tab cookie lock so a racing refresh cannot restore the wrong session ([#648](#648)) ([ff18beb](ff18beb)) * **web:** make the entity picker read as a search field and focus it on open ([#736](#736)) ([66ef667](66ef667)), closes [#735](#735) * **web:** page truncated customer and movement tables with usePagedList ([7cfe4d6](7cfe4d6)) * **web:** reconcile Sales line edits with refreshed orders ([#717](#717)) ([d7dd2c9](d7dd2c9)) * **web:** the audit date filter accepts low-numbered years, and its empty state covers every narrowing ([#666](#666)) ([af52d25](af52d25)) * **web:** the audit date filter rejects impossible dates, and its history guard actually guards ([#666](#666)) ([8d51846](8d51846)) * **web:** the expense range bounds are not capped at today, which the month-end default exceeds ([#667](#667)) ([7e01864](7e01864)) * **web:** the help text calls the expiry field what the field calls itself ([#666](#666)) ([2fd1f3c](2fd1f3c)) * **web:** the stock lot date range sits in the bounded toolbar ([#653](#653)) ([43dec5e](43dec5e)) ### Refactoring * **web:** extract SalesPage's dialog-write wrapper into a shared useDialogAction hook ([#703](#703)) ([#704](#704)) ([60ee9d9](60ee9d9)) ### Documentation * add k6 preparation steps to the dev-database fixture runbook ([#643](#643)) ([a4f1f09](a4f1f09)) * add runbook for loading the simulation fixture into a dev database ([#639](#639)) ([2d143b8](2d143b8)) * **agents:** a PR closes its issue from the body, not the title ([#744](#744)) ([39be13c](39be13c)) * **agents:** drop the commit and push gate, and require screenshots on UI changes ([#757](#757)) ([6225172](6225172)) * **agents:** find guards by grepping registry readers; amend issues a PR overtakes ([#580](#580)) ([fe3fde8](fe3fde8)) * **agents:** the Playwright specs have been in CI since 2026-08-08 ([#768](#768)) ([68ee612](68ee612)) * **aspire:** record the second local database and pin the AppHost dashboard ports ([#623](#623)) ([713b941](713b941)) * compress AGENTS.md to one paragraph per rule, and draw the two orders that matter ([#551](#551)) ([997ae8a](997ae8a)) * item 7 names each screen's actual initial filter value ([#666](#666)) ([70a53d8](70a53d8)) * multi-farm tenancy decision record and AGENTS/GLOSSARY sync ([#537](#537)) ([#601](#601)) ([2c34771](2c34771)) * name the scoped filtered-empty key and state the [#653](#653) relationship plainly ([#666](#666)) ([0e93dac](0e93dac)) * note that a PackageReference in Directory.Build.props is invisible to the dependency graph ([4845724](4845724)) * **plans:** commit the [#722](#722) and [#745](#745) design records ([#754](#754)) ([c942fcd](c942fcd)) * record [#579](#579) as won't-fix — suspension is immediate for use, not issuance ([#582](#582)) ([7a3be40](7a3be40)) * record the [#508](#508) audit ordering key and the tracked-file guard lesson ([#701](#701)) ([08964e9](08964e9)) * **runbooks:** add procedure to rename the default farm's code after upgrade ([#731](#731)) ([2f6e242](2f6e242)) * screenshots of the running SPA in the README ([#550](#550)) ([711488a](711488a)) * **sim:** commit the dashboard screenshot, capture the palette matrix, and record the [#651](https://github.com/mforce/cluckwork/issues/651)/[#652](https://github.com/mforce/cluckwork/issues/652) conventions ([#660](#660), [#662](#662), [#663](#663), [#664](#664)) ([#665](#665)) ([930ea30](930ea30)) * specify searchable entity picker ([#641](#641)) ([91d4300](91d4300)) * split the README into audience-scoped docs and adopt repo-template scaffolding ([#548](#548)) ([b3f3fcf](b3f3fcf)) * surface Aspire local development workflow ([#568](#568)) ([a343baa](a343baa)) * **web:** record the per-screen idempotency-key policies and runWrite's refresh contract ([#703](#703)) ([#707](#707)) ([8bee651](8bee651)) * **web:** the date-cap help text covers every stocked item, not only feed ([#666](#666), [#667](#667)) ([c8433c5](c8433c5)) * **web:** the help text claims only what is true of recording, and says nothing about filter caps ([#666](#666), [#667](#667)) ([e2f63d1](e2f63d1)) * **web:** the help text describes the date-range filters that shipped ([#666](#666), [#667](#667)) ([c3275b7](c3275b7)) * **web:** the help text stops describing a cap the filters no longer have ([#666](#666), [#667](#667)) ([49654cd](49654cd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: cluckwork-lockfix[bot] <309265648+cluckwork-lockfix[bot]@users.noreply.github.com>
Closes #545. Slice S4 of epic #530 (Phase 1.6 multi-farm tenancy) — the fourth scaling blocker.
What
Moves the per-account report concurrency cap (#311) off the process-local
PartitionedRateLimiteronto the shared renewable lease (#543), keyed perAccountId. N replicas now enforce one combined per-account permit count instead of N × PermitLimit.How
report-cc:{accountId}:{slot}) built from the single-holderILease; a dead holder's slot is TTL-reclaimed (never a bare counter).ResilientLeaseis deleted; the cap owns pinned failover.Tests
7 deterministic contract tests (incl. no-silent-eviction across a flap, and a same-slot scan-continues guard) + a Testcontainers-Redis two-instance proof of the combined count. HTTP e2e retained.
Verify
dotnet test Cluckwork.sln— build0 Warning(s) 0 Error(s); relevant subset 61/61 green (Redis included). Two load-bearing mutants (renewal-broken → the two eviction tests red; fail-open → the local-ceiling test red) re-run and confirmed by the driver.Implemented via runbook transcription; independently verified (build + tests + both mutants re-run) before publishing.