Repository navigation
fix(pds): page listRecords without repeating or stalling - #252
Open
decoded-cipher wants to merge 2 commits into
Open
decoded-cipher wants to merge 2 commits into
decoded-cipher wants to merge 2 commits into
Conversation
listRecords passed limit straight through, capped only at 100. limit=0 or a negative value returned an empty page whose cursor pointed back at the start of the collection, so clients that follow the cursor looped forever. A non-numeric limit became NaN, which disabled the limit and returned every record in the collection in one response. Clamp it to 1-100 and fall back to the default of 50 when it is missing or not a number, as spaces.ts already does for listSpaces.
listRecords started each page with repo.walkRecords(cursor). The cursor was the last record of the previous page, and MST.walkFrom yields its start key twice when that key exists, so the boundary record came back up to three times across pages. With limit=2 a page was just that record twice, so the cursor never moved. reverse=true walked forwards and reversed the page afterwards, which made the cursor the smallest key of the page, so following it returned the same page forever. Walk the collection's MST range directly instead, in either direction, excluding the cursor key and skipping subtrees outside the range. Only the records on the page are read. Records are listed newest first by default and oldest first with reverse=true, matching the reference PDS; the order used to be the other way round. The cursor is the bare rkey of the last record, as in the reference PDS, and cursors in the old collection/rkey form are still accepted. Listing a collection with no records no longer reads every record after it in the repo. Closes ascorbic#251
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.
Summary
com.atproto.repo.listRecordspagination was broken in two ways (#251):repo.walkRecords(cursor), andMST.walkFromreturns its start key twice when that key exists. So the record at each page boundary came back up to three times. Withlimit=2a page was just that record twice, and the cursor never moved.reverse=truenever advanced. The walk always ran forwards and the page was reversed afterwards. The cursor ended up as the smallest key on the page, so following it returned the same page forever.This PR:
reverse=true, matching the reference PDS. Cirrus had this the other way round.collection/rkeycursors are still accepted, so clients in the middle of paging across an upgrade keep working.limitto 1–100, defaulting to 50 when it's missing or not a number. Before,limit=0or a negative limit looped forever, andlimit=abcreturned the whole collection in one response.Closes #251
Commits
fix(pds): clamplistRecordslimit to the lexicon range.fix(pds): pagelistRecordswithout repeating or stalling, with tests, changeset and plan doc row.Notes for review
minor. This matches the reference PDS (orderBy(uri, reverse ? 'asc' : 'desc')). Apps that uselimit=1to fetch the latest record get the newest one now.limitis clamped, not rejected with 400 as in the reference PDS. This is consistent with howspaces.tshandleslistSpaces.@atproto/repo. Upstream never passes an existing key towalkFrom:list()skips the start key andlistWithPrefix()starts from a prefix that isn't a real key. Cirrus was relying on behaviourwalkFromdoesn't promise.Test plan
limitclamping. The new endpoint tests fail onmain.Before / after
main/ 0.19.0 (public instance, read-only requests):limit=3limit=2limit=3&reverse=trueThis PR, on
pds-test.arjunkrishna.dev, compared againstcom.atproto.sync.getRepo:reverse=truelimitof 0, -5, abc, 101, 1000, missingThe test instance is still running with this data, in case it's useful: