findings of the reviews of audit stage 1
test / test (push) Canceled after 0s

Technical review (Opus) and security review (Fable) of 9efc9c7..a59e3d4;
reports in the session's scratchpad. Fixed:

- B1 (verified bug): store.UpdateStatus read before writing in its
  transaction; a commit by another connection in between (the dmarc-ingest
  process opens the same file) failed it at once with "database is locked"
  and the delivery line was lost. The first statement is now a write.
- R3: a delivery line updated every row of its queue id and recipient; it
  now updates only the newest (Postfix reuses short ids). The sweep closes
  by row id (QueuedDelivery.ID, CloseUnreported(id)).
- R4: a contradicted sweep keeps a mark, reason "reconcile-contradicted",
  instead of losing which row it was.
- R1: contradicted cannot see a lost line (never read late). New counter
  reconcile_missed: at close, the sweep looks for the row's delivery line
  in mail.log and mail.log.1 (QueueLines). 0007 edited - it has run on no
  database but throwaway stands.
- R2: queued_over_lifetime cannot measure "what removing the sweep would
  leave" while the sweep runs; its text now says rows stuck despite the
  sweep, and the rule uses closed - missed - contradicted.
- cert-reload (R5, R6, R7, security medium/low): both files copied to .new
  and the copies checked as a pair before both are renamed; acts on the copy
  only when postconf says Postfix reads it; a failed reload is retried next
  pass; umask 077; says so when the mounted files cannot be read. The e2e
  case asserts Postfix reads the copy, the copy equals the mount, the key is
  600 root.
- Security low: a recipient with a quoted local part - the journal cut
  "a b"@x at the space, deliveryRe cut "a>b"@x at the ">" - matched no
  delivery line and was closed by the sweep. cleanAddress ends a bracketed
  address at the first ">" outside quotes; deliveryRe takes the recipient
  up to the ">, " before relay= (test fixtures given the relay= every real
  delivery line has).
- Security low: postqueue -p's error carries at most 512 bytes of output.
- Nits: addDeliveryLogDay refuses a column not on its list; the truncation
  test checks its message.
- To stage 2b (page wording, mockup-first): the history step of a bounced
  row says the receiving server refused it, false for a row the sweep
  closed. To the owner when stage 2b is reached: since M13 the "deferred"
  figure counts whole messages too, which "Connections turned away" would
  not name.

Found while verifying: the e2e's main positive path failed once with "553
not owned by user app1" - an smtpd idle from before the panel's reload took
the connection in the reload's second. Measured on the stand (one-off
probe, not committed): sending 0, 10, 40, 110 s after the save (two seconds
at the soonest) always met an smtpd started after the reload. The e2e now
waits for the sender map after each application it creates
(waitForSenderMap: AUTH, MAIL, RCPT, RSET - no message, no row), and the
level-1 case uses it instead of its own loop.

Docs: architecture § Queue reconcile, schema-migrations (rows 7 and 8,
chain head 8), CHANGELOG, audit plan (stage 1 note, the measured window,
a new stage 2b line: 62 lines), roadmap 15/62.

Evidence on the test server for this tree: go vet clean, go test ./... 23
packages ok, make e2e exit 0 (renewed certificate, hung panel, level 1 at
49/60).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
mix
2026-10-14 15:21:52 +03:00
co-authored by Claude Opus 5.5
parent f51f5db245
commit 60c7b85f84
26 changed files with 503 additions and 143 deletions
+29 -15
View File
@@ -417,27 +417,41 @@ guessing where it need not: it starts only after the tailer has read to
end-of-file once (on a restart the log itself holds the answer); the two-minute
grace covers messages merely in flight; a `postqueue` that cannot be read, or
whose listing is over the 4 MiB cap, leaves every row untouched rather than
closing them all or those past the cut; and the close is conditional on the
row still being `queued`, so a status the log wrote in the meantime stands.
closing them all or those past the cut; and the close, by the row's id, is
conditional on the row still being `queued`, so a status the log wrote in the
meantime stands. A delivery line updates only the newest row of its queue id
and recipient: Postfix reuses a short queue id once the message has left the
queue, and an old row must not be rewritten. `UpdateStatus`'s transaction
starts with a write, so a commit by the `dmarc-ingest` process in between
cannot fail it with `database is locked`.
Now that the log survives the container, reaching this path should mean the
lines are gone for good — rotated past fourteen files while the panel was down,
or deleted — or that the tailer was not reading.
Whether the sweep earns its place is measured, not argued (audit 2026-10-12,
H3). `delivery_log_days` (migration `0007`) keeps, per UTC day for a year:
`reconcile_closed`, the rows the sweep closed; `reconcile_contradicted`, those
of them a delivery line later spoke of - the log's status replaces the sweep's
and the reason is cleared, so a row counts once; `tailer_blind_seconds`, the
time `follow()` could not read `mail.log` (the file gone, reopening it failing;
stretches under three seconds - a rotation's tick between rename and create,
which loses nothing - are not counted); and `queued_over_lifetime`, the rows
still `queued` past the queue lifetime plus a maximal backoff, as the day's
last sweep saw them. No page shows the table: it is read off a copy of the
database. The rule, written before the data: `reconcile_closed = 0` for a month
→ remove the sweep; closed but never contradicted → keep it; contradicted →
each case explained by blind time means fix the tailer and keep the sweep,
otherwise mark such rows `lost` or remove the sweep. Decided after thirty days,
or sooner if `reconcile_contradicted` is ever non-zero.
`reconcile_closed`, the rows the sweep closed; `reconcile_missed`, those of
them whose delivery line was in `mail.log` or `mail.log.1` when the sweep
closed them - the tailer did not read a line that was there, which is what a
lost line looks like (it is never read late); `reconcile_contradicted`, those
a delivery line read later gave a status - the log's status replaces the
sweep's and the row's reason becomes `reconcile-contradicted`, so a row counts
once and can be found; `tailer_blind_seconds`, the time `follow()` could not
read `mail.log` (the file gone, reopening it failing; stretches under three
seconds - a rotation's tick between rename and create, which loses nothing -
are not counted); and `queued_over_lifetime`, the rows still `queued` past the
queue lifetime plus a maximal backoff, as the day's last sweep saw them - rows
stuck although the sweep runs (mail on hold, or a queue that could not be
listed). What removing the sweep would leave queued is closed − missed −
contradicted: the rows nothing in the log ever resolved. No page shows the
table: it is read off a copy of the database. The rule, written before the
data: `reconcile_closed = 0` for a month → remove the sweep; closed, but
neither missed nor contradicted → keep it; missed or contradicted → the tailer
loses or delays lines - fix the tailer (the two deferred steps: the rotation
window, pausing the sweep while blind) and keep the sweep; if what remains
after that is never resolved by the log, mark such rows `lost` or remove the
sweep. Decided after thirty days, or sooner if `reconcile_missed` or
`reconcile_contradicted` is ever non-zero.
**Stored lines** (`send_log_lines`, since 2.2.0;
[internal/logtail/lines.go](../internal/logtail/lines.go)). A send-log row
+32 -2
View File
@@ -584,7 +584,12 @@ has already run is never edited — the next file is added; the published
from`. In the product this is a second or so after an application is
saved in which its first message can get a permanent 553; not worth a
change (an application is configured after it is created, not within the
second). The other cases are held to `451 4.7.1` (level 2, both MAIL FROM
second). *Measured 2026-10-14:* applications created and used after 0,
10, 40 and 110 s of waiting (the first send two seconds after the save at
the soonest) were all served by an smtpd started after the reload; the
stale one only ever answered within the reload's own second. The e2e
waits for the map after each application it creates (`waitForSenderMap`),
after the main positive path failed on it once. The other cases are held to `451 4.7.1` (level 2, both MAIL FROM
of one connection), `553` at RCPT (sender mismatch), `554 … Client host
rejected` at RCPT (no AUTH, and the relay case, whose comment claimed
`reject_unauth_destination` - on 465 the client restriction comes first;
@@ -621,6 +626,15 @@ has already run is never edited — the next file is added; the published
explained by `tailer_blind` means fix the tailer and keep the sweep,
otherwise `lost` or remove. Decide after 30 days of data, or sooner if
`contradicted` is ever non-zero. — **Opus**
*Changed by the reviews of stage 1 (2026-10-14):* a fifth counter,
`reconcile_missed` - a closed row whose delivery line was in `mail.log` or
`mail.log.1` when the sweep closed it; `contradicted` alone cannot see a
lost line, which is never read late. `queued_over_lifetime` cannot show
"what removing the sweep would leave visible" while the sweep runs (those
rows are closed by then); it counts rows stuck despite the sweep, and what
removing it would leave is `closed - missed - contradicted`. A
contradicted row keeps the mark `reconcile-contradicted`. The rule reads
accordingly in architecture.md § Log tailer.
- *Deferred until the data is in* (not a step yet): pausing the sweep while the tailer is
not reading (publish "healthy" from `follow`, skip the sweep otherwise),
and the rotation window (keep draining the old inode until it has not
@@ -747,6 +761,16 @@ order - each stands on the one before.
sentence stays only for a row whose line was truly never written or is
older than this change, and says that. Done when the e2e shows the stored
line on the page of each of the three refusals. - **Opus**
- [ ] **A message the sweep closed does not say the receiving server refused
it** (review of stage 1, 2026-10-14). The history step of a `bounced` row
reads "the receiving server refused the message permanently, or Postfix
gave up ... The reason is in the delivery log below"
([handlers_monitor.go](../internal/web/handlers/handlers_monitor.go)); for
a row with reason `reconcile` neither is known and the log has nothing.
Such a row gets its own step: Postfix no longer held the message and
`mail.log` never said what became of it, so the panel closed it as a
failure. Wording in the mockup `out-message` first (`design:` commit),
with the box of the step above. - **Sonnet**
- [ ] **A refusal the panel did not get to write is written from
`mail.log`** (owner, 2026-10-13, from the Not done list of
from-header-binding; built on the step above). The check answers 550 and
@@ -775,7 +799,13 @@ order - each stands on the one before.
becomes *Connections turned away*. Postfix refuses from the connection on
while the check does not answer, before any message is offered, so a
client that connects three times with one message is three, and a monitor
that only connects is one; messages cannot be counted. The mockup `health`
that only connects is one; messages cannot be counted. *To ask the owner
when this step is reached (2026-10-14):* since the M13 fix the figure
also counts whole messages - one a panel that hangs mid-message makes
cleanup defer (one per message, with a queue id) - so "Connections turned
away" would be half true; my recommendation: "Turned away" with the help
saying it counts connections refused at the start and messages deferred
in the middle. The mockup `health`
and help topic `sender-check` first (`design:` commit), then the page and
`view/help.go`, word for word. - **Sonnet**
- [ ] Docs: architecture § Log tailer (the rotated file, the saved place, a
+1 -1
View File
@@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
| from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) |
| panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) |
| schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) |
| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 15/61 | [audit-2026-10-12.md](audit-2026-10-12.md) |
| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 15/62 | [audit-2026-10-12.md](audit-2026-10-12.md) |
**Recommended order** (not binding; owner, 2026-10-09: feature order follows
development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and
+6 -4
View File
@@ -17,8 +17,8 @@ When you add or review a migration, update this file in the same change.
| Field | Value |
|---|---|
| Chain head | `user_version = 7` |
| Files | `0001_init.sql` (the 2.0 baseline), `0002_inbound_spam_log.sql`, `0003_send_log_lines.sql`, `0004_notifications.sql`, `0005_send_log_rejected_index.sql`, `0006_sender_check.sql`, `0007_delivery_log_days.sql` |
| Chain head | `user_version = 8` |
| Files | `0001_init.sql` (the 2.0 baseline), `0002_inbound_spam_log.sql`, `0003_send_log_lines.sql`, `0004_notifications.sql`, `0005_send_log_rejected_index.sql`, `0006_sender_check.sql`, `0007_delivery_log_days.sql`, `0008_tenant_boundary.sql` |
| File mark | `PRAGMA application_id = 0x53503230` ("SP20") |
| Database file | `/data/selfpost.db` (bind mount) |
| Compatibility | 2.x MINOR releases must boot a `2.0.0` data directory. **Nothing is carried over from 1.x.** |
@@ -66,7 +66,8 @@ inbound-antispam-panel (`2.1.0`), then `0003_send_log_lines.sql` of
delivery-log-storage (`2.2.0`), `0004_notifications.sql` and
`0005_send_log_rejected_index.sql` of panel-notifications (`2.3.0`), and
`0006_sender_check.sql` of from-header-binding (`2.4.0`), and
`0007_delivery_log_days.sql` of the 2026-10-12 audit (`2.5.0`).
`0007_delivery_log_days.sql` and `0008_tenant_boundary.sql` of the 2026-10-12
audit (`2.5.0`).
---
@@ -80,7 +81,8 @@ delivery-log-storage (`2.2.0`), `0004_notifications.sql` and
| 4 | `0004_notifications.sql` | 2.3.0 | DDL | `users.notify` (default 1: every user starts with notifications on) and `notify_state` |
| 5 | `0005_send_log_rejected_index.sql` | 2.3.0 | DDL | `idx_send_log_rejected`, a partial index of the refused rows of `send_log` by time; a file of its own because the test installation had run 0004 before the review asked for it |
| 6 | `0006_sender_check.sql` | 2.4.0 | DDL | `send_log.reason` and `send_log.header_from` (both default `''`: a row refused earlier keeps no reason), and `sender_check_days` |
| 7 | `0007_delivery_log_days.sql` | 2.5.0 | DDL | `delivery_log_days`: what the reconcile sweep and the log tailer did, a row per UTC day, kept a year, shown nowhere - the measurement that decides the sweep's fate (audit 2026-10-12, stage 1). `send_log.reason` gains the value `reconcile` without a schema change |
| 7 | `0007_delivery_log_days.sql` | 2.5.0 | DDL | `delivery_log_days`: what the reconcile sweep and the log tailer did, a row per UTC day, kept a year, shown nowhere - the measurement that decides the sweep's fate (audit 2026-10-12, stage 1). `send_log.reason` gains the values `reconcile` and `reconcile-contradicted` without a schema change |
| 8 | `0008_tenant_boundary.sql` | 2.5.0 | DDL | `idx_applications_login_nocase`: an application login is unique without regard to case (audit 2026-10-12, H4) |
**Kind:** *DDL* — schema only; *data* — `INSERT`/`UPDATE` that must stay correct
for operators upgrading from older 2.x images.