Skip to content

fix(pagination): offset slice math returns wrong rows; add keyset cursors - #10

Open
frankstupak wants to merge 1 commit into
SkinnnyJay:mainfrom
frankstupak:lumen-uplift/pagination
Open

fix(pagination): offset slice math returns wrong rows; add keyset cursors#10
frankstupak wants to merge 1 commit into
SkinnnyJay:mainfrom
frankstupak:lumen-uplift/pagination

Conversation

@frankstupak

Copy link
Copy Markdown
Contributor

What was wrong

1. No cursor pagination at all. The README correctly warns that offset result sets "can shift during pagination" — and then ships no fix. Insert one row at the head of the list between two requests and offset page 2 re-serves the last row of page 1; delete three rows and it silently skips rows 6–8. Both failure modes are now demonstrated in tests against the shipped implementation.

2. Silent wrong-rows math. page/offset/limit flowed raw into Array.slice. page=1.5, limit=2slice(1, 3) → returns rows 2 and 3 with a 200 and no error. limit=NaNtotalPages: NaN and an empty page, also a 200.

3. Lax query parsing. parseInt accepted page=5abc as 5 and truncated page=1.5 to 1 — client typos got wrong data instead of a 400.

4. Untestable server. startServer() ran at module load, so importing createApp from a test bound port 3001. There were zero HTTP-level tests.

5. /employees/stats rescanned the full dataset once per department (O(d×n)).

What's better

Cursor-based (keyset) pagination — the Stripe/Slack/GitHub pattern:

  • Opaque base64url cursor encoding the (sortBy value, id) anchor tuple. The id tiebreaker makes non-unique sort fields safe: sweeping 101 rows sorted by a 3-value department field returns every row exactly once (tested).
  • Immune to insert/delete drift — the same mutations that make offset duplicate/skip rows leave cursor sequences intact (tested side-by-side). Works even when the anchor row itself was deleted.
  • Forward and backward via nextCursor/prevCursor; nextCursor: null terminates.
  • Cursors are scoped: replaying one against a different sort, direction, or department filter throws InvalidCursorError → clean 400, never silently wrong rows.
  • Binary-search seek on the sorted view instead of the linear findIndex a first-pass implementation uses.

Benchmarks (npm run bench, 1,000,000 rows, 100/page, committed at bench/cursor-bench.ts):

scenario linear seek binary seek speedup
full sweep, 10,000 pages 17.86s (1786µs/page) 111.8ms (11.2µs/page) 159.8x
single page @ 90% depth 3002µs 7.0µs 428x

Fixes:

  • All numeric pagination inputs integer-normalized (src/normalize.ts); fractional/NaN/Infinity can no longer reach slice math. Regression tests pin the old wrong-rows behavior.
  • Strict integer query validation: page=5abc, page=1.5, offset=2.5, limit=10x → 400.
  • New GET /employees/cursor-based + type=cursor_based on the unified /employees/paginate.
  • require.main guard → new HTTP inject() suite (server.test.ts) covering cursor sweeps, scope rejection, and the strict-parsing regressions.
  • /employees/stats single pass.
  • fastify + @fastify/swagger added to the subproject's own deps so a standalone npm install && tsc works.

Compatibility: everything additive. Existing exports, endpoints, and response shapes unchanged; all 36 original tests pass untouched.

Numbers

  • Subproject: 81/81 tests (+45 new), suite ~6s
  • Root npm run test:all: 784 passed, 0 failed
  • tsc --noEmit clean, eslint clean

— Lumen Industries

@frankstupak frankstupak changed the title pagination: cursor (keyset) pagination + the wrong-rows bug your slice math was hiding fix(pagination): offset slice math returns wrong rows; add keyset cursors Aug 13, 2026
@SkinnnyJay SkinnnyJay closed this Aug 21, 2026
@SkinnnyJay SkinnnyJay reopened this Aug 21, 2026
@SkinnnyJay

Copy link
Copy Markdown
Owner

CI has now run on this branch for the first time — the earlier runs sat unapproved (fork PR, first-time-contributor policy) and expired, so nothing was ever reported here.

Result: failure on Node 20, in this PR's own README. Node 18 and 22 were cancelled once 20 failed.

src/api/pagination/README.md:64:67 error MD060/table-column-style
  Table column style [Table pipe does not align with header for style "aligned"]

Line 64 is the row you added:

| `/employees/cursor-based` | GET    | Cursor (keyset) pagination |

Cursor (keyset) pagination is 26 characters and the Description column was sized to 23, so the closing pipe no longer lines up with the header. markdownlint-cli2 v0.21 enforces this via MD060; the rule did not exist when this README was last touched, which is why it reads as a new failure on old-looking content.

npx prettier --write src/api/pagination/README.md fixes it — prettier re-sizes the whole table to the widest cell, and the result passes markdownlint with 0 errors. I verified that locally on this branch.

Worth knowing before you re-push: main is currently red on Node 18 for an unrelated reason, and #14 fixes that. Until it lands, the other two legs here will still come back red.

@SkinnnyJay

Copy link
Copy Markdown
Owner

Status update. main is green again and the other ten PRs in this batch have landed — #1, #2, #4, #5, #6, #7, #8, #9, #11, #12, all with CI passing on Node 20 and 22.

Two things had been masking the real verdicts:

  1. fix(ci): drop Node 18 from the matrix #14 dropped Node 18 from the matrix. Its websocket teardown failure was hitting every PR in the batch, including ones that touched nothing but src/algorithms.
  2. The merge refs were stale — closing and reopening a PR does not recompute refs/pull/N/merge against a moved base, so the first re-runs still executed the old three-version matrix. update-branch on each PR fixed that.

This one is now the only thing standing between the branch and a merge — the failure is in your own change, detailed in the comment above. Once it is fixed, rebase or use "Update branch" so CI runs against current main, and it should go green.

Also filed #15 for a flake you may hit on a re-run: cache.test.ts › should handle TTL correctly fails intermittently on Node 22 regardless of branch. If you see that one, it is not yours.

…strict query validation

- NEW cursor-based strategy (Stripe/Slack/GitHub pattern): opaque base64url
  cursor encoding the (sortBy value, id) anchor tuple; id tiebreaker makes
  non-unique sort fields safe; forward/backward via nextCursor/prevCursor;
  cursors scoped to sort+filter and rejected with InvalidCursorError when
  replayed against a different scope; anchor deletion tolerated. Binary-search
  seek: 1M-row full sweep 112ms vs 17.9s naive linear seek (~160x), single
  page at 90% depth ~430x (bench/cursor-bench.ts, npm run bench).
- Fixed silent wrong-rows bug: fractional page/offset/limit flowed raw into
  Array.slice (page=1.5, limit=2 returned rows 2-3); NaN limit produced NaN
  totalPages + empty page with a 200. All numeric inputs now integer-normalized
  (src/normalize.ts).
- Fixed lax server parsing: parseInt accepted "5abc" as 5 and truncated
  "1.5" to 1; numeric query params now strictly validated (400 on garbage).
- /employees/cursor-based endpoint + cursor_based support in /employees/paginate.
- require.main guard on startServer: importing the module no longer binds :3001,
  enabling a new HTTP inject() suite (server.test.ts).
- /employees/stats single-pass counting (was O(departments x employees)).
- fastify + @fastify/swagger added to subproject deps (standalone install).
- +45 tests (81 total), incl. offset dup/skip failure demos vs cursor immunity.
@frankstupak
frankstupak force-pushed the lumen-uplift/pagination branch from ba4d42f to b85f6fe Compare August 30, 2026 12:55
@RealLumenHere

Copy link
Copy Markdown
Contributor

Rebased onto main, both checks green now. The failure was a markdown-lint table-alignment error in src/api/pagination/README.md (a description cell wider than its column, unrelated to the pagination logic) — reformatted the table, no content change. Cursor/keyset pagination fix itself is untouched.

Ready for review whenever you get a chance.

@SkinnnyJay

Copy link
Copy Markdown
Owner

Sorry for the delay — you cleared this on Aug 30 and it sat. Confirmed: test (20) and test (22) both green, mergeable: CLEAN. The markdown-lint table fix is fine and I agree it's unrelated to the pagination logic.

Before I review the substance, one scope question. This PR is +25,331 / -42 across 15 files, and ~95% of that isn't pagination:

status additions file
added 11,560 src/api/nextjs-backend/package-lock.json
added 6,494 src/api/api-scenarios/package-lock.json
added 5,935 src/api/autocomplete/package-lock.json
added 28 src/api/pagination/package-lock.json

That's 23,989 lines of brand-new lockfiles for three sub-packages this PR doesn't otherwise touch — none of them exist on main today. The pagination work itself is about 1,300 lines (cursor.ts, cursor-pagination.test.ts, server.ts, normalize.ts, the bench), which is a perfectly reasonable PR and the thing I actually want to read.

Was that deliberate, or did a npm install at the repo root walk into the sibling packages? If it's incidental, please drop the three unrelated ones — keeping src/api/pagination/package-lock.json is fine since that's your package.

Two reasons beyond review ergonomics:

  1. src/api/nextjs-backend/package-lock.json overlaps fix(nextjs-backend): patch secret leak and invalid CORS header; harden users API #13, which is the nextjs-backend PR. fix(nextjs-backend): patch secret leak and invalid CORS header; harden users API #13 adds no lockfile right now, so both are CLEAN against main — but landing a lockfile for that package from here means whichever merges second is resolving against a dependency tree the other PR introduced, and neither author intended that.
  2. Committing first-ever lockfiles for three packages is a real decision about how this repo pins dependencies, and it should be its own PR with that as the title, not a rider on a pagination fix. Related: fix(caching): O(n^2) eviction, sub-second TTL crash, LFU frequency reset #3 is currently blocked on a bad transitive pin (@types/ioredis-mock@8.2.6) in the root lockfile, so lockfile policy here is live and worth deciding on purpose.

Strip those three and I'll review the cursor/keyset work properly — that part I'm interested in, and the offset slice math bug is worth having fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants