fix(admin): don't let one unreadable pad empty the Manage-pads list (#7935)

src/node/hooks/express/adminsettings.ts

@@ -11,6 +11,7 @@ import settings, {getEpVersion, getGitCommit, reloadSettings} from '../../utils/
 import {getLatestVersion} from '../../utils/UpdateCheck';
 import {redactSettings} from '../../utils/AdminSettingsRedact';
 const padManager = require('../../db/PadManager');
+const db = require('../../db/DB');
 const api = require('../../db/API');
 import {deleteRevisions} from '../../utils/Cleanup';
 
@@ -26,6 +27,24 @@ const queryPadLimit = 12;
 const PAD_HYDRATE_CONCURRENCY = 16;
 const logger = log4js.getLogger('adminSettings');
 
+// Errors thrown while reading a pad record can embed the raw stored value
+// in their message — e.g. Pad.init's `'pool' in value` TypeError stringifies
+// the offending value ("Cannot use 'in' operator to search for 'pool' in
+// <value>"). For a corrupt record that value may be actual pad text, so
+// logging it verbatim would leak content, bloat the log, and let embedded
+// newlines forge log lines. Reduce any error to its name plus a single-line,
+// length-capped message before logging.
+const safeErr = (err: unknown): string => {
+  const e = err as {name?: unknown, message?: unknown} | null;
+  const name = (e && typeof e.name === 'string' && e.name) || 'Error';
+  const msg = String((e && e.message) ?? err ?? '')
+      .replace(/[\r\n\t]+/g, ' ')
+      .replace(/\s{2,}/g, ' ')
+      .trim()
+      .slice(0, 120);
+  return `${name}: ${msg}`;
+};
+
 // Concurrency-limited Promise.all replacement. Preserves the input
 // order in the returned array (caller slices later). Used by padLoad
 // to bound DB reads during hydration.
@@ -147,6 +166,7 @@ exports.socketio = (hookName: string, {io}: any) => {
 
 
     socket.on('padLoad', async (query: PadSearchQuery) => {
+     try {
       const {padIDs} = await padManager.listAllPads();
 
       // ── 1. Pattern filter (cheap, by name only) ─────────────────────
@@ -172,13 +192,27 @@ exports.socketio = (hookName: string, {io}: any) => {
       const needsFullScan = filter !== 'all' || query.sortBy !== 'padName';
 
       const loadMeta = async (padName: string): Promise<PadQueryResult> => {
-        const pad = await padManager.getPad(padName);
-        return {
-          padName,
-          lastEdited: await pad.getLastEdit(),
-          userCount: api.padUsersCount(padName).padUsersCount,
-          revisionNumber: pad.getHeadRevisionNumber(),
-        };
+        // A single unreadable record must not take out the whole listing.
+        // `findKeys('pad:*', '*:*:*')` returns every key under the `pad:`
+        // prefix, including legacy/foreign or migration-corrupted records
+        // (e.g. a value stored as a JSON *string* rather than a pad object,
+        // which makes Pad.init throw `'pool' in value`). Before this guard
+        // one such key rejected the whole `padLoad` handler — the admin
+        // "Manage pads" page then showed *no* pads at all (issue #7935) and
+        // the unhandled rejection could exit the server. Surfacing the bad
+        // pad with zeroed metadata lets an admin see and delete it instead.
+        try {
+          const pad = await padManager.getPad(padName);
+          return {
+            padName,
+            lastEdited: await pad.getLastEdit(),
+            userCount: api.padUsersCount(padName).padUsersCount,
+            revisionNumber: pad.getHeadRevisionNumber(),
+          };
+        } catch (err) {
+          logger.warn(`padLoad: skipping unreadable pad "${padName}": ${safeErr(err)}`);
+          return {padName, lastEdited: 0 as any, userCount: 0, revisionNumber: 0};
+        }
       };
 
       // Lazily lifted so we don't load every pad twice on the fast path.
@@ -256,16 +290,51 @@ exports.socketio = (hookName: string, {io}: any) => {
 
       const data: {total: number, results?: PadQueryResult[]} = {total, results};
       socket.emit('results:padLoad', data);
+     } catch (err) {
+      // Never leave the SPA hanging on a missing reply (it would show an
+      // empty "No results" state forever) and never let this bubble up to
+      // the process-level unhandledRejection handler, which would exit the
+      // whole server. Always emit a terminal reply for the request.
+      logger.error(`padLoad failed: ${safeErr(err)}`);
+      socket.emit('results:padLoad',
+          {total: 0, results: [], error: safeErr(err)});
+     }
     })
 
 
     socket.on('deletePad', async (padId: string) => {
-      const padExists = await padManager.doesPadExists(padId);
-      if (padExists) {
-        logger.info(`Deleting pad: ${padId}`);
-        const pad = await padManager.getPad(padId);
-        await pad.remove();
+      try {
+        if (await padManager.doesPadExists(padId)) {
+          // Healthy pad — full relational cleanup (revs, chat, readonly,
+          // authors, deletion token, hooks).
+          logger.info(`Deleting pad: ${padId}`);
+          const pad = await padManager.getPad(padId);
+          await pad.remove();
+          socket.emit('results:deletePad', padId);
+          return;
+        }
+
+        // doesPadExists() is false either because nothing is stored under
+        // this id, or because the record is unreadable (a non-object value
+        // leaves `value.atext` undefined). The latter is exactly what
+        // padLoad now surfaces with zeroed metadata — getPad()/Pad.remove()
+        // would throw on it, so fall back to a raw key purge. Without this
+        // the surfaced corrupt pad is undeletable from the admin UI.
+        const raw = await db.get(`pad:${padId}`);
+        if (raw != null) {
+          logger.info(`Deleting unreadable pad record via raw key purge: ${padId}`);
+          // Best-effort sweep of sub-keys (revs/chat/deletionToken/…) and
+          // the readonly mapping, then the main key + pad-list/cache entry.
+          const subKeys: string[] = (await db.findKeys(`pad:${padId}:*`, null)) || [];
+          await Promise.all(subKeys.map((k) => db.remove(k)));
+          await db.remove(`pad2readonly:${padId}`);
+          await padManager.removePad(padId);
+        }
+        // Always emit a terminal reply (even for an already-absent id) so the
+        // UI clears the row instead of silently doing nothing.
         socket.emit('results:deletePad', padId);
+      } catch (err) {
+        logger.error(`deletePad failed for "${padId}": ${safeErr(err)}`);
       }
     })
 

src/tests/backend/specs/admin/padLoadResilience.ts

@@ -0,0 +1,180 @@
+'use strict';
+
+// Regression test for issue #7935 ("Display issue of notes"): pads exist
+// (visible on the welcome page, returned by the API `listAllPads`) but the
+// admin "Manage pads" UI shows none.
+//
+// Root cause: the admin /settings `padLoad` handler hydrates every pad via
+// `padManager.getPad()` to build the listing (the default `lastEdited` sort
+// forces a full scan). `findKeys('pad:*', '*:*:*')` returns *every* key under
+// the `pad:` prefix, including legacy / foreign / migration-corrupted records
+// — e.g. a value stored as a JSON *string* rather than a pad object, which is
+// exactly what a botched dirty.db → PostgreSQL migration produces. Loading
+// such a record makes `Pad.init` throw `Cannot use 'in' operator to search
+// for 'pool' in <string>`. Before the fix that single rejection took out the
+// whole handler: no `results:padLoad` was ever emitted (the SPA showed an
+// empty "No results" state forever) and the unhandled rejection could exit
+// the server. The handler now skips unreadable pads (surfacing them with
+// zeroed metadata so an admin can still delete them) and always emits a
+// terminal reply.
+
+import {strict as assert} from 'assert';
+import setCookieParser from 'set-cookie-parser';
+
+const io = require('socket.io-client');
+const common = require('../../common');
+const settings = require('../../../../node/utils/Settings');
+const padManager = require('../../../../node/db/PadManager');
+const db = require('../../../../node/db/DB');
+
+const adminSocket = async () => {
+  settings.users = settings.users || {};
+  settings.users['test-admin'] = {password: 'test-admin-password', is_admin: true};
+  const savedRequireAuthentication = settings.requireAuthentication;
+  settings.requireAuthentication = true;
+  let res: any;
+  try {
+    res = await (common.agent as any)
+        .get('/admin/')
+        .auth('test-admin', 'test-admin-password');
+  } finally {
+    settings.requireAuthentication = savedRequireAuthentication;
+  }
+  const resCookies = setCookieParser.parse(res, {map: true});
+  const reqCookieHdr = Object.entries(resCookies)
+      .map(([name, cookie]: [string, any]) =>
+          `${name}=${encodeURIComponent(cookie.value)}`)
+      .join('; ');
+  const socket = io(`${common.baseUrl}/settings`, {
+    forceNew: true,
+    query: {cookie: reqCookieHdr},
+  });
+  await new Promise<void>((resolve, reject) => {
+    const onErr = (err: any) => { socket.off('connect', onConn); reject(err); };
+    const onConn = () => { socket.off('connect_error', onErr); resolve(); };
+    socket.once('connect', onConn);
+    socket.once('connect_error', onErr);
+  });
+  return socket;
+};
+
+const ask = (socket: any, evt: string, payload: any, replyEvt: string, timeoutMs = 10000) =>
+    new Promise<any>((resolve, reject) => {
+      const timer = setTimeout(
+          () => reject(new Error(`no \`${replyEvt}\` reply within ${timeoutMs}ms`)), timeoutMs);
+      socket.once(replyEvt, (data: any) => { clearTimeout(timer); resolve(data); });
+      socket.emit(evt, payload);
+    });
+
+describe(__filename, function () {
+  let socket: any;
+  let savedUsers: any;
+  let savedRequireAuthentication: boolean;
+  let setupCompleted = false;
+  const tag = `padLoadResilience-${Date.now()}-${Math.floor(Math.random() * 1e6)}`;
+  const goodId = `${tag}-good`;
+  const corruptId = `${tag}-corrupt`;
+
+  before(async function () {
+    this.timeout(120000);
+    await common.init();
+
+    savedUsers = settings.users;
+    savedRequireAuthentication = settings.requireAuthentication;
+    setupCompleted = true;
+
+    try {
+      socket = await adminSocket();
+    } catch (err: any) {
+      console.warn(
+          `[padLoadResilience] admin socket connect failed (${err && err.message}); ` +
+          "skipping suite — likely an authenticate-hook plugin rejecting the test's " +
+          'admin credentials.');
+      this.skip();
+      return;
+    }
+
+    // A normal, readable pad — this is what must still show up.
+    await padManager.getPad(goodId, 'good content\n');
+
+    // A pad that enters the pad-name index normally, then has its stored
+    // value clobbered into a non-object (a JSON string) to mimic a
+    // migration-corrupted / foreign `pad:*` record. Evicting it from the
+    // in-memory cache forces the next getPad() to re-read the bad value.
+    await padManager.getPad(corruptId, 'temp\n');
+    await db.set(`pad:${corruptId}`, 'corrupt-non-object-value');
+    padManager.unloadPad(corruptId);
+
+    // Sanity-check that the setup actually reproduces the failing read; if
+    // this stops throwing the test is no longer exercising the bug.
+    await assert.rejects(padManager.getPad(corruptId),
+        'expected the corrupted pad record to make getPad throw');
+  });
+
+  after(async function () {
+    if (socket) socket.disconnect();
+    if (!setupCompleted) return;
+    if (settings.users) delete settings.users['test-admin'];
+    settings.users = savedUsers;
+    settings.requireAuthentication = savedRequireAuthentication;
+    for (const id of [goodId, corruptId]) {
+      try { await db.remove(`pad:${id}`); } catch { /* ignore */ }
+      try { await db.remove(`pad:${id}:revs:0`); } catch { /* ignore */ }
+      try { padManager.unloadPad(id); } catch { /* ignore */ }
+    }
+  });
+
+  it('a single corrupt pad does not hide every other pad (issue #7935)', async function () {
+    this.timeout(30000);
+    // The default query the SPA sends on initial load: lastEdited sort forces
+    // the full-scan hydration path that touches every pad — including the
+    // corrupt one.
+    const res = await ask(socket, 'padLoad', {
+      pattern: tag, offset: 0, limit: 12,
+      sortBy: 'lastEdited', ascending: false, filter: 'all',
+    }, 'results:padLoad');
+
+    const names = res.results.map((r: any) => r.padName);
+    assert.ok(names.includes(goodId),
+        `the readable pad must still be listed; got ${JSON.stringify(names)}`);
+    // The bad pad is surfaced (zeroed metadata) rather than silently dropped,
+    // so an admin can see and delete it.
+    assert.ok(names.includes(corruptId),
+        `the corrupt pad should surface for deletion; got ${JSON.stringify(names)}`);
+    assert.equal(res.total, 2, `expected total=2, got ${JSON.stringify(res)}`);
+  });
+
+  it('still replies on the fast path (padName sort) with a corrupt pad present', async function () {
+    this.timeout(30000);
+    const res = await ask(socket, 'padLoad', {
+      pattern: tag, offset: 0, limit: 12,
+      sortBy: 'padName', ascending: true, filter: 'all',
+    }, 'results:padLoad');
+    const names = res.results.map((r: any) => r.padName);
+    assert.ok(names.includes(goodId), `got ${JSON.stringify(names)}`);
+    assert.ok(names.includes(corruptId), `got ${JSON.stringify(names)}`);
+  });
+
+  // Runs last: surfacing a corrupt pad is only useful if it can be removed.
+  // deletePad's normal path (doesPadExists + getPad + Pad.remove) can't touch
+  // an unreadable record, so it must fall back to a raw key purge.
+  it('a surfaced corrupt pad can be deleted from the admin UI', async function () {
+    this.timeout(30000);
+    const ack = await ask(socket, 'deletePad', corruptId, 'results:deletePad');
+    assert.equal(ack, corruptId, `expected deletePad to ack "${corruptId}", got ${JSON.stringify(ack)}`);
+
+    // Assert the user-facing outcome — the corrupt pad is gone from the
+    // listing (its pad-list entry was dropped) while the good pad stays.
+    // (We deliberately don't probe `db.get('pad:<id>')` here: ueberdb2's
+    // per-backend read/write buffering can still return the just-removed
+    // value immediately after `remove()`, so that would be a flaky
+    // storage-internals assertion rather than a behavioural one.)
+    const res = await ask(socket, 'padLoad', {
+      pattern: tag, offset: 0, limit: 12,
+      sortBy: 'padName', ascending: true, filter: 'all',
+    }, 'results:padLoad');
+    const names = res.results.map((r: any) => r.padName);
+    assert.ok(!names.includes(corruptId), `corrupt pad still listed: ${JSON.stringify(names)}`);
+    assert.ok(names.includes(goodId), `good pad missing after delete: ${JSON.stringify(names)}`);
+  });
+});

src/tests/frontend-new/admin-spec/admin_pads_page.spec.ts

@@ -0,0 +1,54 @@
+import {expect, test} from "@playwright/test";
+import {loginToAdmin} from "../helper/adminhelper";
+import {goToPad, writeToPad} from "../helper/padHelper";
+
+// End-to-end coverage for issue #7935: a pad that exists must be visible
+// both on the welcome page's "recent pads" list (driven by localStorage,
+// i.e. gated on the browser having opened the pad) and in the admin
+// "Manage pads" UI (driven by the /settings socket `padLoad` handler,
+// which enumerates the DB). The admin side regressed when a single
+// unreadable pad record made `padLoad` throw and silently return nothing
+// — see specs/admin/padLoadResilience.ts for the server-side guard.
+
+// /admin tests mutate global server state, so keep them serial.
+test.describe.configure({mode: 'serial'});
+
+const ADMIN_URL = 'http://localhost:9001/admin';
+
+test.describe('a created pad shows up on the home page and in /admin', () => {
+  // Unique, URL-safe id so the recent-pads localStorage entry and the admin
+  // search both target exactly this pad and ignore leftovers from other suites.
+  const padId = `pw-pads-7935-${Date.now()}`;
+
+  test('opening a pad lists it in the welcome page recent-pads', async ({page}) => {
+    await goToPad(page, padId);
+    await writeToPad(page, 'hello from 7935');
+
+    // Opening the pad writes it to `recentPads` localStorage (colibris
+    // pad.js). The welcome page renders that list — same browser context,
+    // so the entry carries over.
+    await page.goto('http://localhost:9001/');
+    const recentPad = page.locator('.recent-pad', {hasText: padId});
+    await expect(recentPad).toBeVisible({timeout: 10000});
+    await expect(recentPad.locator('a')).toHaveText(padId);
+  });
+
+  test('the same pad is listed in the admin Manage pads UI', async ({page}) => {
+    await loginToAdmin(page, 'admin', 'changeme1');
+    await page.goto(`${ADMIN_URL}/pads`);
+
+    await expect(page.getByRole('heading', {name: 'Manage pads'}))
+        .toBeVisible({timeout: 30000});
+
+    // Narrow the (full-scan) listing to our pad. The search is debounced
+    // server-side; allow the round-trip to settle.
+    const search = page.getByPlaceholder('Search for pads');
+    await search.fill(padId);
+
+    await expect(page.locator('.pm-pad-title', {hasText: padId}))
+        .toBeVisible({timeout: 15000});
+    // The "No results" empty state must NOT be showing — the exact #7935
+    // symptom was an empty Manage-pads list for pads that demonstrably exist.
+    await expect(page.locator('.pm-empty')).toHaveCount(0);
+  });
+});