diff --git a/ERROR-ALERTING-PLAN.md b/ERROR-ALERTING-PLAN.md index 299bda0..17df0ff 100644 --- a/ERROR-ALERTING-PLAN.md +++ b/ERROR-ALERTING-PLAN.md @@ -1,171 +1,349 @@ -# FileMill Error Alerting — Plan - -Goal: when something goes **wrong** in FileMill, email an operator alert to -`support@example.com`, so an unattended background service (it runs at logon on -a personal laptop) doesn't fail silently. - -Status: **plan only, not implemented.** Written 2026-07-20. - -The *sending* is trivial — FileMill already has a working Mailgun Send path -(`Service.send` in `internal/mailgun/outbound.go`). The work is deciding -**which** failures alert, **not** drowning `support@` in noise, and handling the -cases email structurally can't cover. +# FileMill Error Alerting — Implementation Plan (issue #7) + +**Status:** Implemented and verified in production 2026-09-11. Phases 1–4 are +built (the `internal/alert` core and store `Ledger`, the job taxonomy split, the +Mailgun alert sites and wiring, crash reporting across restarts), and phase 5's +live checks passed against a branch build (`v0.2.1-20-g0aa8f96`): the test send, +one `restart` email from a killed worker with four further restarts suppressed, +one `job-systemic` email from the `alert_probe` transformer, and a second one +15 minutes later carrying "14 more". Only the bad-Mailgun-domain check is +outstanding, and it is deliberately skipped: it delays real senders' replies, +and the send path is covered by tests and by the four alerts above. First drafted 2026-07-19; **revised +2026-09-11** against the current code. Since the first draft, FileMill gained the +supervisor loop (#4), boot start, the two retention sweeps, sheets-link delivery, +and non-blocking delivery. Each adds alert sites, and the supervisor changes how a +crash can be reported at all. + +Goal: when FileMill fails *systemically*, email the operator (the address in +`config/email.yaml`, which is gitignored; examples here use `support@example.com`), +so an unattended worker doesn't fail silently. + +Sending is the easy part: `Service.send` in `internal/mailgun/outbound.go` +already works. The real work is deciding **which** failures alert, **throttling** +them, and being honest about what email **cannot** report. --- -## 1. Guiding principles - -1. **Alert on systemic failures, stay silent on expected ones.** A malformed - PDF that a transformer cleanly rejected is a *normal outcome the sender - already hears about* — it must not page an operator. A transformer that - crashed, timed out, or produced no valid result is *systemic* — alert. -2. **Throttle everything.** Loops and bots can generate thousands of identical - errors. Deduplication + rate limiting is mandatory, not optional. -3. **Never let alerting recurse or crash the app.** An alert-send that fails is - logged and dropped; it must never trigger another alert or panic. -4. **Email can't report that the app is gone.** Total-crash / machine-asleep - detection needs an external heartbeat, not self-sent email. In scope as a - complementary Phase 4, called out honestly. +## 1. Principles + +1. **Alert on systemic failures, stay silent on expected ones.** A PDF that a + transformer cleanly rejected is a normal outcome the sender already hears about. + A transformer that crashed, timed out, or broke the contract is systemic. +2. **Throttle everything, and keep the throttle state across restarts.** The + delivery loop ticks every second, and the supervisor restarts a crashing worker + every ≤120s. An in-memory throttle resets on every restart, so a crash-loop + would still send an alert per restart. Throttle state lives in SQLite. +3. **Alerting never blocks, recurses, or crashes.** `Report` enqueues and returns. + A failed alert send is logged and dropped, never re-reported. +4. **A process can't report its own death.** A crash is reported by the *next* + process, the one the supervisor restarts. "Never came back" or "machine offline" + is the heartbeat's job (#5), not this issue's. --- -## 2. What alerts, and what doesn't - -| Failure | Code site | Alert `support@`? | Why | -|---|---|---|---| -| Forged / replayed webhook (401) | `webhook.go` `receive` | **No** | Bots scan public URLs; would flood | -| Malformed / oversize body (400) | `receive` | **No** | Client noise | -| Unrouted recipient / disallowed sender (silent 200) | `receive` | **No** | Benign | -| **Intake failure (500)** — storage / filesystem / Submit | `receive` → `intake` | **Yes** | Systemic | -| **Job: transformer missing from config** | `app.go` `execute` | **Yes** | Systemic (misconfig) | -| **Job: transformer timed out** | `execute` | **Yes** | Systemic | -| **Job: missing / corrupt `result.json`** | `execute` → `readResult` | **Yes** | Systemic (contract violation) | -| **Job: nonzero exit with no usable result** | `execute` | **Yes** | Systemic (crash) | -| Job: transformer cleanly reported `success:false` (bad input) | `execute` | **No** | Expected; sender already told in the reply | -| **Delivery failure** — Mailgun send non-2xx | `outbound.go` `deliverPending`/`send` | **Yes (throttled)** | Replies aren't going out | -| **Worker loop dies** (`store.Next` error → `fatal`) | `app.go` `Run` → `main` | **Yes** | Catastrophic | -| **Panic** in worker or delivery goroutine | `Run`, `Deliver` | **Yes** | Would otherwise crash silently | - -The subtle one is the two kinds of "job failed." Today `a.finish(id,"failed",msg)` -lumps them. The split hinges on **whether the transformer honored the contract**: -a valid `result.json` with `success:false` = *handled* (no alert); a crash / -timeout / missing / corrupt result = *systemic* (alert). +## 2. What alerts + +| Failure | Code site | Alert? | Category | Notes | +|---|---|---|---|---| +| Forged/stale webhook (401), malformed/oversize body (400) | `webhook.go` `receive` | No | — | Bot/client noise | +| Unrouted recipient, disallowed sender, no attachments, rejected file type | `receive` | No | — | Benign or sender's fault | +| **Intake failure (500)** — storage/filesystem/Submit | `receive` → `intake` | **Yes** | `intake` | Mailgun retries ~8h; the throttle absorbs the retry burst | +| `store(notify=)` route misconfiguration warning | `receive` | **Yes** | `route-config` | Today it's a log WARNING only; every real submission is being lost | +| **Transformer missing from config** | `app.go` `execute` | **Yes** | `job-systemic` | Misconfiguration | +| **Transformer timed out** | `execute` | **Yes** | `job-systemic` | | +| **Missing/corrupt/invalid `result.json`** | `execute` → `readResult` | **Yes** | `job-systemic` | Contract violation | +| **Nonzero exit without a valid `success:false` result** | `execute` | **Yes** | `job-systemic` | Crash | +| Valid `result.json` with `success:false` | `execute` | No | — | Sender told in the reply | +| **Panic while running a job** | `execute` (new `recover`) | **Yes** | `panic` | Job marked failed; worker continues | +| **Reply send failing** (non-2xx, timeout) for ≥5 min | `outbound.go` `deliver` | **Yes** | `delivery` | Time-based, not per tick, to ride out a blip (§3.5) | +| **`MarkEmailDelivered` failing after a successful send** | `deliver` | **Yes, no grace period** | `delivery-mark` | The database is failing. Only the mark is retried, not the send (§3.5), but every restart until it recovers sends one duplicate reply | +| **Sheets-link publish failing** (token expired, quota, Drive outage) | `deliver` → `publish` | **Yes** | `publish` | Can persist for hours | +| **Drive file orphaned** (`PutDelivery` failed after upload) | `publish` | **Yes** | `publish-orphan` | Names the file ID for manual cleanup | +| **Job claim error** (`store.Next`) persisting ≥1 min | `app.go` `Run` | **Yes** | `worker-claim` | Today it retries silently forever; a stuck DB halts all work | +| Retention sweep: Drive delete failures | `mailgun/retention.go` | **Yes** | `sweep-drive` | World-editable files outliving the 30-day promise | +| Retention sweep: workspace delete failures | `app/retention.go` | **Yes** | `sweep-jobs` | Low urgency; throttled daily anyway | +| **Worker restarted after a crash** | startup (via supervisor, §3.6) | **Yes** | `restart` | Includes exit code and rapid-restart count | +| **Jobs interrupted** (running at the last shutdown) | `store.Open` marks them `interrupted` | **Yes, if N > 0** | `restart` | Second crash signal that needs no supervisor | +| Startup `fatal()` (bad config, DB, incomplete env, bind failure) | `main.go` | **No** | — | No reporter exists yet; covered by heartbeat #5 | +| Mailgun send itself failing | `send` | **Can't** | — | The alert channel is the broken thing; heartbeat #5 | + +**The job-failure split** (`execute`), decided by whether the transformer honored +the contract: + +- `result.json` valid and `success:false` → handled. No alert, whatever the exit code. +- `result.json` valid, `success:true`, but nonzero exit → systemic (the result contradicts the exit code). + The job's message says so rather than repeating the transformer's success text, + which would tell the sender their report is ready in a reply with nothing attached. +- Anything else that fails (timeout, missing/invalid result, missing transformer, + a command that won't start) → systemic. +- Killed because the worker itself is shutting down (Ctrl+C, a failed webhook + listener: the job's context is cancelled) → not the transformer's failure, so + no alert. Otherwise every restart that lands mid-job would read as a crash. + The job is still marked failed, as before. --- ## 3. Design -### 3.1 A `Reporter` dependency (DI, testable) +### 3.1 New package `internal/alert` -Introduce an interface, following the `Engine` pattern already in the codebase: +Neither `app` nor `mailgun` can own this: `mailgun` imports `app`, and both need to +report. A leaf package both import: ```go -type Reporter interface { - // Report records a systemic failure. Implementations must be non-blocking - // enough for hot paths, must throttle, and must never panic or recurse. - Report(category string, detail string, err error) +type Alert struct { + Category string // throttle key, e.g. "job-systemic" + Summary string // one line; becomes the subject + Detail string // job id, operation, input name, error, truncated output } -``` - -- A **no-op reporter** is the default (alerting disabled). -- A **Mailgun-backed reporter** emails `support@example.com` via the existing - send path, with throttling in front. -- Both `*app.App` and the mailgun `Service` take a `Reporter` (constructor - injection), so job-side and email-side systemic failures funnel to one place. -- Tests use a `fakeReporter` that records calls — per the project's - isolation-testing preference. - -### 3.2 Throttle / dedupe (the mandatory middle layer) - -A small stateful wrapper in front of the Mailgun reporter: - -- **Per-category cooldown:** at most one email per `category` per window - (default ~15 min). The next email for a suppressed category includes a - "N more occurrences since last alert" count. -- **Global cap:** a hard ceiling (e.g. ≤ N alert emails/hour) as a backstop. -- Rationale: the delivery loop ticks every second, so an unthrottled delivery - failure = one email/second; a bot spraying 401s (already excluded) would be - worse still. -### 3.3 The feedback loop +// Reporter records a systemic failure. Report must not block or panic. +type Reporter interface{ Report(Alert) } -- The Mailgun reporter's own send failures are **log-only** — never re-reported. -- "Mailgun send is failing" therefore may not reach `support@` at all (it needs - the very channel that's down). This is the structural gap that Phase 4's - heartbeat covers. +type Nop struct{} // the default: alerting disabled -### 3.4 Configuration +// Mailer sends one plain-text message. *mailgun.Service satisfies it. +type Mailer interface { + SendAlert(ctx context.Context, to, subject, text string) error +} -Add to `config/email.yaml` (non-secret; secrets stay in env): +// Ledger is the persisted throttle state. *store.Store satisfies it. +type Ledger interface { + LastAlert(category string) (sentAt time.Time, suppressed int, err error) // zero sentAt: never sent + RecordAlertSent(category string, at time.Time) error // for the caps; leaves suppressed alone + RecordAlertSuppressed(category string) error // suppressed++ + ClearSuppressed(category string) error // after an email carrying the count went out + AlertSendsSince(t time.Time) ([]time.Time, error) // sends after t, oldest first +} -```yaml -alert_recipient: support@example.com # empty/absent => alerting disabled -# alert_from: filemill@mill.example.com # optional; defaults to REPLY_FROM -# alert_cooldown_minutes: 15 +// Emailer is the real Reporter: a throttle in front of a Mailer, draining a +// buffered channel on its own goroutine so Report never blocks a hot path. +func NewEmailer(m Mailer, l Ledger, to string, cfg Config, now func() time.Time, log *log.Logger) *Emailer +func (e *Emailer) Run(ctx context.Context) // started by main; drains the queue ``` -Alerting is enabled only when `alert_recipient` is set and Mailgun is configured. - -### 3.5 Alert content +- `Report` does a non-blocking send on a buffered channel (e.g. 64). If the buffer + is full, it logs and drops the alert. A flood that fills it would be throttled + anyway. +- The throttle is **per-category cooldown** (default 15 min), plus two **global + caps**: 10 emails/hour and **20 emails/day** (a rolling 24h window). A + suppressed alert increments the count, and the next email for that category + says "N more since the last alert". +- **The daily cap is the binding one.** The Mailgun Free plan allows 100 sends a + day, *shared with replies*, and hard-rejects past that until the next day. The + hourly cap alone would allow 240/day, enough to lock out every reply to senders. + 20/day keeps alerts to at most a fifth of the day's budget. When the daily cap + is hit, one last alert says so ("daily alert cap reached; further alerts are + logged only until