Skip to content

feat: add paged findKeys (memory-bounded iteration) - #981

Merged
JohnMcLear merged 5 commits into
mainfrom
fix/7830-paged-find-keys
May 22, 2026
Merged

feat: add paged findKeys (memory-bounded iteration)#981
JohnMcLear merged 5 commits into
mainfrom
fix/7830-paged-find-keys

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

Summary

  • Add Database#findKeysPaged(key, notKey, {limit, after}) returning up to limit keys sorted ascending; pages by passing the last returned key as after
  • Native implementations on mysql (BINARY \key` > ? ORDER BY BINARY `key` LIMIT ?) and **postgres** (key > $n ORDER BY key LIMIT $m`)
  • Other backends fall back to findKeys + JS slicing via CacheAndBufferLayer — correct, but defeats the OOM-mitigation benefit for very large keyspaces
  • Bumps to 6.1.0

Why

Drives the fix for ether/etherpad#7830, where SessionStore._cleanup() loaded every sessionstorage:* key into memory at once and OOMed on decade-old MariaDB installs with millions of stale sessions. The reporter's heap snapshot pinned retention on _pool → _allConnections → … → _command._rows → keys — the unbounded result set dragged the process out of heap.

Test plan

  • pnpm exec vitest run test/memory test/dirty — 200 passed (exercises the layer fallback)
  • pnpm exec vitest run test/mysql — 100 passed (native paging path, all 4 new findKeysPaged cases green)
  • pnpm exec vitest run test/postgres — 101 passed (native paging path)
  • pnpm run ts-check clean
  • pnpm run lint clean

🤖 Generated with Claude Code

Adds Database#findKeysPaged(key, notKey, {limit, after}) returning up to
`limit` keys sorted ascending. Callers page by passing the last returned
key as `after` on the next call; the final page returns fewer than
`limit` keys.

Implemented natively on mysql (BINARY ranged WHERE + LIMIT) and postgres
(ranged WHERE + LIMIT). Other backends fall back to findKeys + JS-side
slicing via CacheAndBufferLayer — correct for small/embedded datasets,
but defeats the OOM-mitigation purpose for very large keyspaces.

Drives the fix for ether/etherpad#7830, where SessionStore._cleanup()
loaded every sessionstorage:* key into memory at once and OOMed on
decade-old databases with millions of stale sessions.

Bumps minor to 6.1.0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Add paged findKeys API for memory-bounded iteration

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add memory-bounded findKeysPaged() API for paginated key iteration
• Native implementations on MySQL and PostgreSQL with cursor-based paging
• Fallback to in-memory slicing for backends without native support
• Resolves OOM issue in SessionStore cleanup with millions of stale keys
Diagram
flowchart LR
  A["Database#findKeysPaged<br/>key, notKey, limit, after"] --> B{Backend Type}
  B -->|MySQL| C["BINARY key > after<br/>ORDER BY BINARY key LIMIT"]
  B -->|PostgreSQL| D["key > after<br/>ORDER BY key LIMIT"]
  B -->|Other| E["findKeys + JS slice<br/>via CacheAndBufferLayer"]
  C --> F["Paginated Results"]
  D --> F
  E --> F

Loading

File Changes

1. databases/mysql_db.ts ✨ Enhancement +26/-0

Add MySQL native paged key iteration

• Implements native findKeysPaged() with BINARY-safe cursor-based pagination
• Uses BINARY key > ? comparison for deterministic paging across case-insensitive collations
• Validates limit parameter and constructs parameterized SQL query
• Returns sorted keys up to the specified limit

databases/mysql_db.ts


2. databases/postgres_db.ts ✨ Enhancement +31/-0

Add PostgreSQL native paged key iteration

• Implements callback-based findKeysPaged() for PostgreSQL
• Uses parameterized queries with dynamic parameter numbering
• Supports cursor-based pagination via key > $n comparison
• Maintains consistency with existing callback pattern

databases/postgres_db.ts


3. index.ts ✨ Enhancement +14/-0

Expose findKeysPaged in public API

• Exposes findKeysPaged() method on public Database class
• Includes JSDoc documentation explaining pagination semantics
• Delegates to underlying database implementation

index.ts


View more (4)
4. lib/AbstractDatabase.ts 📝 Documentation +6/-0

Document findKeysPaged override expectations

• Adds documentation comment explaining backend override expectations
• Notes that backends with large keyspaces should implement native findKeysPaged()
• Clarifies that CacheAndBufferLayer fallback defeats OOM-mitigation purpose

lib/AbstractDatabase.ts


5. lib/CacheAndBufferLayer.ts ✨ Enhancement +43/-1

Add CacheAndBufferLayer paging support

• Adds findKeysPaged() to InternalDB type definition
• Updates promisification loop to include findKeysPaged for legacy callback-based backends
• Implements fallback findKeysPaged() using binary search to find cursor position
• Falls back to in-memory findKeys() + slicing when backend lacks native implementation

lib/CacheAndBufferLayer.ts


6. test/lib/test_lib.ts 🧪 Tests +68/-0

Add findKeysPaged test coverage

• Adds comprehensive test suite for findKeysPaged() with 5 test cases
• Tests empty results, single page, limit enforcement, cursor-based pagination
• Tests notKey exclusion across multiple pages
• Uses isolated prefixes to prevent test interference

test/lib/test_lib.ts


7. package.json ⚙️ Configuration changes +1/-1

Bump version to 6.1.0

• Bumps version from 6.0.4 to 6.1.0 (minor version increment)

package.json


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented May 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Missing limit validation ✓ Resolved 🐞 Bug ≡ Correctness
Description
CacheAndBufferLayer.findKeysPaged() does not validate options.limit before slicing or
delegating, so invalid limits (0/negative/NaN/floats) can silently return empty pages and behave
differently than MySQL/Postgres which reject them. This can break typical paging loops (e.g., `while
(page.length === limit)`) and lead to infinite loops or silent data loss.
Code

lib/CacheAndBufferLayer.ts[R341-365]

Evidence
The wrapper implements a JS-slicing fallback that uses options.limit without validation, while
both native implementations explicitly reject invalid limits. This creates observable behavior
differences across backends and makes invalid inputs silently succeed in the fallback path.

lib/CacheAndBufferLayer.ts[341-365]
databases/mysql_db.ts[158-165]
databases/postgres_db.ts[154-162]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`CacheAndBufferLayer.findKeysPaged()` lacks input validation for `options.limit`. In the fallback path it uses `options.limit` directly in `slice()`, and in the non-fallback path it delegates to the backend without guaranteeing consistent validation.
MySQL/Postgres implementations explicitly reject non-positive/non-integer limits, so callers get inconsistent behavior depending on backend and/or whether the fallback is used.
## Issue Context
This API is intended for deterministic, safe iteration over large keyspaces. Returning an empty page for an invalid limit (instead of throwing) can break pagination loops and can cause silent failures.
## Fix Focus Areas
- lib/CacheAndBufferLayer.ts[341-366]
## Suggested fix
Add the same guard used by MySQL/Postgres at the start of `CacheAndBufferLayer.findKeysPaged()` (before doing any work), e.g.:
- if `!options || !Number.isInteger(options.limit) || options.limit <= 0`, throw `new Error('findKeysPaged requires a positive integer limit')`.
Optionally also add the same validation in `index.ts` `Database.findKeysPaged()` to enforce the contract at the public API boundary.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

The new tests use `prefix:*` patterns; existing surrealdb findKeys tests
carefully avoid `:` because that backend treats it differently in keys.
The pre-existing `findKeys works` tests already work around this — match
their skip pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread lib/CacheAndBufferLayer.ts
The native mysql/postgres paths throw on non-positive/non-integer
limits, but the JS-slicing fallback would silently return an empty
slice — which hangs the usual `while (page.length === limit)` paging
loop and masks misuse. Validate at the wrapper boundary so behaviour
is uniform across backends.

Addresses Qodo review on #981.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JohnMcLear

Copy link
Copy Markdown
Member Author

Addressed the missing-limit-validation bug in 6e80e77 — now throws on non-positive/non-integer limits, matching the mysql/postgres native paths so behaviour is uniform. Backend tests (memory + dirty + mysql, 300 cases) still green locally.

JohnMcLear and others added 2 commits May 22, 2026 10:14
Adds a usage example and the API contract (ascending byte-order,
exclusive `after` cursor, empty-page termination, positive-integer
limit). Updates the feature-support matrix with a findKeysPaged column
flagging native (mysql/maria/postgres) vs fallback (everyone else).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the per-row native/fallback labels in the feature matrix — every
backend supports findKeysPaged. Move the SQL-only memory-bounded caveat
into the Limitations section where the other findKeys notes live.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JohnMcLear
JohnMcLear merged commit e4ee980 into main May 22, 2026
17 checks passed
@JohnMcLear
JohnMcLear deleted the fix/7830-paged-find-keys branch May 22, 2026 09:24
JohnMcLear added a commit that referenced this pull request May 22, 2026
The npmpublish.yml workflow was authenticating via a stored NPM_TOKEN
secret that recently started failing E404 on PUT — first observed after
#981 merged at 6.1.1 (run 26279691349). Same E404 pattern that hit
ep_hljs four days ago.

Stored publish tokens have two failure modes that this PR removes
entirely:
  1. They expire / get rotated and silently break automated publishes.
  2. They survive in compromised CI logs / forks long enough to be
     abused.

OIDC trusted publishing exchanges a short-lived GitHub-issued id-token
for a per-publish credential at the registry. No secret on our side,
no expiry, and the resulting publish is attested with `--provenance`
so the npm package page shows the signing GHA run.

Removes:
  - "Set publishing config" step that consumed secrets.NPM_TOKEN
  - "Add package to etherpad organization" step (the access grant is
    idempotent and was set at initial publish; it doesn't need to run
    on every release and the OIDC credential isn't authorised for it)

Adds:
  - `permissions: { contents: write, id-token: write }` on the publish
    job (contents:write was already implicit via GITHUB_TOKEN for the
    version-bump push; id-token:write is the OIDC enabler)
  - `registry-url` on setup-node so the auth header lands at npmjs.org
  - `--no-git-checks` on pnpm publish (skip the dirty-tree guard that
    would otherwise trip on the just-pushed version-bump commit)
  - `--provenance` for the signing attestation

One-time setup required on https://www.npmjs.com/package/ueberdb2/access
before this lands — Trusted Publishers → Add → Provider: GitHub Actions,
Org: ether, Repo: ueberDB, Workflow: npmpublish.yml, Environment: blank.
Documented at the top of the publish-npm job.

Bumps the publish-job pnpm pin from 10 to 11 to match the test job;
trusted publishing requires pnpm >= 10.4 either way.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant