fix(filter): make filter execution single-owner per account, default off #399

2026-08-10T19:58:15Z

KofTwentyTwo commented

2026-08-10 19:58:15 +00:00

(Migrated from github.com)

Summary of Changes

Makes filter execution single-owner per account, defaulting to off.

⚠️ This is a deliberate production behavior change. Filter rules stop running until an account is explicitly opted in. Please read the rationale before approving.

The defect

Every daemon syncing an account decides independently that a message is new — "new" means not in this daemon's store (sync.rs, via existing_message_ids). So every daemon fires the same rules against the same message, and pending_remote_mutations has no uniqueness constraint of any kind.

MOVE/COPY/FLAG survive that: ten moves converge on one outcome. FORWARD and CALL WEBHOOK do not. They land on a third party with no shared resource to compare against, so N daemons send N forwards and page an on-call engineer N times per alert.

Why the engine can't fix this itself

ADR 0002's thesis is that the servers are already the coordinator. That holds for state and fails for actions — there is nothing to compare-and-set against for an effect that leaves the system. Until a server-side claim is available (CONDSTORE + custom keywords, tracked separately), ownership has to be declared.

Default off, including for existing accounts

An account already configured on several machines would otherwise have all of them start forwarding the moment they upgrade. Off costs one setting to correct; a sent forward cannot be recalled. The corrupt-database salvage path deliberately does not recover the flag either, and says so — the column may be among what the corruption took, and re-enabling on a guess would start forwarding from a daemon the user never chose.

What a non-owning daemon does

Still syncs, still stores, still indexes — it is a full mail client. It simply does not act. Triage is bound by the same rule, since it produces the identical side effects, and refuses outright (FAILED_PRECONDITION) when this daemon owns no account rather than streaming a scan that reports progress and applies nothing — which from the caller's side is indistinguishable from "no rules matched".

Idempotency key

Webhooks now carry Idempotency-Key, stable across retries of a rule/message pair, so a redelivery caused by a lost response can be discarded rather than acted on twice.

Scope note, stated plainly: it is not yet stable across daemons. rule_id is OsRng-random per daemon by design — generate_rule_id's doc comment explains that content-derived ids would make a second CreateRule with identical text silently overwrite the first. So #389's "make rule ids a content hash" cannot be done as written; it needs a separate deterministic rule_key carried onto the outbox row, which is a schema change that collides with #391 and the monitor plan. Deferred rather than done badly. Single-owner execution is what covers the cross-daemon case today.

Type of Change

Bug fix (non-breaking change fixing an issue)
New feature (non-breaking change adding functionality)
Breaking change (fix or feature causing existing functionality to change)
Documentation update

Breaking in behavior, not in API: filters no longer run until opted in.

Related Issues

Refs #389. Design: ADR 0002 §1 (PR #385).

Quality Verification Checklist

I have verified code formatting (cargo fmt --all -- --check)
I have verified clippy lints (cargo clippy --all-targets --workspace -- -D warnings)
I have executed the full test suite (cargo test --workspace) and all tests pass (599 passed, 0 failed)
Business logic is implemented in headless engine crates, keeping UI presentation shells skinny
No secrets, keys, or credentials are added to tracked files

Notes for review

New tests

A non-owning daemon syncs and stores the message but applies no action, writes no execution log, and enqueues no mutation.
Ownership round-trips through the account row (and defaults to off on a freshly constructed account).
Triage refuses with FAILED_PRECONDITION when nothing is owned, and genuinely applies nothing.

Two existing triage E2E tests changed. They previously passed because filters ran unconditionally; they now seed an opted-in account. That edit is the gate working, not a workaround.

Migration is additive with DEFAULT 0, checked with PRAGMA table_info so it is safe on every startup.

Not in this PR: no .proto change, so there is no CLI or RPC surface to set the flag yet — it is settable only via the account row. Exposing it on Accounts.UpdateAccount needs the proto slot and should be sequenced against #395. Worth deciding whether that blocks merge; my read is it does not, since the safe default is the one that ships.

Unrelated pre-existing flake: nunciod's create_rule_handler_logs_domain_event_under_request_scope fails ~1 run in 3 on clean dev (measured with these changes stashed). It is the same thread-local-subscriber/callsite-interest defect PR #384 fixes in nuncio-filter, now visible in nunciod. Not addressed here; worth a follow-up applying the same fix.

## Summary of Changes Makes filter execution **single-owner per account**, defaulting to **off**. ⚠️ **This is a deliberate production behavior change.** Filter rules stop running until an account is explicitly opted in. Please read the rationale before approving. ### The defect Every daemon syncing an account decides independently that a message is new — "new" means *not in this daemon's store* (`sync.rs`, via `existing_message_ids`). So every daemon fires the same rules against the same message, and `pending_remote_mutations` has no uniqueness constraint of any kind. `MOVE`/`COPY`/`FLAG` survive that: ten moves converge on one outcome. **`FORWARD` and `CALL WEBHOOK` do not.** They land on a third party with no shared resource to compare against, so N daemons send N forwards and page an on-call engineer N times per alert. ### Why the engine can't fix this itself [ADR 0002](docs/adr/0002-multi-engine-sync-model.md)'s thesis is that the servers are already the coordinator. That holds for *state* and fails for *actions* — there is nothing to compare-and-set against for an effect that leaves the system. Until a server-side claim is available (CONDSTORE + custom keywords, tracked separately), ownership has to be **declared**. ### Default off, including for existing accounts An account already configured on several machines would otherwise have all of them start forwarding the moment they upgrade. **Off costs one setting to correct; a sent forward cannot be recalled.** The corrupt-database salvage path deliberately does not recover the flag either, and says so — the column may be among what the corruption took, and re-enabling on a guess would start forwarding from a daemon the user never chose. ### What a non-owning daemon does Still syncs, still stores, still indexes — it is a full mail client. It simply does not act. `Triage` is bound by the same rule, since it produces the identical side effects, and **refuses outright** (`FAILED_PRECONDITION`) when this daemon owns no account rather than streaming a scan that reports progress and applies nothing — which from the caller's side is indistinguishable from "no rules matched". ### Idempotency key Webhooks now carry `Idempotency-Key`, stable across retries of a rule/message pair, so a redelivery caused by a lost response can be discarded rather than acted on twice. **Scope note, stated plainly:** it is **not** yet stable across daemons. `rule_id` is `OsRng`-random per daemon by design — `generate_rule_id`'s doc comment explains that content-derived ids would make a second `CreateRule` with identical text silently overwrite the first. So #389's "make rule ids a content hash" cannot be done as written; it needs a *separate* deterministic `rule_key` carried onto the outbox row, which is a schema change that collides with #391 and the monitor plan. Deferred rather than done badly. Single-owner execution is what covers the cross-daemon case today. ## Type of Change - [x] Bug fix (non-breaking change fixing an issue) - [ ] New feature (non-breaking change adding functionality) - [x] Breaking change (fix or feature causing existing functionality to change) - [ ] Documentation update Breaking in behavior, not in API: filters no longer run until opted in. ## Related Issues Refs #389. Design: [ADR 0002 §1](docs/adr/0002-multi-engine-sync-model.md) (PR #385). ## Quality Verification Checklist - [x] I have verified code formatting (`cargo fmt --all -- --check`) - [x] I have verified clippy lints (`cargo clippy --all-targets --workspace -- -D warnings`) - [x] I have executed the full test suite (`cargo test --workspace`) and all tests pass (**599 passed, 0 failed**) - [x] Business logic is implemented in headless engine crates, keeping UI presentation shells skinny - [x] No secrets, keys, or credentials are added to tracked files ## Notes for review **New tests** - A non-owning daemon syncs and stores the message but applies no action, writes no execution log, and enqueues no mutation. - Ownership round-trips through the account row (and defaults to off on a freshly constructed account). - `Triage` refuses with `FAILED_PRECONDITION` when nothing is owned, **and genuinely applies nothing**. **Two existing triage E2E tests changed.** They previously passed because filters ran unconditionally; they now seed an opted-in account. That edit is the gate working, not a workaround. **Migration** is additive with `DEFAULT 0`, checked with `PRAGMA table_info` so it is safe on every startup. **Not in this PR:** no `.proto` change, so there is no CLI or RPC surface to set the flag yet — it is settable only via the account row. Exposing it on `Accounts.UpdateAccount` needs the proto slot and should be sequenced against #395. Worth deciding whether that blocks merge; my read is it does not, since the safe default is the one that ships. **Unrelated pre-existing flake:** `nunciod`'s `create_rule_handler_logs_domain_event_under_request_scope` fails ~1 run in 3 on clean `dev` (measured with these changes stashed). It is the same thread-local-subscriber/callsite-interest defect PR #384 fixes in `nuncio-filter`, now visible in `nunciod`. Not addressed here; worth a follow-up applying the same fix.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#399