From 605ad280682d522fafa736d4f52f06c0c1973844 Mon Sep 17 00:00:00 2001
From: John McLear <john@mclear.co.uk>
Date: Mon, 6 Apr 2026 11:19:34 +0100
Subject: [PATCH] fix: prevent race condition in session cleanup timeout
 (#7471)

When the cleanup timeout fires, check the in-memory exp.real before
reading from the DB. If touch() extended the expiry (but the old
timeout fires late, e.g. on slow CI), reschedule instead of reading
potentially stale cached data from the DB and destroying the session.

Also increased test expiry times so the "touch after eligible for
refresh" test isn't sensitive to event loop delays on slow machines.

Fixes flaky SessionStore test from #7448.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 src/node/db/SessionStore.ts             | 25 ++++++++++++++++---------
 src/tests/backend/specs/SessionStore.ts |  4 ++--
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/node/db/SessionStore.ts b/src/node/db/SessionStore.ts
index 2bbfd5a89..9ce81a709 100644
--- a/src/node/db/SessionStore.ts
+++ b/src/node/db/SessionStore.ts
@@ -122,15 +122,22 @@ class SessionStore extends expressSession.Store {
       // If reading from the database, update the expiration with the latest value from touch() so
       // that touch() appears to write to the database every time even though it doesn't.
       if (typeof expires === 'string') sess.cookie.expires = new Date(exp.real).toJSON();
-      // Use this._get(), not this._destroy(), to destroy the DB record for the expired session.
-      // This is done in case multiple Etherpad instances are sharing the same database and users
-      // are bouncing between the instances. By using this._get(), this instance will query the DB
-      // for the latest expiration time written by any of the instances, ensuring that the record
-      // isn't prematurely deleted if the expiration time was updated by a different Etherpad
-      // instance. (Important caveat: Client-side database caching, which ueberdb does by default,
-      // could still cause the record to be prematurely deleted because this instance might get a
-      // stale expiration time from cache.)
-      exp.timeout = setTimeout(() => this._get(sid), exp.real - now);
+      // Schedule cleanup when the session is expected to expire. When the timeout fires, check
+      // the in-memory expiry first — touch() may have extended it without rescheduling the timeout
+      // (e.g., if touch's clearTimeout raced with the timer on a slow system). If the session was
+      // extended, reschedule instead of reading from the DB which may return stale cached data.
+      exp.timeout = setTimeout(() => {
+        const currentExp = this._expirations.get(sid);
+        if (currentExp && currentExp.real > Date.now()) {
+          // Expiry was extended (e.g., by touch). Reschedule.
+          currentExp.timeout = setTimeout(() => this._get(sid), currentExp.real - Date.now());
+          return;
+        }
+        // Use this._get(), not this._destroy(), to query the DB for the latest expiration in case
+        // multiple Etherpad instances share the database. (Caveat: client-side DB caching could
+        // still cause premature deletion if the cache returns a stale expiration time.)
+        this._get(sid);
+      }, exp.real - now);
       this._expirations.set(sid, exp);
     } else {
       this._expirations.delete(sid);
diff --git a/src/tests/backend/specs/SessionStore.ts b/src/tests/backend/specs/SessionStore.ts
index 9f5266dc2..415ebc3c4 100644
--- a/src/tests/backend/specs/SessionStore.ts
+++ b/src/tests/backend/specs/SessionStore.ts
@@ -224,10 +224,10 @@ describe(__filename, function () {
 
     it('touch after eligible for refresh updates db', async function () {
       const start = Date.now();
-      const sess:any  = {foo: 'bar', cookie: {expires: new Date(start + 200)}};
+      const sess:any  = {foo: 'bar', cookie: {expires: new Date(start + 2000)}};
       await set(sess);
       await new Promise((resolve) => setTimeout(resolve, 100));
-      const sess2:any  = {foo: 'bar', cookie: {expires: new Date(start + 400)}};
+      const sess2:any  = {foo: 'bar', cookie: {expires: new Date(start + 4000)}};
       await touch(sess2);
       await new Promise((resolve) => setTimeout(resolve, 110));
       assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess2));