fix(engagement): claim the seed guard atomically, and let a rule name a digest body
Both defects came out of Phase 13's acceptance walk, and neither was visible to any test. **1. The one-shot rule-group guard was not a guard.** `seedRuleGroup` and `coreRules.seedGroup` both read their settings stamp, inserted the whole group, and wrote the stamp AFTER the loop. Two instances booting in the same moment both read "absent" and both insert -- the walk ended up with 52 module rules where the module ships 26. `docker compose up --scale app=2` and a rolling restart both start two instances on purpose, so this is ordinary rather than exotic. `settings.db` gains `claim(key, value)`: the same `INSERT IGNORE` as `seedDefault`, reporting its own `affectedRows`, so exactly one caller can win a key. The atomicity is the PRIMARY KEY's -- no transaction, no lock, the same bargain `engagementWorker`'s row claim already makes. Both seeders claim before inserting. The trade the code already documented is unchanged, only its order: 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. A duplicate rule is two mails per event, for every rule in the group, which both functions' own comments already call the worse outcome. **Why no test caught it:** the stubs supplied `get` and `set` over a Map, which cannot race, so they agreed with the bug -- Phase 4a's `foundRows` finding in a different costume. The fake is now `claim`-shaped and decides without yielding, exactly as the table does, and both suites gained a test that runs two seeders with `Promise.all` and asserts one insert each. Verified by reverting the fix: the module test then reports every rule inserted twice. **2. A rule that names a `digest` body could not be saved.** `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 MODULE_API.md 2.4 tells modules they may point `template_keys` at `notify.digest`. `engagementRules.model.js` then rejected any key that was not one of the rule's channels. So every rule shipping a digest body answered an operator who opened it and pressed Save with a 400 naming a key they had never typed: core's own Team and news rules, and sixteen of module-uo's. The only way to save was to delete the digest body, silently dropping digest support from that rule. The two validators now agree; anything that is neither a channel nor `digest` is still refused, with a test for each direction. No MODULE_API bump: no member is added, removed or changed, and the documented contract is what the code now honours rather than something new. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -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 })
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user