# Principal Engineer Review (principal-2) — jmap-states ## Summary The refactor is well-motivated and mostly well-executed: one canonical `jmap_data_types` table, one place that decides the `'J'` prefix, and a deliberate move to make `vnd.fastmail.jmapStates` agree with the JMAP API. Memory management in the new `UserState/get` parser is correct, and the J-prefix unification is a real maintainability win. But the central premise of the branch — *"`UserState/get` and the event report exactly the state string that the corresponding `Foo/get` returns"* — is **only true for the counter-derived types** (Email, Mailbox, Calendar, CalendarEvent, AddressBook, ContactCard, EmailSubmission). For **Note, SieveScript, and VacationResponse** the live `Foo/get` methods derive `state` from a *mailbox `i.highestmodseq`* or a *per-resource modseq*, not from the per-user counter that UserState and the event read. The `user_state` test asserts equality of these two distinct counters and passes only by fresh-account coincidence. That is the headline finding. A second, structural finding: the new `EVENT_JMAP_STATES` event-mask gate is **self-consistent** with the fill site (both call `mboxevent_expected_param`), so the "unexpected param" direction is fine — but the mask still includes event classes (`SUBS_EVENTS`, and some `MAILBOX_EVENTS` sub-paths) that never call `mboxevent_extract_mailbox`, so the param is *expected but never filled* → the `assert(filled_params())` can still fire. The branch narrows the pre-existing over-broad gate but does not narrow it enough. Proposed verdict: **REQUEST CHANGES** (the correspondence gap is a stated-contract violation with a test that gives false assurance; the gate is an incomplete fix to the very assert class it cites). ## What I Explored - `imap/jmap_data_types.gperf` — the canonical table; verified which types have a `modseq_offset >= 0` and their `mbtype`/offset. Confirmed `Contact`/`ContactGroup` are not in the table at all (so "dropped" is really "never present in the canonical table"); `Quota` is present but offset `-1`; `Racl` absent. - `imap/mboxname.h` — `struct mboxname_counters` layout; confirmed `jmap_req_t.counters` is a by-value `struct mboxname_counters` (jmap_api.h:173), so the offset math is type-correct. - `imap/jmap_api.c:1270+` `jmap_modseq` — confirmed flags=0 → item modseq (`mailmodseq`, `caldavmodseq`, `carddavmodseq`, `submissionmodseq`, `sievemodseq`). - Traced every `Foo/get` `get.state` assignment: `jmap_mailbox.c:816`, `jmap_calendar.c:910/960/3826`, `jmap_contact.c:376/1936`, `jmap_mail_submission.c:1344`, `jmap_sieve.c:290`, `jmap_vacation.c:291`, `jmap_notes.c:444`. This is where the correspondence gap surfaced. - `imap/mboxevent.c` — the fill site (1817–1832), the gate (582–588), `filled_params` (asserted at 781), and the group-mask macros (38–48). - All callers of `mboxevent_extract_mailbox` (grep) plus the subscribe event creation site (`mboxlist.c:5470`) and a remote `MAILBOX_DELETE` path (`mboxlist.c:2600`) to test the gate. - `jmap_get_fini` (jmap_api.c:1726) to validate the hand-rolled parser's ownership against the freeing path. - `master:imap/mboxevent.c` to establish the pre-existing (unconditional) gate. ## Findings ### Finding 1 — UserState/event state does NOT match Foo/get for Note, SieveScript, VacationResponse - **Severity:** High (correctness / stated-contract violation; misleading test) - **Location:** `imap/jmap_core.c` `add_userstate_cb`; `imap/mboxevent.c` `add_jmap_state_cb`; the contract stated in `jmap_core.c` comment ("exactly as that type's own Foo/get would") and in `cassandane/tiny-tests/JMAPCore/user_state`. Counter-examples: `jmap_notes.c:444`, `jmap_sieve.c:290`, `jmap_vacation.c:291`. - **Issue:** UserState and the event compute every type's state from `req->counters.modseq` via the gperf `modseq_offset`. But three live `Foo/get` methods compute `get.state` from a *different* source: - `Note/get` → `mbox->i.highestmodseq` of the notes mailbox (not `notesmodseq`). - `SieveScript/get` → `mailbox->i.highestmodseq` of the sieve mailbox (not `sievemodseq`). - `VacationResponse/get` → `sdata->modseq` (the vacation script *resource* modseq), or literal `0` when no vacation script exists (not `sievemodseq`). These are genuinely different counters. A folder `i.highestmodseq` advances on operations that don't bump the per-user type counter (and vice-versa), and `sievemodseq` is shared between SieveScript and VacationResponse while their two `Foo/get`s return *different* values. So UserState cannot equal both — yet the test asserts `UserState.SieveScript == SieveScript/get` AND `UserState.VacationResponse == VacationResponse/get` against the *same* `sievemodseq`. The only reason the test passes is that, on a freshly created account where the sole modseq-advancing event for each collection is its own creation, the folder highestmodseq and the user counter happen to coincide. That is a false-assurance test, not a proof of correspondence. - **Why it matters:** The whole branch is justified by "report exactly what `Foo/get` reports." For three of ten types that is simply untrue, and the test that is supposed to guarantee it is coincidence-dependent. A client that trusts `UserState.Note` as a poll token for `Note/get` will desync (miss or spuriously re-fetch) whenever the two counters diverge. This is exactly the kind of subtle, hard-to-reproduce JMAP state desync that costs days downstream. - **Suggestion:** Three honest options, in order of preference: 1. **Reconcile `Foo/get` to the counter.** Make Note/SieveScript/VacationResponse report `jmap_modseq(req, mbtype, …)` like the other types. This is the real unification and makes the contract true. (Note: gperf maps `Note` → `MBTYPE_COLLECTION` with `notesmodseq`; `SieveScript`/`VacationResponse` → `MBTYPE_SIEVE` with `sievemodseq` — so VacationResponse and SieveScript would then share a state, which may itself be a product question.) 2. **Scope the contract down.** If the counter-vs-Foo/get divergence is intentional/acceptable, change the comments and docstring to say UserState reports the *per-user type counter*, which for some types differs from the resource-level `Foo/get` state — and weaken the test to not assert equality for the three divergent types (assert non-null + `J?\d+` shape only). 3. At minimum, make the test *prove* its claim by mutating each type (create a sieve script, set a vacation response, create a note) and re-asserting equality — that would fail today for at least VacationResponse vs SieveScript sharing `sievemodseq`, surfacing the gap instead of hiding it. - **Verify with other reviewers:** testing reviewer should confirm the test passes only on a virgin account; security reviewer can ignore (no exposure issue). ### Finding 2 — EVENT_JMAP_STATES mask still includes events that never fill the param (incomplete fix → assert can still fire) - **Severity:** Medium (debug-build assert / dropped notification in prod; pre-existing but the branch advertises a fix and leaves it incomplete) - **Location:** `imap/mboxevent.c:582-588` (gate) vs `:1817` (fill, same predicate); non-filling paths at `imap/mboxlist.c:5470` (subscribe/unsubscribe) and the conditional extract at `imap/mboxlist.c:2600` (remote `MAILBOX_DELETE`). - **Issue:** The fill at 1817 and the expectation at 582 both call `mboxevent_expected_param(type, EVENT_JMAP_STATES)`, so they can never disagree on the "unexpected" direction — good, and worth noting the Tech Lead's worry about that direction is unfounded. The *real* hazard is "expected but not filled": the fill only happens inside `mboxevent_extract_mailbox` and only when `mboxname_read_counters` succeeds. But the mask now includes `SUBS_EVENTS` (`EVENT_MAILBOX_SUBSCRIBE`/`UNSUBSCRIBE`), and those events are notified via `mboxevent_set_access` **without ever calling `mboxevent_extract_mailbox`** (`mboxlist.c:5470-5477`). With `vnd.fastmail.jmapStates` enabled (which this branch now does for the entire Cassandane MboxEvent suite), such an event is "expected" to carry jmapStates, the param is never filled, and `assert(filled_params(type, event))` (mboxevent.c:781) fires in a debug build (LOG_ALERT + dropped notification otherwise). Some `MAILBOX_DELETE` paths (remote, or where the mailbox can't be opened) also skip the conditional `mboxevent_extract_mailbox`. - **Why it matters:** The branch's stated purpose for re-gating is to stop mailbox-less events (Login) tripping that exact assert — but it draws the line at "events whose macro group sounds mailbox-y" rather than "events that actually fill the param." `SUBS_EVENTS` is the clearest counter-example. A `cyradm` subscribe on a debug build, or a sufficiently exercised CI run, will hit this. - **Suggestion:** Tie the expectation to the *actual* fill condition, not to a hand-curated event-class list. Simplest robust fix: in `mboxevent_expected_param` for `EVENT_JMAP_STATES`, return `(extra_params & …JMAPSTATES) && event->params[EVENT_URI].filled`-style logic isn't available there (no event pointer), so instead either (a) restrict the mask to the events that provably call `mboxevent_extract_mailbox` *and* always succeed (drop `SUBS_EVENTS`, audit the `MAILBOX_DELETE`/remote paths), or (b) make `filled_params` treat `EVENT_JMAP_STATES` like it already treats `EVENT_MODSEQ`/`EVENT_FLAG_NAMES` — a conditionally-present param that is not flagged missing. Option (b) matches the established pattern in `filled_params` and is the least brittle. Either way, add a test that emits a `MailboxSubscribe` event with the param enabled and asserts no assert/no missing-param. ### Finding 3 — `(off_t)` pointer cast is a signed-arithmetic regression from the prior `(size_t)` - **Severity:** Low (portability smell; works on supported LP64 targets) - **Location:** `imap/jmap_core.c` `add_userstate_cb`: `*(modseq_t *)((off_t) &srock->req->counters + dtype->modseq_offset)`; `imap/mboxevent.c` `add_jmap_state_cb`: `(off_t) jrock->counters + …`. - **Issue:** Casting a pointer to `off_t` (a *signed* type) and doing integer arithmetic is implementation-defined; the idiomatic, well-defined form is `char *` arithmetic: `*(modseq_t *)((const char *)&counters + dtype->modseq_offset)`. Notably the deleted `mboxevent.c` code used `(size_t)` (unsigned), so this branch moved *toward* a signed cast. `modseq_offset` is now an `int` that can be `-1`, but both call sites guard `if (dtype->modseq_offset < 0) return;` before the math, so no negative offset reaches it — verified. Pure hygiene. - **Why it matters:** It is the kind of cast that survives review for years and then bites on an unusual ABI or under UBSan. Cheap to fix while the code is being touched. - **Suggestion:** Use `(const char *)` (or `uintptr_t`) for the base in both new callbacks; this also reads more obviously as "byte offset into a struct." ### Finding 4 — Two divergent copies of the data-types table is a deliberate but fragile arrangement - **Severity:** Low (design note / future-maintenance trap) - **Location:** `imap/jmap_data_types.c` (new TU into base lib) + `imap/jmap_api.c` (http daemon copy); explanatory comments in both. - **Issue:** The hidden-visibility constraint is real and the comments are good. But there are now two independently-linked instantiations of the *same* gperf table. They're generated from one `.gperf`, so they can't drift in *content* — fine. The trap is subtler: a future contributor adding a non-table symbol (a new exported helper near the table, say) must remember which TU is visible where, and the `jmap_data_types_foreach` iterator is defined in the `.gperf` trailer and thus also duplicated into both link units. If anyone ever adds *state* (a cache, a lazy-init) to that iterator, the two copies will diverge at runtime. - **Why it matters:** It's a correctly-reasoned workaround today, but it quietly establishes "this table is duplicated across link boundaries" as a load-bearing invariant with no compile-time guard. The next person won't have this branch's context. - **Suggestion:** Consider EXPORTing the base-library copy's `jmap_data_types_lookup`/ `_foreach` and having the http daemon use those, eliminating the second copy entirely (the comment implies the base symbols are hidden — confirm whether exporting just these two is acceptable). If duplication must stay, a one-line note in the `.gperf` trailer ("this iterator must remain stateless — it is compiled into two link units") would protect the invariant. ## What's Working Well - The J-prefix decision now lives in exactly one function (`jmap_state_string_cstate`) and both new call sites and the existing API funnel through it. That is the right consolidation and removes a class of "did this path remember the prefix?" bugs. The old `mboxevent` `snprintf` path silently lacked the prefix; this is a genuine correctness improvement for the counter-based types. - The `UserState/get` hand-rolled parser is careful and its memory ownership is correct against `jmap_get_fini`: `get.props` is `xzmalloc`+`construct_hash_table` (freed by `free_hash_table`+`free`), `get.ids` is `json_incref`'d (decref'd), `get.state` is `buf_release` (freed). Borrowed `properties` keys come from `req->args`, which outlives the table. The comment explaining *why* `jmap_get_parse` can't be reused (gperf perfect-hash vs. type-name validation) is exactly the kind of non-obvious rationale worth signing. - The mask gate, whatever its over-inclusion, is at least *self-consistent* with the fill site (same predicate), which eliminates the "unexpected param" failure mode the Tech Lead flagged. Good structural choice to reuse `mboxevent_expected_param` at the fill. - The negative-offset guard (`modseq_offset < 0`) is present at both new call sites, so the gperf default `-1` sentinel can't drive an out-of-struct read. ## Clarifying Questions 1. Is `UserState`/the event meant to report the *per-user type counter*, or the exact `Foo/get` state? These differ for Note/SieveScript/VacationResponse today. Pick one and make code + test + docstring agree (Finding 1). 2. Is it intentional that `Mailbox/get` (and now the event's `Mailbox` key) reports `mailmodseq` — the *item* (Email) modseq — so that a mailbox rename/ACL change that bumps `mailfoldersmodseq` does *not* advance the reported `Mailbox` state? This is pre-existing `Mailbox/get` behavior the branch now propagates to the event; flagging in case the wire-change is the moment to reconsider it. 3. Is the `vnd.fastmail.jmapStates` shape change (Mailbox/Calendar now item-modseq; `Contact`/`ContactGroup`/`Racl` gone; `'J'` prefix added) coordinated with the actual downstream consumer(s) of that event parameter? "Matches the API" is the right end state, but it is still a wire change to a vendor param. ## Questions for Other Reviewers - **Testing:** Can you confirm `JMAPCore/user_state` passes only because it runs on a pristine account? Specifically, does adding a vacation response or a second sieve script before the assertions break the `UserState.VacationResponse == VacationResponse/get` and/or the SieveScript equality? That would substantiate Finding 1. - **Testing:** Does flipping `conversations => yes` for the *entire* MboxEvent suite perturb any existing case (event counts, modseq assertions)? And is there any existing MboxEvent test that emits a `MailboxSubscribe`/`MailboxUnSubscribe` with the new global param enabled — i.e., does CI already exercise Finding 2's assert path, or is it latent? - **Security:** Agreed the marginal exposure of `UserState/get` is ~nil (modseqs already exposed via `UserCounters/get`), but please confirm `accountId` authorization is enforced upstream in `jmap_api()` identically to `UserCounters/get`, since both are `JMAP_USERCOUNTERS_EXTENSION` + `JMAP_NEED_CSTATE` and the new method reads `req->counters` directly.