CLDSRV-982: Guard against storing a request logger on a long-lived object - #6262
CLDSRV-982: Guard against storing a request logger on a long-lived object#6262anurag4DSB wants to merge 2 commits 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // lib/utilities/logger is a werelogs Logger, not a RequestLogger: it | ||
| // writes through and drops sub-level entries. This is what long-lived | ||
| // objects must use. | ||
| const logger = require('../../../lib/utilities/logger'); |
There was a problem hiding this comment.
require() inside an it() block — the project convention is to keep all requires at the top of the file. Moving this to the top-level would also make the dependency obvious at a glance.
That said, lib/utilities/logger triggers Config initialization as a side effect, so if that's intentional isolation, a short comment explaining why would help.
There was a problem hiding this comment.
Hoisted to the top in 903e71d, with a note on the Config-on-load side effect — the unit env (CI=true, S3BACKEND=mem) is set before mocha loads any file, so it is safe there, same as every test that pulls in helpers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.3 #6262 +/- ##
================================================
Coverage 85.17% 85.17%
================================================
Files 206 206
Lines 13424 13424
================================================
Hits 11434 11434
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 |
A werelogs RequestLogger buffers every entry it is handed and only flushes when something logs at or above the dump threshold. That is safe for one request and a leak for anything outliving it: CLDSRV-979 grew the buffer 10 times a second per account, and CLDSRV-981 does the same on every Scuba health check transition. Both shipped with unit coverage of the module they lived in, because the tests hand over a stub with no entries buffer. A no-restricted-syntax rule now flags storing log, logger, _log or _logger on `this` under lib/, which forces an explicit decision at each site. The two streamingV4 transforms are genuinely per request and are destroyed with it, so they disable the rule on the line and say why. This leaves two violations on development/9.3 - the tokenBucket and Scuba sites - which are the bugs fixed by CLDSRV-979 and CLDSRV-981. Lint goes from 0 errors to 2 until those merge, and green afterwards.
DummyRequestLogger counts calls but has no entries buffer, so no test using it can see werelogs buffering. makeRealRequestLogger() hands out a real RequestLogger and bufferedEntryCount() reads its buffer, so a test can assert that something is not accumulating. requestLoggerBuffering.js pins the behaviour the lint rule protects against: sub-level entries are retained rather than dropped, everything below error keeps accumulating, one error-level write flushes the lot, and the plain server logger has no buffer at all. If werelogs changes this contract, that is where it surfaces.
903e71d to
3dfbdf8
Compare
2 participants
Intent: why does this change exist?
CLDSRV-979 and CLDSRV-981 are the same mistake in two places, and both shipped with unit coverage of the module they lived in. The tests hand the code a stub logger, which has no entries buffer, so the werelogs behaviour that is the bug was never exercised. This adds the thing that would have caught them.
System impact: what's affected, including downstream?
A
no-restricted-syntaxrule scoped tolib/**, plus two test helpers. No production code behaviour changes.eslint.config.mjsuses only built-in ESLint capability, so there is no new dependency or plugin.Merge order: this must land after #6259 (CLDSRV-979) and #6261 (CLDSRV-981). Lint goes from 0 errors to 2 on
development/9.3, and those 2 are exactly the tokenBucket and Scuba sites those PRs remove. Draft until then; it turns green on rebase with no further edits.Preserved behavior: what explicitly stays the same?
Everything at runtime.
DummyRequestLoggerstays as it is and every existing test using it is untouched — the new fixture sits alongside it for tests that need to observe buffering.Intended change: what's different after this PR?
this.x = logunderlib/is a lint error, with a message naming the two correct choices:lib/utilities/loggerfor anything outliving the request, aRequestLoggerfor anything that does not. The two streamingV4 transforms are genuinely per-request and destroyed with the request, so they disable the rule on the line and say why — the point of the rule is to force that decision to be explicit rather than to ban the assignment.Tests get
makeRealRequestLogger()andbufferedEntryCount()intests/unit/helpers.js, andrequestLoggerBuffering.jspins the werelogs contract itself: sub-level entries are retained rather than dropped, everything belowerrorkeeps accumulating, one error-level write flushes the lot, and the plain server logger has no buffer at all.Verification: how do we know this worked, or how would we know if it didn't?
Checked the rule against the real tree rather than trusting the selector: it flags all four
this.x = logsites ondevelopment/9.3, and after exempting the two per-request transforms exactly the two known bugs remain.yarn lintgoes 0 errors → 2 errors with the same 2945 pre-existing warnings.The four buffering tests pass and are the executable statement of why the rule exists.
tests/unit5118 passing, with the 15 pre-existing failures this branch inherits (bucketPut/bucketUpdateQuotaquota metric seeding,ScubaClientImpl— the last four of which #6261 fixes).One caveat worth stating: the rule is deliberately shallow. It catches assignment of an identifier named
log,loggeror_logto athisproperty, which is how both bugs were written, but it will not catch a logger arriving under another name or reached through a longer path. It raises the cost of the mistake rather than making it impossible.