From a4da1cc4389927f2aa9977bdce701f6f3f97b5f3 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Wed, 12 Aug 2026 09:05:21 -0500 Subject: [PATCH] fix(modules): stop a module before purging its tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uninstall-with-purge ran purge.sql while the module was still started: the tables went, and the module kept serving and ingesting against a schema that no longer existed until lifecycle.stop() finished — up to the five-second hook budget. For module-uo that is the uo-link WebSocket writing shard events into dropped tables, and requests in flight answering 500 where a stopped module answers 404. Nothing required the old order. The comment justified it as "purge while the SQL is still readable", but removeDir is the only step that touches the filesystem, so purge.sql stays readable until after the stop. The 400 for a module that ships no purge.sql is now resolved before anything is stopped, so a refused request leaves the module exactly as it found it. Found while proving Phase 4's acceptance criterion 2 against the real module-uo v0.3.0 release on an empty database (MODULE_SYSTEM.md §2.7.2). 742 server tests (+1); routes.manifest.json and swagger-output.json byte-identical. AI disclosure: this contribution was AI-assisted (Claude Code). Co-Authored-By: Claude --- .../src/router/v1/admin/modules.controller.js | 31 +++++++----- server/test/adminModules.test.js | 47 +++++++++++++++---- 2 files changed, 58 insertions(+), 20 deletions(-) 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' })