M5: Incremental IMAP sync with progress reporting and per-item FETCH timeout #195

New Issue

2026-07-28T14:04:37Z

KofTwentyTwo commented

2026-07-28 14:04:37 +00:00

(Migrated from github.com)

Context

Story for milestone M5. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work.

nuncio-mail's MailBackend trait already has the right shape for incremental sync — sync_messages(&self, folder_id: &str, since_state: Option<&str>) -> Result<(Vec<Email>, String), MailError> (crates/nuncio-mail/src/backend.rs:16-20) takes a checkpoint and returns an updated one — but nothing actually honors it end to end. ImapEngine::sync_messages (crates/nuncio-mail/src/imap.rs:499-507) names its parameter _since_state and never reads it; it always calls sync_folder_messages, which always issues session.uid_fetch("1:*", query) (crates/nuncio-mail/src/imap.rs:222-223) — a full-mailbox re-fetch every time — and fabricates a "state" string (format!("imap-modseq-{}", emails.len()), line 505) that is not derived from any real server state (no UIDVALIDITY/UIDNEXT/HIGHESTMODSEQ tracking exists anywhere in this crate). On the daemon side, nunciod::sync::fetch_and_persist (crates/nunciod/src/sync.rs:54-68) hardcodes backend.sync_messages(&folder.id, None) (line 61) and discards the returned state entirely — it is never written anywhere, so even if ImapEngine respected since_state, there is nowhere to read a prior checkpoint from on the next sync (nuncio-store's schema has no sync-state table or columns at all — grep -in "sync_state\|modseq\|checkpoint" crates/nuncio-store/src/db.rs matches nothing relevant). There is also no sync progress reporting granularity below the whole-batch SyncStarted/SyncCompleted pair (crates/nuncio-core/src/lib.rs has a CoreEvent::BatchFilterProgress precedent for filter batches, but no mail-sync equivalent), and no timeout wraps the FETCH response stream loop (crates/nuncio-mail/src/imap.rs:231, while let Some(fetch_res) = fetch_stream.next().await) — a stalled connection against a huge mailbox hangs indefinitely with no feedback (#183). Fixes #184.

Goal

Make IMAP sync genuinely incremental: persist a real per-account-per-folder checkpoint after each successful sync, pass it into the next sync's FETCH so only new/changed messages are pulled (not the whole mailbox every time), emit per-folder/per-batch progress events during a sync, and bound each FETCH item's wait with a timeout so a stalled server surfaces as an honest timeout error instead of hanging the caller forever. This must NOT fake incrementality: if the underlying protocol logic cannot narrow the FETCH range for a genuine reason, that must be an honest limitation, not a checkpoint string that is stored and returned but silently ignored on the next call (today's exact bug).

Files to touch / patterns to mirror

crates/nuncio-store/src/db.rs — add a new table (e.g. folder_sync_state(account_id TEXT NOT NULL, folder_id TEXT NOT NULL, state TEXT NOT NULL, PRIMARY KEY (account_id, folder_id))) in migrate (~line 351-436 is where the other CREATE TABLE IF NOT EXISTS statements live) plus get_folder_sync_state/save_folder_sync_state methods, mirroring the shape of save_email/other single/few-row CRUD helpers in this file.
crates/nuncio-mail/src/imap.rs — make sync_folder_messages/sync_folder_messages_with_session (line 210-320, 323+) accept and use since_state: Option<&str>: parse a real UID-based checkpoint (e.g. store UIDNEXT as the state string) and build the FETCH range as "{uidnext}:*" instead of always "1:*" (line 223) when a checkpoint is present; fall back to a full "1:*" fetch when since_state is None (first sync). Wrap the fetch_stream.next().await call (line 231) in tokio::time::timeout(..), surfacing a new MailError variant (e.g. MailError::FetchStalled) on expiry rather than hanging.
crates/nuncio-mail/src/backend.rs — no trait-signature change needed (since_state/return-state are already there); double check MockMailBackend (used across nunciod tests) supports simulating a since_state-aware fetch (returning fewer emails on a second call with a non-None state) so downstream tests can assert incrementality without a real IMAP server.
crates/nunciod/src/sync.rs — fetch_and_persist (line 54-68) must read the folder's last-saved state via the new store method, pass it into backend.sync_messages(&folder.id, last_state.as_deref()), and persist the returned state back via save_folder_sync_state after a successful fetch for that folder. Add a CoreEvent-based progress event (new variant, e.g. CoreEvent::SyncProgress{account_id, folder_id, fetched, total} in crates/nuncio-core/src/lib.rs, mirroring the existing CoreEvent::BatchFilterProgress shape at crates/nuncio-core/src/lib.rs:96-97) published per folder (or per N messages) during the loop at line 60-66.
crates/nuncio-core/src/lib.rs — add the new progress CoreEvent variant near BatchFilterProgress (~line 69-97).

Implementation steps

Add the folder_sync_state table + store methods in nuncio-store; unit test save/read round-trip.
Add MailError::FetchStalled (or similarly named) variant in nuncio-mail; wrap the FETCH stream's .next() with a bounded tokio::time::timeout in sync_folder_messages_with_session; make the timeout duration a named constant (not a magic literal) with a short doc comment on why that value was chosen.
Change ImapEngine::sync_messages's UID range construction to use since_state when present (parse it as a UID; fall back to "1:*" on parse failure or None, never silently swallowing a malformed checkpoint into a full-mailbox refetch without at least a debug-visible signal — up to you whether that's a log line or a typed distinction, but do not make a corrupt checkpoint indistinguishable from "no checkpoint" in test assertions).
Update sync_folder_messages's "no credentials" mock branch (line 323-340) consistently if it also needs to honor since_state for its own unit tests, or leave it as the existing pure-mock fallback and test the real-session path separately.
Add a progress CoreEvent variant; publish it from fetch_and_persist/sync_with_backend in nunciod::sync at a reasonable granularity (e.g. once per folder is enough — do not over-engineer per-message events if the existing event bus/consumer story doesn't need that granularity yet).
Wire fetch_and_persist to read/write folder_sync_state; add a test using MockMailBackend that runs sync_with_backend twice for the same account/folder and asserts the second call's sync_messages was invoked with Some(state) equal to what the first call returned (this is the core regression test for #184 — extend MockMailBackend with a way to assert/capture the since_state argument it was called with, if it does not already expose that).
Add a bounded-timeout test in nuncio-mail proving a stalled/never-responding fake session surfaces MailError::FetchStalled (or equivalent) within a short bounded test time — use an in-memory duplex stream (tokio::io::duplex) that never writes a FETCH response, feeding sync_folder_messages_with_session directly, rather than a real network listener.
Note in the PR: there is no existing wiremock-style (HTTP) mock for IMAP in this repo (wiremock mocks HTTP; IMAP is a stateful line protocol) — the closest existing pattern is crates/nuncio-mail/src/imap.rs's own #[cfg(test)] module, which tests ImapEngine in its "no credentials" mock-data mode (line 533-547) rather than against a scripted real session. If a genuine session-level test is needed for the incremental-FETCH-range assertion, build it against sync_folder_messages_with_session directly over a tokio::io::duplex pair fed with literal scripted IMAP protocol responses (async_imap's Session works over any AsyncRead + AsyncWrite), not by inventing a nonexistent "wiremock IMAP" helper.

Acceptance criteria

A second sync of the same account/folder issues a narrower FETCH than the first (verified via a MockMailBackend-level test capturing the since_state argument, and/or a session-level test over an in-memory duplex stream asserting the literal FETCH range sent).
The daemon persists the checkpoint returned by sync_messages and passes it into the next sync for that account/folder — proven by a nunciod::sync test that syncs twice and asserts the second call used the first call's returned state.
A progress event is published during a sync (new CoreEvent variant), observable via the existing EventBus subscription pattern used in crates/nunciod/src/sync.rs's tests.
A FETCH item that never responds within the bounded timeout surfaces as an honest error (new MailError variant), never an indefinite hang — proven by a test with a bounded wall-clock budget (e.g. must complete within a few seconds even though the simulated server never responds).
No behavior change for a first-ever sync (since_state: None): still a full fetch, exactly as today.
Offline test only: no live IMAP server connection in any test (in-memory duplex streams / MockMailBackend only).
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m5-s2-incremental-sync off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story — no nuncio.proto changes required (the Mail.Sync RPC's request/response shape is unaffected; incrementality is purely internal to the daemon/engine). Can run in parallel with proto-touching M5 stories.

Prior art (reference only, DO NOT trust): branch wip/unreviewed-account-tls-sync, commit 16d0a11 ("feat(mail): instrument IMAP sync + bound per-item FETCH stalls"), produced by a runaway agent that connected to a live server during development, contains an unreviewed attempt at progress instrumentation and stall timeouts. You MAY read it for ideas (git show 16d0a11) but MUST NOT cherry-pick it wholesale, MUST NOT connect to any live server, and MUST re-verify every claim in it (including whether it actually achieves incrementality or only adds the timeout/progress pieces) against this issue's acceptance criteria before reusing any of its approach.

Fixes #184, #183. Rough size: L.

## Context Story for milestone M5. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `nuncio-mail`'s `MailBackend` trait already has the right shape for incremental sync — `sync_messages(&self, folder_id: &str, since_state: Option<&str>) -> Result<(Vec<Email>, String), MailError>` (`crates/nuncio-mail/src/backend.rs:16-20`) takes a checkpoint and returns an updated one — but nothing actually honors it end to end. `ImapEngine::sync_messages` (`crates/nuncio-mail/src/imap.rs:499-507`) names its parameter `_since_state` and never reads it; it always calls `sync_folder_messages`, which always issues `session.uid_fetch("1:*", query)` (`crates/nuncio-mail/src/imap.rs:222-223`) — a full-mailbox re-fetch every time — and fabricates a "state" string (`format!("imap-modseq-{}", emails.len())`, line 505) that is not derived from any real server state (no UIDVALIDITY/UIDNEXT/HIGHESTMODSEQ tracking exists anywhere in this crate). On the daemon side, `nunciod::sync::fetch_and_persist` (`crates/nunciod/src/sync.rs:54-68`) hardcodes `backend.sync_messages(&folder.id, None)` (line 61) and **discards the returned state entirely** — it is never written anywhere, so even if `ImapEngine` respected `since_state`, there is nowhere to read a prior checkpoint from on the next sync (`nuncio-store`'s schema has no sync-state table or columns at all — `grep -in "sync_state\|modseq\|checkpoint" crates/nuncio-store/src/db.rs` matches nothing relevant). There is also no sync progress reporting granularity below the whole-batch `SyncStarted`/`SyncCompleted` pair (`crates/nuncio-core/src/lib.rs` has a `CoreEvent::BatchFilterProgress` precedent for filter batches, but no mail-sync equivalent), and no timeout wraps the FETCH response stream loop (`crates/nuncio-mail/src/imap.rs:231`, `while let Some(fetch_res) = fetch_stream.next().await`) — a stalled connection against a huge mailbox hangs indefinitely with no feedback (#183). Fixes #184. ## Goal Make IMAP sync genuinely incremental: persist a real per-account-per-folder checkpoint after each successful sync, pass it into the next sync's FETCH so only new/changed messages are pulled (not the whole mailbox every time), emit per-folder/per-batch progress events during a sync, and bound each FETCH item's wait with a timeout so a stalled server surfaces as an honest timeout error instead of hanging the caller forever. This must NOT fake incrementality: if the underlying protocol logic cannot narrow the FETCH range for a genuine reason, that must be an honest limitation, not a checkpoint string that is stored and returned but silently ignored on the next call (today's exact bug). ## Files to touch / patterns to mirror - `crates/nuncio-store/src/db.rs` — add a new table (e.g. `folder_sync_state(account_id TEXT NOT NULL, folder_id TEXT NOT NULL, state TEXT NOT NULL, PRIMARY KEY (account_id, folder_id))`) in `migrate` (~line 351-436 is where the other `CREATE TABLE IF NOT EXISTS` statements live) plus `get_folder_sync_state`/`save_folder_sync_state` methods, mirroring the shape of `save_email`/other single/few-row CRUD helpers in this file. - `crates/nuncio-mail/src/imap.rs` — make `sync_folder_messages`/`sync_folder_messages_with_session` (line 210-320, 323+) accept and use `since_state: Option<&str>`: parse a real UID-based checkpoint (e.g. store `UIDNEXT` as the state string) and build the FETCH range as `"{uidnext}:*"` instead of always `"1:*"` (line 223) when a checkpoint is present; fall back to a full `"1:*"` fetch when `since_state` is `None` (first sync). Wrap the `fetch_stream.next().await` call (line 231) in `tokio::time::timeout(..)`, surfacing a new `MailError` variant (e.g. `MailError::FetchStalled`) on expiry rather than hanging. - `crates/nuncio-mail/src/backend.rs` — no trait-signature change needed (`since_state`/return-state are already there); double check `MockMailBackend` (used across `nunciod` tests) supports simulating a since_state-aware fetch (returning fewer emails on a second call with a non-`None` state) so downstream tests can assert incrementality without a real IMAP server. - `crates/nunciod/src/sync.rs` — `fetch_and_persist` (line 54-68) must read the folder's last-saved state via the new store method, pass it into `backend.sync_messages(&folder.id, last_state.as_deref())`, and persist the returned state back via `save_folder_sync_state` after a successful fetch for that folder. Add a `CoreEvent`-based progress event (new variant, e.g. `CoreEvent::SyncProgress{account_id, folder_id, fetched, total}` in `crates/nuncio-core/src/lib.rs`, mirroring the existing `CoreEvent::BatchFilterProgress` shape at `crates/nuncio-core/src/lib.rs:96-97`) published per folder (or per N messages) during the loop at line 60-66. - `crates/nuncio-core/src/lib.rs` — add the new progress `CoreEvent` variant near `BatchFilterProgress` (~line 69-97). ## Implementation steps 1. Add the `folder_sync_state` table + store methods in `nuncio-store`; unit test save/read round-trip. 2. Add `MailError::FetchStalled` (or similarly named) variant in `nuncio-mail`; wrap the FETCH stream's `.next()` with a bounded `tokio::time::timeout` in `sync_folder_messages_with_session`; make the timeout duration a named constant (not a magic literal) with a short doc comment on why that value was chosen. 3. Change `ImapEngine::sync_messages`'s UID range construction to use `since_state` when present (parse it as a UID; fall back to `"1:*"` on parse failure or `None`, never silently swallowing a malformed checkpoint into a full-mailbox refetch without at least a debug-visible signal — up to you whether that's a log line or a typed distinction, but do not make a corrupt checkpoint indistinguishable from "no checkpoint" in test assertions). 4. Update `sync_folder_messages`'s "no credentials" mock branch (line 323-340) consistently if it also needs to honor `since_state` for its own unit tests, or leave it as the existing pure-mock fallback and test the real-session path separately. 5. Add a progress `CoreEvent` variant; publish it from `fetch_and_persist`/`sync_with_backend` in `nunciod::sync` at a reasonable granularity (e.g. once per folder is enough — do not over-engineer per-message events if the existing event bus/consumer story doesn't need that granularity yet). 6. Wire `fetch_and_persist` to read/write `folder_sync_state`; add a test using `MockMailBackend` that runs `sync_with_backend` twice for the same account/folder and asserts the second call's `sync_messages` was invoked with `Some(state)` equal to what the first call returned (this is the core regression test for #184 — extend `MockMailBackend` with a way to assert/capture the `since_state` argument it was called with, if it does not already expose that). 7. Add a bounded-timeout test in `nuncio-mail` proving a stalled/never-responding fake session surfaces `MailError::FetchStalled` (or equivalent) within a short bounded test time — use an in-memory duplex stream (`tokio::io::duplex`) that never writes a FETCH response, feeding `sync_folder_messages_with_session` directly, rather than a real network listener. 8. Note in the PR: there is no existing `wiremock`-style (HTTP) mock for IMAP in this repo (`wiremock` mocks HTTP; IMAP is a stateful line protocol) — the closest existing pattern is `crates/nuncio-mail/src/imap.rs`'s own `#[cfg(test)]` module, which tests `ImapEngine` in its "no credentials" mock-data mode (line 533-547) rather than against a scripted real session. If a genuine session-level test is needed for the incremental-FETCH-range assertion, build it against `sync_folder_messages_with_session` directly over a `tokio::io::duplex` pair fed with literal scripted IMAP protocol responses (`async_imap`'s `Session` works over any `AsyncRead + AsyncWrite`), not by inventing a nonexistent "wiremock IMAP" helper. ## Acceptance criteria - [ ] A second sync of the same account/folder issues a narrower FETCH than the first (verified via a `MockMailBackend`-level test capturing the `since_state` argument, and/or a session-level test over an in-memory duplex stream asserting the literal FETCH range sent). - [ ] The daemon persists the checkpoint returned by `sync_messages` and passes it into the next sync for that account/folder — proven by a `nunciod::sync` test that syncs twice and asserts the second call used the first call's returned state. - [ ] A progress event is published during a sync (new `CoreEvent` variant), observable via the existing `EventBus` subscription pattern used in `crates/nunciod/src/sync.rs`'s tests. - [ ] A FETCH item that never responds within the bounded timeout surfaces as an honest error (new `MailError` variant), never an indefinite hang — proven by a test with a bounded wall-clock budget (e.g. must complete within a few seconds even though the simulated server never responds). - [ ] No behavior change for a first-ever sync (`since_state: None`): still a full fetch, exactly as today. - [ ] Offline test only: no live IMAP server connection in any test (in-memory duplex streams / `MockMailBackend` only). - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m5-s2-incremental-sync` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a `[PROTO]` story — no `nuncio.proto` changes required (the `Mail.Sync` RPC's request/response shape is unaffected; incrementality is purely internal to the daemon/engine). Can run in parallel with proto-touching M5 stories. **Prior art (reference only, DO NOT trust):** branch `wip/unreviewed-account-tls-sync`, commit `16d0a11` ("feat(mail): instrument IMAP sync + bound per-item FETCH stalls"), produced by a runaway agent that connected to a live server during development, contains an unreviewed attempt at progress instrumentation and stall timeouts. You MAY read it for ideas (`git show 16d0a11`) but MUST NOT cherry-pick it wholesale, MUST NOT connect to any live server, and MUST re-verify every claim in it (including whether it actually achieves incrementality or only adds the timeout/progress pieces) against this issue's acceptance criteria before reusing any of its approach. Fixes #184, #183. Rough size: L.

KofTwentyTwo commented

2026-07-30 02:33:58 +00:00

(Migrated from github.com)

Delivered via PR #231 (squash-merged to dev). IMAP sync is now genuinely incremental: a real per-account-per-folder checkpoint encoding {uidvalidity}:{uidnext} is persisted (folder_sync_state table) and fed back so the next FETCH narrows to {uidnext}:* instead of re-fetching 1:* every time; a new SyncProgress CoreEvent is published per folder; each FETCH item is bounded by a 60s timeout surfacing MailError::FetchStalled instead of hanging (#183). Opus adversarial review confirmed genuine narrowing (session-level duplex FETCH-range assertion), no live network, bounded stall — and caught a CRITICAL silent-mail-loss bug (UIDVALIDITY not tracked → a mailbox renumber would drive {old}:* and skip mail); fixed by tracking UIDVALIDITY and forcing a safe full re-fetch on change/corrupt/missing state. Fixes #184, #183. Deferred to M5 whole-branch review: empty-poll body re-fetch (F2), unbounded SELECT (F3).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#195