Repository navigation
Split oversized pagination pages instead of resetting - #1
Merged
Merged
Conversation
A page's range is pinned by its journal, so inserts into that range grow it. Until now the helper had no answer for that: the page grew until the server called it incomplete (pageStatus "SplitRequired"), and the only correction was a full reset, which drops every loaded page and snaps the list back to the first one. Port convex-js's page splitting. When a page is SplitRequired, SplitRecommended, or has grown past twice initial_num_items, and the server gave a splitCursor, subscribe two halves bounded by explicit cursor/endCursor pairs and swap them in for the original only once both have results. The original stays live until then, so the combined list never shows a gap or a duplicate. The first half starts at the original page's cursor, not at null. convex-js sent null there and duplicated every item ahead of the page (get-convex/convex-js#54981); the regression test covers it. An incomplete page with no splitCursor still resets, since there is no split point to work with. That reset now has a cap: a query whose first page always comes back that way would otherwise reset forever, each one a fresh uncached query. After two, the snapshot reports an error naming the cause instead.
The unit tests drive splits through the mock transport, which proves the state machine but not that a real backend hands out a splitCursor where we expect one. Add messages:listPaginatedDesc, ordered newest-first. Inserts then land inside the first page's cursor range instead of after the last page, so the page grows the way a chat feed grows. The test loads two pages, sends three more messages to push the first page from 2 items to 5, and checks the list stays whole and ordered across the split, that no snapshot ever showed an item twice, and that load_more still works afterwards. Verified against the local self-hosted backend: the split fires on the server's splitCursor, not on a client-side guess.
Contributor
Author
|
Ran the live suite locally now that a backend is up. All 10 tests pass against the self-hosted backend (the two skips are the env-gated admin-auth and cloud-TLS ones). Pushed a live test for the split path itself, since the mock transport only proves the state machine. Confirmed with temporary instrumentation (removed before commit) that the split fires on the server's |
…ange Two defects in the split state machine, both found by a Codex review of the branch. A failed split half marked the original page split_disabled and left it in place. That is right for a page that is merely oversized, but a page the server reported as SplitRequired is missing items: keeping it showed that gap forever, with a normal can_load_more status and no way back. The optional case still keeps the page and stops retrying; the required case now falls back to the reset path. The reset cap counted per session, and every reset reloads the first page, which is usually healthy and cleared the count. So the cap only ever bound a bad *first* page. A bad page further down reset forever: reset, first page loads clean, count cleared, consumer pages back down, same failure. An infinite-scroll consumer drives that on its own. The count now follows the failing page's start cursor and is cleared only when that range comes back whole. Renamed the state to unrepairable_*, since a failed split reaches it too. Not changed: between a SplitRequired update and both halves loading, the incomplete page is what the snapshot shows, so a truncated page leaves a gap for one round trip. convex-js splits from the updated page result in exactly the same way, and parity is the design goal. Documented in the header instead.
A page the server marks SplitRequired may be missing part of its range. Publishing it put a hole in the middle of the list: the page's surviving items, then the next page starting at a cursor past the missing ones. Stop before it instead. Pages ahead of it stay visible, the status goes back to loading, and the full range appears when the split (or reset) that repairs it lands. The loading status now keys off whether any items are visible rather than the page count, since the page that stalls the list is no longer always the last one. This is what convex-js's usePaginatedQuery does — it returns early with the items before the incomplete page, for the same stated reason. Its newer PaginatedQueryClient has no such guard, which is what misled the earlier note in the header claiming the gap was upstream parity.
Five defects from a second review, all in the repair paths themselves. The reset budget was keyed on the failing page's start cursor. On a newest-first feed every insert moves the page boundaries, so each failure looked like a new range, the count restarted, and the cap never tripped — the same loop the cursor keying was introduced to bound, since counting per session had the mirror-image hole (a reset reloads the healthy first page, which cleared the count). It is now positional: every consecutive failure counts, and the streak ends only when the session gets back to that position with a complete page. A SplitRequired page that carries a splitCursor is still incomplete, but it used to clear the count on its way to starting a split. A split whose half kept failing therefore refilled its own budget forever. Only a complete page counts as repaired now. split_disabled was honored before the page's status was parsed, so a page disabled after a failed optional split had no path back if it later went incomplete: no split, no reset, and the snapshot hides incomplete pages, so the list truncated and sat in loading with nothing in flight. Parse first; an incomplete disabled page takes the reset path. Reaching the cap leaves the failing subscription live on purpose, but recovery only cleared the counter, not the error flag, so a range that healed itself stayed in the synthetic error until the caller reset. That error also returned before the page loop, dropping every item the caller could see the moment the status flipped. It is now reported at the failing page, keeping the prefix, like any other page error. Each new test was confirmed to fail against the previous commit's paginated.cpp — five failures, one per finding, with the rest passing.
Two defects from a third review, both from failing_index being a position in a vector that changes. A completing split replaces one page with two, shifting everything after it down by one, but the marker stayed put. A half of an earlier page then landed on the marker and looked like the failing range coming back healthy, which cleared the budget: a first page that splits after every reset refilled the budget forever, and the same insertion after the cap had tripped cleared the error and left the incomplete page behind a permanent loading status. The marker now moves with the pages. The capped state keeps the failing subscription alive so a range that heals can recover on its own. If that subscription reported InvalidCursor instead, do_reset built a fresh single-page session while unrepairable_page and failing_index survived — pointing at a page that no longer existed, so the snapshot's fallback kept reporting the error and load_more refused to run while the status was error. Nothing could reach note_page_repaired. InvalidCursor now takes hard_reset: stale cursors mean the ranges the count was about are meaningless anyway. Both new tests were confirmed to fail against the previous commit's paginated.cpp, with the other 27 passing.
convex-js never resets for an incomplete page. Its hook stops the results before the page and waits, because the page is a live subscription that re-runs whenever the data changes. This helper instead reset the whole session to force the issue, which needed a budget to bound the resets, a marker to track which range was failing, a terminal error for when the budget ran out, and a flag to stop retrying a failed split. None of that has an upstream counterpart, and all of it existed to bound a loop that only happened because we reset. Four review rounds found eleven defects in this file; ten were in that machinery. So: match convex-js. An incomplete page stays out of the snapshot and its subscription waits for a result that fits the read limits. A failed split drops and is retried on the next update of that page — one attempt per server transition, not a loop we drive. Resets are back to what upstream does them for: changed args, reset(), and InvalidCursor. What this gives up is the diagnostic. A query too heavy to ever return a complete page now leaves the list loading indefinitely instead of eventually reporting an error naming initial_num_items. That error was useful and upstream has nothing like it, but it cannot be produced without deciding the failure is permanent, and deciding that is the machinery. Documented in the header so the behavior is at least discoverable. 161 lines out of paginated.cpp, 501 across the branch.
advance_split returned on the both-results guard before it checked whether either half had failed, so a half failing while its peer was still loading left a dead split in place. consider_split skips a page while a split of it is pending, so every later update of that page — including one carrying a usable splitCursor — was ignored, and when the peer finally landed the split was merely erased without revisiting the page. If the peer never returned, the page stayed blocked for good. For an incomplete page, which is hidden from the snapshot, that means a list stuck loading with a repairable result already in hand. Check the populated halves for failure first, and drop the split on the first ordinary error (InvalidCursor resets before reaching here). Retries stay driven by the next update of the page. Retrying from the failure path would re-subscribe the same failing query, whose cached error comes back synchronously from subscribe(), and recurse without bound. The existing failure test delivers the successful half first, so both results were populated by the time the failure was seen and this ordering went uncovered. The new test fails against the previous commit.
Contributor
Author
|
Merging after five review passes, each finding checked against the code before it was acted on. Twelve defects were confirmed and fixed; nothing that survived verification turned out to be wrong. Where they were is worth recording, because the distribution was lopsided. Ten of the twelve were in a recovery apparatus this branch added and no longer has: reset-on-unrepairable, a retry budget, a cap, a terminal error, a positional failure marker, and a flag that stopped retrying a failed split. Each round's fix seeded the next round's defect. A per-session budget that a healthy first page cleared, then a cursor-keyed one that moving page boundaries defeated, then a positional one that a split could invalidate. convex-js has none of this, and the loop the budget existed to bound only happened because of the reset it was bounding. It came out in 5278a8b — 501 lines across the branch. The other two were ordinary bugs in the splitting. One of them, in Each regression test was run against the commit before its fix and required to fail there. That check caught one test that had been passing for the wrong reason — hardcoded transition versions rather than the behavior it claimed to cover. One note for anyone comparing this against upstream. convex-js has two pagination implementations, and only 107 unit tests and 10 live integration tests, green on Linux and Windows. |
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.
A page's range is pinned by its journal, so inserts into that range grow it.
paginated_queryhad no answer for that: the page grew until the server reportedpageStatus: "SplitRequired", and the only correction was a full reset — every loaded page dropped, the list snapped back to the first one. Twenty pages into a busy feed, that collapse is what the user sees.This ports convex-js's page splitting.
A page is split when the server reports
SplitRequiredorSplitRecommended, or when it has grown past twiceinitial_num_items, and the result carries asplitCursor. The two halves are subscribed with explicitcursor/endCursorbounds covering exactly the range the original covered, and they replace it only once both have results — so the list is never gapped or duplicated at any point in between. A half can be split again, so a page that keeps growing keeps getting divided.The first half starts at the original page's cursor. convex-js sent
nullthere, which made splitting any page but the first re-read the list from the beginning and duplicate everything ahead of it (get-convex/convex-js#54981, fixed upstream in 1.42.3).SplitFirstHalfStartsAtTheOriginalPagesCursorcovers that specific mistake.SplitRequiredmeans the page may be missing items, so the snapshot stops before it: the pages ahead of it stay visible, and the status returns to loading until a split repairs it. A short list beats a list with a hole in it.usePaginatedQuerydoes the same. Worth noting for anyone comparing against upstream: convex-js's newerPaginatedQueryClienthas no such guard and concatenates the incomplete page.What this deliberately does not do
If the server calls a page incomplete but gives no
splitCursorto split on, nothing forces the issue. The page stays out of the snapshot and its subscription waits for a result that fits the read limits. A query so heavy that no page ever fits leaves the list loading indefinitely, with no error.An earlier revision of this branch reset the session in that case, bounded by a retry budget, with an error once the budget ran out. It was removed. The loop the budget existed to bound only happened because of the reset, convex-js has no equivalent, and four review passes found ten defects in that machinery against one in the splitting itself. The diagnostic is the real loss — producing it means deciding the failure is permanent, and deciding that is the machinery.
No public API change.
Testing
106 unit tests pass, 20 of them covering pagination: the split wire shape and session id, cursor chaining, the
#54981cursor, size-driven splits with nopageStatus, an incomplete page staying hidden and then recovering on its own, a failed split half being retried on the next update rather than disabled,InvalidCursorinside a half, and split bounds surviving a reconnect.The live integration suite passes against the self-hosted backend, locally and in CI (10 tests; the admin-auth and cloud-TLS cases self-skip without credentials). It includes
messages:listPaginatedDesc, ordered newest-first so inserts land inside the first page's range rather than after the last. That test grows the first page from 2 items to 5 and checks the list stays whole and ordered across the split, that no snapshot along the way showed an item twice, and thatload_morestill chains afterwards.