counters/review

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
  • B — security, contracts and operability: reviewer-b-security-contracts.md
• Each worked in its own scratch copy and proved findings with tests: _proof_tests/.
• 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.