diff --git a/server/src/engagement/coreRules.js b/server/src/engagement/coreRules.js index 8d159b8..e819f3d 100644 --- a/server/src/engagement/coreRules.js +++ b/server/src/engagement/coreRules.js @@ -159,8 +159,15 @@ const NEWS_RULES = [ async function seedGroup(key, rules, note) { const summary = { inserted: 0, skipped: 0 } try { - const seen = await settingsDb.get(key) - if (seen) return { ...summary, skipped: rules.length } + // **Claimed BEFORE the loop, atomically**, and the stamp is the claim. A + // `get()` here with a `set()` after the inserts is not a guard when two + // instances boot together — both read "absent", both seed — and a duplicate + // rule is two mails per event. `claim()` is an `INSERT IGNORE` reporting its + // own `affectedRows`, so exactly one caller proceeds. See the note below on + // what a partial run costs: that trade is unchanged, only its ordering. + if (!(await settingsDb.claim(key, new Date().toISOString()))) { + return { ...summary, skipped: rules.length } + } for (const rule of rules) { try { @@ -181,10 +188,10 @@ async function seedGroup(key, rules, note) { log.error('team rule seed failed', { trigger: rule.trigger_id, message: err.message }) } } - // Stamped even on a partial run. Re-running would duplicate the rules that - // did insert, and a duplicate rule is two mails per event — a worse outcome - // than the one missing rule an operator can add from the screen. - await settingsDb.set(key, new Date().toISOString()) + // Stamped even on a partial run — the claim above is the stamp. Re-running + // would duplicate the rules that did insert, and a duplicate rule is two + // mails per event, a worse outcome than the one missing rule an operator can + // add from the screen. if (summary.inserted) { log.info('seeded engagement rules, all disabled', { rules: summary.inserted, note }) } diff --git a/server/src/engagement/moduleSeeds.js b/server/src/engagement/moduleSeeds.js index 06e62b6..3c09bdc 100644 --- a/server/src/engagement/moduleSeeds.js +++ b/server/src/engagement/moduleSeeds.js @@ -100,10 +100,11 @@ async function seedModuleTemplates(owner, templates, deps = {}) { /** * Seed one named rule group, once, under its own guard. * - * Mirrors `coreRules.seedGroup` deliberately, including the stamp-on-partial - * behaviour: re-running would duplicate the rules that DID insert, and a - * duplicate rule is two mails per event — worse than the one missing rule an - * operator can add from the Rules screen. + * Mirrors `coreRules.seedGroup` deliberately, including the claim-before-insert + * ordering and what it costs: re-running would duplicate the rules that DID + * insert, and a duplicate rule is two mails per event — worse than the one + * missing rule an operator can add from the Rules screen. The guard is taken + * atomically for the same reason; see the comment at the claim. */ async function seedRuleGroup(owner, group, deps = {}) { const rules_ = deps.rulesDb || rulesDb @@ -111,8 +112,23 @@ async function seedRuleGroup(owner, group, deps = {}) { const summary = { inserted: 0, skipped: 0 } const key = guardKey(owner, group.key) try { - const seen = await settings_.get(key) - if (seen) return { ...summary, skipped: group.rules.length } + // **Claim BEFORE inserting, not after.** The guard used to be a `get()` here + // and a `set()` after the loop, which is not a guard under concurrency: two + // processes starting in the same moment both read "absent" and both insert + // the whole group. That is not hypothetical — `docker compose up + // --scale app=2` and a rolling restart both boot two instances deliberately, + // and Phase 13's acceptance walk hit it with two, ending up with 52 module + // rules where the module ships 26. `claim()` is an `INSERT IGNORE` reporting + // its own `affectedRows`, so exactly one caller wins. + // + // The cost is the one this function already accepted below: a process that + // dies mid-loop leaves the group stamped and partly seeded, and the missing + // rules are an operator's visit to the "new rule" form. Duplicates are the + // worse failure — two mails per event, for every rule in the group — which + // is why the order is this way round rather than the other. + if (!(await settings_.claim(key, new Date().toISOString()))) { + return { ...summary, skipped: group.rules.length } + } for (const rule of group.rules) { try { @@ -127,7 +143,6 @@ async function seedRuleGroup(owner, group, deps = {}) { }) } } - await settings_.set(key, new Date().toISOString()) if (summary.inserted) { log.info('seeded module engagement rules, all disabled', { owner, diff --git a/server/src/model/engagement/engagementRules.model.js b/server/src/model/engagement/engagementRules.model.js index c09fa9c..00befe6 100644 --- a/server/src/model/engagement/engagementRules.model.js +++ b/server/src/model/engagement/engagementRules.model.js @@ -40,6 +40,12 @@ const MAX_DELAY_SECONDS = 86_400 // what makes rules-as-data safe (§7.1 Q3). const MAX_SENDS_PER_HOUR = 10_000 +// The one key in `template_keys` that is not a delivery channel. `teamDigestWorker` +// renders it for a rule whose email channel an individual has set to digest mode, +// so it belongs to a MODE rather than to the rule's channel list and can never +// appear there. +const DIGEST_SLOT = 'digest' + const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v) /** @@ -91,7 +97,17 @@ async function validate(input, { existing = null } = {}) { errors.push('templateKeys must be an object of { channel: templateKey }') } else { for (const [channel, key] of Object.entries(raw.templateKeys || {})) { - if (!wanted.includes(channel)) { + // **`digest` is a template SLOT, not a channel**, and it is legal here for + // exactly the reason `registries.js` `checkSeedRule` says it is: it names + // the body `teamDigestWorker` renders for a rule whose email channel an + // individual has set to digest mode, so it never appears in `channels` and + // never could. Rejecting it made every rule that ships one unsaveable from + // the Rules screen — core's own team and news rules included, and sixteen + // of module-uo's — with a 400 naming a key the operator never typed, whose + // only remedy was deleting the digest body and silently dropping digest + // support. Found by Phase 13's acceptance walk; the two validators now + // agree about what `digest` is. + if (channel !== DIGEST_SLOT && !wanted.includes(channel)) { errors.push(`templateKeys names "${channel}", which is not one of this rule's channels`) continue } diff --git a/server/src/model/settings/settings.db.js b/server/src/model/settings/settings.db.js index 290b6f4..f2b17bb 100644 --- a/server/src/model/settings/settings.db.js +++ b/server/src/model/settings/settings.db.js @@ -36,6 +36,24 @@ async function seedDefault(key, value) { await query('INSERT IGNORE INTO settings (`key`, value) VALUES (?, ?)', [key, value]) } +/** + * Take a one-shot guard, atomically. `true` means THIS caller wrote the row. + * + * The same `INSERT IGNORE` as `seedDefault`, and the difference is the whole + * point: this one reports whether it won. A guard read with `get()` and written + * later with `set()` is not a guard at all under concurrency — two processes + * both read "absent" and both proceed — and this is used where proceeding twice + * means seeding a rule group twice, i.e. two mails per event. + * + * The atomicity is the PRIMARY KEY's: exactly one INSERT can create a given + * `key`, so exactly one caller sees `affectedRows === 1`. No transaction and no + * lock, the same bargain `engagementWorker`'s claim makes. + */ +async function claim(key, value) { + const res = await query('INSERT IGNORE INTO settings (`key`, value) VALUES (?, ?)', [key, value]) + return Number(res && res.affectedRows) === 1 +} + // Delete a settings row. "Reset to defaults" for the theming/nav keys is the // *absence* of a row, not a stored copy of the defaults — see // docs/website/THEMING_AND_NAV.md §2. Deleting a key that was never set is a @@ -44,4 +62,4 @@ async function remove(key) { await query('DELETE FROM settings WHERE `key` = ?', [key]) } -module.exports = { getAll, get, getRow, set, seedDefault, remove } +module.exports = { getAll, get, getRow, set, seedDefault, claim, remove } diff --git a/server/test/engagementEmail.test.js b/server/test/engagementEmail.test.js index edbaa35..bb0a9f3 100644 --- a/server/test/engagementEmail.test.js +++ b/server/test/engagementEmail.test.js @@ -371,8 +371,7 @@ test('a GET on the unsubscribe endpoint mutates nothing and lands on the page', // ── The seeded rules (decision 3) ────────────────────────────────────────── test('core seeds a rule for each Team trigger, and every one of them is OFF', async () => { - patch(settingsDb, 'get', async () => null) - patch(settingsDb, 'set', async (k, v) => world.settings.set(k, v)) + patch(settingsDb, 'claim', async (k, v) => { world.settings.set(k, v); return true }) patch(rulesDb, 'insert', async (rule) => { world.inserted.push(rule); return world.inserted.length }) const summary = await coreRules.seedTeamRules() @@ -400,9 +399,32 @@ test('every seeded rule names a template that actually exists', () => { // Seeded once, not ensured: an operator who deletes a rule must not find it back // after a restart, and one they enabled must not be reset to off. test('a second boot seeds nothing', async () => { - patch(settingsDb, 'get', async () => '2026-08-29T00:00:00.000Z') + // The guard is a CLAIM, so "already seeded" is the claim losing rather than a + // read finding a row. It is the same one-shot promise, made atomically: two + // instances booting together used to both read "absent" and both seed, and a + // duplicate rule is two mails per event (Phase 13's acceptance walk). + patch(settingsDb, 'claim', async () => false) patch(rulesDb, 'insert', async () => { throw new Error('must not insert') }) const summary = await coreRules.seedTeamRules() assert.equal(summary.inserted, 0) assert.equal(summary.skipped, 4) }) + +test('two instances booting together seed the Team rules once, not twice', async () => { + // Not awaited in turn on purpose: interleaving the two calls is the test, and + // awaiting the first would pass against the read-then-write guard this + // replaced. `claim` decides and writes without yielding, exactly as the + // settings table's PRIMARY KEY does. + const claimed = new Set() + patch(settingsDb, 'claim', async (k) => { + if (claimed.has(k)) return false + claimed.add(k) + return true + }) + patch(rulesDb, 'insert', async (rule) => { world.inserted.push(rule); return world.inserted.length }) + + const [a, b] = await Promise.all([coreRules.seedTeamRules(), coreRules.seedTeamRules()]) + + assert.equal(a.inserted + b.inserted, 4) + assert.equal(world.inserted.length, 4) +}) diff --git a/server/test/engagementEngine.test.js b/server/test/engagementEngine.test.js index e8a3d61..613ef6b 100644 --- a/server/test/engagementEngine.test.js +++ b/server/test/engagementEngine.test.js @@ -627,6 +627,44 @@ test('the hourly ceiling counts sends, not attempts', async () => { assert.equal(result.enqueued, 1) }) +// ── templateKeys: what a channel is, and what `digest` is ────────────────── + +test('a rule may name a `digest` body, which is a template slot rather than a channel', async () => { + // The defect Phase 13's acceptance walk found. `registries.js` `checkSeedRule` + // permits `digest` in as many words — it is the body `teamDigestWorker` + // renders for a rule whose email channel an individual set to digest mode, so + // it never appears in `channels` and never could — and core's own Team and + // news rules ship one, as do sixteen of module-uo's. This validator rejected + // it, so every one of those rules answered an operator who opened it and + // pressed Save with a 400 naming a key they had never typed, and the only way + // to save was to delete the digest body. + const checked = await rules.validate({ + triggerId: 'uo.house.idoc_warning', + name: 'IDOC warning', + channels: ['email'], + audience: 'owner', + templateKeys: { email: 'notify.event', digest: 'notify.digest' }, + }) + + assert.equal(checked.ok, true, checked.errors && checked.errors.join(' ')) + assert.equal(checked.rule.template_keys.digest, 'notify.digest') +}) + +test('a templateKeys entry that is neither a channel nor `digest` is still refused', async () => { + // The rule that was right all along, kept: `digest` is one named exception + // with a renderer behind it, not a hole that admits any word. + const checked = await rules.validate({ + triggerId: 'uo.house.idoc_warning', + name: 'IDOC warning', + channels: ['email'], + audience: 'owner', + templateKeys: { email: 'notify.event', carrierpigeon: 'notify.event' }, + }) + + assert.equal(checked.ok, false) + assert.match(checked.errors.join(' '), /carrierpigeon/) +}) + // ── Ceilings: the security boundary, both halves ─────────────────────────── test('a rule may not be SAVED with an audience wider than its trigger permits', async () => { diff --git a/server/test/moduleEngagementSeeds.test.js b/server/test/moduleEngagementSeeds.test.js index 6be47e6..4ff0c18 100644 --- a/server/test/moduleEngagementSeeds.test.js +++ b/server/test/moduleEngagementSeeds.test.js @@ -64,6 +64,26 @@ function trySeeds(owner, seeds) { // ── The registry: what a module may and may not ship ─────────────────────── +/** + * The settings table, as far as these tests need it — and specifically its + * ATOMICITY. `claim` is the real `INSERT IGNORE`: it decides and writes without + * yielding, so exactly one caller can win a key, which is the property the guard + * depends on. A fake that read with `get` and wrote later with `set` could never + * express that, and agreed with the bug Phase 13's walk found. + */ +function fakeSettings(store = new Map()) { + return { + store, + get: async (k) => store.get(k) || null, + set: async (k, v) => { store.set(k, v) }, + claim: async (k, v) => { + if (store.has(k)) return false + store.set(k, v) + return true + }, + } +} + test('a well-formed batch registers and reads back under its owner', () => { assert.equal(trySeeds('demo', { templates: [tpl()], @@ -225,10 +245,7 @@ test('templates are re-ensured every run and rule groups are seeded once', async staleCustomized: async () => [], }, rulesDb: { insert: async (r) => { insertedRules.push(r.trigger_id) } }, - settingsDb: { - get: async (k) => settings.get(k) || null, - set: async (k, v) => { settings.set(k, v) }, - }, + settingsDb: fakeSettings(settings), } await moduleSeeds.seedModuleEngagement({ seeds, ...stub }) @@ -259,16 +276,46 @@ test('a partial rule group is still stamped', async () => { if (inserts === 2) throw new Error('duplicate') }, }, - settingsDb: { - get: async (k) => settings.get(k) || null, - set: async (k, v) => { settings.set(k, v) }, - }, + settingsDb: fakeSettings(settings), }) assert.equal(inserts, 2) assert.ok(settings.has(moduleSeeds.guardKey('demo', 'v1'))) }) +test('two instances booting together seed the group once, not twice', async () => { + // The defect Phase 13's acceptance walk found, and the reason the guard is a + // CLAIM rather than a read followed by a write. `docker compose up + // --scale app=2` and a rolling restart both start two instances on purpose; + // under the old ordering both read "not seeded" before either stamped, and + // both inserted the whole group. The walk ended up with 52 module rules where + // the module ships 26 — and a duplicate rule is two mails per event. + // + // The two calls are deliberately NOT awaited in turn: interleaving them is the + // whole test, and awaiting the first would pass against the bug. + const seeds = registered('demo', { + templates: [tpl()], + ruleGroups: [{ key: 'v1', rules: [rule(), rule({ trigger_id: 'demo.house.gone', name: 'Gone' })] }], + }) + + const settings = fakeSettings() + const inserted = [] + const stub = { + seeds, + templatesDb: { seedOne: async () => 'inserted', staleCustomized: async () => [] }, + rulesDb: { insert: async (r) => { inserted.push(r.trigger_id) } }, + settingsDb: settings, + } + + await Promise.all([ + moduleSeeds.seedModuleEngagement({ ...stub }), + moduleSeeds.seedModuleEngagement({ ...stub }), + ]) + + assert.deepEqual(inserted, ['demo.house.warning', 'demo.house.gone']) + assert.ok(settings.store.has(moduleSeeds.guardKey('demo', 'v1'))) +}) + test('a skipped owner is seeded not at all', async () => { // The operator's switch means what it says even for content that is only rows. const seeds = registered('demo', { @@ -281,7 +328,7 @@ test('a skipped owner is seeded not at all', async () => { skip: new Set(['demo']), templatesDb: { seedOne: async () => { touched += 1; return 'inserted' }, staleCustomized: async () => [] }, rulesDb: { insert: async () => { touched += 1 } }, - settingsDb: { get: async () => null, set: async () => {} }, + settingsDb: fakeSettings(), }) assert.equal(touched, 0) }) @@ -298,7 +345,7 @@ test('a database failure is logged, never thrown — this is the boot path', asy staleCustomized: async () => { throw new Error('also gone') }, }, rulesDb: { insert: async () => { throw new Error('gone too') } }, - settingsDb: { get: async () => { throw new Error('and gone') }, set: async () => {} }, + settingsDb: { claim: async () => { throw new Error('and gone') }, set: async () => {} }, }) }) @@ -313,7 +360,7 @@ test('an invalid block array is refused rather than stored', async () => { seeds, templatesDb: { seedOne: async () => { stored += 1; return 'inserted' }, staleCustomized: async () => [] }, rulesDb: { insert: async () => {} }, - settingsDb: { get: async () => null, set: async () => {} }, + settingsDb: fakeSettings(), }) assert.equal(stored, 0) assert.equal(totals.templates, 0)