From 8aab6b1c52990ee34630a0c5ea0b3701b2bb2909 Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Fri, 24 Jul 2026 22:56:10 -0700 Subject: [PATCH 1/8] Split oversized pagination pages instead of resetting 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. --- README.md | 6 +- include/convex/paginated.h | 24 ++-- src/paginated.cpp | 245 ++++++++++++++++++++++++++++++--- tests/paginated_test.cpp | 275 ++++++++++++++++++++++++++++++++++--- 4 files changed, 502 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index cbd1cda..e5354b8 100644 --- a/README.md +++ b/README.md @@ -17,8 +17,10 @@ On top of it, `convex::paginated_query` implements the convex-js `usePaginatedQuery` state machine as a growing, live-updating list over a paginated query. It keeps one live subscription per page, exposes `load_more`, avoids seams at page boundaries via query journals, and resets -on argument changes or stale cursors. For one-shot calls without a -WebSocket there is `convex::http_client`, plus file storage +on argument changes or stale cursors. A page that grows past twice the +requested size is split in two, the same way convex-js does it, so a busy +list does not drift into the server's read limits. For one-shot calls +without a WebSocket there is `convex::http_client`, plus file storage upload/download helpers. The protocol core is sans-IO: `convex::base_client` does no networking at diff --git a/include/convex/paginated.h b/include/convex/paginated.h index e2885c7..5cf1603 100644 --- a/include/convex/paginated.h +++ b/include/convex/paginated.h @@ -11,12 +11,18 @@ // (paginationOptsValidator) and return a PaginationResult: // { page: [...], isDone: bool, continueCursor: string }. // -// Divergence from convex-js (v1): page splitting is NOT implemented. When -// the server reports pageStatus == "SplitRequired" (a page grew past the -// read limits and may be incomplete), convex-js splits the page in two via -// splitCursor; this helper instead resets pagination, which re-fetches -// correctly-sized pages. "SplitRecommended" is ignored. Revisit if reset -// churn ever matters in practice. +// Page splitting. A page's range is fixed by its journal, so inserts into +// that range make the page grow; left alone it eventually outgrows the +// server's read limits and comes back incomplete. Like convex-js, this +// helper splits such a page in two at the server-supplied splitCursor — +// subscribing both halves with explicit cursor/endCursor bounds and swapping +// them in for the original only once both have loaded, so the combined list +// is gapless and duplicate-free at every moment. A page is split when the +// server reports pageStatus "SplitRequired" or "SplitRecommended", or when +// it has grown past twice initial_num_items. An incomplete page with no +// splitCursor (too few items to have a split point) cannot be repaired that +// way and resets pagination instead; if re-fetching keeps producing one, the +// helper stops resetting and reports an error rather than looping. // // Threading. All callbacks (page updates) arrive through the owning client's // delivery mechanism — the process_events() pump by default. The on_update @@ -53,7 +59,8 @@ enum class pagination_status : std::uint8_t { /// The server reached the end of the list. exhausted, /// A page failed with an error other than InvalidCursor (those reset - /// pagination instead). See paginated_snapshot::error. + /// pagination instead), or the query cannot produce a complete first + /// page. See paginated_snapshot::error. error, }; @@ -88,7 +95,8 @@ struct paginated_impl; /// Pagination resets — dropping every page and re-subscribing a fresh first /// page with a new cache-buster id — happen when set_args() changes the /// arguments, when a page fails with InvalidCursor (cursors went stale), or -/// when the server reports a page as SplitRequired (see file comment). +/// when a page is incomplete and cannot be split (see file comment). +/// Splitting an oversized page is not a reset: loaded pages stay put. class paginated_query { public: using snapshot_callback = std::function; diff --git a/src/paginated.cpp b/src/paginated.cpp index ad30614..ae39337 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -2,6 +2,7 @@ #include +#include #include #include #include @@ -11,6 +12,12 @@ namespace convex { namespace { +/// How many times in a row an incomplete page that cannot be split may reset +/// the session before the helper gives up and reports an error. Each reset +/// re-runs the query under a new cache-buster id, so a page that always comes +/// back the same way would otherwise loop forever. +constexpr unsigned MAX_UNSPLITTABLE_RESETS = 2; + /// Cache-buster shared with every helper instance in the process, mirroring /// convex-js's module-global counter. A unique id per pagination session /// keeps its pages' query journals independent (no deduplication onto an @@ -26,7 +33,11 @@ struct page_fields { const value_array* items = nullptr; bool is_done = false; std::string continue_cursor; + /// Where the server suggests this page be cut in two. Absent when the + /// page is too small to have a split point. + std::optional split_cursor; bool split_required = false; + bool split_recommended = false; }; std::optional parse_page(const value& v) { @@ -42,9 +53,15 @@ std::optional parse_page(const value& v) { f.items = &page_it->second.as_array(); f.is_done = done_it->second.as_boolean(); f.continue_cursor = cursor_it->second.as_string(); - if (const auto status_it = o.find("pageStatus"); status_it != o.end()) { - f.split_required = - status_it->second.is_string() && status_it->second.as_string() == "SplitRequired"; + if (const auto split_it = o.find("splitCursor"); + split_it != o.end() && split_it->second.is_string()) { + f.split_cursor = split_it->second.as_string(); + } + if (const auto status_it = o.find("pageStatus"); + status_it != o.end() && status_it->second.is_string()) { + const std::string& status = status_it->second.as_string(); + f.split_required = status == "SplitRequired"; + f.split_recommended = status == "SplitRecommended"; } return f; } @@ -85,6 +102,10 @@ std::string_view pagination_status_name(pagination_status status) { } struct paginated_impl : std::enable_shared_from_this { + /// Identifies a page across reorderings. Page positions shift when a + /// split swaps one page for two, so callbacks address pages by key. + using page_key = std::uint64_t; + // Immutable after construction. client* owner; std::string udf_path; @@ -103,10 +124,39 @@ struct paginated_impl : std::enable_shared_from_this { bool stopped = false; struct page { + page_key key = 0; client::subscription sub; std::optional result; + /// Start of the page; absent means the beginning of the query. + std::optional cursor; + /// End of the page. Only set on the halves of a split, which pin + /// both ends of their range so the two of them cover exactly what + /// the page they replace covered. + std::optional end_cursor; + /// Set when a split of this page failed, so it is not retried on + /// every subsequent update of the same page. + bool split_disabled = false; + }; + + /// A split in flight: two half-pages loading in parallel while the page + /// they will replace stays live and visible. Swapping them in only once + /// both have results keeps the combined list gapless at every moment. + struct pending_split { + page_key original = 0; + page first; + page second; }; + + /// Active pages, in result order. std::vector pages; + /// Splits in flight. Their halves are not in `pages` yet. + std::vector splits; + page_key next_key = 0; + /// Consecutive resets caused by an incomplete page with no split point. + unsigned unsplittable_resets = 0; + /// Set when those resets hit MAX_UNSPLITTABLE_RESETS: the query cannot + /// return a complete page, so the snapshot reports an error instead. + bool unsplittable_page = false; paginated_impl(client& c, paginated_query::options&& o, paginated_query::snapshot_callback&& cb) @@ -117,46 +167,189 @@ struct paginated_impl : std::enable_shared_from_this { on_update(std::move(cb)), pagination_id(next_pagination_id()) {} - // Requires `m`. Appends and subscribes one page. - void add_page(std::size_t num_items, std::optional cursor) { + // Requires `m`. Looks up a page by key, in `pages` or in a split's halves. + page* find_page(page_key key) { + for (page& p : pages) { + if (p.key == key) return &p; + } + for (pending_split& s : splits) { + if (s.first.key == key) return &s.first; + if (s.second.key == key) return &s.second; + } + return nullptr; + } + + // Requires `m`. Index into `splits` of the split `key` is a half of. + std::optional split_of_half(page_key key) const { + for (std::size_t i = 0; i < splits.size(); ++i) { + if (splits[i].first.key == key || splits[i].second.key == key) return i; + } + return std::nullopt; + } + + // Requires `m`. True while `key` is being replaced by two halves. + bool is_splitting(page_key key) const { + return std::any_of(splits.begin(), splits.end(), + [key](const pending_split& s) { return s.original == key; }); + } + + // Requires `m`. Subscribes an already-stored page to its page query. + void subscribe_page(page& p, std::size_t num_items) { const std::uint64_t gen = generation; - const std::size_t index = pages.size(); - pages.emplace_back(); + const page_key key = p.key; value_object opts; // numItems and id are float64 on the wire: paginationOptsValidator // uses v.number(), which rejects Convex int64 ($integer). opts.emplace("numItems", value(static_cast(num_items))); - opts.emplace("cursor", cursor ? value(std::move(*cursor)) : value(nullptr)); + opts.emplace("cursor", p.cursor ? value(*p.cursor) : value(nullptr)); + if (p.end_cursor) opts.emplace("endCursor", value(*p.end_cursor)); opts.emplace("id", value(static_cast(pagination_id))); value_object args = user_args; args.insert_or_assign("paginationOpts", value(std::move(opts))); - pages[index].sub = owner->subscribe( + // subscribe() delivers an already-known result synchronously, so the + // page callback can run — and reset the session, destroying `p` — + // before this returns. Everything after it goes through find_page; + // if the page is gone, the local subscription drops on scope exit. + client::subscription sub = owner->subscribe( udf_path, std::move(args), - [self = shared_from_this(), gen, index](const function_result& r) { - self->on_page_result(gen, index, r); + [self = shared_from_this(), gen, key](const function_result& r) { + self->on_page_result(gen, key, r); }); + if (gen != generation) return; + if (page* current = find_page(key)) current->sub = std::move(sub); + } + + // Requires `m`. Appends and subscribes one page at the end of the list. + void add_page(std::size_t num_items, std::optional cursor) { + pages.emplace_back(); + page& p = pages.back(); + p.key = ++next_key; + p.cursor = std::move(cursor); + subscribe_page(p, num_items); } // Requires `m`. Drops every page and starts a fresh session. void do_reset() { ++generation; pagination_id = next_pagination_id(); + splits.clear(); pages.clear(); // unsubscribes add_page(initial_num_items, std::nullopt); } - void on_page_result(std::uint64_t gen, std::size_t index, const function_result& r) { + // Requires `m`. A caller-driven reset (new args, or reset()) also clears + // the unsplittable-page state: the query may well behave differently now. + void hard_reset() { + unsplittable_resets = 0; + unsplittable_page = false; + do_reset(); + } + + // Requires `m`. Starts replacing `original` with two halves covering the + // same range: (cursor, split_cursor] and (split_cursor, continue_cursor]. + void start_split(page_key original, const std::string& split_cursor, + const std::string& continue_cursor) { + const page* orig = find_page(original); + if (orig == nullptr) return; + pending_split s; + s.original = original; + s.first.key = ++next_key; + // The first half starts where the original page started — NOT at the + // beginning of the query. convex-js sent a null cursor here, which + // duplicated every item before the page (get-convex/convex-js#54981). + s.first.cursor = orig->cursor; + s.first.end_cursor = split_cursor; + s.second.key = ++next_key; + s.second.cursor = split_cursor; + s.second.end_cursor = continue_cursor; + const page_key first_key = s.first.key; + const page_key second_key = s.second.key; + splits.push_back(std::move(s)); + // Re-find between the two: subscribing the first half can re-enter + // and move the splits vector out from under us. + if (page* p = find_page(first_key)) subscribe_page(*p, initial_num_items); + if (page* p = find_page(second_key)) subscribe_page(*p, initial_num_items); + } + + // Requires `m`. Called when a half of split `index` gets a result. + void advance_split(std::size_t index) { + { + pending_split& s = splits[index]; + if (!s.first.result || !s.second.result) return; + if (!s.first.result->ok() || !s.second.result->ok()) { + // A half failed for a reason other than InvalidCursor (that + // resets before we get here). Abandon the split: the original + // page is still live and still shows a usable result. Don't + // retry it, or every later update of that page re-runs the + // same failing query. + if (page* orig = find_page(s.original)) orig->split_disabled = true; + splits.erase(splits.begin() + static_cast(index)); + return; + } + } + pending_split done = std::move(splits[index]); + splits.erase(splits.begin() + static_cast(index)); + const auto it = std::find_if(pages.begin(), pages.end(), [&](const page& p) { + return p.key == done.original; + }); + if (it == pages.end()) return; + const auto at = it - pages.begin(); + const page_key first_key = done.first.key; + const page_key second_key = done.second.key; + *it = std::move(done.first); // unsubscribes the page being replaced + pages.insert(pages.begin() + at + 1, std::move(done.second)); + // A half can be oversized in its own right (the range it inherited + // may still be too big), so keep splitting until it isn't. + consider_split(first_key); + consider_split(second_key); + } + + // Requires `m`. Splits `key` if the server asked for it, or if the page + // has outgrown twice the requested size. Mirrors convex-js's + // processPaginatedQuerySplits. + void consider_split(page_key key) { + page* p = find_page(key); + if (p == nullptr || p->split_disabled || !p->result || !p->result->ok()) return; + if (is_splitting(key)) return; + const auto f = parse_page(p->result->get_value()); + if (!f) return; + if (f->split_required && !f->split_cursor) { + // No split point: a page the server calls incomplete cannot be + // repaired in place, so start over with right-sized pages. + reset_for_unsplittable_page(); + return; + } + unsplittable_resets = 0; // a page came back whole: not looping + if (!f->split_cursor) return; + if (f->split_required || f->split_recommended || + f->items->size() > initial_num_items * 2) { + start_split(key, *f->split_cursor, f->continue_cursor); + } + } + + // Requires `m`. + void reset_for_unsplittable_page() { + if (++unsplittable_resets > MAX_UNSPLITTABLE_RESETS) { + // Re-fetching keeps producing the same incomplete page, and each + // reset costs a fresh (uncached) query. Stop and report. + unsplittable_page = true; + return; + } + do_reset(); + } + + void on_page_result(std::uint64_t gen, page_key key, const function_result& r) { std::lock_guard lk(m); if (stopped || gen != generation) return; - pages[index].result = r; + page* p = find_page(key); + if (p == nullptr) return; + p->result = r; if (is_invalid_cursor(r)) { do_reset(); - } else if (r.ok()) { - if (const auto f = parse_page(r.get_value()); f && f->split_required) { - // The page outgrew the server's read limits and may be - // incomplete. convex-js splits it; we reset (see header). - do_reset(); - } + } else if (const auto index = split_of_half(key); index) { + advance_split(*index); + } else { + consider_split(key); } notify(); } @@ -164,6 +357,15 @@ struct paginated_impl : std::enable_shared_from_this { // Requires `m`. paginated_snapshot compute_snapshot() const { paginated_snapshot snap; + if (unsplittable_page) { + snap.status = pagination_status::error; + snap.error = function_result::error( + "convex: paginated query \"" + udf_path + + "\" keeps returning an incomplete page (pageStatus \"SplitRequired\") that " + "has no splitCursor to split it on. Reduce initial_num_items, or make the " + "query read less data per item."); + return snap; + } bool all_loaded = true; bool last_is_done = false; for (const page& p : pages) { @@ -209,6 +411,7 @@ struct paginated_impl : std::enable_shared_from_this { std::lock_guard lk(m); stopped = true; on_update = nullptr; + splits.clear(); pages.clear(); } }; @@ -255,7 +458,7 @@ bool paginated_query::set_args(value_object args) { if (impl_->stopped) return false; if (serialize_args(args) == serialize_args(impl_->user_args)) return false; impl_->user_args = std::move(args); - impl_->do_reset(); + impl_->hard_reset(); impl_->notify(); return true; } @@ -264,7 +467,7 @@ void paginated_query::reset() { if (!impl_) return; std::lock_guard lk(impl_->m); if (impl_->stopped) return; - impl_->do_reset(); + impl_->hard_reset(); impl_->notify(); } diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index e157173..f63e100 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -1,6 +1,6 @@ // paginated_query tests against the mock transport: paginationOpts wire // shape, cursor chaining, combined results across live page subscriptions, -// status transitions, args-change / InvalidCursor / SplitRequired resets, +// status transitions, args-change / InvalidCursor resets, page splitting, // and journal resend on reconnect (the seam-stability guarantee). #include @@ -50,9 +50,10 @@ constexpr const char* TS4 = "BAAAAAAAAAA="; // A PaginationResult value: string items for easy comparison. json page_value(const std::vector& items, bool is_done, const std::string& cursor, - const char* page_status = nullptr) { + const char* page_status = nullptr, const char* split_cursor = nullptr) { json v{{"page", items}, {"isDone", is_done}, {"continueCursor", cursor}}; if (page_status) v["pageStatus"] = page_status; + if (split_cursor) v["splitCursor"] = split_cursor; return v; } @@ -115,9 +116,41 @@ struct harness { json sent_json(std::size_t attempt, std::size_t index) { return json::parse(transport->sent(attempt).at(index)); } + // The Add modification for `query_id`, from anywhere in the sent stream + // (a split adds two pages, so positions are no longer predictable). + json add_for(int query_id, std::size_t attempt = 0) { + for (const std::string& raw : transport->sent(attempt)) { + const json j = json::parse(raw); + if (j.value("type", "") != "ModifyQuerySet") continue; + for (const json& mod : j["modifications"]) { + if (mod["type"] == "Add" && mod["queryId"] == query_id) return mod; + } + } + return json::object(); + } + bool sent_remove(int query_id, std::size_t attempt = 0) { + for (const std::string& raw : transport->sent(attempt)) { + const json j = json::parse(raw); + if (j.value("type", "") != "ModifyQuerySet") continue; + for (const json& mod : j["modifications"]) { + if (mod["type"] == "Remove" && mod["queryId"] == query_id) return true; + } + } + return false; + } + // A copy of the paginationOpts a page was subscribed with. Returned by + // value: a reference into add_for's temporary would dangle. + json opts_for(int query_id, std::size_t attempt = 0) { + const json add = add_for(query_id, attempt); + return add.contains("args") ? add["args"][0]["paginationOpts"] : json::object(); + } + double pagination_id(int query_id) { return opts_for(query_id)["id"].get(); } bool wait_status(pagination_status s) { return pump_until(c, [&] { return pq.snapshot().status == s; }); } + // Pump with nothing to wait for, so anything pending lands before an + // assertion that something did *not* happen. + void settle() { pump_until(c, [] { return false; }, 50ms); } }; } // namespace @@ -360,36 +393,244 @@ TEST(Paginated, OtherErrorsSurfaceWithoutReset) { EXPECT_EQ(h.transport->sent(0).size(), 3u) << "no reset traffic for ordinary errors"; } -TEST(Paginated, SplitRequiredResetsPagination) { +TEST(Paginated, SplitRequiredSplitsPageInTwo) { harness h; + // A page that outgrew the read limits: incomplete, with a split point. h.transport->server_send( 0, transition_page({0, TS0}, {1, TS1}, 0, - page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired"), "j0")); - // v1 has no page splitting: an incomplete (SplitRequired) page triggers - // a reset instead, which re-fetches right-sized pages. The reset runs in - // the pumped page callback, so pump while waiting for + Remove(q0), Add(q1). + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + // Two halves are subscribed; the split runs in the pumped page callback. ASSERT_TRUE(pump_until(h.c, [&] { return h.transport->sent(0).size() >= 4; })); - EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); - const json add_mqs = h.sent_json(0, 3); - const json& add = add_mqs["modifications"][0]; - EXPECT_EQ(add["type"], "Add"); - EXPECT_EQ(add["queryId"], 1); - EXPECT_TRUE(add["args"][0]["paginationOpts"]["cursor"].is_null()); + + const json first_opts = h.opts_for(1); + const json second_opts = h.opts_for(2); + ASSERT_FALSE(first_opts.empty()); + ASSERT_FALSE(second_opts.empty()); + // (beginning, s1] and (s1, c1]: together exactly the original range. + EXPECT_TRUE(first_opts["cursor"].is_null()); + EXPECT_EQ(first_opts["endCursor"], "s1"); + EXPECT_EQ(second_opts["cursor"], "s1"); + EXPECT_EQ(second_opts["endCursor"], "c1"); + EXPECT_EQ(first_opts["numItems"].get(), 2.0); + EXPECT_EQ(first_opts["id"].get(), h.pagination_id(0)) + << "a split stays inside the same pagination session"; + + // Until both halves load, the original page stays live and visible. + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})); + EXPECT_EQ(h.pq.snapshot().status, pagination_status::can_load_more); + EXPECT_FALSE(h.sent_remove(0)); h.transport->server_send( - 0, transition_page({1, TS1}, {3, TS2}, 1, page_value({"a", "b"}, false, "e1"), "j1")); + 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); + h.settle(); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})) + << "half loaded: no partial swap, no duplicated items"; + EXPECT_FALSE(h.sent_remove(0)); + + h.transport->server_send( + 0, transition_page({2, TS2}, {3, TS3}, 2, page_value({"c", "d"}, false, "c1"), "j2")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(0); })); + const auto snap = h.pq.snapshot(); + EXPECT_EQ(snap.results, items({"a", "b", "c", "d"})) << "same items, now over two pages"; + EXPECT_EQ(snap.status, pagination_status::can_load_more); + + // The list keeps growing from the second half's cursor. + ASSERT_TRUE(h.pq.load_more(2)); + EXPECT_EQ(h.opts_for(3)["cursor"], "c1"); +} + +TEST(Paginated, SplitFirstHalfStartsAtTheOriginalPagesCursor) { + // Regression guard for get-convex/convex-js#54981: convex-js sent a null + // cursor for the first half, so splitting any page but the first re-read + // the list from the beginning and duplicated everything before it. + harness h; + h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b"}, false, "c1"), "j0")); ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); + ASSERT_TRUE(h.pq.load_more(2)); + ASSERT_TRUE(h.transport->wait_for_sent(0, 3)); + + // The second page (which starts at c1) grew and needs splitting. + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, + page_value({"c", "d", "e"}, false, "c2", "SplitRecommended", "s2"), + "j1")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.transport->sent(0).size() >= 5; })); + + const json first_opts = h.opts_for(2); + EXPECT_FALSE(first_opts["cursor"].is_null()) << "must not restart from the beginning"; + EXPECT_EQ(first_opts["cursor"], "c1"); + EXPECT_EQ(first_opts["endCursor"], "s2"); + EXPECT_EQ(h.opts_for(3)["cursor"], "s2"); + + h.transport->server_send( + 0, transition_page({2, TS2}, {3, TS3}, 2, page_value({"c", "d"}, false, "s2"), "j2")); + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"e"}, true, "c2"), "j3")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d", "e"})); +} + +TEST(Paginated, OversizedPageSplitsWithoutServerAsking) { + // convex-js splits any page that grew past twice the requested size, + // whatever pageStatus says. + harness h; // initial_num_items = 2 + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.transport->sent(0).size() >= 4; })); + EXPECT_EQ(h.opts_for(1)["endCursor"], "s1"); + EXPECT_EQ(h.opts_for(2)["cursor"], "s1"); } -TEST(Paginated, SplitRecommendedIsIgnored) { +TEST(Paginated, PageWithinSizeAndNoSplitCursorIsLeftAlone) { harness h; h.transport->server_send( 0, transition_page({0, TS0}, {1, TS1}, 0, page_value({"a", "b", "c"}, false, "c1", "SplitRecommended"), "j0")); ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c"})); - EXPECT_EQ(h.transport->sent(0).size(), 2u) << "no reset, no extra traffic"; + EXPECT_EQ(h.transport->sent(0).size(), 2u) << "nothing to split on: no extra traffic"; +} + +TEST(Paginated, SplitRequiredWithoutSplitCursorResetsPagination) { + harness h; + // Incomplete, but too few items to have a split point: the only fix is + // to re-fetch right-sized pages. + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a"}, false, "c1", "SplitRequired"), "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.transport->sent(0).size() >= 4; })); + EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); + EXPECT_TRUE(h.sent_remove(0)); + const json opts = h.opts_for(1); + EXPECT_TRUE(opts["cursor"].is_null()); + EXPECT_FALSE(opts.contains("endCursor")); + EXPECT_NE(opts["id"].get(), h.pagination_id(0)) << "a reset is a new session"; + + h.transport->server_send( + 0, transition_page({1, TS1}, {3, TS2}, 1, page_value({"a", "b"}, false, "e1"), "j1")); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); +} + +TEST(Paginated, RepeatedUnsplittablePageStopsResettingAndErrors) { + harness h; + // Every re-fetch produces the same unsplittable incomplete page. Reset + // is capped so this cannot spin on the server forever. + const json bad = page_value({"a"}, false, "c1", "SplitRequired"); + h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, bad, "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); + h.transport->server_send(0, transition_page({1, TS1}, {2, TS2}, 1, bad, "j1")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + h.transport->server_send(0, transition_page({2, TS2}, {3, TS3}, 2, bad, "j2")); + + ASSERT_TRUE(h.wait_status(pagination_status::error)); + const auto snap = h.pq.snapshot(); + ASSERT_TRUE(snap.error.has_value()); + EXPECT_NE(snap.error->error_message().find("SplitRequired"), std::string::npos); + EXPECT_NE(snap.error->error_message().find("initial_num_items"), std::string::npos); + const std::size_t sent = h.transport->sent(0).size(); + h.settle(); + EXPECT_EQ(h.transport->sent(0).size(), sent) << "no further reset traffic"; + + // reset() clears the give-up state and tries again. + h.pq.reset(); + EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"x"}, true, "d1"), "k0")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"x"})); +} + +TEST(Paginated, FailedSplitHalfKeepsTheOriginalPage) { + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); + h.transport->server_send( + 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); + // Both half subscriptions are dropped; the original page keeps serving. + ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })); + const auto snap = h.pq.snapshot(); + EXPECT_EQ(snap.status, pagination_status::can_load_more) << "a failed split is not an error"; + EXPECT_EQ(snap.results, items({"a", "b", "c", "d"})); + EXPECT_FALSE(h.sent_remove(0)); + + // The same page updating again must not retry the failing split. + const std::size_t sent = h.transport->sent(0).size(); + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 0, + page_value({"a", "b", "c", "d", "e"}, false, "c1", "SplitRequired", + "s1"), + "j0b")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.pq.snapshot().results.size() == 5; })); + EXPECT_EQ(h.transport->sent(0).size(), sent) << "no retry of the failed split"; +} + +TEST(Paginated, InvalidCursorInASplitHalfResetsPagination) { + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + + h.transport->server_send( + 0, transition_failed({1, TS1}, {2, TS2}, 1, + "InvalidCursor: Tried to run a query starting from a cursor " + "created by a different query")); + ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)); + EXPECT_TRUE(h.pq.snapshot().results.empty()); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); + const json opts = h.opts_for(3); + EXPECT_TRUE(opts["cursor"].is_null()); + EXPECT_FALSE(opts.contains("endCursor")); + EXPECT_NE(opts["id"].get(), h.pagination_id(0)); +} + +TEST(Paginated, SplitPagesResubscribeWithTheirEndCursorsAfterReconnect) { + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); + h.transport->server_send( + 0, transition_page({2, TS2}, {3, TS3}, 2, page_value({"c", "d"}, false, "c1"), "j2")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(0); })); + + h.transport->server_close(0, "SimulatedNetworkBlip"); + ASSERT_TRUE(h.transport->wait_for_attempts(2)); + h.transport->open(1); + ASSERT_TRUE(h.transport->wait_for_sent(1, 2)); + + const json mqs = h.sent_json(1, 1); + ASSERT_EQ(mqs["modifications"].size(), 2u) << "only the two halves are active"; + for (const json& mod : mqs["modifications"]) { + const json& opts = mod["args"][0]["paginationOpts"]; + ASSERT_TRUE(opts.contains("endCursor")) << "split bounds survive a reconnect"; + if (mod["queryId"] == 1) { + EXPECT_TRUE(opts["cursor"].is_null()); + EXPECT_EQ(opts["endCursor"], "s1"); + EXPECT_EQ(mod["journal"], "j1"); + } else { + EXPECT_EQ(mod["queryId"], 2); + EXPECT_EQ(opts["cursor"], "s1"); + EXPECT_EQ(opts["endCursor"], "c1"); + EXPECT_EQ(mod["journal"], "j2"); + } + } } TEST(Paginated, MalformedPaginationResultIsAnError) { From d2795bb7868c1dfb6ad4048dd0564e99f055e9fc Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Fri, 24 Jul 2026 23:04:09 -0700 Subject: [PATCH 2/8] Cover page splitting in the live integration suite 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. --- integration/README.md | 3 + .../convex-test-project/convex/messages.ts | 14 ++++ integration/tests/integration_test.cpp | 74 +++++++++++++++++++ 3 files changed, 91 insertions(+) diff --git a/integration/README.md b/integration/README.md index 24f19ba..fee53c5 100644 --- a/integration/README.md +++ b/integration/README.md @@ -143,6 +143,9 @@ curl -X POST https://.convex.cloud/api/query \ - `messages:list` (query): args `{channel}`, returns messages oldest-first. - `messages:listPaginated` (query): args `{channel, paginationOpts}`, returns a `PaginationResult` page of messages oldest-first (for `paginated_query`). +- `messages:listPaginatedDesc` (query): the same, newest-first. Inserts land + inside the first page's range, so this is the one that grows a page past + its requested size and makes `paginated_query` split it. - `messages:send` (mutation): args `{channel, author, body}`, returns the new id. - `messages:clearAll` (mutation): no args, deletes all, returns count removed. - `counters:get` (query): args `{name}`, returns `number | null`. diff --git a/integration/convex-test-project/convex/messages.ts b/integration/convex-test-project/convex/messages.ts index 32cf619..352c14d 100644 --- a/integration/convex-test-project/convex/messages.ts +++ b/integration/convex-test-project/convex/messages.ts @@ -27,6 +27,20 @@ export const listPaginated = query({ }, }); +// Paginated list of messages, newest first. Inserts land inside the first +// page's cursor range instead of after the last page, so this is the query +// the paginated_query page-splitting test grows a page with. +export const listPaginatedDesc = query({ + args: { channel: v.string(), paginationOpts: paginationOptsValidator }, + handler: async (ctx, { channel, paginationOpts }) => { + return await ctx.db + .query("messages") + .withIndex("by_channel", (q) => q.eq("channel", channel)) + .order("desc") + .paginate(paginationOpts); + }, +}); + // Send (insert) a new message. Returns the new document id. export const send = mutation({ args: { diff --git a/integration/tests/integration_test.cpp b/integration/tests/integration_test.cpp index 78c2e3b..11e65a7 100644 --- a/integration/tests/integration_test.cpp +++ b/integration/tests/integration_test.cpp @@ -8,6 +8,7 @@ // CONVEX_CLOUD_URL enables the TLS smoke test against a cloud dev // deployment when set (wss://) +#include #include #include #include @@ -269,6 +270,79 @@ TEST(Live, PaginatedQueryChainsPagesAndSeesLiveInserts) { (std::vector{"m1", "m2", "m3", "m4", "m5", "m6"})); } +TEST(Live, PaginatedQuerySplitsAGrowingPage) { + // Newest-first, so every insert lands inside the first page's cursor + // range rather than after the last page. That is how a page outgrows the + // size it was requested at, and what page splitting exists to handle. + auto c = make_client(local_url()); + const std::string channel = unique_name("split"); + + auto send = [&](const std::string& body) { + auto m = c.mutation("messages:send", {{"channel", value(channel)}, + {"author", value("cpp")}, + {"body", value(body)}}); + ASSERT_TRUE(await(m).ok()); + }; + for (int i = 1; i <= 3; ++i) send("m" + std::to_string(i)); + + std::mutex mu; + std::condition_variable cv; + std::vector snaps; + paginated_query pq( + c, {"messages:listPaginatedDesc", {{"channel", value(channel)}}, /*initial_num_items=*/2}, + [&](const paginated_snapshot& s) { + std::lock_guard lk(mu); + snaps.push_back(s); + cv.notify_all(); + }); + auto wait_snap = [&](auto pred) { + std::unique_lock lk(mu); + return cv.wait_for(lk, 15s, [&] { return !snaps.empty() && pred(snaps.back()); }); + }; + auto bodies = [](const paginated_snapshot& s) { + std::vector out; + for (const value& doc : s.results) out.push_back(doc.as_object().at("body").as_string()); + return out; + }; + + ASSERT_TRUE(wait_snap([](const paginated_snapshot& s) { + return s.status == pagination_status::can_load_more && s.results.size() == 2; + })); + ASSERT_TRUE(pq.load_more(2)); + ASSERT_TRUE(wait_snap( + [](const paginated_snapshot& s) { return s.status == pagination_status::exhausted; })); + EXPECT_EQ(bodies(pq.snapshot()), (std::vector{"m3", "m2", "m1"})); + + // Three more inserts push the first page from 2 items to 5, past twice + // the size it was requested at. The helper splits it against the real + // server's splitCursor; the list must stay whole and in order through it. + for (int i = 4; i <= 6; ++i) send("m" + std::to_string(i)); + ASSERT_TRUE(wait_snap([](const paginated_snapshot& s) { return s.results.size() == 6; })); + const auto after_split = pq.snapshot(); + EXPECT_EQ(after_split.status, pagination_status::exhausted); + EXPECT_EQ(bodies(after_split), + (std::vector{"m6", "m5", "m4", "m3", "m2", "m1"})); + + // No snapshot along the way may repeat an item: the halves replace the + // page they split only once both have loaded. + std::vector worst; + { + std::lock_guard lk(mu); + for (const paginated_snapshot& s : snaps) { + std::vector seen = bodies(s); + std::sort(seen.begin(), seen.end()); + const auto dup = std::adjacent_find(seen.begin(), seen.end()); + if (dup != seen.end()) worst = bodies(s); + } + } + EXPECT_TRUE(worst.empty()) << "a snapshot showed an item twice: " << testing::PrintToString(worst); + + // The list still grows from the last page after the split. + send("m7"); + ASSERT_TRUE(wait_snap([](const paginated_snapshot& s) { return s.results.size() == 7; })); + EXPECT_EQ(bodies(pq.snapshot()).front(), "m7"); +} + TEST(Live, ConvexErrorCarriesData) { auto c = make_client(local_url()); auto future = c.query("errors:throwConvexError", {}); From 3fd3dab1e194fb1ba54edd9344abb4c066e9ffd8 Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Fri, 24 Jul 2026 23:44:42 -0700 Subject: [PATCH 3/8] Repair incomplete pages when a split fails, and bound the retry per range 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. --- include/convex/paginated.h | 17 +++++-- src/paginated.cpp | 90 +++++++++++++++++++++++++++----------- tests/paginated_test.cpp | 89 ++++++++++++++++++++++++++++++++++--- 3 files changed, 159 insertions(+), 37 deletions(-) diff --git a/include/convex/paginated.h b/include/convex/paginated.h index 5cf1603..7b528d6 100644 --- a/include/convex/paginated.h +++ b/include/convex/paginated.h @@ -19,10 +19,19 @@ // them in for the original only once both have loaded, so the combined list // is gapless and duplicate-free at every moment. A page is split when the // server reports pageStatus "SplitRequired" or "SplitRecommended", or when -// it has grown past twice initial_num_items. An incomplete page with no -// splitCursor (too few items to have a split point) cannot be repaired that -// way and resets pagination instead; if re-fetching keeps producing one, the -// helper stops resetting and reports an error rather than looping. +// it has grown past twice initial_num_items. +// +// An incomplete page that cannot be repaired by splitting — no splitCursor to +// split on, or a half that fails — resets pagination instead. That reset is +// counted per page range, and after a few tries on the same range the helper +// stops resetting and reports an error rather than looping forever. A page +// that is merely oversized (complete, but larger than asked for) is never +// reset: if its split fails, it stays as it is. +// +// Known, and deliberate for parity with convex-js: between a SplitRequired +// update arriving and both halves loading, the incomplete page is what the +// snapshot shows. If the server truncated it, the list has a gap for that one +// round trip. convex-js splits from the updated page result the same way. // // Threading. All callbacks (page updates) arrive through the owning client's // delivery mechanism — the process_events() pump by default. The on_update diff --git a/src/paginated.cpp b/src/paginated.cpp index ae39337..5dda3ab 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -12,11 +12,11 @@ namespace convex { namespace { -/// How many times in a row an incomplete page that cannot be split may reset -/// the session before the helper gives up and reports an error. Each reset -/// re-runs the query under a new cache-buster id, so a page that always comes -/// back the same way would otherwise loop forever. -constexpr unsigned MAX_UNSPLITTABLE_RESETS = 2; +/// How many times in a row one incomplete page that cannot be repaired may +/// reset the session before the helper gives up and reports an error. Each +/// reset re-runs the query under a new cache-buster id, so a page that always +/// comes back the same way would otherwise loop forever. +constexpr unsigned MAX_UNREPAIRABLE_RESETS = 2; /// Cache-buster shared with every helper instance in the process, mirroring /// convex-js's module-global counter. A unique id per pagination session @@ -152,11 +152,16 @@ struct paginated_impl : std::enable_shared_from_this { /// Splits in flight. Their halves are not in `pages` yet. std::vector splits; page_key next_key = 0; - /// Consecutive resets caused by an incomplete page with no split point. - unsigned unsplittable_resets = 0; - /// Set when those resets hit MAX_UNSPLITTABLE_RESETS: the query cannot - /// return a complete page, so the snapshot reports an error instead. - bool unsplittable_page = false; + /// Start cursor of the page that keeps coming back unrepairable, and how + /// many resets in a row it has caused. Held per range, not per session: + /// after a reset only the first page exists, so counting any healthy page + /// as progress would clear the count before the consumer can page back + /// down to the one that is actually broken. + std::optional> failing_cursor; + unsigned unrepairable_resets = 0; + /// Set when those resets hit MAX_UNREPAIRABLE_RESETS: the query cannot + /// return a complete page there, so the snapshot reports an error instead. + bool unrepairable_page = false; paginated_impl(client& c, paginated_query::options&& o, paginated_query::snapshot_callback&& cb) @@ -238,10 +243,11 @@ struct paginated_impl : std::enable_shared_from_this { } // Requires `m`. A caller-driven reset (new args, or reset()) also clears - // the unsplittable-page state: the query may well behave differently now. + // the unrepairable-page state: the query may well behave differently now. void hard_reset() { - unsplittable_resets = 0; - unsplittable_page = false; + failing_cursor.reset(); + unrepairable_resets = 0; + unrepairable_page = false; do_reset(); } @@ -271,6 +277,14 @@ struct paginated_impl : std::enable_shared_from_this { if (page* p = find_page(second_key)) subscribe_page(*p, initial_num_items); } + // Requires `m`. True when the server called this page's result incomplete, + // so leaving it in place means showing a gap. + bool page_is_incomplete(const page& p) const { + if (!p.result || !p.result->ok()) return false; + const auto f = parse_page(p.result->get_value()); + return f && f->split_required; + } + // Requires `m`. Called when a half of split `index` gets a result. void advance_split(std::size_t index) { { @@ -278,12 +292,22 @@ struct paginated_impl : std::enable_shared_from_this { if (!s.first.result || !s.second.result) return; if (!s.first.result->ok() || !s.second.result->ok()) { // A half failed for a reason other than InvalidCursor (that - // resets before we get here). Abandon the split: the original - // page is still live and still shows a usable result. Don't - // retry it, or every later update of that page re-runs the - // same failing query. - if (page* orig = find_page(s.original)) orig->split_disabled = true; + // resets before we get here). Abandon the split. + const page_key original = s.original; splits.erase(splits.begin() + static_cast(index)); + page* orig = find_page(original); + if (orig == nullptr) return; + if (page_is_incomplete(*orig)) { + // The page being repaired is missing items. Keeping it + // would show that gap forever, so fall back to the reset + // path, which is capped and eventually reports. + reset_for_unrepairable_page(orig->cursor); + return; + } + // The page is complete, only too big. Keep it, and stop + // retrying a split that fails: otherwise every later update + // of the page runs the same failing query again. + orig->split_disabled = true; return; } } @@ -316,10 +340,16 @@ struct paginated_impl : std::enable_shared_from_this { if (f->split_required && !f->split_cursor) { // No split point: a page the server calls incomplete cannot be // repaired in place, so start over with right-sized pages. - reset_for_unsplittable_page(); + reset_for_unrepairable_page(p->cursor); return; } - unsplittable_resets = 0; // a page came back whole: not looping + // This range came back fine. Only that clears its own failure count — + // a reset reloads the first page, and counting that as progress would + // let a bad page further down reset forever. + if (failing_cursor && *failing_cursor == p->cursor) { + failing_cursor.reset(); + unrepairable_resets = 0; + } if (!f->split_cursor) return; if (f->split_required || f->split_recommended || f->items->size() > initial_num_items * 2) { @@ -327,12 +357,20 @@ struct paginated_impl : std::enable_shared_from_this { } } - // Requires `m`. - void reset_for_unsplittable_page() { - if (++unsplittable_resets > MAX_UNSPLITTABLE_RESETS) { + // Requires `m`. `cursor` is the start of the page that cannot be repaired; + // the count follows that range rather than the session, so a page deep in + // the list cannot reset forever behind a healthy first page. + void reset_for_unrepairable_page(const std::optional& cursor) { + if (failing_cursor && *failing_cursor == cursor) { + ++unrepairable_resets; + } else { + failing_cursor = cursor; + unrepairable_resets = 1; + } + if (unrepairable_resets > MAX_UNREPAIRABLE_RESETS) { // Re-fetching keeps producing the same incomplete page, and each // reset costs a fresh (uncached) query. Stop and report. - unsplittable_page = true; + unrepairable_page = true; return; } do_reset(); @@ -357,12 +395,12 @@ struct paginated_impl : std::enable_shared_from_this { // Requires `m`. paginated_snapshot compute_snapshot() const { paginated_snapshot snap; - if (unsplittable_page) { + if (unrepairable_page) { snap.status = pagination_status::error; snap.error = function_result::error( "convex: paginated query \"" + udf_path + "\" keeps returning an incomplete page (pageStatus \"SplitRequired\") that " - "has no splitCursor to split it on. Reduce initial_num_items, or make the " + "could not be repaired by splitting. Reduce initial_num_items, or make the " "query read less data per item."); return snap; } diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index f63e100..cb1fcef 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -546,11 +546,57 @@ TEST(Paginated, RepeatedUnsplittablePageStopsResettingAndErrors) { EXPECT_EQ(h.pq.snapshot().results, items({"x"})); } -TEST(Paginated, FailedSplitHalfKeepsTheOriginalPage) { +TEST(Paginated, UnrepairablePageAfterTheFirstAlsoHitsTheCap) { + // The reset count follows the failing range, not the session. A reset + // reloads a healthy first page every time, so counting that as progress + // would let a bad second page reset forever. + harness h; + const json good = page_value({"a", "b"}, false, "c1"); + const json bad = page_value({"c"}, false, "c2", "SplitRequired"); + + // (first page ok, load_more, second page unrepairable) x3. The first two + // rounds reset; the third gives up. + int query = 0; + version ts{0, TS0}; + for (int round = 0; round < 3; ++round) { + const int first = query; + const int second = query + 1; + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, first, good, + "j0")); + ts = {ts.query_set + 1, TS1}; + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; + ASSERT_TRUE(h.pq.load_more(2)) << "round " << round; + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second).empty(); })); + EXPECT_EQ(h.opts_for(second)["cursor"], "c1") << "round " << round; + + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, second, bad, + "j1")); + ts = {ts.query_set + 1, TS2}; + if (round < 2) { + // Reset: both pages dropped, a fresh first page subscribed. + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second + 1).empty(); })) + << "round " << round << " should have reset"; + EXPECT_TRUE(h.opts_for(second + 1)["cursor"].is_null()); + query = second + 1; + } + } + + ASSERT_TRUE(h.wait_status(pagination_status::error)); + const auto snap = h.pq.snapshot(); + ASSERT_TRUE(snap.error.has_value()); + EXPECT_NE(snap.error->error_message().find("SplitRequired"), std::string::npos); + const std::size_t sent = h.transport->sent(0).size(); + h.settle(); + EXPECT_EQ(h.transport->sent(0).size(), sent) << "no further reset traffic"; +} + +TEST(Paginated, FailedOptionalSplitKeepsTheCompletePage) { + // Oversized but complete (no pageStatus): if the split fails there is + // nothing wrong with the page itself, so keep it and stop retrying. harness h; h.transport->server_send( 0, transition_page({0, TS0}, {1, TS1}, 0, - page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s1"), "j0")); ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); @@ -562,20 +608,49 @@ TEST(Paginated, FailedSplitHalfKeepsTheOriginalPage) { ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })); const auto snap = h.pq.snapshot(); EXPECT_EQ(snap.status, pagination_status::can_load_more) << "a failed split is not an error"; - EXPECT_EQ(snap.results, items({"a", "b", "c", "d"})); - EXPECT_FALSE(h.sent_remove(0)); + EXPECT_EQ(snap.results, items({"a", "b", "c", "d", "e"})); + EXPECT_FALSE(h.sent_remove(0)) << "the page is complete: no reset"; // The same page updating again must not retry the failing split. const std::size_t sent = h.transport->sent(0).size(); h.transport->server_send( 0, transition_page({3, TS3}, {4, TS4}, 0, - page_value({"a", "b", "c", "d", "e"}, false, "c1", "SplitRequired", - "s1"), + page_value({"a", "b", "c", "d", "e", "f"}, false, "c1", nullptr, "s1"), "j0b")); - ASSERT_TRUE(pump_until(h.c, [&] { return h.pq.snapshot().results.size() == 5; })); + ASSERT_TRUE(pump_until(h.c, [&] { return h.pq.snapshot().results.size() == 6; })); EXPECT_EQ(h.transport->sent(0).size(), sent) << "no retry of the failed split"; } +TEST(Paginated, FailedRequiredSplitResetsInsteadOfKeepingAGap) { + // SplitRequired means the page is missing items. If the split that would + // repair it fails, keeping the page would show that gap forever, so this + // falls back to the (capped) reset. + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); + h.transport->server_send( + 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); + + ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)); + EXPECT_TRUE(h.pq.snapshot().results.empty()); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); + EXPECT_TRUE(h.sent_remove(0)) << "the incomplete page is dropped, not kept"; + const json opts = h.opts_for(3); + EXPECT_TRUE(opts["cursor"].is_null()); + EXPECT_NE(opts["id"].get(), h.pagination_id(0)) << "a reset is a new session"; + + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"a", "b"}, false, "e1"), "k0")); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); +} + TEST(Paginated, InvalidCursorInASplitHalfResetsPagination) { harness h; h.transport->server_send( From 7854f5c7c7cca5f9c16b0235b6be116eb916d07f Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Sat, 25 Jul 2026 00:05:01 -0700 Subject: [PATCH 4/8] Stop the results before an incomplete page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- include/convex/paginated.h | 9 ++++---- src/paginated.cpp | 13 ++++++++++-- tests/paginated_test.cpp | 43 ++++++++++++++++++++++++++++++++------ 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/include/convex/paginated.h b/include/convex/paginated.h index 7b528d6..ecd54ee 100644 --- a/include/convex/paginated.h +++ b/include/convex/paginated.h @@ -28,10 +28,11 @@ // that is merely oversized (complete, but larger than asked for) is never // reset: if its split fails, it stays as it is. // -// Known, and deliberate for parity with convex-js: between a SplitRequired -// update arriving and both halves loading, the incomplete page is what the -// snapshot shows. If the server truncated it, the list has a gap for that one -// round trip. convex-js splits from the updated page result the same way. +// A page the server reports as SplitRequired may be missing part of its +// range, so the snapshot stops before it: the pages ahead of it stay visible +// and the status goes back to loading until the split (or reset) repairs it. +// A list with a hole in it is worse than a shorter list. convex-js's +// usePaginatedQuery does the same. // // Threading. All callbacks (page updates) arrive through the owning client's // delivery mechanism — the process_events() pump by default. The on_update diff --git a/src/paginated.cpp b/src/paginated.cpp index 5dda3ab..7b7aec8 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -424,12 +424,21 @@ struct paginated_impl : std::enable_shared_from_this { "\" returned a value that is not a PaginationResult"); return snap; } + if (f->split_required) { + // The server could not read this page in full, so its items + // may be missing part of the range. Stop before it rather + // than publish a list with a hole in it — the split (or the + // reset) that repairs it is already under way. This is what + // convex-js's usePaginatedQuery does. + all_loaded = false; + break; + } snap.results.insert(snap.results.end(), f->items->begin(), f->items->end()); last_is_done = f->is_done; } if (!all_loaded) { - snap.status = (pages.size() == 1) ? pagination_status::loading_first_page - : pagination_status::loading_more; + snap.status = snap.results.empty() ? pagination_status::loading_first_page + : pagination_status::loading_more; } else { snap.status = last_is_done ? pagination_status::exhausted : pagination_status::can_load_more; diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index cb1fcef..a04d050 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -416,16 +416,17 @@ TEST(Paginated, SplitRequiredSplitsPageInTwo) { EXPECT_EQ(first_opts["id"].get(), h.pagination_id(0)) << "a split stays inside the same pagination session"; - // Until both halves load, the original page stays live and visible. - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})); - EXPECT_EQ(h.pq.snapshot().status, pagination_status::can_load_more); - EXPECT_FALSE(h.sent_remove(0)); + // The page is incomplete, so it is not published while the split runs: + // a short list beats a list with a hole in it. + EXPECT_TRUE(h.pq.snapshot().results.empty()); + EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); + EXPECT_FALSE(h.sent_remove(0)) << "the original page stays subscribed"; h.transport->server_send( 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); h.settle(); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})) - << "half loaded: no partial swap, no duplicated items"; + EXPECT_TRUE(h.pq.snapshot().results.empty()) + << "one half is not a page: no partial swap"; EXPECT_FALSE(h.sent_remove(0)); h.transport->server_send( @@ -440,6 +441,36 @@ TEST(Paginated, SplitRequiredSplitsPageInTwo) { EXPECT_EQ(h.opts_for(3)["cursor"], "c1"); } +TEST(Paginated, IncompletePageHidesItselfButNotThePagesBeforeIt) { + // Everything up to the incomplete page keeps showing; the truncated page + // and anything after it wait for the split. Mirrors convex-js's + // usePaginatedQuery, which stops results before a SplitRequired page. + harness h; + h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b"}, false, "c1"), "j0")); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + ASSERT_TRUE(h.pq.load_more(2)); + + // Page two comes back truncated: the server read past its limit, so "c" + // may not be all of (c1, c2]. + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, + page_value({"c"}, false, "c2", "SplitRequired", "s2"), "j1")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); + const auto during = h.pq.snapshot(); + EXPECT_EQ(during.results, items({"a", "b"})) << "page one is complete and stays"; + EXPECT_EQ(during.status, pagination_status::loading_more); + EXPECT_TRUE(during.is_loading()); + + // Both halves land: the full range appears at once. + h.transport->server_send( + 0, transition_page({2, TS2}, {3, TS3}, 2, page_value({"c"}, false, "s2"), "j2")); + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"d"}, true, "c2"), "j3")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})); +} + TEST(Paginated, SplitFirstHalfStartsAtTheOriginalPagesCursor) { // Regression guard for get-convex/convex-js#54981: convex-js sent a null // cursor for the first half, so splitting any page but the first re-read From c8797421b977869cfc68677b9a6b7fb395b6ce30 Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Sat, 25 Jul 2026 00:23:49 -0700 Subject: [PATCH 5/8] Make the unrepairable-page budget positional, and stop it leaking states MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/paginated.cpp | 117 +++++++++++++++++++++++++------------ tests/paginated_test.cpp | 121 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 37 deletions(-) diff --git a/src/paginated.cpp b/src/paginated.cpp index 7b7aec8..73e9f71 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -152,12 +152,17 @@ struct paginated_impl : std::enable_shared_from_this { /// Splits in flight. Their halves are not in `pages` yet. std::vector splits; page_key next_key = 0; - /// Start cursor of the page that keeps coming back unrepairable, and how - /// many resets in a row it has caused. Held per range, not per session: - /// after a reset only the first page exists, so counting any healthy page - /// as progress would clear the count before the consumer can page back - /// down to the one that is actually broken. - std::optional> failing_cursor; + /// Position of the page that could not be repaired, and how many resets in + /// a row it has caused. Positional on purpose: a start cursor moves as the + /// list changes underneath (on a newest-first feed every insert shifts the + /// page boundaries), so keying this on the cursor would make each failure + /// look like a new one and the count would never build. Counting per + /// session does not work either — a reset reloads the first page, which is + /// usually healthy, and calling that progress clears the count before the + /// consumer can page back down to the page that is actually broken. So: + /// count every consecutive failure, and clear only once the session gets + /// back to that position with a complete page. + std::optional failing_index; unsigned unrepairable_resets = 0; /// Set when those resets hit MAX_UNREPAIRABLE_RESETS: the query cannot /// return a complete page there, so the snapshot reports an error instead. @@ -184,6 +189,15 @@ struct paginated_impl : std::enable_shared_from_this { return nullptr; } + // Requires `m`. Position of an active page, or nullopt if `key` is not one + // (a split half, or already gone). + std::optional index_of(page_key key) const { + for (std::size_t i = 0; i < pages.size(); ++i) { + if (pages[i].key == key) return i; + } + return std::nullopt; + } + // Requires `m`. Index into `splits` of the split `key` is a half of. std::optional split_of_half(page_key key) const { for (std::size_t i = 0; i < splits.size(); ++i) { @@ -245,7 +259,7 @@ struct paginated_impl : std::enable_shared_from_this { // Requires `m`. A caller-driven reset (new args, or reset()) also clears // the unrepairable-page state: the query may well behave differently now. void hard_reset() { - failing_cursor.reset(); + failing_index.reset(); unrepairable_resets = 0; unrepairable_page = false; do_reset(); @@ -301,7 +315,9 @@ struct paginated_impl : std::enable_shared_from_this { // The page being repaired is missing items. Keeping it // would show that gap forever, so fall back to the reset // path, which is capped and eventually reports. - reset_for_unrepairable_page(orig->cursor); + if (const auto at = index_of(original)) { + reset_for_unrepairable_page(*at); + } return; } // The page is complete, only too big. Keep it, and stop @@ -333,23 +349,28 @@ struct paginated_impl : std::enable_shared_from_this { // processPaginatedQuerySplits. void consider_split(page_key key) { page* p = find_page(key); - if (p == nullptr || p->split_disabled || !p->result || !p->result->ok()) return; + if (p == nullptr || !p->result || !p->result->ok()) return; if (is_splitting(key)) return; + // Parse before looking at split_disabled: a page that was complete + // when its split failed can still turn incomplete later, and that + // needs the reset path rather than the flag's silence. const auto f = parse_page(p->result->get_value()); if (!f) return; - if (f->split_required && !f->split_cursor) { - // No split point: a page the server calls incomplete cannot be - // repaired in place, so start over with right-sized pages. - reset_for_unrepairable_page(p->cursor); - return; - } - // This range came back fine. Only that clears its own failure count — - // a reset reloads the first page, and counting that as progress would - // let a bad page further down reset forever. - if (failing_cursor && *failing_cursor == p->cursor) { - failing_cursor.reset(); - unrepairable_resets = 0; + const auto at = index_of(key); + if (f->split_required) { + if (!f->split_cursor || p->split_disabled) { + // Incomplete, and it cannot be repaired in place: either the + // server gave no split point, or splitting this page has + // already failed. Start over with right-sized pages. + if (at) reset_for_unrepairable_page(*at); + return; + } + // Still incomplete, so it is not progress — the failure count + // has to survive the split about to start below. + } else if (at) { + note_page_repaired(*at); } + if (p->split_disabled) return; if (!f->split_cursor) return; if (f->split_required || f->split_recommended || f->items->size() > initial_num_items * 2) { @@ -357,16 +378,24 @@ struct paginated_impl : std::enable_shared_from_this { } } - // Requires `m`. `cursor` is the start of the page that cannot be repaired; - // the count follows that range rather than the session, so a page deep in - // the list cannot reset forever behind a healthy first page. - void reset_for_unrepairable_page(const std::optional& cursor) { - if (failing_cursor && *failing_cursor == cursor) { - ++unrepairable_resets; - } else { - failing_cursor = cursor; - unrepairable_resets = 1; - } + // Requires `m`. A complete page at or past the position that could not be + // repaired means the session got back to it and it is fine now, whatever + // its cursor has become. That ends the streak. + void note_page_repaired(std::size_t index) { + if (!failing_index || index < *failing_index) return; + failing_index.reset(); + unrepairable_resets = 0; + // The failing page keeps its subscription after the cap trips, so a + // range that heals on its own has to be able to leave the error too. + unrepairable_page = false; + } + + // Requires `m`. `index` is the position of the page that cannot be + // repaired. Every consecutive failure counts, whether or not it is at the + // same position as the last one: on a live list the boundaries move. + void reset_for_unrepairable_page(std::size_t index) { + ++unrepairable_resets; + failing_index = index; if (unrepairable_resets > MAX_UNREPAIRABLE_RESETS) { // Re-fetching keeps producing the same incomplete page, and each // reset costs a fresh (uncached) query. Stop and report. @@ -395,18 +424,25 @@ struct paginated_impl : std::enable_shared_from_this { // Requires `m`. paginated_snapshot compute_snapshot() const { paginated_snapshot snap; - if (unrepairable_page) { - snap.status = pagination_status::error; - snap.error = function_result::error( + const auto unrepairable_error = [this] { + return function_result::error( "convex: paginated query \"" + udf_path + "\" keeps returning an incomplete page (pageStatus \"SplitRequired\") that " "could not be repaired by splitting. Reduce initial_num_items, or make the " "query read less data per item."); - return snap; - } + }; bool all_loaded = true; bool last_is_done = false; - for (const page& p : pages) { + for (std::size_t i = 0; i < pages.size(); ++i) { + const page& p = pages[i]; + if (unrepairable_page && failing_index && i == *failing_index) { + // Report where the list actually stops, so the pages the + // caller could already see stay visible — the same shape as + // an ordinary page error. + snap.status = pagination_status::error; + snap.error = unrepairable_error(); + return snap; + } if (!p.result) { all_loaded = false; break; @@ -436,6 +472,13 @@ struct paginated_impl : std::enable_shared_from_this { snap.results.insert(snap.results.end(), f->items->begin(), f->items->end()); last_is_done = f->is_done; } + if (unrepairable_page) { + // The failing page is no longer where it was (a reset or a split + // moved it). Still an error; just report it at the end. + snap.status = pagination_status::error; + snap.error = unrepairable_error(); + return snap; + } if (!all_loaded) { snap.status = snap.results.empty() ? pagination_status::loading_first_page : pagination_status::loading_more; diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index a04d050..2caa46b 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -616,11 +616,132 @@ TEST(Paginated, UnrepairablePageAfterTheFirstAlsoHitsTheCap) { const auto snap = h.pq.snapshot(); ASSERT_TRUE(snap.error.has_value()); EXPECT_NE(snap.error->error_message().find("SplitRequired"), std::string::npos); + EXPECT_EQ(snap.results, items({"a", "b"})) + << "the healthy prefix survives the error, like any page error"; const std::size_t sent = h.transport->sent(0).size(); h.settle(); EXPECT_EQ(h.transport->sent(0).size(), sent) << "no further reset traffic"; } +TEST(Paginated, MovingPageBoundariesStillHitTheCap) { + // Same shape as the test above, but page one's continueCursor moves every + // round, the way a newest-first feed behaves under inserts. The failure + // count must not restart just because the bad page now starts elsewhere. + harness h; + const char* cursors[] = {"c1", "c2", "c3", "c4"}; + int query = 0; + version ts{0, TS0}; + for (int round = 0; round < 3; ++round) { + const int first = query; + const int second = query + 1; + h.transport->server_send( + 0, transition_page(ts, {ts.query_set + 1, TS1}, first, + page_value({"a", "b"}, false, cursors[round]), "j0")); + ts = {ts.query_set + 1, TS1}; + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; + ASSERT_TRUE(h.pq.load_more(2)) << "round " << round; + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second).empty(); })); + EXPECT_EQ(h.opts_for(second)["cursor"], cursors[round]) << "boundary moved"; + + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, second, + page_value({"z"}, false, "end", + "SplitRequired"), + "j1")); + ts = {ts.query_set + 1, TS2}; + if (round < 2) { + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second + 1).empty(); })) + << "round " << round << " should have reset"; + query = second + 1; + } + } + + ASSERT_TRUE(h.wait_status(pagination_status::error)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})) + << "the pages before the failure stay visible"; +} + +TEST(Paginated, RepeatedFailedRequiredSplitHitsTheCap) { + // A SplitRequired page that *has* a split cursor is still incomplete, so + // seeing it again must not count as progress. Otherwise the failing-half + // loop refills its own budget and never terminates. + harness h; + const json bad = page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"); + int query = 0; + version ts{0, TS0}; + for (int round = 0; round < 3; ++round) { + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, bad, + "j0")); + ts = {ts.query_set + 1, TS1}; + // Two halves go out; one of them fails. + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 2).empty(); })) + << "round " << round; + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, query + 1, + page_value({"a", "b"}, false, "s1"), "j1")); + ts = {ts.query_set + 1, TS2}; + h.transport->server_send(0, transition_failed(ts, {ts.query_set + 1, TS3}, query + 2, + "Server Error: Uncaught Error: boom")); + ts = {ts.query_set + 1, TS3}; + if (round < 2) { + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 3).empty(); })) + << "round " << round << " should have reset"; + query += 3; + } + } + ASSERT_TRUE(h.wait_status(pagination_status::error)); + ASSERT_TRUE(h.pq.snapshot().error.has_value()); + EXPECT_NE(h.pq.snapshot().error->error_message().find("SplitRequired"), std::string::npos); +} + +TEST(Paginated, DisabledSplitPageThatTurnsIncompleteIsStillRepaired) { + // A failed optional split disables splitting for that page. If the page + // later goes incomplete, the flag must not silence it: nothing else would + // be in flight, and the snapshot hides incomplete pages. + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + h.transport->server_send( + 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); + h.transport->server_send( + 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })); + ASSERT_EQ(h.pq.snapshot().status, pagination_status::can_load_more); + + // The page keeps growing and the server now calls it incomplete. + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 0, + page_value({"a", "b", "c", "d", "e", "f"}, false, "c1", + "SplitRequired", "s1"), + "j0b")); + ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)) + << "must not sit on an incomplete page with nothing in flight"; + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); + EXPECT_TRUE(h.sent_remove(0)); + EXPECT_TRUE(h.opts_for(3)["cursor"].is_null()); +} + +TEST(Paginated, RangeThatHealsItselfLeavesTheErrorState) { + // The cap does not unsubscribe, so the failing page keeps updating. If the + // data changes and it comes back whole, the helper must recover on its own. + harness h; + const json bad = page_value({"a"}, false, "c1", "SplitRequired"); + h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, bad, "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); + h.transport->server_send(0, transition_page({1, TS1}, {2, TS2}, 1, bad, "j1")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + h.transport->server_send(0, transition_page({2, TS2}, {3, TS3}, 2, bad, "j2")); + ASSERT_TRUE(h.wait_status(pagination_status::error)); + + // Same subscription, healthy result this time. + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 2, page_value({"a", "b"}, true, "c1"), "j2b")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); + EXPECT_FALSE(h.pq.snapshot().error.has_value()); +} + TEST(Paginated, FailedOptionalSplitKeepsTheCompletePage) { // Oversized but complete (no pageStatus): if the split fails there is // nothing wrong with the page itself, so keep it and stop retrying. From 11c5a3f4d87c91c9d970570fd62507228c81ead7 Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Sat, 25 Jul 2026 00:52:18 -0700 Subject: [PATCH 6/8] Keep the failure marker valid when pages move under it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/paginated.cpp | 13 +++++- tests/paginated_test.cpp | 91 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/src/paginated.cpp b/src/paginated.cpp index 73e9f71..36f1ff7 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -337,6 +337,13 @@ struct paginated_impl : std::enable_shared_from_this { const page_key first_key = done.first.key; const page_key second_key = done.second.key; *it = std::move(done.first); // unsubscribes the page being replaced + // One page becomes two, so everything after it moves down. The failure + // marker is a position, so it has to move with them — otherwise a half + // of an earlier page lands on the marker and looks like the failing + // range came back healthy. + if (failing_index && static_cast(at) < *failing_index) { + ++(*failing_index); + } pages.insert(pages.begin() + at + 1, std::move(done.second)); // A half can be oversized in its own right (the range it inherited // may still be too big), so keep splitting until it isn't. @@ -412,7 +419,11 @@ struct paginated_impl : std::enable_shared_from_this { if (p == nullptr) return; p->result = r; if (is_invalid_cursor(r)) { - do_reset(); + // The cursors went stale, so the ranges the failure count was + // about no longer mean anything — and the page it points at may + // not exist in the new session, which would strand a capped error + // at an index nothing can reach. Start genuinely fresh. + hard_reset(); } else if (const auto index = split_of_half(key); index) { advance_split(*index); } else { diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index 2caa46b..ba3ad5f 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -660,6 +660,97 @@ TEST(Paginated, MovingPageBoundariesStillHitTheCap) { << "the pages before the failure stay visible"; } +TEST(Paginated, SplitBeforeTheFailingPageDoesNotClearTheBudget) { + // The failure marker is a position, and a completing split inserts a page. + // A half of an *earlier* page must not be mistaken for the failing range + // coming back healthy, or a first page that keeps splitting refills the + // budget forever. + harness h; + const json bad = page_value({"z"}, false, "end", "SplitRequired"); + version ts{0, TS0}; + auto send = [&](int query, const json& value, const char* journal) { + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, value, + journal)); + ts = {ts.query_set + 1, TS1}; + }; + + // Round one: page two is unrepairable. Budget 1, marker at index 1. + send(0, page_value({"a", "b"}, false, "c1"), "j0"); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + ASSERT_TRUE(h.pq.load_more(2)); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); + send(1, bad, "j1"); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + + // Round two: the new first page is oversized and splits, inserting a page + // ahead of the failing position. + send(2, page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s0"), "j2"); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(4).empty(); })); + send(3, page_value({"a", "b"}, false, "s0"), "j3"); + send(4, page_value({"c", "d", "e"}, false, "c1"), "j4"); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + ASSERT_EQ(h.pq.snapshot().results.size(), 5u) << "the split landed"; + + // The same bad range again: budget 2, not 1. + ASSERT_TRUE(h.pq.load_more(2)); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(5).empty(); })); + send(5, bad, "j5"); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(6).empty(); })); + + // Round three: budget 3 trips the cap. + send(6, page_value({"a", "b"}, false, "c1"), "j6"); + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); + ASSERT_TRUE(h.pq.load_more(2)); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(7).empty(); })); + send(7, bad, "j7"); + ASSERT_TRUE(h.wait_status(pagination_status::error)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); +} + +TEST(Paginated, InvalidCursorAfterTheCapStartsCleanInsteadOfStranding) { + // The capped error keeps the failing subscription alive so the range can + // heal. If that subscription reports InvalidCursor instead, the fresh + // session has no page at the failing position — carrying the capped state + // into it would leave an error nothing could clear. + harness h; + const json bad = page_value({"z"}, false, "end", "SplitRequired"); + version ts{0, TS0}; + int query = 0; + for (int round = 0; round < 3; ++round) { + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, + page_value({"a", "b"}, false, "c1"), "j0")); + ts = {ts.query_set + 1, TS1}; + ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; + ASSERT_TRUE(h.pq.load_more(2)); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 1).empty(); })); + h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, query + 1, bad, + "j1")); + ts = {ts.query_set + 1, TS2}; + if (round < 2) { + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 2).empty(); })); + query += 2; + } + } + ASSERT_TRUE(h.wait_status(pagination_status::error)); + const int failing_query = query + 1; + + // The still-live failing subscription now reports InvalidCursor. + h.transport->server_send( + 0, transition_failed(ts, {ts.query_set + 1, TS3}, failing_query, + "InvalidCursor: Tried to run a query starting from a cursor " + "created by a different query")); + ts = {ts.query_set + 1, TS3}; + ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)); + EXPECT_FALSE(h.pq.snapshot().error.has_value()) << "the capped error must not survive"; + + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(failing_query + 1).empty(); })); + h.transport->server_send( + 0, transition_page(ts, {ts.query_set + 1, TS4}, failing_query + 1, + page_value({"x", "y"}, true, "d1"), "k0")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"x", "y"})); +} + TEST(Paginated, RepeatedFailedRequiredSplitHitsTheCap) { // A SplitRequired page that *has* a split cursor is still incomplete, so // seeing it again must not count as progress. Otherwise the failing-half From 5278a8b5813a261ddd3fc2da5adab7ba48703b0e Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Sat, 25 Jul 2026 01:21:18 -0700 Subject: [PATCH 7/8] Drop the reset-and-cap machinery for unrepairable pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- include/convex/paginated.h | 30 ++-- src/paginated.cpp | 161 ++--------------- tests/paginated_test.cpp | 359 +++---------------------------------- 3 files changed, 49 insertions(+), 501 deletions(-) diff --git a/include/convex/paginated.h b/include/convex/paginated.h index ecd54ee..3ffbf2f 100644 --- a/include/convex/paginated.h +++ b/include/convex/paginated.h @@ -21,18 +21,19 @@ // server reports pageStatus "SplitRequired" or "SplitRecommended", or when // it has grown past twice initial_num_items. // -// An incomplete page that cannot be repaired by splitting — no splitCursor to -// split on, or a half that fails — resets pagination instead. That reset is -// counted per page range, and after a few tries on the same range the helper -// stops resetting and reports an error rather than looping forever. A page -// that is merely oversized (complete, but larger than asked for) is never -// reset: if its split fails, it stays as it is. -// // A page the server reports as SplitRequired may be missing part of its // range, so the snapshot stops before it: the pages ahead of it stay visible -// and the status goes back to loading until the split (or reset) repairs it. -// A list with a hole in it is worse than a shorter list. convex-js's -// usePaginatedQuery does the same. +// and the status goes back to loading until a split repairs it. A list with a +// hole in it is worse than a shorter list. convex-js's usePaginatedQuery does +// the same. +// +// Nothing here forces the issue beyond that. If the server reports a page +// incomplete but gives no splitCursor to split it on, the page simply stays +// out of the snapshot; its subscription is live, so a later result that fits +// the read limits repairs it. A query heavy enough that no page ever fits +// leaves the list loading indefinitely — again matching convex-js, which has +// no recovery path for that case either. The alternative, re-fetching the +// whole session to force the issue, is a loop with nothing to bound it. // // Threading. All callbacks (page updates) arrive through the owning client's // delivery mechanism — the process_events() pump by default. The on_update @@ -69,8 +70,7 @@ enum class pagination_status : std::uint8_t { /// The server reached the end of the list. exhausted, /// A page failed with an error other than InvalidCursor (those reset - /// pagination instead), or the query cannot produce a complete first - /// page. See paginated_snapshot::error. + /// pagination instead). See paginated_snapshot::error. error, }; @@ -104,9 +104,9 @@ struct paginated_impl; /// /// Pagination resets — dropping every page and re-subscribing a fresh first /// page with a new cache-buster id — happen when set_args() changes the -/// arguments, when a page fails with InvalidCursor (cursors went stale), or -/// when a page is incomplete and cannot be split (see file comment). -/// Splitting an oversized page is not a reset: loaded pages stay put. +/// arguments, when reset() is called, and when a page fails with InvalidCursor +/// (cursors went stale). Splitting an oversized page is not a reset: loaded +/// pages stay put. class paginated_query { public: using snapshot_callback = std::function; diff --git a/src/paginated.cpp b/src/paginated.cpp index 36f1ff7..924a0e8 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -12,12 +12,6 @@ namespace convex { namespace { -/// How many times in a row one incomplete page that cannot be repaired may -/// reset the session before the helper gives up and reports an error. Each -/// reset re-runs the query under a new cache-buster id, so a page that always -/// comes back the same way would otherwise loop forever. -constexpr unsigned MAX_UNREPAIRABLE_RESETS = 2; - /// Cache-buster shared with every helper instance in the process, mirroring /// convex-js's module-global counter. A unique id per pagination session /// keeps its pages' query journals independent (no deduplication onto an @@ -133,9 +127,6 @@ struct paginated_impl : std::enable_shared_from_this { /// both ends of their range so the two of them cover exactly what /// the page they replace covered. std::optional end_cursor; - /// Set when a split of this page failed, so it is not retried on - /// every subsequent update of the same page. - bool split_disabled = false; }; /// A split in flight: two half-pages loading in parallel while the page @@ -152,21 +143,6 @@ struct paginated_impl : std::enable_shared_from_this { /// Splits in flight. Their halves are not in `pages` yet. std::vector splits; page_key next_key = 0; - /// Position of the page that could not be repaired, and how many resets in - /// a row it has caused. Positional on purpose: a start cursor moves as the - /// list changes underneath (on a newest-first feed every insert shifts the - /// page boundaries), so keying this on the cursor would make each failure - /// look like a new one and the count would never build. Counting per - /// session does not work either — a reset reloads the first page, which is - /// usually healthy, and calling that progress clears the count before the - /// consumer can page back down to the page that is actually broken. So: - /// count every consecutive failure, and clear only once the session gets - /// back to that position with a complete page. - std::optional failing_index; - unsigned unrepairable_resets = 0; - /// Set when those resets hit MAX_UNREPAIRABLE_RESETS: the query cannot - /// return a complete page there, so the snapshot reports an error instead. - bool unrepairable_page = false; paginated_impl(client& c, paginated_query::options&& o, paginated_query::snapshot_callback&& cb) @@ -189,15 +165,6 @@ struct paginated_impl : std::enable_shared_from_this { return nullptr; } - // Requires `m`. Position of an active page, or nullopt if `key` is not one - // (a split half, or already gone). - std::optional index_of(page_key key) const { - for (std::size_t i = 0; i < pages.size(); ++i) { - if (pages[i].key == key) return i; - } - return std::nullopt; - } - // Requires `m`. Index into `splits` of the split `key` is a half of. std::optional split_of_half(page_key key) const { for (std::size_t i = 0; i < splits.size(); ++i) { @@ -256,15 +223,6 @@ struct paginated_impl : std::enable_shared_from_this { add_page(initial_num_items, std::nullopt); } - // Requires `m`. A caller-driven reset (new args, or reset()) also clears - // the unrepairable-page state: the query may well behave differently now. - void hard_reset() { - failing_index.reset(); - unrepairable_resets = 0; - unrepairable_page = false; - do_reset(); - } - // Requires `m`. Starts replacing `original` with two halves covering the // same range: (cursor, split_cursor] and (split_cursor, continue_cursor]. void start_split(page_key original, const std::string& split_cursor, @@ -291,14 +249,6 @@ struct paginated_impl : std::enable_shared_from_this { if (page* p = find_page(second_key)) subscribe_page(*p, initial_num_items); } - // Requires `m`. True when the server called this page's result incomplete, - // so leaving it in place means showing a gap. - bool page_is_incomplete(const page& p) const { - if (!p.result || !p.result->ok()) return false; - const auto f = parse_page(p.result->get_value()); - return f && f->split_required; - } - // Requires `m`. Called when a half of split `index` gets a result. void advance_split(std::size_t index) { { @@ -306,24 +256,11 @@ struct paginated_impl : std::enable_shared_from_this { if (!s.first.result || !s.second.result) return; if (!s.first.result->ok() || !s.second.result->ok()) { // A half failed for a reason other than InvalidCursor (that - // resets before we get here). Abandon the split. - const page_key original = s.original; + // resets before we get here). Drop the split and leave the + // page it was repairing in place. The next update of that page + // tries again — one attempt per server transition, not a loop + // we drive ourselves. splits.erase(splits.begin() + static_cast(index)); - page* orig = find_page(original); - if (orig == nullptr) return; - if (page_is_incomplete(*orig)) { - // The page being repaired is missing items. Keeping it - // would show that gap forever, so fall back to the reset - // path, which is capped and eventually reports. - if (const auto at = index_of(original)) { - reset_for_unrepairable_page(*at); - } - return; - } - // The page is complete, only too big. Keep it, and stop - // retrying a split that fails: otherwise every later update - // of the page runs the same failing query again. - orig->split_disabled = true; return; } } @@ -337,13 +274,6 @@ struct paginated_impl : std::enable_shared_from_this { const page_key first_key = done.first.key; const page_key second_key = done.second.key; *it = std::move(done.first); // unsubscribes the page being replaced - // One page becomes two, so everything after it moves down. The failure - // marker is a position, so it has to move with them — otherwise a half - // of an earlier page lands on the marker and looks like the failing - // range came back healthy. - if (failing_index && static_cast(at) < *failing_index) { - ++(*failing_index); - } pages.insert(pages.begin() + at + 1, std::move(done.second)); // A half can be oversized in its own right (the range it inherited // may still be too big), so keep splitting until it isn't. @@ -358,26 +288,11 @@ struct paginated_impl : std::enable_shared_from_this { page* p = find_page(key); if (p == nullptr || !p->result || !p->result->ok()) return; if (is_splitting(key)) return; - // Parse before looking at split_disabled: a page that was complete - // when its split failed can still turn incomplete later, and that - // needs the reset path rather than the flag's silence. const auto f = parse_page(p->result->get_value()); if (!f) return; - const auto at = index_of(key); - if (f->split_required) { - if (!f->split_cursor || p->split_disabled) { - // Incomplete, and it cannot be repaired in place: either the - // server gave no split point, or splitting this page has - // already failed. Start over with right-sized pages. - if (at) reset_for_unrepairable_page(*at); - return; - } - // Still incomplete, so it is not progress — the failure count - // has to survive the split about to start below. - } else if (at) { - note_page_repaired(*at); - } - if (p->split_disabled) return; + // No split point means nothing to do: an incomplete page stays out of + // the snapshot, and its subscription is live, so a later result that + // fits the limits repairs it on its own. if (!f->split_cursor) return; if (f->split_required || f->split_recommended || f->items->size() > initial_num_items * 2) { @@ -385,33 +300,6 @@ struct paginated_impl : std::enable_shared_from_this { } } - // Requires `m`. A complete page at or past the position that could not be - // repaired means the session got back to it and it is fine now, whatever - // its cursor has become. That ends the streak. - void note_page_repaired(std::size_t index) { - if (!failing_index || index < *failing_index) return; - failing_index.reset(); - unrepairable_resets = 0; - // The failing page keeps its subscription after the cap trips, so a - // range that heals on its own has to be able to leave the error too. - unrepairable_page = false; - } - - // Requires `m`. `index` is the position of the page that cannot be - // repaired. Every consecutive failure counts, whether or not it is at the - // same position as the last one: on a live list the boundaries move. - void reset_for_unrepairable_page(std::size_t index) { - ++unrepairable_resets; - failing_index = index; - if (unrepairable_resets > MAX_UNREPAIRABLE_RESETS) { - // Re-fetching keeps producing the same incomplete page, and each - // reset costs a fresh (uncached) query. Stop and report. - unrepairable_page = true; - return; - } - do_reset(); - } - void on_page_result(std::uint64_t gen, page_key key, const function_result& r) { std::lock_guard lk(m); if (stopped || gen != generation) return; @@ -419,11 +307,7 @@ struct paginated_impl : std::enable_shared_from_this { if (p == nullptr) return; p->result = r; if (is_invalid_cursor(r)) { - // The cursors went stale, so the ranges the failure count was - // about no longer mean anything — and the page it points at may - // not exist in the new session, which would strand a capped error - // at an index nothing can reach. Start genuinely fresh. - hard_reset(); + do_reset(); } else if (const auto index = split_of_half(key); index) { advance_split(*index); } else { @@ -435,25 +319,9 @@ struct paginated_impl : std::enable_shared_from_this { // Requires `m`. paginated_snapshot compute_snapshot() const { paginated_snapshot snap; - const auto unrepairable_error = [this] { - return function_result::error( - "convex: paginated query \"" + udf_path + - "\" keeps returning an incomplete page (pageStatus \"SplitRequired\") that " - "could not be repaired by splitting. Reduce initial_num_items, or make the " - "query read less data per item."); - }; bool all_loaded = true; bool last_is_done = false; - for (std::size_t i = 0; i < pages.size(); ++i) { - const page& p = pages[i]; - if (unrepairable_page && failing_index && i == *failing_index) { - // Report where the list actually stops, so the pages the - // caller could already see stay visible — the same shape as - // an ordinary page error. - snap.status = pagination_status::error; - snap.error = unrepairable_error(); - return snap; - } + for (const page& p : pages) { if (!p.result) { all_loaded = false; break; @@ -483,13 +351,6 @@ struct paginated_impl : std::enable_shared_from_this { snap.results.insert(snap.results.end(), f->items->begin(), f->items->end()); last_is_done = f->is_done; } - if (unrepairable_page) { - // The failing page is no longer where it was (a reset or a split - // moved it). Still an error; just report it at the end. - snap.status = pagination_status::error; - snap.error = unrepairable_error(); - return snap; - } if (!all_loaded) { snap.status = snap.results.empty() ? pagination_status::loading_first_page : pagination_status::loading_more; @@ -559,7 +420,7 @@ bool paginated_query::set_args(value_object args) { if (impl_->stopped) return false; if (serialize_args(args) == serialize_args(impl_->user_args)) return false; impl_->user_args = std::move(args); - impl_->hard_reset(); + impl_->do_reset(); impl_->notify(); return true; } @@ -568,7 +429,7 @@ void paginated_query::reset() { if (!impl_) return; std::lock_guard lk(impl_->m); if (impl_->stopped) return; - impl_->hard_reset(); + impl_->do_reset(); impl_->notify(); } diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index ba3ad5f..96bc152 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -526,372 +526,59 @@ TEST(Paginated, PageWithinSizeAndNoSplitCursorIsLeftAlone) { EXPECT_EQ(h.transport->sent(0).size(), 2u) << "nothing to split on: no extra traffic"; } -TEST(Paginated, SplitRequiredWithoutSplitCursorResetsPagination) { +TEST(Paginated, IncompletePageWithNoSplitPointWaitsInsteadOfResetting) { + // The server says the page is incomplete but gives nothing to split on. + // There is no client-side repair for that, so the page stays out of the + // snapshot and its live subscription waits. Re-fetching the whole session + // to force the issue would be a loop with nothing to bound it. harness h; - // Incomplete, but too few items to have a split point: the only fix is - // to re-fetch right-sized pages. h.transport->server_send( 0, transition_page({0, TS0}, {1, TS1}, 0, page_value({"a"}, false, "c1", "SplitRequired"), "j0")); - ASSERT_TRUE(pump_until(h.c, [&] { return h.transport->sent(0).size() >= 4; })); - EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); - EXPECT_TRUE(h.sent_remove(0)); - const json opts = h.opts_for(1); - EXPECT_TRUE(opts["cursor"].is_null()); - EXPECT_FALSE(opts.contains("endCursor")); - EXPECT_NE(opts["id"].get(), h.pagination_id(0)) << "a reset is a new session"; - - h.transport->server_send( - 0, transition_page({1, TS1}, {3, TS2}, 1, page_value({"a", "b"}, false, "e1"), "j1")); - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); -} - -TEST(Paginated, RepeatedUnsplittablePageStopsResettingAndErrors) { - harness h; - // Every re-fetch produces the same unsplittable incomplete page. Reset - // is capped so this cannot spin on the server forever. - const json bad = page_value({"a"}, false, "c1", "SplitRequired"); - h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, bad, "j0")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); - h.transport->server_send(0, transition_page({1, TS1}, {2, TS2}, 1, bad, "j1")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - h.transport->server_send(0, transition_page({2, TS2}, {3, TS3}, 2, bad, "j2")); - - ASSERT_TRUE(h.wait_status(pagination_status::error)); - const auto snap = h.pq.snapshot(); - ASSERT_TRUE(snap.error.has_value()); - EXPECT_NE(snap.error->error_message().find("SplitRequired"), std::string::npos); - EXPECT_NE(snap.error->error_message().find("initial_num_items"), std::string::npos); - const std::size_t sent = h.transport->sent(0).size(); h.settle(); - EXPECT_EQ(h.transport->sent(0).size(), sent) << "no further reset traffic"; - - // reset() clears the give-up state and tries again. - h.pq.reset(); EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); - h.transport->server_send( - 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"x"}, true, "d1"), "k0")); - ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); - EXPECT_EQ(h.pq.snapshot().results, items({"x"})); -} - -TEST(Paginated, UnrepairablePageAfterTheFirstAlsoHitsTheCap) { - // The reset count follows the failing range, not the session. A reset - // reloads a healthy first page every time, so counting that as progress - // would let a bad second page reset forever. - harness h; - const json good = page_value({"a", "b"}, false, "c1"); - const json bad = page_value({"c"}, false, "c2", "SplitRequired"); - - // (first page ok, load_more, second page unrepairable) x3. The first two - // rounds reset; the third gives up. - int query = 0; - version ts{0, TS0}; - for (int round = 0; round < 3; ++round) { - const int first = query; - const int second = query + 1; - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, first, good, - "j0")); - ts = {ts.query_set + 1, TS1}; - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; - ASSERT_TRUE(h.pq.load_more(2)) << "round " << round; - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second).empty(); })); - EXPECT_EQ(h.opts_for(second)["cursor"], "c1") << "round " << round; - - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, second, bad, - "j1")); - ts = {ts.query_set + 1, TS2}; - if (round < 2) { - // Reset: both pages dropped, a fresh first page subscribed. - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second + 1).empty(); })) - << "round " << round << " should have reset"; - EXPECT_TRUE(h.opts_for(second + 1)["cursor"].is_null()); - query = second + 1; - } - } - - ASSERT_TRUE(h.wait_status(pagination_status::error)); - const auto snap = h.pq.snapshot(); - ASSERT_TRUE(snap.error.has_value()); - EXPECT_NE(snap.error->error_message().find("SplitRequired"), std::string::npos); - EXPECT_EQ(snap.results, items({"a", "b"})) - << "the healthy prefix survives the error, like any page error"; - const std::size_t sent = h.transport->sent(0).size(); - h.settle(); - EXPECT_EQ(h.transport->sent(0).size(), sent) << "no further reset traffic"; -} - -TEST(Paginated, MovingPageBoundariesStillHitTheCap) { - // Same shape as the test above, but page one's continueCursor moves every - // round, the way a newest-first feed behaves under inserts. The failure - // count must not restart just because the bad page now starts elsewhere. - harness h; - const char* cursors[] = {"c1", "c2", "c3", "c4"}; - int query = 0; - version ts{0, TS0}; - for (int round = 0; round < 3; ++round) { - const int first = query; - const int second = query + 1; - h.transport->server_send( - 0, transition_page(ts, {ts.query_set + 1, TS1}, first, - page_value({"a", "b"}, false, cursors[round]), "j0")); - ts = {ts.query_set + 1, TS1}; - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; - ASSERT_TRUE(h.pq.load_more(2)) << "round " << round; - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second).empty(); })); - EXPECT_EQ(h.opts_for(second)["cursor"], cursors[round]) << "boundary moved"; - - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, second, - page_value({"z"}, false, "end", - "SplitRequired"), - "j1")); - ts = {ts.query_set + 1, TS2}; - if (round < 2) { - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(second + 1).empty(); })) - << "round " << round << " should have reset"; - query = second + 1; - } - } - - ASSERT_TRUE(h.wait_status(pagination_status::error)); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})) - << "the pages before the failure stay visible"; -} - -TEST(Paginated, SplitBeforeTheFailingPageDoesNotClearTheBudget) { - // The failure marker is a position, and a completing split inserts a page. - // A half of an *earlier* page must not be mistaken for the failing range - // coming back healthy, or a first page that keeps splitting refills the - // budget forever. - harness h; - const json bad = page_value({"z"}, false, "end", "SplitRequired"); - version ts{0, TS0}; - auto send = [&](int query, const json& value, const char* journal) { - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, value, - journal)); - ts = {ts.query_set + 1, TS1}; - }; - - // Round one: page two is unrepairable. Budget 1, marker at index 1. - send(0, page_value({"a", "b"}, false, "c1"), "j0"); - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - ASSERT_TRUE(h.pq.load_more(2)); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); - send(1, bad, "j1"); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - - // Round two: the new first page is oversized and splits, inserting a page - // ahead of the failing position. - send(2, page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s0"), "j2"); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(4).empty(); })); - send(3, page_value({"a", "b"}, false, "s0"), "j3"); - send(4, page_value({"c", "d", "e"}, false, "c1"), "j4"); - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - ASSERT_EQ(h.pq.snapshot().results.size(), 5u) << "the split landed"; - - // The same bad range again: budget 2, not 1. - ASSERT_TRUE(h.pq.load_more(2)); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(5).empty(); })); - send(5, bad, "j5"); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(6).empty(); })); - - // Round three: budget 3 trips the cap. - send(6, page_value({"a", "b"}, false, "c1"), "j6"); - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - ASSERT_TRUE(h.pq.load_more(2)); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(7).empty(); })); - send(7, bad, "j7"); - ASSERT_TRUE(h.wait_status(pagination_status::error)); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); -} - -TEST(Paginated, InvalidCursorAfterTheCapStartsCleanInsteadOfStranding) { - // The capped error keeps the failing subscription alive so the range can - // heal. If that subscription reports InvalidCursor instead, the fresh - // session has no page at the failing position — carrying the capped state - // into it would leave an error nothing could clear. - harness h; - const json bad = page_value({"z"}, false, "end", "SplitRequired"); - version ts{0, TS0}; - int query = 0; - for (int round = 0; round < 3; ++round) { - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, - page_value({"a", "b"}, false, "c1"), "j0")); - ts = {ts.query_set + 1, TS1}; - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)) << "round " << round; - ASSERT_TRUE(h.pq.load_more(2)); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 1).empty(); })); - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, query + 1, bad, - "j1")); - ts = {ts.query_set + 1, TS2}; - if (round < 2) { - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 2).empty(); })); - query += 2; - } - } - ASSERT_TRUE(h.wait_status(pagination_status::error)); - const int failing_query = query + 1; - - // The still-live failing subscription now reports InvalidCursor. - h.transport->server_send( - 0, transition_failed(ts, {ts.query_set + 1, TS3}, failing_query, - "InvalidCursor: Tried to run a query starting from a cursor " - "created by a different query")); - ts = {ts.query_set + 1, TS3}; - ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)); - EXPECT_FALSE(h.pq.snapshot().error.has_value()) << "the capped error must not survive"; - - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(failing_query + 1).empty(); })); - h.transport->server_send( - 0, transition_page(ts, {ts.query_set + 1, TS4}, failing_query + 1, - page_value({"x", "y"}, true, "d1"), "k0")); - ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); - EXPECT_EQ(h.pq.snapshot().results, items({"x", "y"})); -} - -TEST(Paginated, RepeatedFailedRequiredSplitHitsTheCap) { - // A SplitRequired page that *has* a split cursor is still incomplete, so - // seeing it again must not count as progress. Otherwise the failing-half - // loop refills its own budget and never terminates. - harness h; - const json bad = page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"); - int query = 0; - version ts{0, TS0}; - for (int round = 0; round < 3; ++round) { - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS1}, query, bad, - "j0")); - ts = {ts.query_set + 1, TS1}; - // Two halves go out; one of them fails. - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 2).empty(); })) - << "round " << round; - h.transport->server_send(0, transition_page(ts, {ts.query_set + 1, TS2}, query + 1, - page_value({"a", "b"}, false, "s1"), "j1")); - ts = {ts.query_set + 1, TS2}; - h.transport->server_send(0, transition_failed(ts, {ts.query_set + 1, TS3}, query + 2, - "Server Error: Uncaught Error: boom")); - ts = {ts.query_set + 1, TS3}; - if (round < 2) { - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(query + 3).empty(); })) - << "round " << round << " should have reset"; - query += 3; - } - } - ASSERT_TRUE(h.wait_status(pagination_status::error)); - ASSERT_TRUE(h.pq.snapshot().error.has_value()); - EXPECT_NE(h.pq.snapshot().error->error_message().find("SplitRequired"), std::string::npos); -} - -TEST(Paginated, DisabledSplitPageThatTurnsIncompleteIsStillRepaired) { - // A failed optional split disables splitting for that page. If the page - // later goes incomplete, the flag must not silence it: nothing else would - // be in flight, and the snapshot hides incomplete pages. - harness h; - h.transport->server_send( - 0, transition_page({0, TS0}, {1, TS1}, 0, - page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s1"), - "j0")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - h.transport->server_send( - 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); - h.transport->server_send( - 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); - ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })); - ASSERT_EQ(h.pq.snapshot().status, pagination_status::can_load_more); - - // The page keeps growing and the server now calls it incomplete. - h.transport->server_send( - 0, transition_page({3, TS3}, {4, TS4}, 0, - page_value({"a", "b", "c", "d", "e", "f"}, false, "c1", - "SplitRequired", "s1"), - "j0b")); - ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)) - << "must not sit on an incomplete page with nothing in flight"; - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); - EXPECT_TRUE(h.sent_remove(0)); - EXPECT_TRUE(h.opts_for(3)["cursor"].is_null()); -} - -TEST(Paginated, RangeThatHealsItselfLeavesTheErrorState) { - // The cap does not unsubscribe, so the failing page keeps updating. If the - // data changes and it comes back whole, the helper must recover on its own. - harness h; - const json bad = page_value({"a"}, false, "c1", "SplitRequired"); - h.transport->server_send(0, transition_page({0, TS0}, {1, TS1}, 0, bad, "j0")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(1).empty(); })); - h.transport->server_send(0, transition_page({1, TS1}, {2, TS2}, 1, bad, "j1")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - h.transport->server_send(0, transition_page({2, TS2}, {3, TS3}, 2, bad, "j2")); - ASSERT_TRUE(h.wait_status(pagination_status::error)); + EXPECT_TRUE(h.pq.snapshot().results.empty()); + EXPECT_FALSE(h.pq.snapshot().error.has_value()) << "loading, not an error"; + EXPECT_EQ(h.transport->sent(0).size(), 2u) << "no reset traffic at all"; + EXPECT_FALSE(h.sent_remove(0)) << "the subscription stays, so it can recover"; - // Same subscription, healthy result this time. - h.transport->server_send( - 0, transition_page({3, TS3}, {4, TS4}, 2, page_value({"a", "b"}, true, "c1"), "j2b")); + // It recovers on its own when the server can read the page in full. + h.transport->server_send(0, transition_page({1, TS1}, {2, TS2}, 0, + page_value({"a", "b"}, true, "c1"), "j0b")); ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); - EXPECT_FALSE(h.pq.snapshot().error.has_value()); } -TEST(Paginated, FailedOptionalSplitKeepsTheCompletePage) { - // Oversized but complete (no pageStatus): if the split fails there is - // nothing wrong with the page itself, so keep it and stop retrying. +TEST(Paginated, FailedSplitLeavesThePageAndRetriesOnTheNextUpdate) { + // A half failing is not fatal: drop the split, keep the page, and try + // again when the server next sends that page. One attempt per transition, + // never a loop the client drives. harness h; h.transport->server_send( 0, transition_page({0, TS0}, {1, TS1}, 0, page_value({"a", "b", "c", "d", "e"}, false, "c1", nullptr, "s1"), "j0")); ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - h.transport->server_send( 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); h.transport->server_send( 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); - // Both half subscriptions are dropped; the original page keeps serving. ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })); + const auto snap = h.pq.snapshot(); EXPECT_EQ(snap.status, pagination_status::can_load_more) << "a failed split is not an error"; EXPECT_EQ(snap.results, items({"a", "b", "c", "d", "e"})); - EXPECT_FALSE(h.sent_remove(0)) << "the page is complete: no reset"; + EXPECT_FALSE(h.sent_remove(0)); - // The same page updating again must not retry the failing split. - const std::size_t sent = h.transport->sent(0).size(); + // The next update of the same page starts a fresh split attempt. h.transport->server_send( 0, transition_page({3, TS3}, {4, TS4}, 0, page_value({"a", "b", "c", "d", "e", "f"}, false, "c1", nullptr, "s1"), "j0b")); - ASSERT_TRUE(pump_until(h.c, [&] { return h.pq.snapshot().results.size() == 6; })); - EXPECT_EQ(h.transport->sent(0).size(), sent) << "no retry of the failed split"; -} - -TEST(Paginated, FailedRequiredSplitResetsInsteadOfKeepingAGap) { - // SplitRequired means the page is missing items. If the split that would - // repair it fails, keeping the page would show that gap forever, so this - // falls back to the (capped) reset. - harness h; - h.transport->server_send( - 0, transition_page({0, TS0}, {1, TS1}, 0, - page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), - "j0")); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); - - h.transport->server_send( - 0, transition_page({1, TS1}, {2, TS2}, 1, page_value({"a", "b"}, false, "s1"), "j1")); - h.transport->server_send( - 0, transition_failed({2, TS2}, {3, TS3}, 2, "Server Error: Uncaught Error: boom")); - - ASSERT_TRUE(h.wait_status(pagination_status::loading_first_page)); - EXPECT_TRUE(h.pq.snapshot().results.empty()); - ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(3).empty(); })); - EXPECT_TRUE(h.sent_remove(0)) << "the incomplete page is dropped, not kept"; - const json opts = h.opts_for(3); - EXPECT_TRUE(opts["cursor"].is_null()); - EXPECT_NE(opts["id"].get(), h.pagination_id(0)) << "a reset is a new session"; - - h.transport->server_send( - 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"a", "b"}, false, "e1"), "k0")); - ASSERT_TRUE(h.wait_status(pagination_status::can_load_more)); - EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(4).empty(); })) + << "the split is retried, not disabled forever"; + EXPECT_EQ(h.opts_for(3)["endCursor"], "s1"); + EXPECT_EQ(h.opts_for(4)["cursor"], "s1"); } TEST(Paginated, InvalidCursorInASplitHalfResetsPagination) { From 76a09eb62120bded582883c81deee58e5323dfeb Mon Sep 17 00:00:00 2001 From: samanqrfk Date: Sat, 25 Jul 2026 01:52:18 -0700 Subject: [PATCH 8/8] Abandon a split as soon as a half fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/paginated.cpp | 19 ++++++++++++------- tests/paginated_test.cpp | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/paginated.cpp b/src/paginated.cpp index 924a0e8..661a528 100644 --- a/src/paginated.cpp +++ b/src/paginated.cpp @@ -253,16 +253,21 @@ struct paginated_impl : std::enable_shared_from_this { void advance_split(std::size_t index) { { pending_split& s = splits[index]; - if (!s.first.result || !s.second.result) return; - if (!s.first.result->ok() || !s.second.result->ok()) { - // A half failed for a reason other than InvalidCursor (that - // resets before we get here). Drop the split and leave the - // page it was repairing in place. The next update of that page - // tries again — one attempt per server transition, not a loop - // we drive ourselves. + // Look for a failure before waiting on the peer. A split with a + // failed half can never complete, and while it sits here + // is_splitting blocks every later attempt to repair the page it + // was splitting — including an update that would have worked. + if ((s.first.result && !s.first.result->ok()) || + (s.second.result && !s.second.result->ok())) { + // An ordinary error; InvalidCursor resets before we get here. + // Drop the split and leave the page it was repairing in place. + // The next update of that page tries again. Retrying from here + // instead would re-subscribe the same failing query, whose + // cached error is delivered synchronously, and recurse. splits.erase(splits.begin() + static_cast(index)); return; } + if (!s.first.result || !s.second.result) return; } pending_split done = std::move(splits[index]); splits.erase(splits.begin() + static_cast(index)); diff --git a/tests/paginated_test.cpp b/tests/paginated_test.cpp index 96bc152..d1a751d 100644 --- a/tests/paginated_test.cpp +++ b/tests/paginated_test.cpp @@ -549,6 +549,45 @@ TEST(Paginated, IncompletePageWithNoSplitPointWaitsInsteadOfResetting) { EXPECT_EQ(h.pq.snapshot().results, items({"a", "b"})); } +TEST(Paginated, SplitIsAbandonedAsSoonAsAHalfFails) { + // The peer may never arrive. Waiting for it before dropping a split that + // cannot complete leaves the page it was repairing blocked, because + // consider_split skips a page while a split of it is pending. + harness h; + h.transport->server_send( + 0, transition_page({0, TS0}, {1, TS1}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(2).empty(); })); + EXPECT_TRUE(h.pq.snapshot().results.empty()) << "incomplete page stays hidden"; + + // One half fails; the other never returns. + h.transport->server_send( + 0, transition_failed({1, TS1}, {2, TS2}, 1, "Server Error: Uncaught Error: boom")); + ASSERT_TRUE(pump_until(h.c, [&] { return h.sent_remove(1) && h.sent_remove(2); })) + << "both halves drop without waiting for the peer"; + EXPECT_EQ(h.pq.snapshot().status, pagination_status::loading_first_page); + EXPECT_FALSE(h.sent_remove(0)) << "the page itself stays subscribed"; + + // The page is no longer blocked: its next update starts a fresh split. + h.transport->server_send( + 0, transition_page({2, TS2}, {3, TS3}, 0, + page_value({"a", "b", "c", "d"}, false, "c1", "SplitRequired", "s1"), + "j0b")); + ASSERT_TRUE(pump_until(h.c, [&] { return !h.add_for(4).empty(); })) + << "a dead split must not block the retry"; + EXPECT_EQ(h.opts_for(3)["endCursor"], "s1"); + EXPECT_EQ(h.opts_for(4)["cursor"], "s1"); + + // And that one completes. + h.transport->server_send( + 0, transition_page({3, TS3}, {4, TS4}, 3, page_value({"a", "b"}, false, "s1"), "k0")); + h.transport->server_send( + 0, transition_page({4, TS4}, {5, TS4}, 4, page_value({"c", "d"}, true, "c1"), "k1")); + ASSERT_TRUE(h.wait_status(pagination_status::exhausted)); + EXPECT_EQ(h.pq.snapshot().results, items({"a", "b", "c", "d"})); +} + TEST(Paginated, FailedSplitLeavesThePageAndRetriesOnTheNextUpdate) { // A half failing is not fatal: drop the split, keep the page, and try // again when the server next sends that page. One attempt per transition,