diff --git a/server/src/router/v1/admin/modules.controller.js b/server/src/router/v1/admin/modules.controller.js index 2d018c0..80b50e1 100644 --- a/server/src/router/v1/admin/modules.controller.js +++ b/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 diff --git a/server/test/adminModules.test.js b/server/test/adminModules.test.js index e14d175..da047f7 100644 --- a/server/test/adminModules.test.js +++ b/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' })