From 47d5f6d5881d17f046448375c7d745c43039fd45 Mon Sep 17 00:00:00 2001 From: Michael Burns Date: Mon, 24 Aug 2026 01:21:28 -0700 Subject: [PATCH] perf(api): exact keyset cursors for the `new` sort The API caps cursor depth at 1000 because `paginate` locates a cursor by scanning the rows it was handed. `new` does not need that: its key, (created_at, id), never moves once a post is written, so the database can seek straight to the cursor row and return only the page. get_listing takes after_id/before_id and compares the whole key as a row value: WHERE (a.created_at, a.id) < (SELECT created_at, id FROM posts WHERE id = ?) which expresses "strictly past that row in this order" in one shot, tiebreaker included. A cursor id that no longer exists makes the subquery NULL, so the comparison is NULL and no rows come back -- a stale cursor reads as "nothing after this", which is the answer the API wants anyway. Walking backwards runs the comparison the other way in ascending order and flips the rows, so callers always get one order. The ranked sorts keep the window-and-cap treatment. That is a property of the data, not a shortcut: hot/controversial/rising rank on live vote counts, so a row's position moves between requests and a cursor into one is approximate however it is implemented. Posts.KEYSET_SORTS is the list, and get_listing asserts when a cursor arrives with a sort that is not on it, because that failure would otherwise be silent and subtly wrong. luacheck caught a real bug while writing this: `local _, cursor_id = cursor and S.parse_fullname(cursor)` truncates to one value, so cursor_id was always nil and the keyset path would never have fired. The tests still passed, because the window path handles those cases correctly -- which is why the spec now captures the filters the endpoint passes the model rather than counting queries. Both paths are a single query, so a query count cannot tell them apart. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0128hUpuk1spKzk4UHburdki --- CHANGELOG.md | 14 ++++++ app/spec/api_pagination_spec.lua | 85 ++++++++++++++++++++++++++++++++ app/src/api.lua | 30 ++++++++++- app/src/models/posts.lua | 49 +++++++++++++++++- app/src/utils/api_serialize.lua | 23 +++++++++ docs/sqlite-features.md | 31 +++++++++--- 6 files changed, 224 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4999d9d..68c6fb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com/). This run took the PoC from a rough, non-booting prototype to a running, test-covered Reddit clone. Highlights, newest first: +### Exact cursors for the `new` sort +- **`new` no longer pages by window.** Its key, `(created_at, id)`, never moves + once a post is written, so the database seeks straight to the cursor row with a + row-value comparison and returns only the page — no depth cap, nothing scanned. + Walking backwards runs the comparison the other way and flips the rows, so + callers always see one order. +- A cursor id that no longer exists makes the subquery NULL, so the comparison is + NULL and no rows return — a stale cursor reads as "nothing after this". +- The **ranked** sorts keep the windowed cap, which is a property of the data: + `hot`/`controversial`/`rising` rank on live vote counts, so a cursor into them + is approximate however it is implemented. `Posts.KEYSET_SORTS` lists what + qualifies, and `get_listing` asserts if a cursor is passed with a sort that + does not — that failure would be silent and subtly wrong otherwise. + ### The JSON API stops reading the whole table for a page - **Listing endpoints fetched every matching row** and then ranked them in Lua, even after the web listings moved to SQL. They now pass `sort` to diff --git a/app/spec/api_pagination_spec.lua b/app/spec/api_pagination_spec.lua index 66e2e38..9f265d8 100644 --- a/app/spec/api_pagination_spec.lua +++ b/app/spec/api_pagination_spec.lua @@ -132,6 +132,91 @@ describe("api listing pagination", function() end) end) + describe("keyset cursors on the `new` sort", function() + -- `new` orders by (created_at, id), which never moves once a post is + -- written, so the database can seek straight to the cursor row instead of + -- scanning a windowed fetch. The ranked sorts cannot -- their key is live + -- vote counts. + local function ids_of(json) + local out = {} + for _, child in ipairs(json.data.children) do + out[#out + 1] = child.data.id + end + return out + end + + it("walks forward exactly, with no repeats or gaps", function() + local _, all = get("/api/listing/new?limit=" .. POSTS) + local expected = ids_of(all) + assert.same(POSTS, #expected) + + local walked, cursor = {}, nil + repeat + local path = "/api/listing/new?limit=4" .. (cursor and ("&after=" .. cursor) or "") + local _, json = get(path) + for _, id in ipairs(ids_of(json)) do + walked[#walked + 1] = id + end + cursor = json.data.after + until not cursor + assert.same(expected, walked) + end) + + it("walks backward to the page before a cursor", function() + local _, first = get("/api/listing/new?limit=4") + local _, second = get("/api/listing/new?limit=4&after=" .. first.data.after) + assert.is_truthy(second.data.before) + + local _, back = get("/api/listing/new?limit=4&before=" .. second.data.before) + assert.same(ids_of(first), ids_of(back)) + end) + + -- Capture the filters the endpoint hands the model, which is the only way + -- to tell the keyset path from the windowed one -- both are a single + -- query, so a query count cannot distinguish them. + local function filters_for(path) + local captured + local real = Posts.get_listing + Posts.get_listing = function(self, filters) -- luacheck: ignore 122 + captured = filters + return real(self, filters) + end + local ok, err = pcall(get, path) + Posts.get_listing = real -- luacheck: ignore 122 + assert.is_true(ok, tostring(err)) + return captured + end + + it("asks the database to seek, rather than fetching a window", function() + local _, first = get("/api/listing/new?limit=2") + + local cursored = filters_for("/api/listing/new?limit=2&after=" .. first.data.after) + assert.is_truthy(cursored.after_id, "should have passed a keyset cursor") + -- Just the page plus one lookahead row, not S.MAX_DEPTH. + assert.same(3, cursored.limit) + + -- A ranked sort has no stable key, so it still pages by window. + local ranked = filters_for("/api/listing/hot?limit=2&after=" .. first.data.after) + assert.is_nil(ranked.after_id) + assert.same(S.MAX_DEPTH, ranked.limit) + end) + + it("treats a vanished cursor row as the end of the listing", function() + local _, json = get("/api/listing/new?limit=4&after=" .. S.fullname("link", 999999)) + assert.same(0, #json.data.children) + assert.is_nil(json.data.after) + end) + + it("refuses a keyset cursor on a sort with no stable key", function() + -- Guard against a caller wiring after_id into a ranked sort, where the + -- key moves between requests and the result would silently be wrong. + local ok = pcall(function() + return Posts:get_listing({ sort = "hot", after_id = 1 }) + end) + assert.is_false(ok) + end) + end) + describe("GET /api/subreddits", function() it("bounds its window and still paginates", function() local status, json = get("/api/subreddits?limit=1") diff --git a/app/src/api.lua b/app/src/api.lua index 8e5fa1b..0e026c3 100644 --- a/app/src/api.lua +++ b/app/src/api.lua @@ -77,8 +77,36 @@ local function link_listing(self, filters) filters.exclude_hidden_for = user.id end filters.sort = sort - filters.limit = S.window(self.params) + -- `new` has a stable ordering key, so the database can seek straight to the + -- cursor row and return only the page -- no depth cap, nothing scanned. The + -- ranked sorts cannot: their key is live vote counts, so they page by + -- window (S.window) and `paginate` finds the cursor among those rows. + local cursor = self.params.after or self.params.before + local cursor_id + if cursor then + -- `x and f()` would truncate to one value, dropping the id. + local _ + _, cursor_id = S.parse_fullname(cursor) + end + if Posts.KEYSET_SORTS[sort] and cursor_id then + local limit = S.clamp_limit(self.params.limit) + filters.limit = limit + 1 + if self.params.after then + filters.after_id = cursor_id + else + filters.before_id = cursor_id + end + local rows = Posts:get_listing(filters) + local page, after, before = S.paginate_keyset(rows, limit, "link", true) + local children = {} + for _, p in ipairs(page) do + children[#children + 1] = S.link(p) + end + return { json = S.listing(children, { after = after, before = before }) } + end + + filters.limit = S.window(self.params) local rows = Posts:get_listing(filters) local page, after, before = S.paginate(rows, self.params, "link") local children = {} diff --git a/app/src/models/posts.lua b/app/src/models/posts.lua index 4c5a609..055f173 100644 --- a/app/src/models/posts.lua +++ b/app/src/models/posts.lua @@ -124,6 +124,15 @@ local ORDER_BY = { ELSE 0 END) DESC]], } +--- Sorts whose ordering key is stable enough to address with a cursor. +-- +-- Only `new`: its key is `(created_at, id)`, which never changes once a post is +-- written. Every other sort ranks on live vote counts, so a row's position moves +-- between requests and a cursor into it could not be exact however it was +-- implemented -- those page by window instead (see utils/api_serialize). +Posts.KEYSET_SORTS = { new = true } +local KEYSET_SORTS = Posts.KEYSET_SORTS + --- Listing rows for a frontpage / subreddit, with the vote and comment -- aggregates the templates expect. -- @@ -199,14 +208,45 @@ function Posts:get_listing(filters) ) end + -- Keyset cursors, for the sorts whose key is stable (see KEYSET_SORTS). + -- Comparing the whole key as a row value -- `(created_at, id) < (...)` -- + -- expresses "strictly past that row in this order" in one shot, including + -- the tiebreaker. A cursor id that no longer exists makes the subquery NULL, + -- so the comparison is NULL, so no rows come back: a stale cursor reads as + -- "nothing after this", which is what the API wants. + local reversed = false + if filters.after_id or filters.before_id then + assert( + KEYSET_SORTS[filters.sort or "new"], + "keyset cursors need a stable sort key; '" .. tostring(filters.sort) .. "' has none" + ) + if filters.after_id then + restrict( + "(a.created_at, a.id) < (SELECT created_at, id FROM posts WHERE id = ?)", + tonumber(filters.after_id) + ) + else + -- Walking backwards: take the rows *above* the cursor in ascending + -- order (so LIMIT keeps the ones nearest it) and flip them back. + restrict( + "(a.created_at, a.id) > (SELECT created_at, id FROM posts WHERE id = ?)", + tonumber(filters.before_id) + ) + reversed = true + end + end + -- `a.id DESC` breaks ties deterministically. Without it, equal-ranked rows -- could swap between requests and LIMIT/OFFSET paging would repeat or skip -- them -- the in-memory `table.sort` this replaces was likewise unstable. local order = ORDER_BY[filters.sort] or ORDER_BY.new + if reversed then + order = "a.created_at ASC" + end if filters.sticky_first then order = "a.stickied DESC, " .. order end - query = query .. " ORDER BY " .. order .. ", a.id DESC" + query = query .. " ORDER BY " .. order .. (reversed and ", a.id ASC" or ", a.id DESC") if filters.limit then query = query .. " LIMIT ? OFFSET ?" @@ -216,6 +256,13 @@ function Posts:get_listing(filters) local rows = db.select(query, unpack(params)) + if reversed then + -- Hand the caller the same descending order every other path returns. + for i = 1, math.floor(#rows / 2) do + rows[i], rows[#rows - i + 1] = rows[#rows - i + 1], rows[i] + end + end + for _, post in ipairs(rows) do post.permalink = "/r/" .. post.subreddit .. "/comments/" .. post.id -- Prefer the stored host; fall back to parsing for any pre-backfill row. diff --git a/app/src/utils/api_serialize.lua b/app/src/utils/api_serialize.lua index 4879e62..9d9cf2e 100644 --- a/app/src/utils/api_serialize.lua +++ b/app/src/utils/api_serialize.lua @@ -337,6 +337,29 @@ local clamp_limit = M.clamp_limit -- and a cursor into them is inherently approximate. M.MAX_DEPTH = 1000 +--- Build a page and its cursors from an already-windowed keyset result. +-- +-- The counterpart to `paginate` for callers that let the database do the +-- seeking: the rows are already exactly the page (plus one lookahead row), so +-- nothing is scanned to find a cursor. +-- @tparam table rows up to `limit + 1` rows, in display order +-- @tparam number limit the page size +-- @tparam string kind the rows' kind, for fullnames +-- @tparam boolean has_prev whether a cursor got us here (so a `before` exists) +-- @treturn table page rows +-- @treturn string|nil after fullname +-- @treturn string|nil before fullname +function M.paginate_keyset(rows, limit, kind, has_prev) + local more = #rows > limit + local page = {} + for i = 1, math.min(#rows, limit) do + page[i] = rows[i] + end + local after = (more and page[#page]) and M.fullname(kind, page[#page].id) or nil + local before = (has_prev and page[1]) and M.fullname(kind, page[1].id) or nil + return page, after, before +end + --- How many rows a listing endpoint should fetch for this request. -- -- Without a cursor -- the overwhelmingly common case, and every first page -- diff --git a/docs/sqlite-features.md b/docs/sqlite-features.md index 8343a9b..5b0dd45 100644 --- a/docs/sqlite-features.md +++ b/docs/sqlite-features.md @@ -158,12 +158,31 @@ endpoints size their window to the request (`S.window`): - **with a cursor**: `S.MAX_DEPTH` (1000) rows, which caps how deep a cursor can address. -Capping is honest for this data rather than a shortcut. True keyset pagination -would need `WHERE (rank, id) < (cursor_rank, cursor_id)`, and for `hot` / -`controversial` / `rising` the rank is computed from live vote counts — it moves -between requests, so a cursor into a ranked listing is inherently approximate no -matter how it is implemented. Search engines and Reddit itself cap deep paging -for the same reason. +**`new` skips the cap entirely.** Its key, `(created_at, id)`, never moves once a +post is written, so the database can seek straight to the cursor row with a +row-value comparison and return only the page: + +```sql +WHERE (a.created_at, a.id) < (SELECT created_at, id FROM posts WHERE id = ?) +``` + +Comparing the whole key as a row value expresses "strictly past that row in this +order" in one shot, tiebreaker included. A cursor id that no longer exists makes +the subquery NULL, so the comparison is NULL and no rows come back — a stale +cursor reads as "nothing after this", which is exactly the wanted answer. +Walking *backwards* runs the comparison the other way in ascending order and +flips the rows, so the caller always sees one order. + +The **ranked** sorts keep the window-and-cap treatment, and that is a property +of the data rather than a shortcut: `hot`, `controversial` and `rising` compute +rank from live vote counts, so a row's position moves between requests and a +cursor into one is approximate however it is implemented. Keyset would relocate +the inaccuracy, not remove it. Search engines and Reddit cap deep paging for the +same reason. + +`Posts.KEYSET_SORTS` is the list, and `get_listing` **asserts** when a cursor is +passed with a sort that is not on it — an unstable-key cursor would fail +silently and subtly otherwise. A cursor that is not in the window (past the cap, or a row since deleted) now returns an **empty page**. It used to silently restart at the top, which left a