counters/review/README.md

# Code review — 2026-10-14

Review of the first implementation of migadu_counters (see `../plans/2026-10-13-migadu-counters.md`).
Nothing in the code was changed by the review.

## How it was done

- Two independent reviewers (agents) each read the whole repository. Neither saw the other's results.
  - **A** — correctness and runtime behaviour: [`reviewer-a-correctness.md`](reviewer-a-correctness.md)
  - **B** — security, contracts and operability: [`reviewer-b-security-contracts.md`](reviewer-b-security-contracts.md)
- Each worked in its own scratch copy and proved findings with tests: [`_proof_tests/`](_proof_tests/README.md).
- The coordinating agent then verified every finding (see "Verification" below), merged duplicates and ranked them.
  18 distinct issues remain; 4 of them were found by both reviewers.

## Findings

Who found it: A / B / both, with the reviewer's own IDs for traceability.

### High

**1. If mailqueuer disconnects or times out before the reply, the count is lost** — A (F1)
- `internal/server/server.go:156` runs the Redis write with the request context. When the client hangs up, Go cancels
  the context and go-redis aborts before sending MULTI.
- Re-run during verification: 20 of 20 delayed requests and 300 of 300 immediate ones were not counted. The most
  dangerous finding, because fire-and-forget clients often close early.
- Fix: run the counting under `context.WithoutCancel(r.Context())` with its own time budget.

**2. A bad value in a field that isn't counted rejects the whole request** — both (B F1, A F3)
- `internal/api/validate.go` strictly validates `mail_from` although it only feeds the exempt check; a 400 there means
  the recipient is never counted.
- Senders trigger it easily: mizu's SMTP parser turns `"john smith"@x.com` into `john smith@x.com`, mizu runs with
  SMTPUTF8 (`info@bücher.de`), address literals and trailing dots pass, and mailqueuer forwards `X-Mail-From` unchecked.
- On `/out`, an unusual external recipient (`x@[192.0.2.1]`, `x@host_name.example.com`) gives 400 and the sender's
  outgoing count is lost too.
- Fix: never reject because of `mail_from`; validate sender and recipient sides separately and count whichever is
  valid; convert UTF-8 domains to punycode.

### Medium

**3. Support/token logins aren't counted** — A (F2)
- mizu puts the raw SASL login into `X-Auth-User` (`pkg/smtp/auth_session.go:160` → `poster.go:186`), so master logins
  arrive as `user@dom@SUFFIX` and fail validation with 400.
- Fix: strip the second `@SUFFIX` and count the base address.

**4. Redis failures with an unclear outcome lose the limit notification** — both (B F3, A F4)
- If Redis applies MULTI/EXEC but the reply times out, or one command inside EXEC fails (a key holding a non-number)
  while the others succeed, the service answers 503 and logs "nothing counted" — but the counters moved.
- If that increment reached the limit, no later increment can reach it again: no notification that day.
- Fix: use each command's own result; after a timeout re-read the keys from the master; log "outcome unknown".

**5. Huge CDB records are re-parsed on every request** — A (F5)
- `boxmailhere.com` (193 KB, 9,798 overrides): about 0.9 ms and 1.4 MB per lookup, 2.2 ms and 1.6 MB per request
  (re-measured).
- Fix: cache parsed records per loaded CDB.

**6. An empty `auth_token` leaves the API open with only a warning** — B (F4)
- A typo such as `auth-token =` starts the daemon unauthenticated; anyone who can reach it can inflate any customer's
  counters, which triggers the web app's limit actions.
- Fix: make an empty token fatal unless explicitly opted out; make unknown config keys errors.

**7. Plus-addressed recipients get their own counters** — B (F2); A notes the Lua plugin did the same
- Mail to `b+1@…` creates `cnt/A/b+1@…`; the mailbox's own counter (the one the web app charts) never moves. If such
  a counter reaches its limit, the web app answers "mailbox not found".
- rcptd strips the `+tag`, so stripping it here too seems right. **Needs a product decision.**

**8. One malformed CDB row breaks counting for anything touching that domain** — both (B F5, A F6)
- A non-numeric org id gives 503 and drops the other side's valid count; the plan says such rows are skipped.
- Fix: treat the row as not hosted and log a warning.

### Low

9. **Notification retries can send customers duplicate emails** — B (F11), A (F9). Resolved disagreement: the
   organisation handler has a once-per-day guard (`migadu_web core/accounts.ex:1928-1956`), the mailbox handler
   (`core/email.ex:3572-3584`) emails on every call, so a retry after a slow-but-successful call means a second
   email. Also: shutdown cuts off pending retries, retries keep their concurrency slot while waiting, and the
   "after 5 attempts" log line can be wrong.
10. **A SIGHUP during startup kills the daemon** — A (F8). The signal handler is installed only after the Redis
    ping, which can take up to 2 s.
11. **Rejected requests leave no trace** — both (B F9, A F2 note). Nothing in the counting log, no trace_id in the app
    log; together with #2 the lost counts are invisible.
12. **Override keys aren't lowercased** — A (F7). `Info@/o=5` would be ignored; none in today's data.
13. **Credentials in the notify URL are written to the log** — B (F7); plain `http://` with basic auth isn't warned about.
14. **Error responses expose internals** — B (F10): internal Redis addresses and paths; `admin_addr` defaults to all
    interfaces without auth.
15. **Sentinel mode starts without a Redis password** — B (F12): every write then fails; only `/health` and metrics show it.
16. **counterctl warns one day too early** — B (F6): a date exactly 31 days back is reported as expired.
17. **A recovered panic isn't counted in `requests_total`** — B (F8).
18. **Work keeps running after a handler timeout** — A (F10, not reproduced): not tracked by shutdown, a late panic is
    silently lost. Largely covered by the fix for #1.

**Test gaps** — B (F13): the suite misses most of the above; `TestValidation_JSONAndFields` asserts the 400 behind #2
as correct; watcher tests can occasionally pass on a stale signal.

## Verification

Done by the coordinating agent after both reviews:

- Both scratch copies were checked to have source files identical to the repo, so the proof tests ran against the
  reviewed code. B's tests fail as described; A's demonstration tests reproduce their evidence (#1 was worse on
  re-run: 0 counted instead of 21 of 300).
- External facts checked in the sibling repos:
  - mailqueuer reads `X-Mail-From` without validating it (`internal/handler/ingest.go`).
  - mizu's go-smtp fork unquotes quoted local parts and copies the domain verbatim (`parse.go` `parseMailbox`,
    `parseLocalPart`); mizu sets `EnableSMTPUTF8 = true` (`cmd/mizu-server/main.go:1363`).
  - mizu stores the raw SASL username as the authenticated user (`pkg/smtp/auth_session.go:160`) and sends it as
    `X-Auth-User`; `user@domain@SUFFIX` logins are documented in mizu's `CLAUDE.md`.
  - Web app: org-level limit handlers are guarded once per day, the mailbox-level ones are not (see #9).
- Reviewer disagreements: #7 (A: Lua parity, B: defect) is left as a product decision; #9 settled as above.

## Suggested order

Fix #1–4, #6, #8 and #10 first (small, they protect the counts), then #5 and #7 (#7 after the decision), then the
low items. Turn each proof test into a regression test as its finding is fixed.