From fdd5bf36f77dc9d4ec7b125280188b27ca302410 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Tue, 1 Sep 2026 15:41:13 -0500 Subject: [PATCH] =?UTF-8?q?docs(engagement):=20Phase=2014=20as=20built=20?= =?UTF-8?q?=E2=80=94=20retention?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last phase of the workstream. ENGAGEMENT.md gets Phase 14's as-built (the six org-lead decisions, three findings, the acceptance line discharged and one rig trap), and its header now reads COMPLETE rather than "Phase 14 is scoped and not started". BACKEND_DESIGN.md gets the reference: a new "Engagement retention" section with the horizon table, why the outbox sweep is terminal-only, why each floor is a correctness bound rather than a UI nicety, why suppressions do not expire, and why this one got a screen when team_activity and user_notifications did not. Plus the three new routes in the admin table and the address_hash reversal on GET /suppressions. Three findings worth not re-deriving: * the per-row purge could not be built without reversing Phase 9's decision to strip address_hash — the row had no handle, and the existing lift asks for an address the screen has never shown. * reclaimStale could not give up, so an interrupted send never became terminal and no retention sweep could ever have bounded the outbox. * the cooldown warning is unreachable through the UI by construction (MAX_COOLDOWN_SECONDS is smaller than the horizon's floor). It is not dead: it catches a hand-edited row, and a future raise of that ceiling. And one the phase text got wrong in the other direction: the Play Data Safety answers correctly did NOT move, because deploy-engagement is deployment-scoped and PLAY_DATA_SAFETY.md is generated from the app-scoped entries only. Code: website#TBD. Co-Authored-By: Claude --- website/BACKEND_DESIGN.md | 60 ++++++++++++++++++++++++-- website/ENGAGEMENT.md | 88 +++++++++++++++++++++++++++++++++++---- 2 files changed, 138 insertions(+), 10 deletions(-) diff --git a/website/BACKEND_DESIGN.md b/website/BACKEND_DESIGN.md index b2b1281..1c91346 100644 --- a/website/BACKEND_DESIGN.md +++ b/website/BACKEND_DESIGN.md @@ -596,7 +596,7 @@ cooldown passes, always. See `ENGAGEMENT.md` Phase 4a. | 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 | +| created_at / updated_at | DATETIME | `updated_at` is what a stale-claim reclaim measures; `created_at` is what the retention sweep measures | `UNIQUE(rule_id, user_id, channel, dedupe_key)`, `INDEX(status, due_at)`, `INDEX(rule_id, user_id, subject_key, status)`. @@ -763,6 +763,58 @@ drops unread items is one whose badge means nothing. The horizon is `settings.us (default 90), so an operator tightens a busy shard without a deploy — `team_activity`'s posture, in the worker that file is modelled on. +### Engagement retention — three sweeps and one recorded refusal (engagement phase 14) + +Four engagement tables grow, and until Phase 14 nothing deleted from any of them. +`utils/engagementRetentionPrune.js` is one nightly worker over three of them — +`setInterval` + `unref` + `stop()`, wired into `server.js` beside `teamActivityPrune` and +`inboxPrune`, batched 1000 × 50 per table, each table's failure caught on its own so a lock +timeout on one does not leave the other two unbounded. + +| table | horizon | setting | what is eligible | +|---|---|---|---| +| `engagement_sends` | 180 days | `engagement_sends_retain_days` (7–3650) | every row; they are all terminal | +| `engagement_cooldowns` | 30 days | `engagement_cooldowns_retain_days` (2–3650) | every row, by `last_fired_at` | +| `engagement_outbox` | 30 days | `engagement_outbox_retain_days` (2–3650) | **terminal rows only** — `sent`, `failed`, `cancelled`, `suppressed` — by `created_at` | +| `engagement_suppressions` | **never** | — | nothing. See below | + +**The outbox sweep is terminal-only, and that is a correctness rule, not a preference.** A +`scheduled` row is a send this deployment still intends to make — `delay_seconds` can legitimately +put one a day out — and a `sending` row may be a worker mid-flight. A sweep by age alone would +cancel sends nobody cancelled, and the only symptom would be mail that never arrived. + +**The floors are not UI niceties.** Below two days, a pruned cooldown row makes the next fire a +FIRST fire — the rule sends twice; `MAX_COOLDOWN_SECONDS` is a validated 86 400, so two days is the +smallest provably-safe value against any rule that can be saved. The send log's floor is a week +because `engagement_sends` has two live readers: the per-rule hourly ceiling counts it +(§7.1 Q3), and Admin → Engagement → Send Log is the operator's only answer to "was this person +told". + +**The cooldown horizon is checked, not assumed.** `engagementRules.db.maxEnabledCooldownSeconds()` +(enabled rules only — a disabled rule writes no cooldown row) is compared against the horizon on +every read of the policy and on every sweep. A horizon that does not clear it produces a warning on +the screen and in the log, **and the sweep runs anyway**: refusing to prune would trade a bounded, +describable fault for the unbounded one this phase exists to end. + +**`engagement_suppressions` does not expire, and that is the recorded decision** (org lead, +2026-09-01). A suppression is a standing decision, not a record of something that happened; ageing +out a hard bounce re-mails an address that already bounced, which is how a sender loses a domain's +reputation. The way out stays deliberate — see the per-row lift in the route table. + +**Retention has a screen**, unlike `team_activity` and `user_notifications`, whose horizons are +invisible settings rows. The send-log horizon changes what an operator-facing page is *able to show*, +so it has to be visible and settable; having made one visible, hiding the other two would split one +question ("what does this deployment keep") across two places. `EngagementSendLog.jsx` reads the +policy and prints *"entries older than N days are removed automatically"* beneath its pager, so the +total it shows stops being quietly wrong. + +**One shipped defect this phase had to fix to be a bound at all.** `outboxDb.reclaimStale` returned +every stale `sending` row to `scheduled`, and `MAX_ATTEMPTS` is consulted only on a graceful `retry` +outcome — so a send that killed the process mid-flight cycled sending → scheduled → sending forever, +never reached a terminal status, and was therefore never eligible for any sweep. It now fails a row +that has burned its attempts **before** reclaiming the rest; the order is the fix, and reversing it +hands the exhausted row straight back to `findDue`. + ### The two block registries — pages and mail (engagement phase 5a) `server/src/blocks/` (the CMS page family) and `server/src/emailBlocks/` (`email.heading`, `email.text`, @@ -1717,9 +1769,11 @@ finished unsuccessfully. `complained` still has no writer: it needs a provider f | Route | Notes | | --- | --- | -| `GET /suppressions` | Paged, filterable by `reason` / `channel` / `search`, plus unfiltered `byReason` totals | +| `GET /suppressions` | Paged, filterable by `reason` / `channel` / `search`, plus unfiltered `byReason` totals. **Returns `address_hash` from Phase 14 on**, reversing Phase 9's decision to strip it: without a handle the only way out of the list was a `window.prompt` asking the operator to retype an address the screen has never shown them. The trade — this route is admin-only, and an admin can already suppress and unsuppress any address they can name, so the hash grants no capability they lack. `GET /sends` still strips its own hash: nothing there needs to act on a row | | `POST /suppressions` | `reason` is forced to `manual` — an admin typing an address is not evidence of a bounce. An address already listed answers 200 with `created: false`, not 409 | -| `DELETE /suppressions` | The only way out of the list. The address goes in the **body**, not the path: a path parameter lands in the access log, the browser history and every proxy in front of the deployment | +| `DELETE /suppressions` | The way out for an address the operator can type. It goes in the **body**, not the path: a path parameter lands in the access log, the browser history and every proxy in front of the deployment | +| `DELETE /suppressions/by-hash/:hash` | The per-row Lift button (Phase 14). Same effect, different input — the operator is looking at a mask and knows only the row's handle. The handle **is** safe in the path where an address is not: a sha256 already served only to an admin session leaks nothing further by being logged. Shape-validated to 64 hex characters before it reaches a WHERE clause; `404` rather than `200` when nothing matched, so a stale screen says so instead of claiming success | +| `GET · PUT /retention` | The three sweep horizons in days, with the bounds each is validated against and a `warnings` array carrying the one check that is not a static bound — a cooldown horizon shorter than the longest cooldown on an ENABLED rule. `engagement_suppressions` is deliberately absent: it does not expire. The `PUT` is **sparse** (saving one select cannot clobber another admin's concurrent change) and **refuses out-of-range rather than clamping**, because storing something other than what was typed would leave the screen describing a policy the deployment is not running | **Neither route ever returns `address_hash`**, the same rule `GET /sends` follows: a sha256 of every address on the deployment, handed to a browser, is an offline dictionary attack. What the list diff --git a/website/ENGAGEMENT.md b/website/ENGAGEMENT.md index 4c30c1d..60877ba 100644 --- a/website/ENGAGEMENT.md +++ b/website/ENGAGEMENT.md @@ -1,11 +1,12 @@ # The Engagement System — findings and plan -**Status:** design of record. **The workstream is BUILT, CUT OVER and ACCEPTED (2026-09-01).** -Phases 0 through 13 are done and on `main` in every repository, and **Phase 13's acceptance walk +**Status:** design of record. **The workstream is COMPLETE (2026-09-01).** +Phases 0 through 14 are done and on `main` in every repository, and **Phase 13's acceptance walk passed** — a clean install from `main`, released artefacts only, one email and one in-app item to the linked owner and nothing to anyone else. It found two defects in shipped core; both are recorded in -Phase 13's as-built and neither is fixed there. **Phase 14 (retention) is scoped and not started.** Each phase carries its own as-built -section below, and the PRs are named there rather than here. +Phase 13's as-built and both were fixed afterwards (website#181). **Phase 14 (retention) is BUILT** — +the last phase, and the one that gave the engagement schema a bound. Each phase carries its own +as-built section below, and the PRs are named there rather than here. The cutover cut `link` **v2.1.0**, the plugin overlay **v1.1.0**, `module-uo` **v1.1.0** and the paired bundle **2026.09.01** (protocol **5**); `MODULE_API_VERSION` is **1.9.0**. Two of Phase 13's @@ -23,7 +24,9 @@ of Phase 8**. Q1's answer added a whole phase (**Phase 1b**, unique email addres the phase's size split **Phase 4 into 4a and 4b**; **Q9** (core's own `news.post` emitter) was answered **2026-08-31 at the start of Phase 11**, along with five further decisions that changed what that phase ships — see its own decision block. Per CLAUDE.md § Conventions, no implementation starts -without the org lead's approval of the phase it belongs to, **and that still holds for Phase 14.** +without the org lead's approval of the phase it belongs to; **Phase 14's six decisions were settled +on 2026-09-01 before any of its code**, and two of the four horizon answers widened it — see its +as-built. **Branching:** every phase landed on **`edge`** in its repo, and `main` was touched once, by the cutover (Phase 13). §6.0a records the blocking precondition it opened with — six `edge` branches @@ -3869,9 +3872,11 @@ engagement system a headline capability — and Phase 12's second finding became ### Phase 14 — Retention: the engagement schema has no sweep -**Status: scoped, not started.** Phase 12 found this and recorded it without fixing it (finding 2 of +**Status: BUILT, 2026-09-01.** Phase 12 found this and recorded it without fixing it (finding 2 of its as-built), on the grounds that a sweep is a `website` change and outside a documentation phase. -Nothing after it picked the finding up, so it is stated here as the phase it always was. +Nothing after it picked the finding up, so it was stated here as the phase it always was — and this +is that phase. Six decisions were settled by the org lead before any code; the as-built is at the end +of this section. **Four tables grow without bound**, and they are not one problem with one horizon: @@ -3907,6 +3912,75 @@ longest enabled rule's cooldown rather than picked; `/privacy` and the Play answ from the inventory; and a rig run shows the sweep deleting terminal rows while leaving a `scheduled` outbox row and an in-window send-log row alone. +#### Phase 14 as built + +**The six decisions, settled by the org lead before any code.** Four horizons were put with the facts +the tree supplied, and two of the four answers went past what was offered: + +1. **`engagement_cooldowns` — 30 days**, plus the runtime guard. (`MAX_COOLDOWN_SECONDS` is a + validated 86 400, so even 2 days is provably safe; 30 was chosen for headroom.) +2. **`engagement_outbox` — terminal rows only, 30 days, plus a stuck-`sending` reaper.** The reaper + half turned out to be half-built already — see finding 2. +3. **`engagement_sends` — admin-configurable, 90 / 180 / 365 / custom.** This is the answer that + widened the phase: none of the three offered options had a UI, and the other two retention workers + in this codebase (`teamActivityPrune`, `userNotificationsPrune`) keep their horizons in invisible + `settings` rows. It bought the phase a screen, and the screen took the other two horizons with it — + "what does this deployment keep" is one question. +4. **`engagement_suppressions` — never expires, plus an admin purge.** The purge half also went past + the offer, and cost a Phase 9 decision — see finding 1. + +**As built.** `utils/engagementRetentionPrune.js` (one worker, three sweeps, batched 1000 × 50, each +table's failure caught on its own), `model/engagement/engagementRetention.model.js` (the policy, its +bounds and the cooldown check), two `.db` additions (`outbox.pruneTerminal`, `sends.prune`) and a +`limit` on the cooldown prune that already existed and had never had a caller. Three routes +(`GET`/`PUT /admin/engagement/retention`, `DELETE /admin/engagement/suppressions/by-hash/:hash`), a +new Admin → Engagement → **Retention** screen, a per-row **Lift** button on Suppressions, and one line +under the Send Log's pager. Two indexes (`idx_engo_sweep`, `idx_engs_sweep`) — every existing index on +those tables has `created_at` in *second* position, which serves a per-rule window and is useless to a +whole-table horizon. Reference: `BACKEND_DESIGN.md` § *Engagement retention*. + +**Three findings.** + +1. **The per-row purge cost a Phase 9 decision, and there was no way to have it otherwise.** Phase 9 + deliberately stripped `address_hash` from every listed row — a sha256 of every address on the + deployment is an offline dictionary attack waiting to be run — and the consequence, which its own + screen shows, is that the only way out of the list was a `window.prompt` asking the operator to + retype the full address. **They do not have it**: the screen stores and shows a mask. So the row + had no handle at all, and a per-row button was not a UI change but a reversal. Taken knowingly: + the route is admin-only and an admin can already suppress and unsuppress any address they can + name, so the hash grants no capability they lack. `GET /sends` still strips its own. + +2. **`reclaimStale` could not give up, so the outbox sweep would not have been a bound.** The + stuck-`sending` reaper decision 2 asked for already existed and already ran every worker tick — + but it returned *every* stale row to `scheduled`, and `MAX_ATTEMPTS` is consulted only on a + graceful `retry` outcome. A send that killed the process mid-flight therefore cycled + sending → scheduled → sending **forever**: never terminal, therefore never eligible for the + terminal-only sweep this phase was adding. Fixed by failing an exhausted row *before* reclaiming + the rest; the order is the fix, and `engagementRetentionSql.test.js` runs the wrong order + deliberately to show what it avoids. + +3. **The cooldown warning is unreachable through the UI, by construction.** `MAX_COOLDOWN_SECONDS` + (86 400) is smaller than the cooldown horizon's floor (2 days), so no rule that can be *saved* can + trip the guard. It is not therefore dead: it fires for a hand-edited row, and it is the thing that + catches a future raise of `MAX_COOLDOWN_SECONDS` that forgets this floor. Proved live by setting a + 5-day cooldown by hand — the screen and the sweep both warned, and the sweep ran anyway. + +**The acceptance line, discharged.** All four tables have a stated policy (three sweeps and one +recorded refusal). The cooldown horizon is checked rather than picked. `/privacy` is regenerated from +`collection.mjs` — **and the Play answers correctly did not move**, which the phase text did not +expect: `deploy-engagement` is `scope: 'deployment'`, and `PLAY_DATA_SAFETY.md` is generated from the +`app`-scoped entries only, because Play asks what the *app* collects rather than what a self-hosted +deployment keeps. `npm run play:datasafety` rewrites the file byte-identically and +`check:datasafety` stays green. The rig run happened twice: `engagementRetentionSql.test.js` +against a throwaway database (7 tests), and the live stack, where a 90-day-old `cancelled` row was +swept and a 90-day-old `scheduled` row survived. + +**One rig trap worth not re-deriving.** The first live run appeared to delete the `scheduled` row — +the acceptance case, apparently failing. It had not: the outbox **worker** was running in the same +server, the planted row's `due_at` was 90 days in the past, so the worker legitimately claimed and +sent it before the sweep saw it. **Plant the scheduled row with a FUTURE `due_at`**, or the worker, +not the sweep, is what the test is measuring. + --- ### Sequencing