[lilyshen0722]

Follow-up to #2277, from sprint-review's non-blocking note on it.

A switch read error mid-run is a blip, not a flip

The digest.sending helper failed closed on a read error. Inside the per-recipient loop, a read error looked exactly like a deliberate flip: both stopped the run. A summary skipped by a stop is never retried, because each run sends only its own (does not sweep an older unemailed digest outside the current run). So a transient read error mid-run cost every remaining recipient that day's digest.

This loss mode already existed at the run-start read. Reading per recipient (#2277) multiplied the chances to hit it.

The helper now returns on / off / unknown:

• At the start of a run, anything but on sends nothing. That's unchanged: the switch fails closed when nothing is known yet.
• Mid-run, the switch is already known to have been on, so only a definite off stops the run. A read error lets it continue.

The runbook says so.

Tests

• A flip between the run's first read and the first send stops that email too. This pins that the read sits before the send; sprint-review's U4 left this green.
• A read error mid-run lets the rest of the run send (U2).
• Mutations, each failing its test:
  • mid-run read stopping on unknown;
  • run-start read failing open;
  • per-recipient read moved after the send.
• The digest-email, scheduler-digest and digest suites pass on main after fix(digest): the stop switch also stops a run that is already sending #2277; tsc and eslint are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_0157AG2wo8mJYqKezXNs48Ya

[lilyshen0722]

Written by sprint-review, a Commonly agent — code gate at 5ef87f973. pod thread

Base 0a1054dc8; origin/main is da1b7da57 and has touched none of these three files since, merge-tree gives 0 conflicts, merge state CLEAN.

This is the second PR in a row implementing a note of mine, so again I tested the suggestion rather than its application.

Both properties I left unpinned now have a test, and each discriminates

BASE and RESTORED both 3 suites / 28 passed, needles asserted:

mutation	failures
G1 run start treats unknown as on	1 — sends nothing when the switch cannot be read
G2 mid-run treats unknown as off again (the #2277 behaviour)	1 — a read error mid-run does not stop the rest of the run
G3 the catch returns 'off' instead of 'unknown'	1 — same test
G4 a definite off no longer detected	3 — the off arm, the mid-run flip, and the flip-before-first-send
G5 the per-recipient read moved after the send	1 — a flip before the first send stops even the first email
G6 the mid-run check removed entirely	2

G2 and G5 are the two I measured as unpinned on #2277, and they are the two that now fail when broken. The findOne double-mock in the first new test (run start → null, first per-recipient read → enabled: false, assert sent: 0) is exactly the discriminator the old test could not be: it puts the flip between the two reads rather than inside a send, which is what makes the read's position observable.

Tier 1 at this exact head: run 38875832366, job Service Tests (Tier 1 — real DBs), headSha 5ef87f973a68c5e09f139fb32a58ec8f59ce6357, conclusion success — 53 suites / 536 tests.

The residual, which belongs on the record rather than in a change

My suggestion traded one failure for its mirror, and the mirror is now the live one: a flip that lands while reads are failing will not stop the run. The run-start read said on, so every subsequent unknown is treated as a blip, and the remaining recipients are emailed.

Reachability is narrow but not empty: it needs the write of enabled: false to have succeeded while reads then fail — same database, so the obvious cases (Mongo down, auth wrong) can't produce it, but a primary stepdown immediately after the write, or a secondary read preference losing its node, can. In that window the lever does nothing. The bound is one run's remaining recipients, and the run-start read still fails closed, so the next run sends nothing.

I think this is the right trade at current volume and I would not change it — but it is worth one line in the runbook, because an operator flipping the switch during a Mongo incident will reasonably expect it to bite, and right now the runbook tells them a mid-run read error "doesn't stop the run" without connecting that to the case where they most want it to. Something like: if Mongo is unhealthy, this lever may not take effect until the next run; stop the backend instead.

One thing I looked at and am not filing

result.disabled is set to true for both a deliberate off and an unreadable switch at run start, which conflates "we chose not to send" with "we could not tell" — normally worth a reason field. I checked the consumers before writing it up: schedulerService.ts:207 is the only caller and it discards the return value entirely, and the two cases already log different lines. So the distinction exists exactly where it is read today, and there is nothing to fix until something reads the struct.

GATE: PASS