Repository navigation
Fix/escrow pagination first zero - #1262
Open
nate-staked wants to merge 2 commits into
Open
nate-staked wants to merge 2 commits into
nate-staked wants to merge 2 commits into
Conversation
…tion graphprotocol#1259 paginates paymentsEscrowAccounts and graphTallyTokensCollecteds with independent cursors and sends `first: 0` for whichever list finishes first. Graph Node only accepts 0 < first <= max_first (1000), so as soon as one list outlives the other the follow-up page is rejected with "The `first` argument must be between 0 and 1000, but is 0". getEscrowAccounts then throws on every cycle, and TAPv2 RAV redemption stalls for any indexer with 1000+ collected-token records. Request a single row for a finished list instead. Its cursor is already past its last row at the pinned block, so the page is empty. The test mock now enforces Graph Node's `first` range; without the fix, four existing pagination tests fail with the production error.
The TAPv2 pending-RAV query and RAV reduction pipelines reported failures at info level, so a persistent failure (e.g. the escrow snapshot query being rejected every cycle) was easy to miss. Log them at error level, matching the equivalent TAPv1 handlers in tap-collector.ts.
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
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.
Problem
Since #1259 (v0.25.12),
getEscrowAccountspagespaymentsEscrowAccountsandgraphTallyTokensCollectedswith independent cursors, and sendsfirst: 0for whichever list finishes first. Graph Node only accepts0 < first <= max_first(build_range), wheremax_firstcomes fromGRAPH_GRAPHQL_MAX_FIRSTand defaults to 1000. This check has been in Graph Node since at least v0.30.0. The follow-up page is therefore rejected with this error, whose wording is misleading because 0 isn't actually accepted.We hit this on our canary right after upgrading to v0.25.12. It logged the following on every TAPv2 cycle (every 15 minutes) until we rolled back to v0.25.11:
This affects any indexer with 1000+
graphTallyTokensCollectedsrecords, or 1000+ escrow accounts. For them, the escrow snapshot query fails every cycle and TAPv2 RAV redemption stops. Since the failure is logged atinfo, it's easy to miss.Fix
first: 1for a list that has finished. Its cursor is already past its last row at the pinned block, so the page comes back empty.firstrange. Without the fix, four existing pagination tests fail with the production error.error, matching TAPv1 intap-collector.ts. This is a separate commit and easy to drop.Testing
In
packages/indexer-common, on Node 22:yarn format,yarn lintandyarn compileare clean, andjest src/allocations/__tests__/horizon-escrow-accounts.test.tspasses 14/14 (10/14 with only the mock change).