SessionStore: Delete DB record when session expires

This only deletes records known to the current Etherpad instance --
old records from previous runs are not automatically cleaned up.

CHANGELOG.md

@@ -2,15 +2,17 @@
 
 ### Notable enhancements and fixes
 
+* Improvements to login session management:
+  * `sessionstorage:*` database records are automatically deleted when the login
+    session expires (with some exceptions that will be fixed in the future).
+  * Requests for static content (e.g., `/robots.txt`) and special pages (e.g.,
+    the HTTP API, `/stats`) no longer create login session state.
 * The following settings from `settings.json` are now applied as expected (they
   were unintentionally ignored before):
   * `padOptions.lang`
   * `padOptions.showChat`
   * `padOptions.userColor`
   * `padOptions.userName`
-* Requests for static content (e.g., `/robots.txt`) and special pages (e.g., the
-  HTTP API, `/stats`) no longer cause the server to generate database records
-  intended to manage browser sessions (`sessionstorage:*`).
 * Fixed the return value of the `getText` HTTP API when called with a specific
   revision.
 * Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`.

src/node/db/SessionStore.js

@@ -26,11 +26,17 @@ class SessionStore extends Store {
     //   - `db`: Session expiration as recorded in the database (ms since epoch, not a Date).
     //   - `real`: Actual session expiration (ms since epoch, not a Date). Always greater than or
     //     equal to `db`.
+    //   - `timeout`: Timeout ID for a timeout that will clean up the database record.
     this._expirations = new Map();
   }
 
+  shutdown() {
+    for (const {timeout} of this._expirations.values()) clearTimeout(timeout);
+  }
+
   async _updateExpirations(sid, sess, updateDbExp = true) {
     const exp = this._expirations.get(sid) || {};
+    clearTimeout(exp.timeout);
     const {cookie: {expires} = {}} = sess || {};
     if (expires) {
       const sessExp = new Date(expires).getTime();
@@ -41,6 +47,15 @@ class SessionStore extends 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);
       this._expirations.set(sid, exp);
     } else {
       this._expirations.delete(sid);
@@ -66,6 +81,7 @@ class SessionStore extends Store {
 
   async _destroy(sid) {
     logger.debug(`DESTROY ${sid}`);
+    clearTimeout((this._expirations.get(sid) || {}).timeout);
     this._expirations.delete(sid);
     await DB.remove(`sessionstorage:${sid}`);
   }

src/node/hooks/express.js

@@ -16,6 +16,7 @@ const webaccess = require('./express/webaccess');
 
 const logger = log4js.getLogger('http');
 let serverName;
+let sessionStore;
 const sockets = new Set();
 const socketsEvents = new events.EventEmitter();
 const startTime = stats.settableGauge('httpStartTime');
@@ -23,15 +24,15 @@ const startTime = stats.settableGauge('httpStartTime');
 exports.server = null;
 
 const closeServer = async () => {
-  if (exports.server == null) return;
+  if (exports.server != null) {
     logger.info('Closing HTTP server...');
     // Call exports.server.close() to reject new connections but don't await just yet because the
     // Promise won't resolve until all preexisting connections are closed.
     const p = util.promisify(exports.server.close.bind(exports.server))();
     await hooks.aCallAll('expressCloseServer');
-  // Give existing connections some time to close on their own before forcibly terminating. The time
+    // Give existing connections some time to close on their own before forcibly terminating. The
-  // should be long enough to avoid interrupting most preexisting transmissions but short enough to
+    // time should be long enough to avoid interrupting most preexisting transmissions but short
-  // avoid a noticeable outage.
+    // enough to avoid a noticeable outage.
     const timeout = setTimeout(async () => {
       logger.info(`Forcibly terminating remaining ${sockets.size} HTTP connections...`);
       for (const socket of sockets) socket.destroy(new Error('HTTP server is closing'));
@@ -49,6 +50,9 @@ const closeServer = async () => {
     exports.server = null;
     startTime.setValue(0);
     logger.info('HTTP server closed');
+  }
+  if (sessionStore) sessionStore.shutdown();
+  sessionStore = null;
 };
 
 exports.createServer = async () => {
@@ -172,9 +176,10 @@ exports.restartServer = async () => {
 
   app.use(cookieParser(settings.sessionKey, {}));
 
+  sessionStore = new SessionStore();
   exports.sessionMiddleware = expressSession({
     secret: settings.sessionKey,
-    store: new SessionStore(),
+    store: sessionStore,
     resave: false,
     saveUninitialized: true,
     // Set the cookie name to a javascript identifier compatible string. Makes code handling it

src/tests/backend/specs/SessionStore.js

@@ -25,7 +25,10 @@ describe(__filename, function () {
   });
 
   afterEach(async function () {
-    if (ss != null && sid != null) await destroy();
+    if (ss != null) {
+      if (sid != null) await destroy();
+      ss.shutdown();
+    }
     sid = null;
     ss = null;
   });
@@ -46,6 +49,9 @@ describe(__filename, function () {
       const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}};
       await set(sess);
       assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess));
+      await new Promise((resolve) => setTimeout(resolve, 110));
+      // Writing should start a timeout.
+      assert(await db.get(`sessionstorage:${sid}`) == null);
     });
 
     it('set of already expired session', async function () {
@@ -54,6 +60,24 @@ describe(__filename, function () {
       // No record should have been created.
       assert(await db.get(`sessionstorage:${sid}`) == null);
     });
+
+    it('switch from non-expiring to expiring', async function () {
+      const sess = {foo: 'bar'};
+      await set(sess);
+      const sess2 = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}};
+      await set(sess2);
+      await new Promise((resolve) => setTimeout(resolve, 110));
+      assert(await db.get(`sessionstorage:${sid}`) == null);
+    });
+
+    it('switch from expiring to non-expiring', async function () {
+      const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}};
+      await set(sess);
+      const sess2 = {foo: 'bar'};
+      await set(sess2);
+      await new Promise((resolve) => setTimeout(resolve, 110));
+      assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess2));
+    });
   });
 
   describe('get', function () {
@@ -77,6 +101,9 @@ describe(__filename, function () {
       const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}};
       await db.set(`sessionstorage:${sid}`, sess);
       assert.equal(JSON.stringify(await get()), JSON.stringify(sess));
+      await new Promise((resolve) => setTimeout(resolve, 110));
+      // Reading should start a timeout.
+      assert(await db.get(`sessionstorage:${sid}`) == null);
     });
 
     it('get of record from previous run (already expired)', async function () {
@@ -99,6 +126,18 @@ describe(__filename, function () {
     });
   });
 
+  describe('shutdown', function () {
+    it('shutdown cancels timeouts', async function () {
+      const sess = {foo: 'bar', cookie: {expires: new Date(Date.now() + 100)}};
+      await set(sess);
+      assert.equal(JSON.stringify(await get()), JSON.stringify(sess));
+      ss.shutdown();
+      await new Promise((resolve) => setTimeout(resolve, 110));
+      // The record should not have been automatically purged.
+      assert.equal(JSON.stringify(await db.get(`sessionstorage:${sid}`)), JSON.stringify(sess));
+    });
+  });
+
   describe('destroy', function () {
     it('destroy deletes the database record', async function () {
       const sess = {cookie: {expires: new Date(Date.now() + 100)}};