From da9f5ac4eed750af2448c42d7973b9b0b243fd54 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 5 Apr 2026 16:12:43 +0100 Subject: [PATCH] fix: add periodic cleanup of expired/stale sessions from database (#7448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add periodic cleanup of expired/stale sessions from database SessionStore now runs a periodic cleanup (every hour, plus once on startup) that removes: - Sessions with expired cookies (expires date in the past) - Sessions with no expiry that contain no data beyond the default cookie (the empty sessions that accumulate indefinitely per #5010) Without this, sessions accumulated forever in the database because: 1. Sessions with no maxAge never got an expiry date 2. On server restart, in-memory expiration timeouts were lost 3. There was no mechanism to clean up sessions that were never accessed again Fixes #5010 Co-Authored-By: Claude Opus 4.6 (1M context) * fix: resolve TypeScript error for sessionStore.startCleanup() Use a local variable for the SessionStore instance to avoid type narrowing issues with the module-level Store|null variable. Co-Authored-By: Claude Opus 4.6 (1M context) * fix: address Qodo review — chained timeouts, cleanup tests, docs - Replace setInterval with chained setTimeout to prevent overlapping cleanup runs on large databases - Store and clear startup timeout in shutdown() to prevent leaks - Add .unref() on all timers so they don't delay process exit - Fix misleading docstring — cleanup removes empty no-expiry sessions, not sessions older than STALE_SESSION_MAX_AGE_MS (removed unused const) - Add 5 regression tests: expired sessions removed, empty sessions removed, sessions with data preserved, valid sessions preserved, shutdown cancels timer Co-Authored-By: Claude Opus 4.6 (1M context) * feat: add cookie.sessionCleanup setting to control session cleanup Session cleanup is now gated behind cookie.sessionCleanup (default true). Admins who want to keep stale sessions can set this to false in settings.json. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- settings.json.template | 8 ++- src/node/db/SessionStore.ts | 72 +++++++++++++++++++++++++ src/node/hooks/express.ts | 6 ++- src/node/utils/Settings.ts | 2 + src/tests/backend/specs/SessionStore.ts | 54 +++++++++++++++++++ 5 files changed, 140 insertions(+), 2 deletions(-) diff --git a/settings.json.template b/settings.json.template index 8c242a311..4d68a10db 100644 --- a/settings.json.template +++ b/settings.json.template @@ -463,7 +463,13 @@ * Automatic session refreshes can be disabled (not recommended) by setting * this to null. */ - "sessionRefreshInterval": 86400000 // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + "sessionRefreshInterval": 86400000, // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + + /* + * Whether to periodically clean up expired and stale sessions from the + * database. Set to false to disable. Default: true. + */ + "sessionCleanup": true }, /* diff --git a/src/node/db/SessionStore.ts b/src/node/db/SessionStore.ts index 15eb5a971..2bbfd5a89 100644 --- a/src/node/db/SessionStore.ts +++ b/src/node/db/SessionStore.ts @@ -9,6 +9,9 @@ const util = require('util'); const logger = log4js.getLogger('SessionStore'); +// How often to run the cleanup of expired/stale sessions. +const CLEANUP_INTERVAL_MS = 60 * 60 * 1000; // 1 hour + class SessionStore extends expressSession.Store { /** * @param {?number} [refresh] - How often (in milliseconds) `touch()` will update a session's @@ -30,10 +33,79 @@ class SessionStore extends expressSession.Store { // equal to `db`. // - `timeout`: Timeout ID for a timeout that will clean up the database record. this._expirations = new Map(); + this._cleanupTimer = null; + this._cleanupRunning = false; + } + + /** + * Start periodic cleanup of expired/stale sessions from the database. + * Uses chained setTimeout (not setInterval) to prevent overlapping runs. + */ + startCleanup() { + this._scheduleCleanup(5000); // First run 5s after startup. + } + + _scheduleCleanup(delay: number) { + this._cleanupTimer = setTimeout(async () => { + try { + await this._cleanup(); + } catch (err) { + logger.error('Session cleanup error:', err); + } + // Schedule the next run only after this one completes. + this._scheduleCleanup(CLEANUP_INTERVAL_MS); + }, delay); + // Don't prevent Node.js from exiting. + if (this._cleanupTimer.unref) this._cleanupTimer.unref(); } shutdown() { for (const {timeout} of this._expirations.values()) clearTimeout(timeout); + if (this._cleanupTimer) { + clearTimeout(this._cleanupTimer); + this._cleanupTimer = null; + } + } + + /** + * Remove expired and empty sessions from the database. + * + * - Sessions with an `expires` date in the past are removed (expired). + * - 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. + */ + async _cleanup() { + const keys = await DB.findKeys('sessionstorage:*', null); + if (!keys || keys.length === 0) return; + const now = Date.now(); + let removed = 0; + for (const key of keys) { + const sess = await DB.get(key); + if (!sess) { + await DB.remove(key); + removed++; + continue; + } + const expires = sess.cookie?.expires; + if (expires) { + // Session has an expiry — remove if expired. + if (new Date(expires).getTime() <= now) { + await DB.remove(key); + removed++; + } + } 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++; + } + } + } + if (removed > 0) { + logger.info(`Session cleanup: removed ${removed} expired/stale sessions out of ${keys.length}`); + } } async _updateExpirations(sid: string, sess: any, updateDbExp = true) { diff --git a/src/node/hooks/express.ts b/src/node/hooks/express.ts index 554ff6a56..ac2b90305 100644 --- a/src/node/hooks/express.ts +++ b/src/node/hooks/express.ts @@ -200,7 +200,11 @@ exports.restartServer = async () => { app.use(cookieParser(secret, {})); - sessionStore = new SessionStore(settings.cookie.sessionRefreshInterval); + const store = new SessionStore(settings.cookie.sessionRefreshInterval); + if (settings.cookie.sessionCleanup !== false) { + store.startCleanup(); + } + sessionStore = store; exports.sessionMiddleware = expressSession({ rolling: true, secret, diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index 1124fdebb..efefff9e6 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -255,6 +255,7 @@ export type SettingsType = { prefix: string, sameSite: boolean | "lax" | "strict" | "none" | undefined, sessionLifetime: number, + sessionCleanup: boolean, sessionRefreshInterval: number, }, requireAuthentication: boolean, @@ -534,6 +535,7 @@ const settings: SettingsType = { prefix: '', sameSite: 'lax', sessionLifetime: 10 * 24 * 60 * 60 * 1000, + sessionCleanup: true, sessionRefreshInterval: 1 * 24 * 60 * 60 * 1000, }, /* diff --git a/src/tests/backend/specs/SessionStore.ts b/src/tests/backend/specs/SessionStore.ts index 5dfc44ff2..9f5266dc2 100644 --- a/src/tests/backend/specs/SessionStore.ts +++ b/src/tests/backend/specs/SessionStore.ts @@ -12,6 +12,9 @@ type Session = { destroy: (sid:string|null) => void; touch: (sid:string|null, sess:any, sess2:any) => void; shutdown: () => void; + startCleanup: () => void; + _cleanup: () => Promise; + _cleanupTimer: any; } describe(__filename, function () { @@ -243,4 +246,55 @@ describe(__filename, function () { assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess)); }); }); + + // Regression tests for https://github.com/ether/etherpad-lite/issues/5010 + describe('cleanup', function () { + it('removes expired sessions', async function () { + const expiredSid = `cleanup_expired_${common.randomString()}`; + await db.set(`sessionstorage:${expiredSid}`, { + cookie: {path: '/', expires: new Date(1).toJSON(), httpOnly: true}, + }); + await ss!._cleanup(); + assert(await db.get(`sessionstorage:${expiredSid}`) == null); + }); + + it('removes empty sessions with no expiry', async function () { + const emptySid = `cleanup_empty_${common.randomString()}`; + await db.set(`sessionstorage:${emptySid}`, { + cookie: {path: '/', _expires: null, originalMaxAge: null, httpOnly: true}, + }); + await ss!._cleanup(); + assert(await db.get(`sessionstorage:${emptySid}`) == null); + }); + + it('preserves sessions with user data and no expiry', async function () { + const dataSid = `cleanup_data_${common.randomString()}`; + const sess = { + cookie: {path: '/', _expires: null, httpOnly: true}, + user: {name: 'test'}, + }; + await db.set(`sessionstorage:${dataSid}`, sess); + await ss!._cleanup(); + assert.equal(JSON.stringify(await db.get(`sessionstorage:${dataSid}`)), JSON.stringify(sess)); + await db.remove(`sessionstorage:${dataSid}`); + }); + + it('preserves non-expired sessions', async function () { + const validSid = `cleanup_valid_${common.randomString()}`; + const sess = { + cookie: {path: '/', expires: new Date(Date.now() + 60000).toJSON(), httpOnly: true}, + }; + await db.set(`sessionstorage:${validSid}`, sess); + await ss!._cleanup(); + assert.equal(JSON.stringify(await db.get(`sessionstorage:${validSid}`)), JSON.stringify(sess)); + await db.remove(`sessionstorage:${validSid}`); + }); + + it('shutdown cancels pending cleanup timer', async function () { + ss!.startCleanup(); + ss!.shutdown(); + // After shutdown, the timer should be cleared. + assert(ss!._cleanupTimer == null); + }); + }); });