CLDSRV-980: Make stopRefillJob actually stop the refill job - #6260
CLDSRV-980: Make stopRefillJob actually stop the refill job#6260anurag4DSB wants to merge 1 commit into
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6260 +/- ##
===================================================
+ Coverage 85.17% 85.19% +0.01%
===================================================
Files 206 206
Lines 13424 13438 +14
===================================================
+ Hits 11434 11448 +14
Misses 1990 1990
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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 |
tick is async and ended with an unconditional setTimeout, so a tick that was awaiting when stopRefillJob() ran would resume and re-arm the timer stop had just cleared. The job then carried on for the rest of the process lifetime. Measured before the fix: 4 further refill passes in the 1.5s after stopRefillJob() returned. refillJobRunning now gates the re-arm, and startRefillJob refuses a second concurrent start rather than orphaning the first timer - stock code left about three overlapping loops behind across a test run, refilling at several times the intended rate. The timer is also unref()'d, as the cleanup job's already is, so it no longer holds the event loop open. Tests opt out via skipUnref, matching startCleanupJob.
d421859 to
36120ee
Compare
Intent: why does this change exist?
stopRefillJob()did not stop the rate limit refill job. Found while investigating RD-2240; unrelated to that memory leak beyond sharing the module.System impact: what's affected, including downstream?
lib/api/apiUtils/rateLimit/refillJob.jsonly.startRefillJobgains an optionaloptionsargument ({ skipUnref }), mirroringstartCleanupJob; the single production caller inlib/server.jsis unchanged and takes the new default. The refill timer is nowunref()'d, so it no longer holds the event loop open — the HTTP servers keep the process alive, as they already did for the cleanup job.Preserved behavior: what explicitly stays the same?
Refill cadence, cleanup cadence, and everything the job does per tick. Start, stop, repeated stop and restart-after-stop all behave as before.
Intended change: what's different after this PR?
A
runningflag gates the re-arm at the end oftick, so a tick that was mid-awaitwhen stop ran no longer replaces the timer stop just cleared.startRefillJobalso refuses a second concurrent start instead of orphaning the first timer, and warns.Production impact was limited before this, because
cleanUp()ends inprocess.exit(0)and overrides a live handle. What it did mean is that the job kept hitting Redis and mutating token buckets through the shutdown window, and leaked a live timer into any test that started it.Verification: how do we know this worked, or how would we know if it didn't?
Two regression tests, written first and confirmed failing against stock. The mid-tick one uses a refill slower than the tick interval so a tick is guaranteed to be in flight at stop; on stock it reports the job still running, and the double-start one measured 12 refills in 450ms where a single loop gives about 4 — roughly three overlapping loops.
Rate limit suite 183 passing, 0 failing. Lint clean apart from two pre-existing
prefer-await-to-thenwarnings on untouched lines.