fix(store): surface unparseable filter rules instead of dropping them #269

2026-07-31T16:50:09Z

KofTwentyTwo commented

2026-07-31 16:50:09 +00:00

(Migrated from github.com)

Summary

Fixes #247. Two related silent-drop bugs in filter-rule loading:

DatabaseEngine::list_filter_rules (crates/nuncio-store/src/db.rs) used if let Ok(...) to re-parse each stored rule's NSQL text, silently discarding any row that failed to parse. A rule could stop being enforced with zero signal anywhere.
nunciod's boot sequence (crates/nunciod/src/main.rs) called db.list_filter_rules().await.unwrap_or_default(), mapping a genuine database error at startup into an empty rule set — indistinguishable from "no rules configured."

Policy chosen (and why)

(1) skip-with-WARN. Each row that fails to re-parse is now tracing::warn!-logged with its rule_id, rule_name, and the parse error, then excluded from the returned set. Chose skip-over-fail so one hand-edited/legacy row can't take down the entire rule set for every other (valid) rule; the WARN makes the loss operator-visible instead of silent. list_filter_rules keeps returning Ok for this case.
(2) propagate the error. Added nunciod::load_initial_filter_rules(&db), which forwards list_filter_rules's Result as-is (after an ERROR-level log naming the failure) instead of collapsing it with unwrap_or_default(). A healthy, empty database still returns Ok(vec![]) (zero rules configured is legitimate); a genuine DB error now fails daemon startup instead of quietly starting with an unknown/effectively-disabled filtering posture.

Changes

crates/nuncio-store/src/db.rs: list_filter_rules WARN-logs and skips unparseable rows instead of dropping them via if let Ok.
crates/nunciod/src/lib.rs: new load_initial_filter_rules helper (ERROR-logs and propagates DB errors; documented rationale in its doc comment).
crates/nunciod/src/main.rs: boot sequence now calls load_initial_filter_rules(&db).await? instead of unwrap_or_default().
crates/nuncio-store/Cargo.toml: added tracing-subscriber as a dev-dependency to capture log output in the new store test.

Tests

nuncio-store::db::tests::list_filter_rules_warns_on_unparseable_row_instead_of_dropping_silently — inserts a rule row with corrupted/unparseable NSQL text via save_filter_rule, installs a capturing tracing_subscriber for the duration of the list_filter_rules call, and asserts (a) the rule is excluded from the returned set and (b) the WARN log contains the specific rule id.
nunciod::tests::load_initial_filter_rules_propagates_db_errors_instead_of_returning_empty — closes an ephemeral DB's pool (deterministic query failure) and asserts load_initial_filter_rules returns Err, not an empty Vec.
nunciod::tests::load_initial_filter_rules_returns_empty_vec_for_a_healthy_empty_database — asserts the legitimate "zero rules configured" case still returns Ok(vec![]), so the fix doesn't conflate "empty" with "error."

Gate output

cargo fmt --all -- --check — clean, no diff.
cargo check-all (clippy --all-targets --workspace -D warnings) — 0 warnings.
cargo test-all — all green, 0 ignored. Notable counts: nuncio-store 68 passed (was 67), nunciod lib 123 passed (was 121), plus all existing integration suites unchanged and passing (grpc_auth_test, spine_e2e_test, daemon_e2e_test, etc.).
Pre-commit hook ran the identical fmt/clippy/test gate on commit and passed.

Test plan

cargo fmt --all -- --check
cargo check-all
cargo test-all (0 ignored)
New tests fail on the pre-fix code (verified logic manually — the WARN-capture test would see an empty log and if let Ok would silently drop the row; the boot-load test would see Ok(vec![]) instead of Err under the old unwrap_or_default())

## Summary Fixes #247. Two related silent-drop bugs in filter-rule loading: 1. `DatabaseEngine::list_filter_rules` (`crates/nuncio-store/src/db.rs`) used `if let Ok(...)` to re-parse each stored rule's NSQL text, silently discarding any row that failed to parse. A rule could stop being enforced with zero signal anywhere. 2. `nunciod`'s boot sequence (`crates/nunciod/src/main.rs`) called `db.list_filter_rules().await.unwrap_or_default()`, mapping a genuine database error at startup into an empty rule set — indistinguishable from "no rules configured." ## Policy chosen (and why) - **(1) skip-with-WARN.** Each row that fails to re-parse is now `tracing::warn!`-logged with its `rule_id`, `rule_name`, and the parse error, then excluded from the returned set. Chose skip-over-fail so one hand-edited/legacy row can't take down the entire rule set for every other (valid) rule; the WARN makes the loss operator-visible instead of silent. `list_filter_rules` keeps returning `Ok` for this case. - **(2) propagate the error.** Added `nunciod::load_initial_filter_rules(&db)`, which forwards `list_filter_rules`'s `Result` as-is (after an `ERROR`-level log naming the failure) instead of collapsing it with `unwrap_or_default()`. A healthy, empty database still returns `Ok(vec![])` (zero rules configured is legitimate); a genuine DB error now fails daemon startup instead of quietly starting with an unknown/effectively-disabled filtering posture. ## Changes - `crates/nuncio-store/src/db.rs`: `list_filter_rules` WARN-logs and skips unparseable rows instead of dropping them via `if let Ok`. - `crates/nunciod/src/lib.rs`: new `load_initial_filter_rules` helper (ERROR-logs and propagates DB errors; documented rationale in its doc comment). - `crates/nunciod/src/main.rs`: boot sequence now calls `load_initial_filter_rules(&db).await?` instead of `unwrap_or_default()`. - `crates/nuncio-store/Cargo.toml`: added `tracing-subscriber` as a dev-dependency to capture log output in the new store test. ## Tests - `nuncio-store::db::tests::list_filter_rules_warns_on_unparseable_row_instead_of_dropping_silently` — inserts a rule row with corrupted/unparseable NSQL text via `save_filter_rule`, installs a capturing `tracing_subscriber` for the duration of the `list_filter_rules` call, and asserts (a) the rule is excluded from the returned set and (b) the WARN log contains the specific rule id. - `nunciod::tests::load_initial_filter_rules_propagates_db_errors_instead_of_returning_empty` — closes an ephemeral DB's pool (deterministic query failure) and asserts `load_initial_filter_rules` returns `Err`, not an empty `Vec`. - `nunciod::tests::load_initial_filter_rules_returns_empty_vec_for_a_healthy_empty_database` — asserts the legitimate "zero rules configured" case still returns `Ok(vec![])`, so the fix doesn't conflate "empty" with "error." ## Gate output - `cargo fmt --all -- --check` — clean, no diff. - `cargo check-all` (clippy `--all-targets --workspace -D warnings`) — 0 warnings. - `cargo test-all` — all green, 0 ignored. Notable counts: `nuncio-store` 68 passed (was 67), `nunciod` lib 123 passed (was 121), plus all existing integration suites unchanged and passing (`grpc_auth_test`, `spine_e2e_test`, `daemon_e2e_test`, etc.). - Pre-commit hook ran the identical fmt/clippy/test gate on commit and passed. ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo check-all` - [x] `cargo test-all` (0 ignored) - [x] New tests fail on the pre-fix code (verified logic manually — the WARN-capture test would see an empty log and `if let Ok` would silently drop the row; the boot-load test would see `Ok(vec![])` instead of `Err` under the old `unwrap_or_default()`)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#269