fix: page sessionstorage cleanup to avoid OOM (#7830) (#7831)

* fix: page sessionstorage cleanup to avoid OOM (#7830)

SessionStore._cleanup() previously called `findKeys('sessionstorage:*',
null)`, materialising every session key into a single array. On decade-
old MariaDB installs with millions of sessions this OOMs the node
process within ~15 minutes — see #7830.

Switch to ueberdb2 6.1.0's findKeysPaged with a 500-key page size, and
yield to the event loop between pages so the DB driver can release each
page's buffered rows and request handlers can interleave.

The break is now driven by `page.length === 0` rather than `page.length
< CLEANUP_PAGE_SIZE` so a stubbed/throttled paged source still iterates
the full keyspace.

Adds a regression test that seeds 50 sessionstorage rows, monkey-patches
`DB.findKeysPaged` to use a 4-key page, runs cleanup, and asserts every
expired row is removed plus every valid row preserved across page
boundaries.

Closes #7830

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

* fix: address Qodo review on #7831

Four follow-ups raised by Qodo on the session cleanup paging fix:

- DB.ts: fail-fast at init() if any required wrapper method (incl.
  findKeysPaged) is missing, so a stale ueberdb2 pin surfaces at boot
  rather than crashing the first cleanup run an hour later.
- SessionStore: bound a single _cleanup() run to 10 minutes. Under
  sustained session creation the keyspace can grow faster than cleanup
  drains it; without a budget the next scheduled run would never fire.
  When the budget hits, log a warning and let the next run continue.
- SessionStore: log the defensive `page[0] <= after` cursor-stall break.
  Previously the loop exited silently, leaving expired rows behind with
  no operator-visible signal of the backend regression.
- Tests: the paged-cleanup regression test now removes both expiredSids
  AND validSids in finally, so a failed assertion doesn't leak rows.

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

* docs: note paged session cleanup in CHANGELOG + settings template

CHANGELOG.md picks up an entry under 3.1.0 Notable fixes describing the
OOM cause, the paged iteration, the 10-minute per-run budget, the
cursor-stall logging, and the fail-fast init guard.

settings.json.template's sessionCleanup comment adds the page-size,
budget, and pointer to #7830 so admins can reason about the new
behaviour from the template alone.

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

* chore: regenerate lockfile against ueberdb2 6.1.2

Now that ether/ueberDB#983 unblocked the publish workflow (OIDC trusted
publishing), ueberdb2 6.1.2 is live on npm and the `^6.1.0` pin in
src/package.json resolves cleanly. Resolves the ERR_PNPM_OUTDATED_LOCKFILE
that was blocking CI on this PR.

29 SessionStore backend tests still green against the published tarball.

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

---------

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

Author: JohnMcLear

CHANGELOG.md

@@ -17,6 +17,7 @@
 
 - **Export HTML — ordered-list counter no longer poisoned by a sibling unordered list.** When an ordered-list level was the only consumer of `olItemCounts`, closing *any* list at that depth (including a `<ul>` that happened to share the level) reset the counter to 0. A subsequent unrelated `<ol>` at the same depth then took the "counter exists but is 0" branch and emitted `<ol class="...">` without the `start=` attribute. The reset is now gated on `line.listTypeName === 'number'` so closing an unordered list never touches the ol bookkeeping. Fixes #7786 / #7787 (#7791).
 - **Export — bad `:rev` returns a meaningful 500 body, not Express's HTML error page.** A non-numeric `:rev` (e.g. `/p/foo/test1/export/txt`) reached `checkValidRev` which throws `CustomError('rev is not a number', 'apierror')`; the message fell through `.catch(next)` and Express's default renderer returned an HTML 500 page. The route handler now catches the apierror and emits `err.message` as a deterministic `text/plain` 500. As a follow-up, `checkValidRev` runs *before* `res.attachment()` so an invalid rev no longer leaves a `Content-Disposition` header in place (browsers were offering to save the error message as a file), and unrelated export failures (conversion, fs, soffice) are surfaced as text/plain rather than the HTML stack page. Fixes #7788 (#7792).
+- **Session cleanup no longer OOMs on huge sessionstorage tables.** Pre-2.7.3 `SessionStore._cleanup()` issued a single unbounded `findKeys('sessionstorage:*', null)` that materialised every key into one JS array; on a decade-old MariaDB install with millions of stale sessions the mysql2 driver retained the rows on the pool connection while the JS array dominated heap, OOMing the process within ~15 minutes of boot. Cleanup now pages the keyspace in 500-key batches via the new `findKeysPaged` API on ueberdb2 6.1.0 (DB-side ranged query on mysql/postgres, JS-side fallback elsewhere), yielding to the event loop between pages. A single run is capped at 10 minutes; the next scheduled run continues. The defensive cursor-stall guard now logs an error rather than silently aborting, and `DB.init()` fails fast if any required wrapper method is missing (a misconfigured ueberdb2 pin surfaces at boot instead of an hour later). Fixes #7830 (#7831).
 
 ### Security hardening
 

pnpm-lock.yaml

@@ -634,6 +634,12 @@
     /*
      * Whether to periodically clean up expired and stale sessions from the
      * database. Set to false to disable. Default: true.
+     *
+     * Cleanup runs hourly and walks the sessionstorage:* keyspace in 500-key
+     * pages so very large session tables don't pin the keys in memory all at
+     * once (see https://github.com/ether/etherpad/issues/7830). A single run
+     * is capped at 10 minutes; if your DB is so large the budget is reached,
+     * a warning is logged and the next scheduled run continues.
      */
     "sessionCleanup": true
   },

settings.json.template

@@ -47,8 +47,13 @@ exports.init = async () => {
       stats.gauge(`ueberdb_${metric}`, () => exports.db.metrics[metric]);
     }
   }
-  for (const fn of ['get', 'set', 'findKeys', 'getSub', 'setSub', 'remove']) {
+  for (const fn of ['get', 'set', 'findKeys', 'findKeysPaged', 'getSub', 'setSub', 'remove']) {
     const f = exports.db[fn];
+    if (typeof f !== 'function') {
+      throw new Error(
+        `ueberdb2 ${exports.db.constructor.name} is missing required method ${fn}; ` +
+          'check that ueberdb2 is at the minimum version pinned in package.json');
+    }
     exports[fn] = async (...args:string[]) => await f.call(exports.db, ...args);
     Object.setPrototypeOf(exports[fn], Object.getPrototypeOf(f));
     Object.defineProperties(exports[fn], Object.getOwnPropertyDescriptors(f));

src/node/db/DB.ts

@@ -12,6 +12,19 @@ const logger = log4js.getLogger('SessionStore');
 // How often to run the cleanup of expired/stale sessions.
 const CLEANUP_INTERVAL_MS = 60 * 60 * 1000; // 1 hour
 
+// Maximum number of session keys fetched from the database per cleanup
+// iteration. Bounded so that instances with very large session keyspaces (see
+// https://github.com/ether/etherpad/issues/7830) don't load every key into
+// memory at once. Tuned for ~50 KB per page assuming ~100-char keys.
+const CLEANUP_PAGE_SIZE = 500;
+
+// Upper bound on a single cleanup run. Under sustained session creation the
+// keyspace can grow faster than cleanup processes it; without a budget the
+// loop would never reach an empty page and the next scheduled run would never
+// fire. When the budget hits, the next scheduled run picks up where this one
+// left off (the database state advances each iteration regardless).
+const CLEANUP_MAX_RUNTIME_MS = 10 * 60 * 1000; // 10 minutes
+
 class SessionStore extends expressSession.Store {
   /**
    * @param {?number} [refresh] - How often (in milliseconds) `touch()` will update a session's
@@ -74,37 +87,71 @@ class SessionStore extends expressSession.Store {
    * - Sessions with no expiry that contain no data beyond the default cookie are removed.
    *   These are the empty sessions that accumulate indefinitely (bug #5010) — they have
    *   `{cookie: {path: "/", _expires: null, ...}}` and nothing else.
+   *
+   * Iterates the keyspace in fixed-size pages (CLEANUP_PAGE_SIZE) so a large
+   * sessionstorage table (#7830) doesn't load every key into memory at once.
    */
   async _cleanup() {
-    const keys = await DB.findKeys('sessionstorage:*', null);
-    if (!keys || keys.length === 0) return;
     const now = Date.now();
+    const startMs = Date.now();
     let removed = 0;
-    for (const key of keys) {
-      const sess = await DB.get(key);
-      if (!sess) {
-        await DB.remove(key);
-        removed++;
-        continue;
+    let scanned = 0;
+    let after: string | undefined;
+    let budgetExhausted = false;
+    // eslint-disable-next-line no-constant-condition
+    while (true) {
+      const page = await DB.findKeysPaged('sessionstorage:*', null, {
+        limit: CLEANUP_PAGE_SIZE,
+        ...(after != null ? {after} : {}),
+      });
+      if (!page || page.length === 0) break;
+      // Defensive: a buggy backend that returns the cursor key would loop
+      // forever. `after` is exclusive, so the first key of the next page must
+      // be strictly greater than the previous cursor. Log so an operator can
+      // notice partial cleanup caused by a pagination regression.
+      if (after != null && page[0] <= after) {
+        logger.error(
+          `Session cleanup: paged cursor did not advance (after=${after}, ` +
+          `page[0]=${page[0]}); aborting this run to prevent an infinite loop`);
+        break;
       }
-      const expires = sess.cookie?.expires;
-      if (expires) {
-        // Session has an expiry — remove if expired.
-        if (new Date(expires).getTime() <= now) {
+      for (const key of page) {
+        scanned++;
+        const sess = await DB.get(key);
+        if (!sess) {
           await DB.remove(key);
           removed++;
+          continue;
         }
-      } else {
-        // Session has no expiry and no user data beyond the cookie — remove as empty/stale.
-        const hasData = Object.keys(sess).some((k) => k !== 'cookie');
-        if (!hasData) {
-          await DB.remove(key);
-          removed++;
+        const expires = sess.cookie?.expires;
+        if (expires) {
+          if (new Date(expires).getTime() <= now) {
+            await DB.remove(key);
+            removed++;
+          }
+        } else {
+          const hasData = Object.keys(sess).some((k) => k !== 'cookie');
+          if (!hasData) {
+            await DB.remove(key);
+            removed++;
+          }
         }
       }
+      after = page[page.length - 1];
+      if (Date.now() - startMs > CLEANUP_MAX_RUNTIME_MS) {
+        budgetExhausted = true;
+        break;
+      }
+      // Yield to the event loop between pages so request handlers can run and
+      // the DB driver can release the previous page's buffered rows.
+      await new Promise((resolve) => setImmediate(resolve));
     }
-    if (removed > 0) {
-      logger.info(`Session cleanup: removed ${removed} expired/stale sessions out of ${keys.length}`);
+    if (budgetExhausted) {
+      logger.warn(
+        `Session cleanup: hit ${CLEANUP_MAX_RUNTIME_MS}ms budget after scanning ` +
+        `${scanned} keys (${removed} removed); next scheduled run will continue`);
+    } else if (removed > 0) {
+      logger.info(`Session cleanup: removed ${removed} expired/stale sessions out of ${scanned}`);
     }
   }
 

src/node/db/SessionStore.ts

@@ -87,7 +87,7 @@
     "surrealdb": "^2.0.3",
     "tinycon": "0.6.8",
     "tsx": "4.22.3",
-    "ueberdb2": "^6.0.3",
+    "ueberdb2": "^6.1.0",
     "underscore": "1.13.8",
     "undici": "^8.3.0",
     "unorm": "1.6.0",

src/package.json

@@ -309,5 +309,62 @@ describe(__filename, function () {
       // After shutdown, the timer should be cleared.
       assert(ss!._cleanupTimer == null);
     });
+
+    // Regression for https://github.com/ether/etherpad/issues/7830 — cleanup
+    // used to load every sessionstorage key into a single array; on huge DBs
+    // this OOMed. Verifies the paged iteration still hits every key when the
+    // count exceeds CLEANUP_PAGE_SIZE — we seed a few-row spread and force a
+    // small page size to keep the test fast.
+    it('pages across a large sessionstorage keyspace', async function () {
+      // Tag rows so the assertion ignores anything other tests left behind.
+      const tag = common.randomString();
+      const expiredSids: string[] = [];
+      const validSids: string[] = [];
+      // Seed 25 expired + 25 valid rows. The default CLEANUP_PAGE_SIZE (500)
+      // would cover this in one call, so we monkey-patch the constant for
+      // this test by stubbing DB.findKeysPaged to enforce a small page.
+      const real = db.findKeysPaged;
+      let pageCalls = 0;
+      db.findKeysPaged = async (key: string, notKey: any, opts: any) => {
+        pageCalls++;
+        return await real.call(db, key, notKey, {...opts, limit: 4});
+      };
+      try {
+        for (let i = 0; i < 25; i++) {
+          const sid = `cleanup_paged_exp_${tag}_${String(i).padStart(2, '0')}`;
+          expiredSids.push(sid);
+          await db.set(`sessionstorage:${sid}`, {
+            cookie: {path: '/', expires: new Date(1).toJSON(), httpOnly: true},
+          });
+        }
+        for (let i = 0; i < 25; i++) {
+          const sid = `cleanup_paged_val_${tag}_${String(i).padStart(2, '0')}`;
+          validSids.push(sid);
+          await db.set(`sessionstorage:${sid}`, {
+            cookie: {
+              path: '/', expires: new Date(Date.now() + 60000).toJSON(), httpOnly: true,
+            },
+          });
+        }
+        await ss!._cleanup();
+        for (const sid of expiredSids) {
+          assert(await db.get(`sessionstorage:${sid}`) == null, `expired ${sid} not removed`);
+        }
+        for (const sid of validSids) {
+          assert(await db.get(`sessionstorage:${sid}`) != null, `valid ${sid} was wrongly removed`);
+        }
+        // page size 4 over 50 rows -> at least 12 paged calls (final page may
+        // be short). Confirms we actually iterated.
+        assert(pageCalls >= 12, `expected paged iteration (got ${pageCalls} calls)`);
+      } finally {
+        db.findKeysPaged = real;
+        // Symmetric cleanup — if an assertion threw earlier, expiredSids may
+        // still be present in the DB. Remove both groups so the test leaves
+        // no rows behind even on failure.
+        for (const sid of [...expiredSids, ...validSids]) {
+          await db.remove(`sessionstorage:${sid}`);
+        }
+      }
+    });
   });
 });