fix(modules): three defects a real install exposed (phase 4, slice 1)

Standing the slice-2 screen up against a live server and installing the
published module-uo v0.3.0 through it found three things, none of which any
unit test in this repo could have caught. Two of them are older than this
phase.

1. The boot refresh nulled every install's provenance
--------------------------------------------------------
`installed_modules.source` and `.sha256` exist so the admin panel can say
where a module came from. They never survived a restart.

`lifecycle.boot()` re-records every scanned module with no source and no
sha256 -- correctly, because a scan finds a directory and never where it came
from -- and `upsert` assigned both columns unconditionally. So an install's
provenance lasted exactly until the restart that install asked for, and the
screen then described a module installed from a URL as "placed on the volume
by hand". Verified live: install, restart, provenance gone.

Nothing could have caught it before now. Phase 4 wrote the first non-null
value these columns had ever had, so lifecycle.js's comment asserting that
"recordInstalled leaves what it is not given" described an intention rather
than the statement below it -- and modules.model.test.js's fake reproduced
the defect faithfully, assigning unconditionally just like the SQL.

Fixed with COALESCE(VALUES(col), col): a value overwrites, a NULL leaves what
is there. The fake now matches, and two tests pin both directions -- a boot
refresh must not wipe it, and a re-install from a new URL must still replace
it, or the column would become write-once and an upgrade would for ever show
where the first version came from.

2. The restart killed the server on Windows instead of stopping it
------------------------------------------------------------------
The route called `process.kill(process.pid, 'SIGTERM')` to reach server.js's
graceful-shutdown handler. That works on Linux. **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 listener close, no pool close, no log
flush. Observed exactly that: the process was gone and the shutdown handler
had logged nothing at all.

`process.on('SIGTERM', ...)` is an ordinary EventEmitter listener, so
`process.emit('SIGTERM')` reaches the same handler on every platform without
involving the OS. One shutdown path, still; it just gets there by an event.

Deployment is Linux containers and would never have shown this. Development
is not, and neither is the smoke that found it.

The test was worse than useless: it stubbed `process.kill` and asserted it
had been called with SIGTERM, which is precisely the call whose MEANING
differs by platform. It now waits for the SIGTERM EVENT -- what server.js is
actually subscribed to -- so a pass here means the handler would run.

3. `present()` did not publish the running version
--------------------------------------------------
An upgrade writes new files and a new row while the old code stays loaded, so
the row's version is a promise about the next boot rather than a description
of this one. Adds `liveVersion` from the loader beside `liveState`, so the
screen can tell the two apart instead of reporting the new version as running.

723 server tests (+2), manifest and OpenAPI both unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>

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),
+       source  = COALESCE(VALUES(source), source),
-       sha256  = VALUES(sha256)`,
+       sha256  = COALESCE(VALUES(sha256), sha256)`,
     [id, name, version, source ?? null, sha256 ?? null],
   )
 

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
+      // Null provenance is what a hand-placed directory looks like, and it is
-      // performed through the admin panel (§2.5, a later phase) writes the row
+      // all this step can honestly say: a scan finds a directory, never where it
-      // with its source and hash first; this refresh deliberately does not
+      // came from. An install through the admin panel writes source and sha256
-      // overwrite either, because recordInstalled leaves what it is not given.
+      // 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.
     }))
   }
 

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
+// It reaches server.js's existing SIGTERM handler rather than doing the work
-// path directly. server.js already has a handler that stops the modules, the
+// itself: that handler stops the modules, the workers and the listeners in the
-// workers and the listeners in the right order and closes the pool and the log
+// right order and closes the pool and the log file before exiting 0, and going
-// file before exiting 0; reaching that through the signal means there is exactly
+// through it means there is exactly one graceful-shutdown path that this route
-// one graceful-shutdown path and this route cannot drift from it.
+// 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()
     })
 }
 

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 () => {
+// These listen for the SIGTERM EVENT rather than stubbing `process.kill`, and
-  // It raises SIGTERM rather than calling the shutdown path directly, so that
+// that is the whole point of them now.
-  // server.js's handler stays the ONE graceful-shutdown path and this route
+//
-  // cannot drift from it.
+// The first version of this route called `process.kill(process.pid, 'SIGTERM')`
-  const originalKill = process.kill
+// and the first version of these tests stubbed `process.kill` and asserted it
-  const signals = []
+// had been called with SIGTERM. Both passed. Both were wrong: Windows has no
-  process.kill = (pid, signal) => { signals.push({ pid, signal }) }
+// 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 {
+test('restart answers first, then triggers the one graceful-shutdown path', async () => {
+  const fired = onceSigterm()
   const res = mockRes()
+
   ctrl.restart(req(), res)
 
-    // Answered synchronously: once the signal lands there is no listener left to
+  // Answered synchronously: once the shutdown starts there is no listener left
-    // flush a response through, so the operator would be told nothing.
+  // to flush a response through, so the operator would be told nothing.
   assert.equal(res.statusCode, 202)
   assert.equal(res.body.restarting, true)
 
-    await new Promise((resolve) => setTimeout(resolve, 400))
+  assert.equal(await fired, true)
-    assert.deepEqual(signals, [{ pid: process.pid, signal: 'SIGTERM' }])
   assert.equal(logged[0].action, 'module.restart')
-  } finally {
-    process.kill = originalKill
-  }
 })
 
 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'])
+  assert.equal(await fired, true)
-  } finally {
-    process.kill = originalKill
-  }
 })

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' }),