diff --git a/website/BACKEND_DESIGN.md b/website/BACKEND_DESIGN.md index 2c57745..ae06cec 100644 --- a/website/BACKEND_DESIGN.md +++ b/website/BACKEND_DESIGN.md @@ -491,6 +491,131 @@ is only harmless while the default is off. Existing subscriptions are carried ac `INSERT IGNORE … SELECT` backfill in `schema.sql`, replay-safe on every boot like the `announce_jobs → announce_job_legs` one it copies. +### engagement_rules — the operator's configuration (engagement phase 4a) +| col | type | notes | +|---|---|---| +| id | INT AUTO_INCREMENT PK | | +| trigger_id | VARCHAR(96) NOT NULL | a declared trigger id. **No FK and no existence check** — a trigger is declared in code, so a rule naming one no module currently registers is *dormant*, never deleted ([`ENGAGEMENT.md`](ENGAGEMENT.md) §7.3) | +| name | VARCHAR(160) NOT NULL | | +| enabled | TINYINT(1) NOT NULL DEFAULT **0** | off by default, so no import, seed or restore can start mailing on its own (§7.1 Q3) | +| audience | VARCHAR(32) NOT NULL DEFAULT 'owner' | a ceiling name — `owner` / `staff` / `subscribers` / `members` / `authenticated` / `everyone` | +| audience_segment_id | INT NULL | a composed segment (§5.1a). **Deliberately no FK** — see below | +| max_sends_per_hour | INT NOT NULL DEFAULT 100 | the hard per-rule ceiling (§7.1 Q3), counted in `engagement_sends` and enforced before an outbox row is written | +| channels | JSON NOT NULL | `['email','inapp']` — a rule may span channels | +| template_keys | JSON NOT NULL | `{ email: 'idoc-warning' }`. Keys are shape-checked, not existence-checked: templates are Phase 5 | +| conditions | JSON NULL | a small closed and/or/not grammar over the trigger's **declared** variables | +| cooldown_seconds | INT NOT NULL DEFAULT 0 | 0 = no cooldown | +| delay_seconds | INT NOT NULL DEFAULT 0 | the grace window (§4.2a) | +| cancel_on | JSON NULL | trigger ids that cancel a pending row for the same subject | +| updated_by | INT NULL FK→users(id) ON DELETE SET NULL | | +| created_at / updated_at | DATETIME | | + +`INDEX(trigger_id, enabled)` — the engine's one indexed read per emit. + +**`audience_segment_id` carries no foreign key on purpose.** The two options a database offers are +both wrong here: `ON DELETE CASCADE` would delete an operator's rules, and `ON DELETE SET NULL` would +silently fall the rule back to its plain `audience` column — and that fallback reaches a **different +set of people**, which is the failure §5.1a rule 4 exists to prevent. A rule whose segment is gone is +dormant and sends nothing, and deleting a segment a rule still uses is refused in the model. + +### engagement_audience_segments — operator-composed audiences (engagement phase 4a) +| col | type | notes | +|---|---|---| +| id | INT AUTO_INCREMENT PK | | +| name | VARCHAR(160) NOT NULL | | +| expression | JSON NOT NULL | a boolean tree of module-declared audience ids + params | +| ceiling | VARCHAR(32) NOT NULL | **derived, never operator-typed** — the narrowest ceiling in the tree | +| updated_by | INT NULL FK→users(id) ON DELETE SET NULL | | +| created_at / updated_at | DATETIME | | + +**Composition narrows, never widens.** `A OR B` takes the *tighter* of the two ceilings, not the +looser: a ceiling states what an expression is allowed to reach, not what it will resolve to, so the +boolean operator's direction is irrelevant. Two incomparable ceilings have no meet and the save is +refused rather than resolved to a guess (`src/modules/ceilings.js`). `not` is legal only inside an +`and` — a complement needs a set to be taken from, and "everyone except…" is a broadcast built out of +a narrow audience — and it contributes no ceiling of its own, since excluding people cannot widen. + +The ceiling is a **stored column rather than a runtime computation** so an audit can read what a rule +was allowed to reach without re-resolving it, and so a module that later widens its own audience's +ceiling cannot retroactively widen a segment saved under the old one. + +### engagement_cooldowns — one fire per (rule, user, subject) (engagement phase 4a) +| col | type | notes | +|---|---|---| +| rule_id | INT NOT NULL FK→engagement_rules(id) ON DELETE CASCADE | | +| user_id | INT NOT NULL FK→users(id) ON DELETE CASCADE | | +| subject_key | VARCHAR(190) NOT NULL DEFAULT '' | opaque to core: a house serial, a vendor id. `''` = this rule cools per user, not per subject | +| last_fired_at | DATETIME NOT NULL | | +| fire_count | INT NOT NULL DEFAULT 1 | | + +`PRIMARY KEY(rule_id, user_id, subject_key)`, `INDEX(last_fired_at)` for a prune. + +**`subject_key` is why this is not a per-user counter.** "One IDOC mail per player per day" is the +wrong rule: a player with four houses decaying should hear about all four, once each, and cooling on +(rule, user) alone silently drops three of them. + +**The claim is two statements, not the one §4.1 originally described** — a guarded `UPDATE` (the +interval in a WHERE clause) falling back to `INSERT IGNORE` for a first fire. The single +`INSERT … ON DUPLICATE KEY UPDATE` form reads its answer out of `affectedRows`, and the mariadb +connector's default `foundRows: true` makes a no-op update report 1 rather than 0 — under which every +cooldown passes, always. See `ENGAGEMENT.md` Phase 4a. + +### engagement_outbox — the send queue (engagement phase 4a) +| col | type | notes | +|---|---|---| +| id | BIGINT AUTO_INCREMENT PK | | +| rule_id | INT NOT NULL FK→engagement_rules(id) ON DELETE CASCADE | | +| trigger_id | VARCHAR(96) NOT NULL | denormalized; survives a rule edit | +| user_id | INT NOT NULL FK→users(id) ON DELETE CASCADE | | +| channel | VARCHAR(32) NOT NULL | VARCHAR, never ENUM: the channel set is data, and a module must not require an ALTER | +| subject_key | VARCHAR(190) NOT NULL DEFAULT '' | | +| payload | JSON NOT NULL | the declared variables, snapshotted at emit | +| dedupe_key | VARCHAR(190) NULL | the emitter's replay guard; NULL never collides | +| status | ENUM('scheduled','sending','sent','failed','cancelled','suppressed') | | +| due_at | DATETIME NOT NULL | the grace window's clock, and the retry backoff's | +| attempts / last_error / sent_at | | | +| created_at / updated_at | DATETIME | `updated_at` is what a stale-claim reclaim measures | + +`UNIQUE(rule_id, user_id, channel, dedupe_key)`, `INDEX(status, due_at)`, +`INDEX(rule_id, user_id, subject_key, status)`. + +**The unique key is scoped, and a global one would have been a data-loss bug.** A dedupe key names the +*event*; one event legitimately becomes one row per (rule, user, channel), so a fifty-person audience +on two channels is a hundred rows carrying the same key. A global `UNIQUE(dedupe_key)` admits the first +and silently ignores the rest. + +**A row is claimed with a compare-and-set** — `UPDATE … SET status='sending' WHERE id=? AND +status='scheduled'` — and the sweeper the server reports `affectedRows = 1` to owns it (§7.1 Q2). That +makes the outbox safe for two app instances; the other four workers in this codebase are still +single-instance, so the deployment as a whole is not. A row stranded in `sending` by a crashed process +is reclaimed after a window, because `status='scheduled'` would otherwise never match it again. + +### engagement_sends — the send log (engagement phase 4a) +| col | type | notes | +|---|---|---| +| id | BIGINT AUTO_INCREMENT PK | | +| outbox_id | BIGINT NULL | | +| rule_id | INT NULL | | +| trigger_id | VARCHAR(96) NOT NULL | | +| user_id | INT NULL FK→users(id) **ON DELETE SET NULL** | the log survives an account deletion | +| channel / transport | VARCHAR(32) | which channel, and which mail transport actually carried it | +| address_hash | CHAR(64) NULL | sha256 — enough to correlate a bounce (Phase 9), useless as a mailing list | +| status | ENUM('sent','failed','suppressed','bounced','complained') | | +| detail | VARCHAR(500) NULL | | +| created_at | DATETIME | | + +`INDEX(trigger_id, created_at)`, `INDEX(user_id, created_at)`, `INDEX(rule_id, created_at)` — the last +of those is the per-rule hourly ceiling's count, which runs once per rule per event. + +G15: "did user X get the mail?" has never been answerable on this deployment. A row is written for +**every terminal outcome**, not only success — "no, and here is why" is an answer this table has to be +able to give — and the hourly ceiling counts only `sent`, so a broken transport cannot silently consume +a rule's budget and mute it. + +**It is deliberately not a second address book.** The address is a hash; the values of a payload never +appear here, and neither do they appear in the engagement log lines, which carry variable *names* and +counts only. + ### mobile_auth_sessions / mobile_auth_codes — mobile SSO bridge (M9) Two short-lived, self-pruning tables that bridge a browser SSO redirect flow to a native client. They diff --git a/website/ENGAGEMENT.md b/website/ENGAGEMENT.md index 9739c3d..0b240a7 100644 --- a/website/ENGAGEMENT.md +++ b/website/ENGAGEMENT.md @@ -1,12 +1,14 @@ # The Engagement System — findings and plan -**Status:** design of record. **Phases 1, 1a, 1b, 2 and 3 are built** (Phase 1: website#165 + +**Status:** design of record. **Phases 1, 1a, 1b, 2, 3 and 4a are built** (Phase 1: website#165 + docs#178, with website#164 as its prerequisite; Phase 1a: website#166 + docs#179; Phase 1b: -website#167 + docs#180; Phase 2: website#168 + docs#181); everything from Phase 4 on is still design. -The scope decisions below are settled; **six of the eight questions in §7.1 are answered** — Q1, Q3, -Q5 and Q7 on 2026-08-28, and Q6 on 2026-08-29 at the start of Phase 2, which also settled §7.2's -namespace question. Q1's answer added a whole phase (**Phase 1b**, unique email addresses). **Q2, Q4 -and Q8** remain open and block Phases 4, 5b and 8 respectively. Per CLAUDE.md § Conventions, no +website#167 + docs#180; Phase 2: website#168 + docs#181; Phase 3: website#169 + docs#182; Phase 4a: +website#170 + docs#183); everything from Phase 4b on is still design. +The scope decisions below are settled; **seven of the eight questions in §7.1 are answered** — Q1, Q3, +Q5 and Q7 on 2026-08-28, Q6 on 2026-08-29 at the start of Phase 2 (which also settled §7.2's +namespace question), and **Q2 and Q4 on 2026-08-29 at the start of Phase 4**. Q1's answer added a +whole phase (**Phase 1b**, unique email addresses); Q4's answer and the phase's size split **Phase 4 +into 4a and 4b**. **Q8** remains open and blocks Phase 8. Per CLAUDE.md § Conventions, no implementation starts without the org lead's approval of the phase it belongs to. **Branching:** every phase lands on **`edge`** in its repo; `main` is touched once, by the cutover @@ -651,8 +653,13 @@ drops three of them. The module supplies `subject` on emit; core stores it opaqu configured cooldown — otherwise this table grows without bound, which is the failure mode `teamActivityPrune` was written for. -**The check is `INSERT … ON DUPLICATE KEY UPDATE` guarded on the interval**, in one statement, so two -concurrent emits cannot both pass a read-then-write check. +**The check must not be a read-then-write**, or two concurrent emits both see an expired cooldown and +both send. The obvious single statement — `INSERT … ON DUPLICATE KEY UPDATE` with the interval guard in +the assignments, reading the answer out of `affectedRows` — **does not work against this codebase's +pool**, and Phase 4a is where that was found: the mariadb connector defaults `foundRows: true`, so a +no-op update reports 1 rather than 0 and every cooldown passes. What ships instead is a guarded UPDATE +(the interval in a WHERE clause, where a row either matches or does not) falling back to an +`INSERT IGNORE` for the first fire. See Phase 4a's as-built for the statements and the races. ### 4.2 Scheduling — a queue for delay, and deliberately no queue for digest @@ -679,7 +686,10 @@ CREATE TABLE IF NOT EXISTS engagement_outbox ( sent_at DATETIME NULL, CONSTRAINT fk_engo_rule FOREIGN KEY (rule_id) REFERENCES engagement_rules(id) ON DELETE CASCADE, CONSTRAINT fk_engo_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, - UNIQUE KEY uq_engo_dedupe (dedupe_key), + -- SCOPED, not global. One event legitimately becomes one row per (rule, user, + -- channel); a global unique index would admit the first recipient's row and + -- silently ignore every other. Corrected in Phase 4a — see its as-built. + UNIQUE KEY uq_engo_dedupe (rule_id, user_id, channel, dedupe_key), INDEX idx_engo_due (status, due_at), INDEX idx_engo_cancel (rule_id, user_id, subject_key, status) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4; @@ -691,8 +701,10 @@ Three things this buys that a straight send does not: - **`status='cancelled'` is the actual point of that window.** `idx_engo_cancel` is what a *resolving* event queries: a `house.decay` back up to `LikeNew` cancels every scheduled row for that (rule, user, house). Without cancellation, a delay is just a late mail. -- **`dedupe_key UNIQUE` makes replay safe.** The sidecar has no schema-migration mechanism and a - reconnect backfills; an at-least-once feed must not become an at-least-once mailer. +- **`dedupe_key` UNIQUE per (rule, user, channel) makes replay safe.** The sidecar has no + schema-migration mechanism and a reconnect backfills; an at-least-once feed must not become an + at-least-once mailer. The scope matters as much as the constraint: a dedupe key names the *event*, + and the event fans out to every recipient of every channel of every matching rule. `channel` is `VARCHAR(32)` and not an `ENUM` for exactly the reason `announce_job_legs.leg` is — the channel set is data, and a module (or a later core channel) must not require an `ALTER`. @@ -1281,7 +1293,8 @@ change is not complete until `docs/` reflects it" — is the floor; this table i | **1b** Unique email | `website/BACKEND_DESIGN.md` — the `users` table (the "not unique" note is now false), the new change/verify routes, and the de-dupe migration as an operator-visible upgrade step | `website/README.md` upgrade notes · a release note naming the admin report and the verification-gate default | | **2** Trigger registry | `website/MODULE_API.md` §1.1 (**1.7.0** + correct the stale "1.6.0 has only ever been on `edge`" paragraph), §2.3 (`ctx.events`, `ctx.inbox`), §2.4 (`registerEventTriggers`, `registerAudiences`), the dormant-rule note (landed as §6.8) · `website/ENGAGEMENT.md` §4.3 and §5.1a kept true · `BACKEND_DESIGN.md` route table | **Both deferred to the Phase 13 cutover window, deliberately — see Phase 2's as-built.** `Integration-kit`'s `ci/core-ref.json` pins a **`main`** sha, so the equality check stays green (and must stay green) for the whole `edge` period; `runicgateway.com`'s `checkFacts.mjs` *fetches* from `main`, so setting `platform.json.moduleApi` → 1.7.0 now would turn that repo red immediately | | **3** Channel preferences | `website/BACKEND_DESIGN.md` route table · `android/PLAN.md` §11 | — | -| **4** Engine | `website/ENGAGEMENT.md` (rules/cooldown/outbox as built) · `BACKEND_DESIGN.md` table inventory | — | +| **4a** Engine | `website/ENGAGEMENT.md` (rules/cooldown/outbox as built, and the two §4 defects it corrects) · `BACKEND_DESIGN.md` table inventory | — | +| **4b** Rules screen | `website/BACKEND_DESIGN.md` route table · `website/ENGAGEMENT.md` §5.1a composition UI | — | | **5a/5b** Templates + editor | `website/ENGAGEMENT.md` §4.6 · a template-authoring section in `BACKEND_DESIGN.md` or its own doc | **`runicgateway.com`**: a new admin docs page for the template editor | | **6** Email channel + Teams migration | `website/TEAMS.md` §6.3/§6.4 **rewritten** — the Team pipeline it describes no longer exists as its own thing | **`runicgateway.com`**: `administration/teams.mdx` notification section | | **7** In-app channel (core+web) | `website/BACKEND_DESIGN.md` routes + tables · `website/ENGAGEMENT.md` | **`runicgateway.com`**: `notifications-and-email.mdx` gains the in-app channel | @@ -1742,15 +1755,163 @@ see something a preference actually governs. ### Phase 4 — The engine: rules, cooldowns, outbox -`engagement_rules`, `engagement_cooldowns`, `engagement_outbox`, `engagement_sends`, the sweep worker -(`setInterval` + `unref` + `stop`, wired into `server.js` like its five siblings), audience resolution, -condition evaluation, delay and cancellation. Admin → Engagement → Rules. +**Split into 4a and 4b** at the start of the phase, on the same argument that split Phase 5: the half +that first makes this system capable of sending is worth reviewing without a React screen in the same +diff, and the ceiling arithmetic that decides who a rule may reach is worth reading on its own. + +#### 4a — the engine (server only) ✅ + +`engagement_rules`, `engagement_audience_segments`, `engagement_cooldowns`, `engagement_outbox`, +`engagement_sends`, the sweep worker (`setInterval` + `unref` + `stop`, wired into `server.js` like +its five siblings), audience resolution, condition evaluation, delay and cancellation, and the +save-path validation the admin surface will call. **No HTTP surface at all** — provably done when a +fired trigger produces an outbox row and a send-log entry with no UI in the picture. + +#### 4b — the admin surface + +Admin → Engagement → Rules, the §5.1a segment composition UI, and the routes underneath them. Q4's +answer places it in **its own top-level nav group** (below), so 4b also creates the group that +Triggers, Templates and the send log join in Phase 5. **Acceptance:** a trigger fired twice inside `cooldown_seconds` for the same (rule, user, subject) sends once; the same trigger for a *different* subject sends again; a scheduled row is cancelled by a `cancel_on` trigger and never sends; a restart mid-window still sends exactly once; a duplicate `dedupe_key` is a successful no-op. -**Guardrails:** swagger + route manifest; a named test for the multi-house cooldown case (§4.1). +**Guardrails:** swagger + route manifest (4b — 4a adds no routes); a named test for the multi-house +cooldown case (§4.1). + +--- + +#### As built — 4a (2026-08-29) + +**Three decisions were settled by the org lead before any code, and two of them are §7.1 questions +this phase was blocked on.** + +| | Question | Decision | +|---|---|---| +| **Q2** | multi-instance: `SKIP LOCKED`, or document single-instance | **Neither, exactly**: a compare-and-set claim — `UPDATE … SET status='sending' WHERE id=? AND status='scheduled'`, the winner being whoever the server reports `affectedRows = 1` to. It is what §4.2a's ENUM was already shaped for (nothing else needs a `sending` state), it needs no open transaction and no MariaDB version floor, and it delivers Q2's intent | +| **Q4** | where the engagement admin surface lives | **its own top-level nav group**, "Engagement", beside Content / Moderation / System — Rules now, Triggers / Templates / Send Log in Phase 5. Email Delivery stays a section of Settings for now | +| — | one PR or two | **4a / 4b**, as above | + +**What Q2's answer does and does not buy.** It makes the *outbox* safe for two app instances. It does +not make the deployment multi-instance: `announceWorker`, `teamDigestWorker`, `teamForumUploadSweep` +and `teamActivityPrune` are all still written for one, and widening them is not this phase's scope. +What it buys is that the one table that will carry mail is ready for the day it is, which is cheap now +and expensive after mail has doubled once. + +**Two defects in this document's own §4, both found by building it.** + +1. **§4.2a's `UNIQUE KEY uq_engo_dedupe (dedupe_key)` was a data-loss bug, not a style question.** A + dedupe key names the EVENT — "house 0x4001 entered IDOC" — and one event legitimately becomes many + outbox rows: an audience of fifty users is fifty rows, a rule spanning email and in-app doubles + that, and a second rule on the same trigger doubles it again. Under a *global* unique index the + first of those inserts wins and every other one is silently ignored, so ninety-nine recipients are + dropped by the mechanism that exists to stop a replayed event becoming a second mail. Shipped as + **`UNIQUE (rule_id, user_id, channel, dedupe_key)`**, which keeps exactly the replay guarantee and + nothing more. A test asserts one key fans out to six rows. + +2. **§4.1's single `INSERT … ON DUPLICATE KEY UPDATE` cooldown claim does not work against this + codebase's pool**, and the way it fails is silent. Its answer is read out of `affectedRows` on the + usual contract — 1 inserted, 2 updated-and-changed, **0 for a duplicate key whose update changed + nothing**, that 0 being "still cooling". **The mariadb Node connector defaults `foundRows: true`**, + which makes `affectedRows` report rows *matched* rather than rows *changed*, and `utils/db.js` does + not override it. Under that pool the no-op returns 1 and is indistinguishable from a fresh insert: + **every cooldown passes, always.** Shipped as two statements instead, each of which is its own + atomic decision and neither of which asks `affectedRows` to mean two things: + + ```sql + -- 1. claim by moving the row, guarded in a WHERE clause where a row either matches or does not + UPDATE engagement_cooldowns SET last_fired_at = ?, fire_count = fire_count + 1 + WHERE rule_id = ? AND user_id = ? AND subject_key = ? + AND last_fired_at <= ? - INTERVAL ? SECOND; + -- 2. matched nothing? then the row is absent or cooling; INSERT IGNORE separates the two + INSERT IGNORE INTO engagement_cooldowns (rule_id, user_id, subject_key, last_fired_at, fire_count) + VALUES (?, ?, ?, ?, 1); + ``` + + Still race-free, and each race resolves the right way: two concurrent first fires both fall to the + INSERT and the primary key picks one; two concurrent fires after expiry serialise on the row lock + and the second re-evaluates its guard against the committed `last_fired_at`. + +**The second defect is the reason this phase has a second test file.** `engagementEngine.test.js` +stubs the five tables and runs the engine's logic against in-memory stand-ins, which is right for +everything the engine *decides* — and it was **green against the broken cooldown claim**, because a +stub can only agree with whoever wrote it, and the same misreading produced both. The statements whose +correctness is a *server* contract now run against a real MariaDB in `engagementEngineSql.test.js`, +which creates a throwaway database, drops it, and **skips when there is none** so CI stays green +without one. The general lesson: a stub is a fine stand-in for a table and a poor one for a protocol. + +**The gate order is the design.** Enabled rules → conditions → audience → **ceiling re-check** → +per-channel preference → per-rule hourly ceiling → cooldown → deduped enqueue. Two of those placements +are load-bearing: + +- **The G24 ceiling is re-checked at SEND time, not only at save.** The save path already ran the same + `ceilings.permits`, so the only way this can fail is the case it exists for — a module upgrade that + *narrows* its trigger's declaration underneath a rule saved when it was wider. Without it, a rule + written against yesterday's declaration keeps reaching yesterday's population forever. This is the + second call site §5.1a promised, not a second implementation. +- **The hourly ceiling is checked before the cooldown**, because the ceiling is about the rule and the + cooldown is about one recipient. A rule that has hit its ceiling should not also burn every + recipient's cooldown slot on sends that never happen. + +**Segments (§5.1a) as built, with one rule the design did not state.** `not` is legal **only as a +child of `and`**. A complement needs a universe, and the only one available that does not widen is +the set its siblings produced: `A AND NOT B` is "A, less B", which is what an operator wants and +cannot be composed into a broadcast. A bare `NOT B` — or `A OR NOT B` — would have to mean "everyone +except…", which is a way to build the whole deployment out of one narrow audience and is precisely the +widening rule 3 forbids. It is refused at save with that sentence. + +The other half of that: **a `not` contributes no ceiling to the meet.** Excluding people cannot widen +who an expression reaches, so folding the excluded audience's ceiling in would refuse safe segments — +`members AND NOT staff` would hit `meet('members','staff') = null` and be rejected even though it +reaches strictly fewer people than `members` alone. + +**Dormancy, three ways, and none of them deletes anything** (§7.3, §5.1a rule 4). A rule naming an +unregistered trigger, a rule whose channel is gone, and a rule whose segment was deleted are all +*listed, flagged and left alone*. In particular `engagement_rules.audience_segment_id` deliberately +carries **no foreign key**: `ON DELETE CASCADE` would delete an operator's rules and `ON DELETE SET +NULL` would silently fall the rule back to its plain `audience` column — and that fallback reaches a +*different set of people*, which is the exact failure §5.1a rule 4 exists to prevent. Deleting a +segment that a rule still uses is refused in the model, with the count. + +**Conditions are a small closed grammar**, not an expression language: and/or/not over comparisons of +one *declared* variable against a literal, every operator renderable as a dropdown, bounded in list +length and nesting depth because the tree comes out of a JSON column an admin can write and is walked +on the emit path. Two properties worth keeping: + +- **An absent variable makes every comparison false, including `ne`.** "Not equal to IDOC" reads as + satisfied by nothing at all, and treating it that way would fire a rule on every event that omits an + optional variable. `present` / `absent` are the honest way to ask. +- **A tree that no longer parses evaluates false**, never "no conditions". Failing closed stops the + mail; failing open mails everyone the rule could ever reach. + +**`ctx.events.emit` does not await the engine.** It is called from inside a game-event handler, and +the caller's job is to say the event happened — not to wait on rule lookups, audience resolution and a +dozen inserts to find out whether it is allowed to carry on. That is the same argument the C# side's +`Emit()` makes about the Core thread. `dispatch` catches everything internally and never rejects. The +consequence a caller must know: **`emit` returns before the outbox rows exist**, so a test that wants +the delivery decision calls `engine.dispatch` directly. + +**Nothing is delivered, and that is visible rather than pretended.** A channel's `deliver` arrives with +email in Phase 6 and the in-app inbox in Phase 7. Until then the worker claims the row, finds no +`deliver`, finishes it `failed`, and writes a send-log row saying so in as many words. Recording +`sent` would be a lie in the one table whose entire purpose is answering "did they get it"; leaving the +row `scheduled` would mean an IDOC warning queued today arriving three weeks later on the deploy that +first shipped a mailer. On a real deployment the path is unreachable anyway — core seeds no rules and +`enabled` defaults to 0, so nothing enqueues until 4b's screen exists and an operator uses it. + +**Three smaller things the phase settled:** + +- **`everyone` and `authenticated` resolve identically.** A signed-out visitor has no address, no + device and no inbox, so the widest set the engine can deliver to is the active user table. The + lattice still distinguishes them — a trigger ceilinged `everyone` permits an `authenticated` rule + and not the reverse — and only the *resolution* coincides. +- **A plain `members` audience with no segment reaches nobody.** `members` is the ceiling for "a + module-declared list"; without a segment there is no list, and core knows no game vocabulary with + which to guess. Inert and visible, rather than quietly falling back to something wider. +- **Every audience is filtered through `users.status = 'active'`, including a module's.** A module's + resolver returns ids over its own store and has no notion of account status; a banned account must + not be mailable by a module returning its id. --- @@ -2009,7 +2170,7 @@ day it ships. ## Part 7 — Open questions and forward-compat notes -### 7.1 Questions for the org lead — six answered, three still open +### 7.1 Questions for the org lead — seven answered, one still open 1. ✅ **ANSWERED — may unverified addresses receive engagement mail?** *"Emails need to be unique and verification blocking sending is an admin setting."* Combined with the opt-in answer, this settles @@ -2025,15 +2186,23 @@ day it ships. *Consequence for ordering:* Phase 9 no longer blocks Phase 11 — the verification *mechanism* moves forward into 1b, and Phase 9 keeps only bounces and suppression. -2. **Multi-instance.** Do we commit to single-instance (status quo) and document it, or add - `SELECT … FOR UPDATE SKIP LOCKED` to the outbox sweep in Phase 4? Recommendation: add it in Phase 4 — - it is cheap there and expensive to retrofit after mail has doubled once. +2. ✅ **ANSWERED — multi-instance.** Neither of the two the question offered, and the third is + better than both: the outbox sweep **claims each row with a compare-and-set** — + `UPDATE … SET status='sending' WHERE id=? AND status='scheduled'` — and the instance the server + reports `affectedRows = 1` to owns it. §4.2a's status ENUM already carried a `sending` state that + nothing else needed, so this is what the schema was shaped for; it needs no open transaction (which + nothing else in this codebase's workers does) and no MariaDB version floor. It makes the **outbox** + safe for two app instances and does not, on its own, make the deployment multi-instance — the four + existing workers are still written for one. Built in Phase 4a. 3. ✅ **ANSWERED — rules as data vs. rules as code.** Data, as recommended: rules are operator-editable rows, `enabled` defaults to `0`, and every rule carries a **hard per-rule hourly send ceiling**. The ceiling is not a nicety — it is the thing that keeps a misconfigured rule from becoming a mail storm, and it is what makes "data" safe enough to choose over "code". Phase 4 builds both. -4. **Does the engagement admin surface belong under Admin → Settings, or its own top-level section?** - It is three screens (Triggers, Rules, Templates) plus a send log. +4. ✅ **ANSWERED — the engagement admin surface gets its own top-level nav group**, "Engagement", + beside Content / Moderation / System. Four screens is too much to bury: Settings is already one long + page of sections, and a send log is a paged table rather than a settings section. Rules lands with + Phase 4b; Triggers, Templates and the send log join it in Phase 5. **Email Delivery stays a section + of Settings** for now — moving it is not part of either phase. 5. ✅ **ANSWERED — which SMTP posture is the documented default?** Document all three; **lead with a relay** (Mailgun/SES/Postmark); name Gmail-with-an-app-password (`smtp.gmail.com:587`) explicitly as the migration path off OAuth2 for the existing deployment (§1.2a). Phase 1 owes all three in