diff --git a/docs/adr/0002-multi-engine-sync-model.md b/docs/adr/0002-multi-engine-sync-model.md new file mode 100644 index 0000000..2e45fa1 --- /dev/null +++ b/docs/adr/0002-multi-engine-sync-model.md @@ -0,0 +1,282 @@ +# ADR 0002 — Convergent sync model for multiple concurrent engines + +- **Status:** Accepted +- **Date:** 2026-08-10 +- **Deciders:** James Maes +- **Extends:** [ADR 0001](0001-engine-first-grpc-architecture.md). Nothing here + changes the engine-first, contract-published architecture; this decides how a + daemon behaves as one of several clients of the same mail account. + +## Context + +ADR 0001 establishes one daemon owning all state. It does not say what happens +when **several** daemons own state derived from the **same** upstream account — +which is the normal case for a local-first product the moment a user runs it on +a laptop and a desktop, and is already the case today whenever the user also has +a phone mail app or Gmail open in a browser. + +The engine currently implements a **single-writer, append-only** model: + +- Mail sync is a forward-only per-folder high-water mark (`{uidnext}:*`), and + `fetch_and_persist` only ever calls `save_email`. Nothing observes deletion. +- CalDAV/CardDAV sync re-queries a full window every time; there is no + sync-token, no CTag, no ETag use, and no write-back at all. +- Local identity is `surrogate_id(account, folder, uidvalidity, uid)`, so the + same message in a different folder is a different message. +- Mutations are fire-and-forget: `apply_mutation` returns `Result<()>` and + `Ok(())` is recorded as `completed` with no check that anything happened. + +The consequences are not hypothetical. A message moved or deleted by any other +client becomes a permanent local ghost. A mutation that lost a race against +another client is reported as success, because a UID command against a UID that +no longer exists returns `OK` having done nothing (RFC 3501 §6.4.8). Two engines +diverge silently; ten diverge identically and confidently. + +A design was drafted, then reviewed independently for protocol correctness, +codebase fit, and adversarially. The reviews falsified four load-bearing claims +in the draft. This ADR records the corrected decision; the evidence is in +[`docs/reviews/2026-08-10-multi-engine-design-review.md`](../reviews/2026-08-10-multi-engine-design-review.md). + +## Pre-release posture + +Nothing is released. There is no installed base, no data anyone is entitled to +keep, and no compatibility obligation. **Getting this model right outranks +continuity of any store that exists today**, and the decisions below are made on +that basis. + +Concretely, and in contrast to how a shipped product would have to reason: + +- **A local store is disposable.** Where a correct model needs data that cannot + be reconstructed from what is persisted, the answer is to reset the store and + re-sync from the server -- never to carry a degraded identity forward forever + so that old rows survive. A permanently un-deduped subset of history is a + worse outcome than a one-time re-sync on a developer's machine. +- **Behaviour may change without a deprecation path.** A default that is wrong + gets corrected, not preserved behind a flag. +- **Mixed-version fleets are not a supported configuration.** Upgrade all + daemons for an account together. `PRAGMA user_version` still earns its place + as cheap insurance against a stale binary silently writing an old model, but + the design owes nothing to interoperating with an older one. + +This section exists to be **deleted at the first real release**, at which point +every migration question below becomes a genuine obligation. + +## Decision + +### 1. The servers are the coordinator. We do not build a distributed system. + +No engine-to-engine messaging, no leader election, no shared database, no vector +clocks or CRDTs, no broker, no custom sync protocol. IMAP and WebDAV are designed +for many concurrent clients against one collection and ship the primitives for +it. If each engine is a **correct** client, N engines is **O(N) server load and +O(1) design complexity**. + +The work is therefore not "add multi-engine support." It is "finish the sync +model"; multi-engine falls out of it. + +### 2. There is one bounded exception: side-effecting actions. + +The servers coordinate *state*, not *actions*. `MOVE`/`COPY`/`FLAG` converge +under at-least-once execution — ten moves produce one net effect. **`FORWARD` +and `CALL WEBHOOK` do not.** They land on third parties with no shared resource +to compare-and-set against, and every engine independently classifies the same +message as new. Ten engines page an on-call engineer ten times per alert. + +- A per-account `filters_enabled` setting, **default off**, gives exactly one + engine ownership of filter execution. This converts a silent duplication bug + into an explicit, documented limitation, and requires no protocol work. +- Where CONDSTORE and custom keywords are available, an engine may instead claim + a message with `UID STORE +FLAGS.SILENT (UNCHANGEDSINCE ) + ($NuncioFiltered)`; a `MODIFIED` response means another engine won the claim. + This is the same compare-and-set primitive as §4, so it adds no new machinery. +- Rule identity becomes a content hash of normalized NSQL rather than random + bytes, and side-effecting actions carry `Idempotency-Key: + hash(rule_key, message_key)`, so receivers can dedup what we cannot make + once-only. + +### 3. Change enumeration replaces the high-water mark — four rungs, per mailbox. + +`CollectionSync::enumerate_changes(collection, since) -> {upserts, removals, +next_token}`. The `removals` set is the entire fix for ghost messages, and it +fixes the single-engine case too. + +| Rung | Mechanism | Providers | +| --- | --- | --- | +| 1 | QRESYNC: `ENABLE QRESYNC` + `SELECT (QRESYNC …)` → `VANISHED` | Fastmail/Cyrus, Dovecot | +| 2 | CONDSTORE: `UID FETCH 1:* (FLAGS) (CHANGEDSINCE n)` + periodic UID-set diff | **Gmail** | +| 3 | `STATUS`/`LIST-STATUS` change gate, then ESEARCH UID-set diff | **Exchange**, Yahoo | +| 4 | Full resync | last resort | + +**Gmail advertises CONDSTORE but not QRESYNC; Exchange advertises neither.** +Detection is per *mailbox*, not per account, because `NOMODSEQ` is a mailbox +property. A two-rung ladder would put the largest provider on the most expensive +path. + +CalDAV and CardDAV both use RFC 6578 `sync-collection` (identical shape), with +`sync-level: infinite`, 507 truncation handling, loop-to-convergence, and +`calendar-multiget`/`addressbook-multiget` to fetch bodies for changed hrefs. +CTag is a vendor extension, not a peer mechanism, and is a fallback only. + +Sync state is structured, not a string: `(uidvalidity, highestmodseq, +known-uid-set, seq-match-sample)`. QRESYNC **never signals modseq expiry** — a +too-old modseq produces a large `VANISHED`, not an error — so a full resync is +always a client-side derivation from a UIDVALIDITY mismatch, `NOMODSEQ`, or a +HIGHESTMODSEQ regression. A periodic full UID-set reconciliation is retained +even on rung 1 as a safety net. + +**Sync tokens carry a scheme tag** (`v2:qresync:…`). The existing checkpoint is +`{uidvalidity}{DELIM}{uid}` — the same shape as `uidvalidity:highestmodseq` with +a different meaning. Untagged reuse would hand a UIDNEXT to a server as a +MODSEQ; because UIDNEXT is typically the larger number, the server would answer +"nothing changed" and everything below it would never be enumerated again. +Unknown schemes force a full resync. + +### 4. Mutations carry a precondition and must prove their effect. Three outcomes. + +``` +MutationOutcome = Applied { token: Option } | Conflict { observed } | Unknown +``` + +`Conflict` requires **positive evidence**: `[MODIFIED …]` in the tagged +response, or HTTP 412. Anything ambiguous is `Unknown`, which triggers +re-enumeration of the affected collections rather than a verdict. + +This matters because the obvious proofs are unreliable: + +- `COPYUID` is only *SHOULD* for MOVE (RFC 6851 §4.3), is advised in an + **untagged** OK, has no empty form, and is legitimately omitted for + UIDNOTSTICKY destinations. Present ⇒ applied; absent ⇒ **unknown**. +- Under QRESYNC, `UID EXPUNGE` returns `VANISHED`, not `EXPUNGE` (RFC 6851 §4.4), + so counting `EXPUNGE` responses reports failure on every successful delete. +- `+FLAGS.SILENT` suppresses the untagged FETCH (RFC 3501 §6.4.6) and is the + normal form for conditional stores, so "no FETCH ⇒ conflict" fires on the most + common flag operation. +- A DAV server that rewrites a submitted object MUST NOT return an ETag + (RFC 4791 §5.3.4), and normalisation is the norm; `Applied` must tolerate a + missing token and schedule a re-read. + +Flags use per-flag last-writer-wins with benign merge. RFC 7162 §3.1.3 only +*SHOULD*s avoiding spurious `MODIFIED` on single-modseq servers, so a blanket +hard-conflict policy would make routine concurrent flag activity a constant +conflict stream. + +### 5. Conflicts are durable and queryable, not a notification. + +Mutations model **desired end-state on an identity**, not an imperative +operation on a placement, so re-enumerate-and-retarget preserves user intent; +re-targets are capped, then a hard conflict is raised. Conflicts persist in a +table with RPCs to list and resolve them. Events become an optimization over a +queryable source of truth — necessary because `Subscribe` is a broadcast stream +that silently drops events on lag, so no event-driven guarantee is currently +sound without a sequence number and resumption. + +### 6. Identity is content-addressed and account-scoped. + +Precedence: `EMAILID` (RFC 8474 OBJECTID) → `X-GM-MSGID` → `(account_id, +normalized Message-ID, content_hash)` → `(account, folder, uidvalidity, uid)`. + +Bodies are keyed by a content hash of the full RFC822 octets, first-write-wins. +**A Message-ID hash alone must not be the key**: Message-IDs are public, echoed +in every `References:` header, and forgeable, so with `INSERT OR REPLACE` a +message reusing a known Message-ID would silently overwrite the body of a +message the user already trusts and would evade filters by never being seen as +new. That hole does not exist today and must not be introduced. The same +Message-ID with different content is two messages, which also covers +list-copy-vs-direct and drafts. + +A message is separated from its **placements** — `PRIMARY KEY(account_id, +folder_id, uidvalidity, uid)`, with `uidvalidity` in the key so UIDs restarting +after a bump cannot collide with old rows. DAV gets the same split: href = +placement, iCalendar/vCard UID = identity, ETag = version. + +### 7. Load is bounded by bandwidth, not connection count. + +Gmail publishes both a 15-simultaneous-connection and a 2,500 MB/day download +cap **per account**. On rungs 2 and 3, ten engines over five accounts reach the +byte cap long before the connection cap. Per-engine budget derives from a +`fleet_size` setting, since cross-engine rate limiting is impossible without the +coordination layer we decline to build. `Retry-After` is honoured *with* jitter +proportional to `fleet_size`, or ten engines retry in the same instant. + +One mutex-guarded connection per (engine, account) with mutation priority; sync +work is chunked per folder so it yields to user actions. + +## Consequences + +- **This gates the M7 API freeze.** Mutation RPCs do not exist (`Mail` has no + Move/Delete/Flag); `Conflict` must be a typed error, not a response field, so + old clients take the error path; `Message` needs identity and placements. + Most individual additions are wire-additive, but redefining `Message.id` + changes the *value* of every existing id while staying wire-compatible — + invisible to the byte-exact descriptor golden, and that token appears at ten + other wire positions. A model that cannot express deletion or conflict must + not be frozen. +- **Shared OAuth accounts across engines are out of scope.** Gmail and + Microsoft 365 mandate OAuth; providers that rotate refresh tokens on use would + have ten engines invalidating each other. A token broker is exactly the + coordination layer this ADR declines to build. Documented limitation. +- **The store resets rather than migrating into a degraded identity.** + `message_key` cannot be derived for rows already stored: Message-ID is never + parsed, never persisted, and the raw octets are discarded after parse. A + shipped product would have to keep those rows under their old surrogate, + permanently un-deduped and invisible to every cross-folder guarantee the model + provides. Pre-release that trade is not worth making: identity-bearing schema + changes bump `PRAGMA user_version`, the message store is rebuilt, and the + account re-syncs from the server, which is authoritative anyway. Account + configuration and credentials are preserved; cached mail is not. +- **The audit ledger is reset with the store, not namespaced.** + `filter_execution_logs.message_id` feeds an HMAC hash chain verified by a live + `Audit.VerifyChain`, so historical ids genuinely cannot be rewritten in place. + Rather than carry a permanently mixed-namespace ledger, the chain is + reinitialised alongside the message store it refers to. A ledger whose entries + point at ids that no longer resolve has no audit value. +- **A new class of test is required.** Convergence cannot be shown by one engine + against one mock. Verification needs ≥3 engines against one stateful server, + plus side-effect-counting SMTP and webhook mocks — final-state placement + convergence is blind to duplicate forwards, so the largest risk in this ADR + would pass a naive convergence test. +- **`async-imap` 0.11.3 cannot express any of this**: no `ENABLE`, its `SELECT` + parser drops `VANISHED`, and `check_status_ok` discards the tagged response + code where `COPYUID` and `[MODIFIED …]` live. A fork-versus-hand-rolled + decision gates the protocol work. + +## Alternatives considered + +- **A coordination layer** (broker, leader election, shared state). Rejected: it + would be the largest and least reliable component in the repo, and it solves a + problem the servers already solve for everything except side-effecting actions. +- **CRDTs / vector clocks.** Rejected: the server holds authoritative state, so + there is no merge to compute — only a state to observe and preconditions to + respect. +- **Freezing `nuncio.v1` first and adding conflict semantics later.** Rejected: + not expressible additively. +- **Preserving existing local stores across the identity change.** Rejected for + now — see [Pre-release posture](#pre-release-posture). It buys nothing anyone + is owed and costs a permanently two-tier message store. + +## Build order + +The dependency that originally forced this sequence — placements waiting for +captured identity to accumulate, because history could not be re-keyed — does +not exist once the store is resettable. Capture still has to land before the +model that consumes it, but only by one merge, not by a soak period. + +| # | Work | Depends on | +| --- | --- | --- | +| P0 | Stateful multi-connection IMAP mock harness | — | +| P1 | Namespace-aware DAV XML reader + href/ETag identity | — | +| P2 | Message-ID + content-hash capture at ingest | — | +| P3 | `user_version` gating + scheme-tagged sync tokens | — | +| P4 | Raw IMAP command layer (`ENABLE`, tagged codes, `VANISHED`) | P0 | +| 1 | Message/placement identity split | P2, P3 | +| 2a | IMAP change enumeration, four-rung ladder | P0, P4 | +| 2b | DAV `sync-collection` + multiget + 507 paging | P1 | +| 3a | Three-state mutation outcomes + `Conflict` | P4 | +| 3b | Mutation RPCs + DAV conditional write-back | 1, 3a | +| 4 | Scheduler, session pooling, rate limiting, backoff | 3a (partly) | +| V | ≥3-engine convergence harness with side-effect counting | P0 | + +**2a remains the highest value per unit of work** and still ships independently: +it fixes ghost messages for a single engine, against either schema. + +Filter-execution ownership (§1) is independent of all of it and ships first. diff --git a/docs/reviews/2026-08-10-multi-engine-design-review.md b/docs/reviews/2026-08-10-multi-engine-design-review.md new file mode 100644 index 0000000..1e5d4a2 --- /dev/null +++ b/docs/reviews/2026-08-10-multi-engine-design-review.md @@ -0,0 +1,346 @@ +# Design review — multi-engine sync model (2026-08-10) + +Evidence behind [ADR 0002](../adr/0002-multi-engine-sync-model.md). A draft +design was reviewed independently along three axes, each instructed to find +errors rather than confirm the draft, and to say plainly when an attack failed. +All three found load-bearing defects. Reviewed against `dev` @ `a997220`. + +Findings are recorded here because several are non-obvious, several cost real +effort to establish, and at least two would otherwise be rediscovered the +expensive way. + +--- + +## A. Protocol correctness + +### A1. The provider capability matrix in the draft was wrong + +The draft claimed Gmail, Fastmail, Dovecot, and Exchange all support CONDSTORE +and QRESYNC. Verified against live capability strings: + +| Provider | CONDSTORE | QRESYNC | +| --- | --- | --- | +| Fastmail (Cyrus) | yes | yes (also RFC 8474 OBJECTID) | +| Dovecot | yes | yes, but `NOMODSEQ` per mailbox where persistent modseqs were never enabled | +| **Gmail** | yes | **no** — not planned | +| **Exchange / Exchange Online** | **no** | **no** | + +Two of four named providers cannot use the draft's headline mechanism. This is +what forced the ladder from two rungs to four, and capability detection from +per-account to per-mailbox (RFC 7162 §3.1.2.2 makes `NOMODSEQ` a mailbox +property). + +The draft also omitted the CONDSTORE-only middle rung entirely — RFC 7162 §3.2.5.1 +states QRESYNC's `SELECT` is semantically equivalent to `UID FETCH … (CHANGEDSINCE +… VANISHED)`, i.e. a round-trip optimisation over CONDSTORE, not a different +capability class. Skipping it put Gmail on full flag sweeps against a published +2,500 MB/day per-account cap. + +### A2. QRESYNC has no modseq-expiry error + +RFC 7162 §3.2.6: a server receiving a mod-sequence older than it remembers +**MUST behave as if asked to report all expunged messages from the provided UID +set**. The failure mode is a bandwidth blowup, not a refusal. `full_resync_required` +can therefore never be *received*; it must be derived client-side from a +UIDVALIDITY mismatch (§3.2.5), `NOMODSEQ` (§3.1.2.2), or a HIGHESTMODSEQ +regression. + +`uidvalidity:highestmodseq` is also insufficient state: §3.2.5.1 and §3.2.5.2 +make the known-UID set and sequence-match data part of what the client supplies. +And §3.2.3 requires a tagged **BAD** if `QRESYNC` is used without `ENABLE QRESYNC` +on the connection. + +### A3. "Absent COPYUID means the mutation failed" is wrong three times over + +- RFC 4315 §3 ABNF has no empty `uid-set`; the states are *present* or *absent*. +- Absence is conforming on success: the client may lack SELECT rights on the + destination, or the destination may be UIDNOTSTICKY. +- RFC 6851 §4.3 makes COPYUID only *SHOULD* for MOVE, and advises sending it in + an **untagged** OK — so a client reading only the tagged OK misses it on every + conforming MOVE. + +Additionally, RFC 6851 §4.4: under QRESYNC the server sends `VANISHED` rather +than `EXPUNGE` for `UID EXPUNGE`, so a delete proved by counting `EXPUNGE` +responses reports conflict on every success. + +Consequence: a third outcome, `Unknown`, is required. `Conflict` needs positive +evidence. + +### A4. MODIFIED does not cover the case the draft cared about + +RFC 7162 §3.1.3 puts `MODIFIED` in the **tagged** response, and only *SHOULD*s +that servers avoid spurious `MODIFIED` on single-modseq servers — so engine B +setting `\Seen` can fail engine A's unrelated `\Flagged` store. More +importantly, `MODIFIED` means "changed by someone else"; a UID another engine +already *moved or expunged* is simply ignored (RFC 3501 §6.4.8) and never +appears in `MODIFIED`. Presence detection needs a separate mechanism. + +Relatedly, the draft's flag proof-of-effect ("untagged FETCH present") is unsound: +`.SILENT` suppresses it (RFC 3501 §6.4.6) and is the normal form for conditional +stores. + +### A5. Message-ID is not a content identifier + +RFC 5322 §3.6.4 makes it *SHOULD*, not MUST. One Message-ID legitimately maps to +different octets for mailing-list-plus-direct copies (Mailman preserves it while +adding `List-*` headers and a footer) and for drafts (Gmail and Exchange keep one +Message-ID across saves). The draft's "hash of raw headers" fallback is also +unstable across exactly the placements it exists to unify, because `Received:` +chains differ. + +Missed primitives that solve this properly: **`EMAILID`** (RFC 8474, Fastmail/Cyrus, +Dovecot) and **`X-GM-MSGID`** (Gmail) are server-assigned identities stable across +folders. + +### A6. DAV specifics the draft omitted + +RFC 6578 §3.6 truncation (207 + `507` + `DAV:number-of-matches-within-limits`) — +"Clients MUST handle the 507 status"; §3.1 loop-to-convergence; `sync-level: +infinite` (§3.5.1 warns `sync-level: 1` is not reliable); discovery via +`DAV:supported-report-set`; and `calendar-multiget`/`addressbook-multiget` +(RFC 4791 §7.9, RFC 6352 §8.7) because `sync-collection` returns hrefs and ETags, +not content. RFC 4791 §5.3.4: a server that rewrites the submitted object MUST NOT +return an ETag. RFC 6638 scheduling is a write hazard — PUTting a VEVENT with +ATTENDEEs on a scheduling-aware server emails real people. + +`getctag` is a Calendar Server vendor extension, not an RFC, and was +mis-presented as a peer of RFC 6578. + +### A7. Other omitted primitives + +`STATUS (… HIGHESTMODSEQ)` (RFC 7162 §3.1.7) and `LIST-STATUS` (RFC 5819, which +Gmail advertises) give a whole-account change probe in one round trip. +`COMPRESS=DEFLATE` (RFC 4978) directly reduces metered bytes. `NOTIFY` (RFC 5465) +watches many mailboxes on one connection, which moots the draft's +"IDLE costs a connection per mailbox" reasoning. ESEARCH `SEARCH RETURN (ALL)` +is far cheaper than `UID FETCH 1:* (FLAGS)` for a vanished-set diff. +`UIDNOTSTICKY` (RFC 4315) invalidates both COPYUID proof and the placement key +and must be detected. + +--- + +## B. Codebase fit + +### B1. `message_key` cannot be derived — the blocker is ingest, not migration + +`MimeParserAdapter::parse_mime` never calls `message_id()`; `Email` has no such +field; the `messages` table has no column for it; and `build_email_from_fetch` +discards the `BODY[]` bytes after parsing. The IMAP `ENVELOPE` does carry +`message-id`, but only `subject`/`from`/`to` are read. + +So in-place migration of a populated database is impossible — there is neither a +Message-ID nor raw headers to hash. Either re-fetch every message (offline-hostile, +impossible for a removed account) or set `message_key := old surrogate`, which +yields one placement per message and no retroactive dedup. Capture must ship +first and accumulate. + +### B2. The audit hash chain pins historical ids permanently + +`filter_execution_logs.message_id` is an input to `compute_log_hash`, verified by +`verify_execution_log_chain`, reachable from the live `Audit.VerifyChain` RPC. +Rewriting historical ids breaks verification permanently. `worm_audit_records` is +harder still — `BEFORE UPDATE`/`BEFORE DELETE` `RAISE(ABORT)` triggers. A +documented namespace boundary with a schema-version marker is the only option. + +### B3. Placements break the filter engine's evaluation contract + +`FilterField::Folder` and `FilterField::Account` evaluate scalar fields on +`Email`; `WHERE FOLDER = 'INBOX'` has no meaning against a multi-placement +message. `evaluate(&Email)` must become per-placement. + +More seriously: **the fire-once guard is `existing_message_ids`, not the +execution log.** Under `message_key`, a genuinely new arrival that already exists +in another folder becomes "not new" and **silently never fires any filter** — a +new correctness bug the identity change introduces. The guard must become +placement-aware *and* gain a real `(rule_id, message_key)` dedup. + +`set_message_read` is `UPDATE messages … WHERE id = ?`; since IMAP `\Seen` is +per-mailbox, read state becomes per-placement and `MARK READ` must name one. + +### B4. FTS orphans are a confidentiality problem + +`messages_fts` holds **plaintext** bodies keyed on `messages.id`, with a +delete-only trigger. Under placements, dropping *a* placement must not delete the +FTS row while others reference the body; dropping the *last* must. Inverted, this +either loses search or leaks plaintext bodies in orphaned rows — in the one table +the schema comments already document as unencrypted. + +Also: there is no `folders` table. Folders are derived from `messages GROUP BY +folder_id` and are currently global across accounts, which placements will expose. + +### B5. `async-imap` 0.11.3 cannot express the design + +| Needed | Status in the dependency | +| --- | --- | +| `COPYUID` | `uid_copy`/`uid_mv` return `Result<()>`; `check_status_ok` discards the response code | +| `[MODIFIED …]` | no `UNCHANGEDSINCE` variant; tagged code discarded | +| `SELECT (QRESYNC …)` | no `select_qresync`; no `ENABLE` command at all | +| `VANISHED` | parsed by `imap-proto` but dropped by async-imap's `select()` parser | +| untagged FETCH / expunged UIDs | reachable; both currently drained and discarded | + +`run_command`/`read_response` are public, so a hand-rolled tagged-response +collector is possible (~80–150 LOC plus per-command parsers) — but it is a +prerequisite, and it is subject to the workspace's `-D warnings` / no-`unwrap` +regime. Fork-versus-hand-roll is a gate on the protocol work. + +### B6. There is no IMAP mock, and no scheduler, and no backoff + +- `wiremock` is HTTP-only. IMAP had only ad-hoc inline `duplex` responders: + single connection, no shared state. *(Addressed by PR #384.)* +- `OutboxManager::calculate_backoff_ms` is **dead code**; retries run on a fixed + 5s tick with `MAX_RETRIES = 5`, so a 25-second blip permanently fails every + queued mutation fleet-wide. +- `sync_interval_secs` is stored and validated but **nothing polls**. There is no + periodic sync at all; `main.rs` spawns only the outbox drain (5s) and an opt-in + update check (24h). +- `SyncProgress` never reaches the wire — it is dropped at the gRPC boundary. +- `Mail.MarkRead` and `Contacts.CreateContact` are silently **local-only** and + report success. +- `AccountProtocol` has no `CardDav` variant, so a CardDAV account is not + representable — contacts sync is unwireable, not merely unwired. +- The DAV "parsers" are substring splits on ``; a different + namespace prefix yields zero events reported as a successful sync. DAV ids are + positional, so re-syncs reshuffle which local row holds which remote object. + +### B7. The proto claim was overstated in the draft's favour and against it + +Three of five changes *are* wire-additive with free tags (mutation RPCs, sync +tokens in `GetStatus`, new `Event` oneof arms). The genuinely dangerous one is +invisible: redefining `Message.id` changes the *value* of every existing id while +staying wire-compatible — the byte-exact descriptor golden cannot see it, and the +token appears at ten other wire positions. + +`STORY-WORKFLOW.md` permits one proto-touching story in flight; since four work +items touch the proto, a single "proto chunk" is not schedulable and must +dissolve into a serialized tail on each. + +### B8. Sizing + +The draft's M/L/M/S/M (~4–6 weeks) is roughly 3× optimistic. Measured against the +tree: **5,800–9,300 production LOC + 4,000–6,600 test LOC across ~90–120 file +touches** — about a quarter of focused work. Chunk 1 alone is ~120–160 edit sites +across ~25–30 files. This substantially *is* WS-B + WS-C + WS-E + parts of M6.5. + +--- + +## C. Adversarial + +### C1. Filter fan-out — the largest hole, absent from the draft entirely + +All ten engines classify the same message as new (per-engine store), and +`pending_remote_mutations` has no uniqueness constraint. State-mutating actions +converge; `FORWARD` and `CALL WEBHOOK` do not — the on-call engineer is paged ten +times per alert. Rule IDs are `OsRng`-random per engine, so the "same" rule has +ten identities and no receiver can dedup from rule identity even in principle. + +Adding a `Conflict` outcome makes the UX *worse* here, not better: the nine losing +MOVEs stop silently fabricating success and instead emit conflict events to two +clients each — eighteen spurious notifications per filtered message. "Server wins, +loudly" is calibrated for rare conflicts; filter fan-out makes conflict routine. + +The draft's one sentence that sounds like it addresses this ("filters key on +`message_key`, so they stop re-firing on the destination copy") is about *self* +re-firing after a move — a different bug. + +### C2. The draft's identity scheme introduces a spoofing primitive + +Message-IDs are public and echoed in `References:`. With `INSERT OR REPLACE` and a +key of `hash(Message-ID)`, an attacker reusing a known Message-ID silently +overwrites the body shown under the original message's placement, and evades +filters entirely by never being classified new. **This hole does not exist today**, +because the current `surrogate_id` includes the UID. The identity change would +have created it. + +### C3. Mixed-version fleets fight on the shared server + +Old engines key identity on folder, so a new engine's move looks like a brand-new +message to them — they re-evaluate the rule set and re-move it. Old and new +engines trade the message back and forth. There is no `PRAGMA user_version`, no +down path, and an old binary opening a new database "works" while writing the old +model, leaving two mutually invisible truths in one file. + +Worst of these: the sync-token shape collision described in ADR 0002 §3, which +fails *silently* rather than loudly. + +### C4. "Server wins, loudly" strands intent and leaves clients nothing to act on + +A user pressing Delete on engine 7 for a message engine 2 already archived has +their intent discarded; re-enumeration removes the placement, so from their seat +the delete appeared to work. Retry is *safe* here — `message_key` is stable and +the new location is known — and the blanket prohibition follows from modelling +mutations as imperative operations on placements rather than desired end-state. + +Clients get no event payload contract, no RPC to act on a conflict, no listing, no +GC, and `Subscribe` silently drops events on lag with no sequence number or +resumption. "Loudly" reduces to a log line that sometimes reaches a UI. + +### C5. Connection math measures the wrong quantity + +The outbox opens a **fresh connection per mutation**: worst case 50 connect/login +cycles per 5s per engine, ×10 engines = 500 authentications per 5 seconds against +one account. Providers rate-limit *authentications* far harder than concurrent +connections, and some respond with a temporary lockout. Sync and outbox also +contend for the single pooled connection the draft specifies — share it and a +mark-read queues behind a 300-second per-account sync timeout; don't, and it is +2 per engine, which the draft's own table marks as over cap. + +Jitter alone does not solve correlated wake-ups (workday start, VPN up, power +restored), and honouring `Retry-After` without jitter creates a synchronized herd. +Rate limiting across engines is impossible without coordination, so an engine must +know its `fleet_size` and derive its budget from it. + +### C6. Placement schema and mutation addressing + +The draft's `PRIMARY KEY(account_id, folder_id, uid)` collides after a UIDVALIDITY +bump, when UIDs restart at 1. And a mutation has no placement coordinate: a +message in both INBOX and Archive with a rule saying MOVE TO Archive has no +defined source. The outbox currently recovers addressing from the *message* row, +which ceases to exist under the split. + +Drafts are unaddressed and will churn the model (APPEND-then-delete per save, new +UID each time, same or new Message-ID depending on client). + +### C7. The proposed verification cannot detect C1 + +Final-state placement convergence is exactly blind to duplicate side effects — ten +forwards and one forward produce identical placements. N=2 also shows a single +duplicate, which reads as an off-by-one. Requires N≥3, side-effect-counting SMTP +and webhook mocks asserting "exactly one forward across the fleet", and an +interleaving harness that can hold one engine's mutation in flight while another +acts on stale state. + +### C8. OAuth breaks the shared-account premise + +Gmail and Microsoft 365 mandate OAuth; there is none in the tree. Providers that +rotate refresh tokens on use would have ten engines invalidating each other in a +loop, and with no backoff that becomes a lockout. A token broker is the +coordination layer the design declines to build — so this is a scope boundary, not +a TODO. + +### C9. Smaller items + +Clock skew across ten machines makes any wall-clock tiebreak unreliable (outbox +ordering, audit ledger). "Body stored once" is per-engine, not fleet-wide. The +append-only hash-chained execution log has no retention policy and will orphan +entries. One poison collection that always fails burns the account's 300s timeout +every cycle. And the draft never stated a consistency guarantee at all — several +of the above decisions fall out of writing one down. + +--- + +## What the reviews agreed the draft got right + +- **"Don't build a distributed system" is correct for state convergence.** For + placements, flags, and DAV objects, the servers genuinely are the coordinator, + and O(N) load with O(1) design complexity is the right trade. The thesis fails + only for side-effecting actions (C1) — a bounded exception, not a repudiation. +- **`CollectionSync` with `{upserts, removals, next_token}` is the right shape**, + and the RFC 7162 / 6578 mapping is sound. +- **"An empty effect must not be reported as success" is correct and important**; + the objection is to the verdict drawn from it, not to detecting it. +- **The M7-freeze argument is correct and understated** — C4 and B6 strengthen it, + since the freeze must also cover a durable conflict surface, event sequence + numbers, and the fact that `MarkRead` will come to mean something different. +- **Change enumeration ships first and alone**, and fixes ghost messages for + single-engine users independently of everything else. The draft was wrong that + it depends on the identity split.