perf(sync): batch new-message detection to remove the is_new_message N+1 #341

2026-07-31T19:09:19Z

KofTwentyTwo commented

2026-07-31 19:09:19 +00:00

(Migrated from github.com)

Summary

Closes issue #325 (WS-F5): eliminates the per-message get_message existence SELECT (an N+1 over each fetched chunk) that fetch_and_persist used to gate filter firing.
Replaces it with one batched DatabaseEngine::existing_message_ids(&[id, ...]) lookup (SELECT id FROM messages WHERE id IN (...)) per fetched folder chunk, executed BEFORE any save_email call for that chunk.
Fire-once semantics are unchanged: a message counts as new iff (a) it was absent from the store before this sync pass began, AND (b) this is its first occurrence within the current chunk (tracked via a local per-pass HashSet, so a backend that reports the same id twice in one fetch still fires at most once, matching the old sequential per-message behavior exactly).
save_email's INSERT OR REPLACE upsert semantics and the surrogate-id keying from WS-B1 are untouched.

Approach chosen: batch-select over upsert-returning

Went with the batch-select approach (SELECT id ... WHERE id IN (...) up front) rather than INSERT ... ON CONFLICT DO NOTHING/UPDATE ... RETURNING, because:

save_email already does real work beyond the row insert (body encryption, FTS5 re-index delete+insert) inside its own transaction; folding "was this genuinely new" into that same statement would require restructuring save_email itself and entangle its return type with sync-specific bookkeeping that only one caller needs.
The batch-select keeps save_email's contract exactly as it is (still returns Result<(), DatabaseError>), and keeps the new/seen classification as a separate, independently testable primitive (existing_message_ids) that other callers (e.g. a future incremental-sync path) can reuse without touching persistence.
It fully eliminates the N+1: one query per fetched chunk instead of one per message, same as an upsert-returning approach would achieve, with less change to existing, well-tested code.

Test plan

sync_with_backend_does_not_refire_filters_on_a_repeat_sync_of_the_same_message (existing fire-once regression) still passes unmodified.
Added sync_with_backend_fires_filters_only_for_new_messages_in_a_mixed_batch: seeds one message via save_email (simulating a prior sync), then runs a sync whose fetched chunk contains that same id plus two genuinely new ids. Asserts exactly 2 filter execution log entries (only the new ones), the pre-seeded message's read flag is untouched by MARK READ, and all 3 messages are still persisted/counted.
Added existing_message_ids_returns_only_the_ids_already_persisted and existing_message_ids_with_empty_input_returns_empty_set_without_querying unit tests for the new DatabaseEngine method.
No change to sync result counts (synced still counts every fetched message, once per occurrence, as before).

Gate output

cargo fmt --all -- --check    -> clean
cargo check-all (clippy -D warnings)  -> Finished, 0 warnings
cargo test-all                -> 100% pass, 0 ignored, 0 failed across all crates
  nuncio-store: 75 passed
  nunciod (lib): 127 passed
  (full workspace: all suites ok, 0 failed, 0 ignored)

Refs #325

## Summary - Closes issue #325 (WS-F5): eliminates the per-message `get_message` existence SELECT (an N+1 over each fetched chunk) that `fetch_and_persist` used to gate filter firing. - Replaces it with one batched `DatabaseEngine::existing_message_ids(&[id, ...])` lookup (`SELECT id FROM messages WHERE id IN (...)`) per fetched folder chunk, executed BEFORE any `save_email` call for that chunk. - Fire-once semantics are unchanged: a message counts as new iff (a) it was absent from the store before this sync pass began, AND (b) this is its first occurrence within the current chunk (tracked via a local per-pass `HashSet`, so a backend that reports the same id twice in one fetch still fires at most once, matching the old sequential per-message behavior exactly). - `save_email`'s `INSERT OR REPLACE` upsert semantics and the surrogate-id keying from WS-B1 are untouched. ## Approach chosen: batch-select over upsert-returning Went with the batch-select approach (`SELECT id ... WHERE id IN (...)` up front) rather than `INSERT ... ON CONFLICT DO NOTHING/UPDATE ... RETURNING`, because: - `save_email` already does real work beyond the row insert (body encryption, FTS5 re-index delete+insert) inside its own transaction; folding "was this genuinely new" into that same statement would require restructuring `save_email` itself and entangle its return type with sync-specific bookkeeping that only one caller needs. - The batch-select keeps `save_email`'s contract exactly as it is (still returns `Result<(), DatabaseError>`), and keeps the new/seen classification as a separate, independently testable primitive (`existing_message_ids`) that other callers (e.g. a future incremental-sync path) can reuse without touching persistence. - It fully eliminates the N+1: one query per fetched chunk instead of one per message, same as an upsert-returning approach would achieve, with less change to existing, well-tested code. ## Test plan - [x] `sync_with_backend_does_not_refire_filters_on_a_repeat_sync_of_the_same_message` (existing fire-once regression) still passes unmodified. - [x] Added `sync_with_backend_fires_filters_only_for_new_messages_in_a_mixed_batch`: seeds one message via `save_email` (simulating a prior sync), then runs a sync whose fetched chunk contains that same id plus two genuinely new ids. Asserts exactly 2 filter execution log entries (only the new ones), the pre-seeded message's read flag is untouched by MARK READ, and all 3 messages are still persisted/counted. - [x] Added `existing_message_ids_returns_only_the_ids_already_persisted` and `existing_message_ids_with_empty_input_returns_empty_set_without_querying` unit tests for the new `DatabaseEngine` method. - [x] No change to sync result counts (`synced` still counts every fetched message, once per occurrence, as before). ## Gate output ``` cargo fmt --all -- --check -> clean cargo check-all (clippy -D warnings) -> Finished, 0 warnings cargo test-all -> 100% pass, 0 ignored, 0 failed across all crates nuncio-store: 75 passed nunciod (lib): 127 passed (full workspace: all suites ok, 0 failed, 0 ignored) ``` Refs #325

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#341