Merge pull request 'fix(modules): stop a module before purging its tables' (#146) from fix/module-uninstall-stop-before-purge into edge

Reviewed-on: #146

server/src/router/v1/admin/modules.controller.js

@@ -265,9 +265,13 @@ async function disable(req, res) {
 // this is about to delete, so after an uninstall there is nothing left to purge
 // with. Ticking the box is the last moment the file exists.
 //
-// The order below is the whole of it, and each step depends on the one above:
-// purge while the SQL is still readable, stop while the code is still loaded,
-// then delete.
+// The order below is the whole of it: locate the purge file, STOP, purge, then
+// delete. Only the last step is destructive to the filesystem, so the file stays
+// readable the whole way through — which is why stopping comes first. Dropping a
+// module's tables while it is still started leaves it serving and ingesting
+// against a schema that no longer exists: module-uo's uo-link WebSocket keeps
+// writing shard events for as long as its onShutdown takes to run, and requests
+// in flight answer 500 where a stopped module answers 404.
 async function remove(req, res) {
   const { id } = req.params
   const purge = req.query.purge === 'true' || req.query.purge === '1'
@@ -276,23 +280,26 @@ async function remove(req, res) {
     const onVolume = install.isInstalled(id)
     if (!current && !onVolume) return res.status(404).json({ message: 'No such module.' })
 
-    let purged = null
+    // Resolved before anything happens, because this branch is a 400: a request
+    // that is going to be refused must not stop the module on its way out.
+    let purgeSql = null
     if (purge) {
-      const file = install.purgeFile(id)
-      if (!file) {
+      purgeSql = install.purgeFile(id)
+      if (!purgeSql) {
         return res.status(400).json({
           message: 'This module ships no purge.sql, so its data cannot be deleted. Uninstall without purging instead.',
         })
       }
-      purged = await schema.runPurge(file)
     }
 
-    // Stop it before its files vanish. A module whose directory is deleted out
-    // from under a running onShutdown is being asked to tear down a world whose
-    // code may already be half-unreadable — and its sockets would otherwise stay
-    // open until the restart, holding a connection on behalf of a module that no
-    // longer exists on disk.
+    // Stop it before its tables and then its files vanish. A module whose
+    // directory is deleted out from under a running onShutdown is being asked to
+    // tear down a world whose code may already be half-unreadable — and its
+    // sockets would otherwise stay open until the restart, holding a connection
+    // on behalf of a module that no longer exists on disk.
     await lifecycle.stop(id)
+
+    const purged = purgeSql ? await schema.runPurge(purgeSql) : null
     const removed = await install.removeDir(id)
 
     // A purge leaves nothing: no directory, no tables, no data. Keeping a

server/test/adminModules.test.js

@@ -11,9 +11,11 @@
 //   - **enable must not touch the loader.** Disable ran the module's onShutdown;
 //     there is no onBoot re-dispatch, so flipping the record back would put a
 //     module with closed sockets and cleared timers back on the nav.
-//   - **purge must run before the directory is removed.** purge.sql lives inside
-//     that directory. Reorder those two lines and the feature silently stops
-//     working, with a 200 and no data deleted.
+//   - **uninstall's four steps have exactly one valid order**: stop, purge,
+//     remove the directory, remove the row. purge.sql lives inside that
+//     directory, so purging after the delete silently does nothing with a 200 —
+//     and purging before the stop drops the tables under a module that is still
+//     running. See the test itself for why neither is visible in a response.
 //
 // Point the DB at a closed port BEFORE requiring anything that builds the pool.
 process.env.DB_HOST = '127.0.0.1'
@@ -333,10 +335,20 @@ test('a shutdown hook that failed is reported rather than swallowed', async () =
 
 // ── uninstall and purge ────────────────────────────────────────────────────
 
-test('uninstall purges BEFORE it removes the directory', async () => {
-  // The ordering that makes decision 5 work at all: purge.sql is a file inside
-  // the directory being deleted. Swap these two and the endpoint still answers
-  // 200 and deletes nothing.
+test('uninstall stops the module, THEN purges, THEN removes the directory', async () => {
+  // Two orderings pinned by one assertion, because both are one line away from
+  // being wrong and neither failure is visible in the response:
+  //
+  //   - purge BEFORE removeDir, or decision 5 stops working entirely — purge.sql
+  //     is a file inside the directory being deleted, and the endpoint would
+  //     still answer 200 having deleted nothing.
+  //   - stop BEFORE purge, or the tables are dropped under a module that is
+  //     still started. It keeps serving and ingesting against a schema that no
+  //     longer exists for as long as its onShutdown takes (module-uo's uo-link
+  //     WebSocket keeps writing shard events), and requests in flight answer 500
+  //     where a stopped module answers 404. Nothing about the old order was
+  //     required: purge.sql stays readable until removeDir, which is the only
+  //     step that touches the filesystem.
   const order = []
   modules.get = async (id) => ({ id, state: 'started' })
   install.isInstalled = () => true
@@ -349,12 +361,31 @@ test('uninstall purges BEFORE it removes the directory', async () => {
   const res = mockRes()
   await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
 
-  assert.deepEqual(order, ['purge', 'stop', 'removeDir', 'removeRow'])
+  assert.deepEqual(order, ['stop', 'purge', 'removeDir', 'removeRow'])
   assert.equal(res.body.purged, 12)
   assert.equal(res.body.restartRequired, true)
   assert.equal(logged[0].action, 'module.purge')
 })
 
+test('a refused purge does not stop the module on its way out', async () => {
+  // The 400 branch below is resolved BEFORE anything is stopped. A request that
+  // is going to be refused must leave the module exactly as it found it —
+  // otherwise "you cannot delete this module's data" would silently take the
+  // module down as a side effect of saying no.
+  const order = []
+  modules.get = async (id) => ({ id, state: 'started' })
+  install.isInstalled = () => true
+  install.purgeFile = () => null
+  lifecycle.stop = async () => { order.push('stop'); return { stopped: true, error: null } }
+  install.removeDir = async () => { order.push('removeDir'); return true }
+
+  const res = mockRes()
+  await ctrl.remove(req({ params: { id: 'uo' }, query: { purge: 'true' } }), res)
+
+  assert.equal(res.statusCode, 400)
+  assert.deepEqual(order, [])
+})
+
 test('a plain uninstall keeps the row and does not purge', async () => {
   const order = []
   modules.get = async (id) => ({ id, state: 'started' })