Add 80% limit warning, stop repeat notifications, fix review findings

Notifications:
- Warn once when a counter reaches warn_percent of its limit, through
  separate warn_*_url endpoints.
- Write ntf/ markers so a lost Redis reply cannot re-send a breach.
- Retry only when the request never left or the web app answered 429/5xx.

Fixes:
- Retry transactions Redis rejected; treat pool timeouts as definite.
- Install the signal handler before startup work.
- Stop logging the login suffix; redact notify URL credentials.
- Reject corrupt or empty CDBs, truncated hash tables included.
- Require a Sentinel password, refuse example placeholders, and default
  admin_addr to localhost.
- Normalise /counters domains, count on the arrival day, log rejections,
  hide internals in errors and exit 1 on a failed drain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Dimitrije Dimitrijevic
2026-11-09 09:01:56 +01:00
co-authored by Claude Opus 5.5
parent c43ff17627
commit 54f9626121
29 changed files with 1673 additions and 245 deletions
+18 -49
View File
@@ -1,6 +1,6 @@
# TODO — migadu_counters
State as of 2026-10-15. Review findings refer to `review/README.md`; the proof tests live in
State as of 2026-11-09. Review findings refer to `review/README.md`; the proof tests live in
`review/_proof_tests/` and should become regression tests as each item is fixed (round 1 and items
7–8 are mapped at the end of `review/_proof_tests/README.md`).
@@ -12,58 +12,24 @@ State as of 2026-10-15. Review findings refer to `review/README.md`; the proof t
- #7 plus-addressing counts against the mailbox (`domains.Mailbox`).
- #8 malformed CDB row is "not hosted" + warning (`domains.ErrMalformed`).
## Review round 2 — remaining findings
## Review round 2 — done 2026-11-09
Suggested order: #10, #15, #9, then the rest in one sweep.
#9-#17 fixed, plus the new findings from the 2026-11-09 review: repeat notifications after a lost Redis
reply, pool timeouts treated as unknown outcomes, login suffix in the counting log, no retry of rejected
transactions, `/counters` punycode, counter day taken from arrival time, CDB corruption checks, context
deadlines on Redis sockets, example config path and placeholders, non-zero exit on a failed drain.
Also added: the 80% early warning (`warn_percent`, `warn_*_url`) and per-counter notification markers.
### Can stop the daemon or lose counts
## Open
- [ ] **#10 SIGHUP during startup kills the daemon.** The signal handler is installed after the Redis
ping (up to 2 s). Install `signal.Notify` before anything slow in `cmd/migadu_counters/main.go`
(`run`), and queue a reload that arrives before the app is ready. Evidence: `review/_proof_tests/a/run/`.
- [ ] **#15 Sentinel mode starts without a Redis password.** Every write fails; only `/health` and
metrics show it. Make an empty `redis.password` fatal in `config.Validate` when `sentinels` is set
(allow an explicit opt-out such as `password_optional = true` for dev). Proof:
`b/internal/config/review_b_test.go`, case "sentinel without password".
- [ ] **Persistent / recoverable counting on Redis outage** (deferred from round 1, #4). A Redis stall
longer than a few seconds fills the queue and new requests are dropped (503, `dropped_total`).
Options: marker key per transaction for exact lost-reply recovery; a disk-backed queue. Decide
whether the drop-and-alert behaviour is acceptable before building anything.
### Correctness
- [ ] **#9 Notification retries can send duplicate customer emails.** The web app's mailbox-level
handler (`core/email.ex:3572-3584`) emails on every call; the org handler is guarded once per day.
In `internal/notify`: treat a slow-but-2xx attempt as success (do not retry after the client timeout
if the request was sent — or make the retry budget shorter than the web app's handling time), do
not keep the concurrency slot while backing off, let Shutdown finish pending retries up to its
deadline, and fix the "after N attempts" count in the failure line.
- [ ] **#12 Override keys are not lowercased.** `Info@/o=5` would be ignored and the domain default
used instead. Lowercase the key in `domains.ParseLimits`; add a test row. No such keys in today's
data.
- [ ] **#16 counterctl warns one day too early.** A date exactly 31 days back is reported as expired.
Compare against the key TTL in whole days (`now.UTC` day minus date > 31). Proof:
`b/cmd/counterctl/review_b_test.go` `TestReviewB_31DayWarningOffByOne`.
- [ ] **#18 Work keeps running after a handler timeout.** Not reproduced by the reviewer and largely
covered by the queue change (the synchronous part only validates). Re-check that
`http.TimeoutHandler` cannot leave a goroutine writing after the 503, then close.
### Operability / security hygiene
- [ ] **#11 Rejected requests leave no trace.** 4xx rejections write nothing to the counting log and no
`trace_id` to the app log. Decode `trace_id` best-effort before rejecting; add a
`rejected: <status> <reason>` line to the counting log and `trace_id` to the warn line. Proof:
`b/internal/server/review_b_test.go` `TestReviewB_RejectLogHasNoTraceID`.
- [ ] **#13 Credentials in the notify URL are written to the log.** Redact `user:pass@` from URLs in
the txlog `sent/failed notification` lines; warn at startup when a notify URL is plain `http://`
with basic auth. Proof: `b/internal/server/review_b3_test.go`.
- [ ] **#14 Error responses expose internals.** 503 bodies carry Redis addresses and file paths; keep
the detail in the log and answer a generic message. Default `admin_addr` to `127.0.0.1:9095`
(currently `:9095`, all interfaces, no auth). Proof: `TestReviewB_ErrorLeak`.
- [ ] **#17 A recovered panic is not counted in `requests_total`.** The recoverer sits outside
`observe`; swap the order or record the 500 explicitly. Proof: `TestReviewB_PanicNotInMetrics`.
- [ ] **Test gaps (B F13).** Watcher tests can occasionally pass on a stale signal — make them wait for
the specific reload they triggered. Add a test for `handler_timeout` vs queue timeout interplay.
Decide whether drop-and-alert is acceptable before building anything.
- [ ] **Exempt `mail_from` on `/in`.** An outside sender using `MAIL FROM:<support@migadu.com>` avoids the
recipient's incoming count, unless mizu rejects such senders inbound (not confirmed). Matches the
old Lua rule; decide whether to keep it.
- [ ] **Test gap.** Watcher tests can occasionally pass on a stale signal: make them wait for the
specific reload they triggered.
## Outside this repo (deployment prerequisites)
@@ -77,9 +43,12 @@ Suggested order: #10, #15, #9, then the rest in one sweep.
`contrib/migadu_counters-pull.cron` (see `contrib/README.md`), agent host key acceptance,
firewall: mailqueuer hosts → :5555, service → Sentinel :26379/:6379 and admin.migadu.com:443.
- [ ] **migadu_web** `/api/limits` `D` branch (domain level); until then `notify.domain_url` stays empty.
- [ ] **migadu_web3 branch `limit-warning-notifications`**: review the warning email wording, merge and deploy,
then set `warn_org_url` and `warn_address_url`. Until then warnings stay off.
- [ ] **Firewall / monitoring**: `admin_addr` now defaults to localhost; set it explicitly if `/metrics` or
`/health` are scraped from another host.
- [ ] **Limits re-tuning**: outgoing is counted per recipient (Lua counted per message), so existing
`*/o` limits are stricter. Review before relying on `outgoing_limit_exceeded!`.
- [ ] **Retire stats-merger's quota module** once this service is the source (only fernrohr reads it).
- [ ] **Dependencies**: `golang.org/x/net` is pinned to v0.43.0 because the offline module cache had
it; run `go get -u golang.org/x/net && go mod tidy` with network access.
- [ ] **git init** the repo (never asked for so far).