CLDSRV-979: Stop retaining a request logger on rate limit token buckets - #6259
Conversation
Hello anurag4dsb,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6259 +/- ##
===================================================
+ Coverage 85.20% 85.31% +0.10%
===================================================
Files 206 206
Lines 13434 13434
===================================================
+ Hits 11447 11461 +14
+ Misses 1987 1973 -14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
f93956c to
5068ed6
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
feb5e77 to
153567e
Compare
|
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: approve |
| this.lastRefillTime = Date.now(); | ||
| // Last request that consulted this bucket. Idle eviction keys on this | ||
| // rather than lastRefillTime, which the refill job keeps fresh. | ||
| this.lastAccessTime = Date.now(); |
There was a problem hiding this comment.
I don't think the addition of lastAccessTime is needed.
lastRefillTime is only set when a bucket is refilled and a refill is not done unless the tokens are below a threshold which prevents it from being updated unless tokens are being used.
153567e to
43c600a
Compare
|
/reset |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const promise = bucket | ||
| .refillIfNeeded(logger) | ||
| .then(bucketRefilled => { | ||
| // Check if refill actually happened | ||
| if (bucketRefilled) { | ||
| refilled++; | ||
| } | ||
| }) |
WorkerTokenBucket stored the werelogs logger of the first request that
touched a resource, and the 100ms refill job then logged through it for
the lifetime of the process. RequestLogger buffers every entry it is
handed in RequestLogger.entries and only drains when something logs at
or above the dump threshold ('error'), so the refill chatter accumulated
forever - about 80MB per account per connector on a 10-worker deployment,
reclaimed only by restarting cloudserver. S3C runs at logLevel info, so
the buffered debug/trace lines were never even printed.
The logger is now supplied per call to refillIfNeeded() and never stored.
The refill job passes the long-lived server logger, which writes through
and drops sub-level entries instead of buffering them.
The existing tests could not have caught this: they all pass a sinon
stub as the logger, so the werelogs buffering that is the bug is never
exercised. The new retention tests assert the invariants directly - no
retained request logger, and refills logged through the caller's logger.
The unit environment has no rate limit Redis instance (the feature is disabled at Config load), so every refill test bounced off isReady() into the catch block and the grant, denial, disconnected and slow paths were never executed - codecov flagged exactly those lines. tokenBucket now reads rateLimitClient.instance at call time instead of destructuring it at module load, which is behaviour-identical in production (the instance is created once, before the first request) and lets tests substitute a fake client. Five new cases cover each outcome; tokenBucket.js line coverage goes from 83% to 98%, leaving only the defensive requested <= 0 guard, unreachable while refillThreshold is below bufferSize.
e121b87 to
9d912a2
Compare
History mismatchMerge commit #76080b58abd36ae825d193f603749ddc974d1b55 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve |
|
/reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: approve |
Build failedThe build for commit did not succeed in branch bugfix/CLDSRV-979-rate-limit-pinned-request-logger The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-979. Goodbye anurag4dsb. The following options are set: approve |
5 participants
Intent: why does this change exist?
With account rate limiting on, cloudserver held about 80MB per account per connector and only gave it back on restart (RD-2240). The retained bytes turned out to be buffered werelogs entries, not rate limiting state.
what's affected, including downstream?
lib/api/apiUtils/rateLimit/only — token buckets and the refill job, for both the account and bucket resource classes.refillIfNeeded()now takes the logger as an argument; the refill job is its only production caller. Nothing else in the request path changes, and there is no config, schema or dependency change.Intended change: what's different after this PR?
Token buckets no longer store the request-scoped logger; the refill job passes the long-lived server logger, which writes through instead of buffering. Idle eviction is keyed on the last request that consulted a bucket rather than on
lastRefillTimeand an empty buffer, both of which the refill job itself kept from ever being true.Two consequences worth flagging. Eviction becoming reachable also makes the pre-existing fail-open-on-create path reachable: a resource idle more than 60s gets
bufferSizelocal tokens before its first Redis grant, about 0.8 RPS of slack against a 60 RPS limit, with Redis still holding the GCRAemptyAtstate. And it removes a latent log bomb, a single error-level write on a pinned logger used to flush its whole buffer at once (measured: 20,000 buffered debug calls emit nothing, oneerror()emits 20,001 lines), whichrefillIfNeeded's own catch block could trigger on any Redis failure.Verification: how do we know this worked, or how would we know if it didn't?
Reproduced end-to-end on
development/9.3with a local Vault-free stack (the platform-default limit path, which is RD-2240's TS16 case) plus Redis, then measured buffered entries in-process across every worker.At
clusters=10with 50 accounts, stock held 500 token buckets all pinning a logger, 838,638 buffered entries and 950MB of worker heap growth, releasing none of it. Patched: same 500 peak buckets, 0 pinned loggers, 0 buffered entries, 4MB growth, and all 500 buckets evicted after idle. Single-worker RSS over 4 minutes went from an unbounded 247→1873MB to a flat plateau at 559MB.Five new regression tests were written first and confirmed failing against stock. Rate limit suite 186 passing; full unit suite 5119 passing against a 5114 baseline, with the same 15 pre-existing unrelated failures (
bucketPutquota seeding,ScubaClientImpl); lint clean.