diff --git a/server/src/model/modules/modules.db.js b/server/src/model/modules/modules.db.js index 66474bb..d7e2bba 100644 --- a/server/src/model/modules/modules.db.js +++ b/server/src/model/modules/modules.db.js @@ -17,6 +17,23 @@ const getOne = (id) => query(`SELECT ${COLS} FROM installed_modules WHERE id = ? // module must not silently disable it, and re-installing a disabled one must not // silently switch it back on. A brand-new row lands in `installed`, the transient // state the next restart resolves. +// Provenance is COALESCEd, and that is the whole difference between this +// working and not. +// +// `lifecycle.boot()` re-records every module it scanned with no source and no +// sha256 — a directory placed on the volume by hand genuinely has neither, and +// the boot has no way to know where one came from. With a plain +// `source = VALUES(source)` that refresh overwrote both columns with NULL on +// EVERY boot, so an admin-panel install's provenance survived exactly until the +// restart that install asked for. Nothing could catch it before Phase 4: until +// then no caller ever passed a non-null value, and lifecycle.js's comment +// asserting that "recordInstalled leaves what it is not given" was a description +// of an intention rather than of this statement. +// +// COALESCE makes that comment true: a value overwrites, a NULL leaves what is +// there. The cost is that re-placing a DIFFERENT bundle by hand over a row that +// was once installed from a URL keeps the old provenance — which is wrong but +// stale, and strictly better than the alternative, which was wrong and blank. const upsert = ({ id, name, version, source, sha256 }) => query( `INSERT INTO installed_modules (id, name, version, source, sha256, state) @@ -24,8 +41,8 @@ const upsert = ({ id, name, version, source, sha256 }) => ON DUPLICATE KEY UPDATE name = VALUES(name), version = VALUES(version), - source = VALUES(source), - sha256 = VALUES(sha256)`, + source = COALESCE(VALUES(source), source), + sha256 = COALESCE(VALUES(sha256), sha256)`, [id, name, version, source ?? null, sha256 ?? null], ) diff --git a/server/src/modules/lifecycle.js b/server/src/modules/lifecycle.js index 737773b..637df88 100644 --- a/server/src/modules/lifecycle.js +++ b/server/src/modules/lifecycle.js @@ -101,10 +101,16 @@ async function boot({ modules, model } = {}) { id: m.id, name: m.name, version: m.version, - // Null provenance is what a hand-placed directory looks like. An install - // performed through the admin panel (§2.5, a later phase) writes the row - // with its source and hash first; this refresh deliberately does not - // overwrite either, because recordInstalled leaves what it is not given. + // Null provenance is what a hand-placed directory looks like, and it is + // all this step can honestly say: a scan finds a directory, never where it + // came from. An install through the admin panel writes source and sha256 + // first, and this refresh must not undo that. + // + // It used to. `upsert` assigned both columns unconditionally, so every + // boot nulled them and an install's provenance survived only until the + // restart it asked for. The statement now COALESCEs — see the note on + // modules.db.js's upsert. Nothing could have caught it before Phase 4: + // this was the only caller, and it has never had a value to pass. })) } diff --git a/server/src/router/v1/admin/modules.controller.js b/server/src/router/v1/admin/modules.controller.js index d01f5e5..2c79dc5 100644 --- a/server/src/router/v1/admin/modules.controller.js +++ b/server/src/router/v1/admin/modules.controller.js @@ -85,6 +85,11 @@ function present(row, live, onVolume) { startedAt: row ? row.startedAt : null, // What is actually mounted in this process, and what it is answering. liveState: live ? live.state : null, + // The version RUNNING, which is not always the version installed: an upgrade + // writes new files and a new row while the old code stays loaded until the + // restart. Without this the screen would report the new version as + // "Running", which is the same lie in a different place. + liveVersion: live ? live.version : null, capabilities: live ? live.capabilities : [], // What is on the volume. onVolume, @@ -349,11 +354,24 @@ async function setSources(req, res) { // "with no shell access to the box" — which a banner saying "please restart your // container" does not deliver. // -// It raises SIGTERM against its own process rather than calling the shutdown -// path directly. server.js already has a handler that stops the modules, the -// workers and the listeners in the right order and closes the pool and the log -// file before exiting 0; reaching that through the signal means there is exactly -// one graceful-shutdown path and this route cannot drift from it. +// It reaches server.js's existing SIGTERM handler rather than doing the work +// itself: that handler stops the modules, the workers and the listeners in the +// right order and closes the pool and the log file before exiting 0, and going +// through it means there is exactly one graceful-shutdown path that this route +// cannot drift from. +// +// It gets there by EMITTING the event, not by signalling the process, and that +// is not a detail. `process.kill(process.pid, 'SIGTERM')` is what this did +// first, and it works on Linux — but **Windows has no POSIX signals, and Node +// documents SIGTERM there as unconditional termination of the target process**. +// So on a Windows host the restart killed the server outright: no module +// `onShutdown`, no pool close, no log flush. Verified by running it — the +// process was gone and the shutdown handler had logged nothing. +// +// `process.on('SIGTERM', …)` is an ordinary EventEmitter listener, so +// `process.emit('SIGTERM')` invokes exactly the same handler on every platform +// without involving the OS at all. Deployment is Linux containers and would +// never have shown this; development is not. // // What brings the process BACK is the supervisor, not this. The shipped // docker-compose.yml declares `restart: unless-stopped` on `app`, which restarts @@ -372,7 +390,7 @@ function restart(req, res) { .finally(() => { // A beat, so the 202 is on the wire. `unref` so this timer is not itself // something keeping the process alive. - setTimeout(() => process.kill(process.pid, 'SIGTERM'), 250).unref() + setTimeout(() => process.emit('SIGTERM'), 250).unref() }) } diff --git a/server/test/adminModules.test.js b/server/test/adminModules.test.js index 1e482d4..46d3e00 100644 --- a/server/test/adminModules.test.js +++ b/server/test/adminModules.test.js @@ -422,42 +422,55 @@ test('an empty allowlist is storable, and means no installs', async () => { // ── restart ──────────────────────────────────────────────────────────────── -test('restart answers before it signals, and signals its own process', async () => { - // It raises SIGTERM rather than calling the shutdown path directly, so that - // server.js's handler stays the ONE graceful-shutdown path and this route - // cannot drift from it. - const originalKill = process.kill - const signals = [] - process.kill = (pid, signal) => { signals.push({ pid, signal }) } +// These listen for the SIGTERM EVENT rather than stubbing `process.kill`, and +// that is the whole point of them now. +// +// The first version of this route called `process.kill(process.pid, 'SIGTERM')` +// and the first version of these tests stubbed `process.kill` and asserted it +// had been called with SIGTERM. Both passed. Both were wrong: Windows has no +// POSIX signals, and Node documents SIGTERM there as unconditional termination — +// so on a Windows host the route killed the server outright, with no module +// `onShutdown`, no pool close and no log flush. A stub of `process.kill` cannot +// see that, because what it asserts is precisely the call whose MEANING differs +// by platform. +// +// Asserting on the event closes the gap: it is what server.js's handler is +// actually subscribed to, so a test passing here means the handler would run. +function onceSigterm() { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + process.removeListener('SIGTERM', handler) + reject(new Error('no SIGTERM was emitted within 1s')) + }, 1000) + function handler() { + clearTimeout(timer) + process.removeListener('SIGTERM', handler) + resolve(true) + } + process.on('SIGTERM', handler) + }) +} - try { - const res = mockRes() - ctrl.restart(req(), res) +test('restart answers first, then triggers the one graceful-shutdown path', async () => { + const fired = onceSigterm() + const res = mockRes() - // Answered synchronously: once the signal lands there is no listener left to - // flush a response through, so the operator would be told nothing. - assert.equal(res.statusCode, 202) - assert.equal(res.body.restarting, true) + ctrl.restart(req(), res) - await new Promise((resolve) => setTimeout(resolve, 400)) - assert.deepEqual(signals, [{ pid: process.pid, signal: 'SIGTERM' }]) - assert.equal(logged[0].action, 'module.restart') - } finally { - process.kill = originalKill - } + // Answered synchronously: once the shutdown starts there is no listener left + // to flush a response through, so the operator would be told nothing. + assert.equal(res.statusCode, 202) + assert.equal(res.body.restarting, true) + + assert.equal(await fired, true) + assert.equal(logged[0].action, 'module.restart') }) test('a failure to write the audit entry does not cancel the restart', async () => { - const originalKill = process.kill - const signals = [] - process.kill = (pid, signal) => { signals.push(signal) } activity.log = async () => { throw new Error('database is gone') } + const fired = onceSigterm() - try { - ctrl.restart(req(), mockRes()) - await new Promise((resolve) => setTimeout(resolve, 400)) - assert.deepEqual(signals, ['SIGTERM']) - } finally { - process.kill = originalKill - } + ctrl.restart(req(), mockRes()) + + assert.equal(await fired, true) }) diff --git a/server/test/modules.model.test.js b/server/test/modules.model.test.js index 4147acc..ec61263 100644 --- a/server/test/modules.model.test.js +++ b/server/test/modules.model.test.js @@ -38,7 +38,17 @@ modulesDb.getOne = async (id) => (rows.has(id) ? [rows.get(id)] : []) modulesDb.upsert = async ({ id, name, version, source, sha256 }) => { const existing = rows.get(id) if (existing) { - Object.assign(existing, { name, version, source: source ?? null, sha256: sha256 ?? null }) + // COALESCE, matching the real statement: a value overwrites, a NULL leaves + // what is there. This fake used to assign unconditionally — faithfully + // reproducing the defect it was supposed to be able to catch, which is why + // the boot refresh nulling an install's provenance survived until a browser + // showed it. + Object.assign(existing, { + name, + version, + source: source ?? existing.source ?? null, + sha256: sha256 ?? existing.sha256 ?? null, + }) return { affectedRows: 1 } } rows.set(id, { @@ -116,6 +126,46 @@ test('a hand-placed module records with no provenance', async () => { assert.equal(mod.sha256, null) }) +test('the boot refresh does not wipe an install\'s provenance', async () => { + // The defect a browser found in Phase 4, and the reason the upsert COALESCEs. + // + // lifecycle.boot() re-records every scanned module with NO source and NO + // sha256, because a hand-placed directory genuinely has neither. That refresh + // used to write both columns as NULL, so an admin-panel install's provenance + // survived exactly until the restart the install asked for — and the screen + // then described a module installed from a URL as "placed on the volume by + // hand". Nothing could see it until Phase 4: no caller had ever passed a + // non-null value. + await modules.recordInstalled({ + id: 'uo', + name: 'Ultima Online', + version: '1.0.0', + source: 'https://gitea.example/x/uo-1.0.0.json', + sha256: 'b'.repeat(64), + }) + + // What the next boot does. + const after = await modules.recordInstalled({ id: 'uo', name: 'Ultima Online', version: '1.0.0' }) + + assert.equal(after.source, 'https://gitea.example/x/uo-1.0.0.json') + assert.equal(after.sha256, 'b'.repeat(64)) +}) + +test('a re-install from a new URL does replace the provenance', async () => { + // The other half: COALESCE must not make the columns write-once, or an upgrade + // would for ever show where the FIRST version came from. + await modules.recordInstalled({ + id: 'uo', name: 'Ultima Online', version: '1.0.0', source: 'https://old/x.json', sha256: 'c'.repeat(64), + }) + const after = await modules.recordInstalled({ + id: 'uo', name: 'Ultima Online', version: '2.0.0', source: 'https://new/y.json', sha256: 'd'.repeat(64), + }) + + assert.equal(after.source, 'https://new/y.json') + assert.equal(after.sha256, 'd'.repeat(64)) + assert.equal(after.version, '2.0.0') +}) + test('recordInstalled refuses a manifest missing id, name or version', async () => { await assert.rejects( () => modules.recordInstalled({ id: 'uo', version: '1.0.0' }),