chore(lint): ban offset-paginated full-set scan loops over db.fns.*#8591
Open
petemoore wants to merge 2 commits into
Open
chore(lint): ban offset-paginated full-set scan loops over db.fns.*#8591petemoore wants to merge 2 commits into
petemoore wants to merge 2 commits into
Conversation
Contributor
|
I don't think we need to change anything here, existing query uses page size Plus I believe that whole architecture around those full reloads should be likely changed, since So I'd probably not touch this for now |
Offset-based pagination over the clients table during scoperesolver's in-memory cache reload could silently skip or duplicate clients when concurrent inserts/expirations shifted the result set mid-scan, causing transient authentication failures for newly-created clients until the next reload. Same root cause as #8586 / #8587 in worker-manager. Switch the scoperesolver scan loop, the auth listClients API handler, and the static-clients sync to a new keyset-paginated DB function get_clients_after in version 0126. The deprecated offset variant get_clients is retained for backwards-compatibility but is no longer called. Fixes #8588
Add a `no-restricted-syntax` rule matching the AST shape of the bug class fixed in #8586 / #8587 / #8588: a loop that contains both an `await db.fns.X(...)` call and an `<id> += <literal>` increment. The rule message points at `paginatedIterator` from `@taskcluster/lib-postgres` as the correct keyset alternative. Across the entire codebase only one site matched after the scoperesolver fix in #8590: `db/test/versions/0122_test.js`, where the test intentionally exercises offset pagination of the `get_tasks_by_task_group_projid` function in a no-concurrent-writes environment to verify stable ORDER BY ordering. That single case is suppressed inline with a one-line `eslint-disable-next-line` comment explaining the intent. Fixes #8589
3cc43b9 to
5a28907
Compare
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
2 participants
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.
Adds a
no-restricted-syntaxESLint rule matching the exact AST shape of the bug class fixed in #8586 / #8587 / #8588: a loop containing both anawait db.fns.X(...)call and an<id> += <literal>counter increment. The rule message points new contributors atpaginatedIteratorfrom@taskcluster/lib-postgresas the correct keyset alternative.Selector
A loop is flagged only when both conditions hold simultaneously inside the loop body:
await db.fns.X(...)call (member access via.fns).<id> += <literal>increment (the counter advancement).The combined match has near-zero false-positive rate: this combination is exactly the offset-pagination bug shape and is virtually never the right thing to write deliberately.
Verified across the codebase
After running
yarn lintagainst allservices/,libraries/, anddb/JS, only one site matched:db/test/versions/0122_test.js, where the test deliberately exercises the offset-paginatedget_tasks_by_task_group_projidfunction in a no-concurrent-writes environment to verify stableORDER BYordering. Suppressed inline with a one-lineeslint-disable-next-linecomment explaining the intent.The rule was tested manually against fixtures covering:
paginatedIterator(no false positive)+=literal but nodb.fns.*call (no false positive)db.fns.*call with non-literal+=(e.g.cumulativeWeight += config.weightfromlaunch-config-selector.js— no false positive)Stacked on #8590
This branch is based on the commit from #8590 (the scoperesolver fix). Until #8590 is merged, this PR's diff includes both commits; once #8590 lands the diff will clean up to just this rule + suppression. Do not merge this until #8590 is merged.
References
over capacityand double-count capacity stats #8586Test plan
yarn lint— full repo passes🤖 Generated with Claude Code