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

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
 

settings.json.template

@@ -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
   },

src/node/db/DB.ts

@@ -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/SessionStore.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/package.json

@@ -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/tests/backend/specs/SessionStore.ts

@@ -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}`);
+        }
+      }
+    });
   });
 });