server/db/purge.sql

@@ -21,7 +21,11 @@
 -- `notification_subs` rows for `shard.*` streams and `announce_job_legs` rows
 -- with leg `towncrier` belong to core's tables, and a module does not delete
 -- from those — core prunes them when it drops the registrations, which it can
--- do because it knows which registrant owned what.
+-- do because it knows which registrant owned what. The two `settings` rows
+-- schema.sql seeds (`game_account_signup`, `uo_link_protocol_3_migrated`) are
+-- the same case with an extra reason: the second is a one-shot MIGRATION
+-- marker, and deleting it would re-arm a protocol bump against tables this
+-- file has just dropped.
 
 DROP TABLE IF EXISTS `shard_atlas_pending`;
 DROP TABLE IF EXISTS `shard_atlas_meta`;

server/db/schema.sql

@@ -615,3 +615,30 @@ ALTER TABLE uo_link_config MODIFY COLUMN protocol INT NOT NULL DEFAULT 3;
 UPDATE uo_link_config SET protocol = 3
   WHERE id = 1 AND protocol < 3
     AND NOT EXISTS (SELECT 1 FROM settings WHERE `key` = 'uo_link_protocol_3_migrated');
+-- **The marker must be written HERE, not in core.** These two statements were
+-- adjacent in core's schema.sql before the extraction; slice 1 moved the UPDATE
+-- and left the INSERT behind, and the two files do not run at the same time —
+-- core's schema is replayed in full BEFORE any module fragment (MODULE_API.md
+-- §2.6). So the marker existed before the UPDATE ever read it, the NOT EXISTS
+-- was true on the first boot of a fresh install and false on every boot of an
+-- upgraded one, and the one-shot could never fire. An install carrying a
+-- protocol-2 row would have stayed pinned at 2 against a v3 sidecar — every
+-- REST call 409, which is precisely the failure this migration exists to
+-- prevent. Latent rather than live: it only bites an install that first boots a
+-- post-slice-1 build while already holding a uo_link_config row, and `edge` has
+-- not cut over yet.
+INSERT IGNORE INTO settings (`key`, value) VALUES ('uo_link_protocol_3_migrated', '1');
+
+-- ── Settings rows this module owns ─────────────────────────────────────────
+--
+-- Both keys predate the module system and both name a game concept, so core
+-- seeding them made core's schema declare a module's settings — the structural
+-- half of what Phase 3 removes (MODULE_SYSTEM.md §2.7.1, slice 4). The KEYS are
+-- deliberately unchanged: they are live rows on every existing install, and
+-- renaming one would silently reset an operator's choice to the default.
+--
+-- INSERT IGNORE, so an install that already carries the row keeps its value and
+-- only a database that has never seen the key gets the default. Nothing in core
+-- reads either one; `game_account_signup` is read through ctx.settings by
+-- server/utils/gameSignup.js, which owns the policy.
+INSERT IGNORE INTO settings (`key`, value) VALUES ('game_account_signup', 'disabled');

server/test/schemaFragment.test.js

@@ -0,0 +1,157 @@
+// `schema.sql` is replayed by core on EVERY boot, and core validates it before
+// anything mounts. The rules are core's (MODULE_API.md §2.6, loader.js), and are
+// restated here for the same reason `manifest.test.js` restates the manifest
+// rules: a mistake should fail in this repo's CI, which can say what is wrong,
+// rather than on an install, where the symptom is a module that is simply absent.
+//
+// The test this file exists for is the ORDER one. Two statements that read each
+// other were adjacent in core's schema.sql until slice 1 moved one of them here
+// and left the other behind — and because core's schema is replayed in full
+// before any module fragment, the marker was written before the migration that
+// reads it and the one-shot could never fire. Nothing caught it: both files were
+// individually valid SQL, both replayed cleanly, and the failure only shows on an
+// upgraded install talking to a real sidecar. An assertion about order is the
+// only thing that would have.
+
+const test = require('node:test')
+const assert = require('node:assert')
+const fs = require('node:fs')
+const path = require('node:path')
+
+const SCHEMA = path.join(__dirname, '..', 'db', 'schema.sql')
+const sql = fs.readFileSync(SCHEMA, 'utf8')
+
+/**
+ * Split into statements the way core's `utils/sqlStatements.js` does: a
+ * character walk, not a regexp.
+ *
+ * Comments are stripped before quotes are considered, because a comment may
+ * contain quotes — line 508 of this very file is `-- '' when randomised per
+ * activation`, and a stripper that opened a string there would swallow the rest
+ * of the file. The reverse case (a `--` inside a string literal) is handled by
+ * the same walk, since a quote opened outside a comment stays open.
+ */
+function splitStatements(text) {
+  const out = []
+  let buf = ''
+  let quote = null
+  for (let i = 0; i < text.length; i++) {
+    const c = text[i]
+    if (quote) {
+      buf += c
+      if (c === '\\') {
+        buf += text[++i] ?? ''
+      } else if (c === quote) {
+        quote = null
+      }
+      continue
+    }
+    if (c === '-' && text[i + 1] === '-') {
+      while (i < text.length && text[i] !== '\n') i++
+      buf += '\n'
+      continue
+    }
+    if (c === "'" || c === '"' || c === '`') {
+      quote = c
+      buf += c
+      continue
+    }
+    if (c === ';') {
+      if (buf.trim()) out.push(buf.trim())
+      buf = ''
+      continue
+    }
+    buf += c
+  }
+  if (buf.trim()) out.push(buf.trim())
+  return out
+}
+
+const statements = splitStatements(sql)
+
+// Core's leading-verb allowlist. Not a DROP denylist: this file replays on every
+// boot, so a TRUNCATE or DELETE would empty a table at each restart.
+const ALLOWED = ['CREATE', 'ALTER', 'INSERT', 'UPDATE']
+
+test('the splitter survives a comment that contains quotes', () => {
+  const parts = splitStatements("SELECT 1; -- '' a quote in a comment\nSELECT 2;")
+  assert.deepEqual(
+    parts.map((s) => s.trim().split('\n')[0].trim()),
+    ['SELECT 1', 'SELECT 2'],
+  )
+})
+
+test('the splitter does not treat a -- inside a string as a comment', () => {
+  const parts = splitStatements("INSERT INTO t VALUES ('a--b');")
+  assert.equal(parts.length, 1)
+  assert.match(parts[0], /'a--b'/)
+})
+
+test('every statement leads with a verb core allows', () => {
+  for (const statement of statements) {
+    const verb = statement.trim().split(/\s+/)[0].toUpperCase()
+    assert.ok(ALLOWED.includes(verb), `statement leads with "${verb}": ${statement.slice(0, 70)}`)
+  }
+})
+
+test('every CREATE TABLE is IF NOT EXISTS', () => {
+  for (const statement of statements) {
+    if (!/^CREATE\s+TABLE/i.test(statement)) continue
+    assert.match(statement, /^CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS/i, statement.slice(0, 70))
+  }
+})
+
+test('every table this fragment declares is prefixed shard_ or uo_link_', () => {
+  // The two prefixes core grandfathers to this module by name
+  // (loader.js LEGACY_TABLE_PREFIXES). A module written after this one prefixes
+  // with its own id instead.
+  const CREATE_TABLE = /CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\s+`?([A-Za-z0-9_]+)`?/gi
+  const tables = []
+  for (const statement of statements) {
+    for (const m of statement.matchAll(CREATE_TABLE)) tables.push(m[1].toLowerCase())
+  }
+  assert.ok(tables.length > 20, `expected the module's tables, found ${tables.length}`)
+  for (const table of tables) {
+    assert.ok(
+      table.startsWith('shard_') || table.startsWith('uo_link_'),
+      `table "${table}" carries neither grandfathered prefix`,
+    )
+  }
+})
+
+// ── The settings rows this module owns ──────────────────────────────────────
+
+const SETTINGS_KEYS = ['game_account_signup', 'uo_link_protocol_3_migrated']
+
+test('both settings seeds are INSERT IGNORE, so a replay never resets a value', () => {
+  for (const key of SETTINGS_KEYS) {
+    const seed = statements.find((s) => /^INSERT/i.test(s) && s.includes(`'${key}'`))
+    assert.ok(seed, `no seed for "${key}"`)
+    assert.match(seed, /^INSERT\s+IGNORE\s+INTO\s+settings/i, key)
+  }
+})
+
+test('the protocol-3 marker is written AFTER the update that reads it', () => {
+  const update = statements.findIndex(
+    (s) => /^UPDATE\s+uo_link_config/i.test(s) && s.includes('uo_link_protocol_3_migrated'),
+  )
+  const marker = statements.findIndex(
+    (s) => /^INSERT/i.test(s) && s.includes("'uo_link_protocol_3_migrated'"),
+  )
+  assert.ok(update >= 0, 'the protocol-3 migration is gone')
+  assert.ok(marker >= 0, 'the one-shot marker is gone')
+  assert.ok(
+    marker > update,
+    'the marker is written before the UPDATE reads it — the one-shot can never fire. ' +
+      'This is the shape of the defect slice 1 introduced by leaving the marker in core, ' +
+      'whose schema replays first.',
+  )
+})
+
+test('the migration is guarded on the marker, not on the column value alone', () => {
+  const update = statements.find((s) => /^UPDATE\s+uo_link_config/i.test(s))
+  assert.match(update, /NOT\s+EXISTS\s*\(\s*SELECT/i)
+  // Without the guard an operator who deliberately pins an older sidecar in
+  // Admin → Shard is silently re-bumped on the next restart.
+  assert.match(update, /uo_link_protocol_3_migrated/)
+})