Plan: Rules Engine Cache Race Fix

On this page

Status

COMPLETE

Context

The rules engine maintains an in-memory cache of compiled JDM rule sets (Arc<RwLock<HashMap<String, Arc<ZenDecision>>>>). All four mutation handlers (create, update, delete, import) call reload_all() after their DB write, which reads ALL rule sets from the database, compiles ALL of them, then atomically replaces the entire HashMap. Under concurrent mutations (e.g., parallel integration tests), one handler’s reload_all() can read stale DB state and overwrite another handler’s freshly-inserted cache entry — causing transient 404s on evaluate.

This was documented in the Roadmap under Known Issues & Tech Debt.

Root Cause

Two concurrent creates (A and B) with reload_all():

  1. A writes to DB, A calls reload_all() — reads DB (sees A only so far), starts building cache

  2. B writes to DB, B calls reload_all() — reads DB (sees A+B), starts building cache

  3. A’s reload_all() finishes, replaces entire cache with {A} — B is lost

  4. B’s reload_all() finishes, replaces cache with \{A, B} — temporarily correct

  5. But the event subscriber then fires, calling reload_all() again — potential further overwrites

The window is small but reproducible under parallel integration tests (~1 in 20 runs).

Fix: Incremental Cache Mutations

Replace reload_all() in API mutation handlers with targeted single-entry cache operations. Additionally, skip self-originated cache invalidation events to prevent the subscriber’s reload_all() from overwriting freshly-inserted cache entries on the same instance.

Keep reload_all() only for:

  • Initial startup (RulesEngine::new())

  • Cross-instance cache invalidation subscriber (main.rs) — events from other instances only

Files Modified

File Change

services/craig-rules/src/engine.rs

Add insert_decision(name, decision), remove_decision(name), instance_id() methods; add instance_id field; include instance_id in cache invalidation events

services/craig-rules/src/store.rs

Change deactivate_rule_set return type from bool to Option<String> via RETURNING name

services/craig-rules/src/api.rs

Update all 4 mutation handlers to use incremental cache mutations

services/craig-rules/src/main.rs

Subscriber skips self-originated events via instance_id comparison; still calls reload_all() for events from other instances

Handler Changes Summary

create_rule_set

Save the compiled decision from validation, then insert_decision(name, decision) after DB insert.

update_rule_set

Fetch old record before update to capture old name. After DB update: remove_decision(old_name), then insert_decision(new_name, decision). If content wasn’t updated, compile from the returned record’s stored content.

delete_rule_set

Use new deactivate_rule_set return type (Option<String>). After deactivation: remove_decision(name).

import_rule_set

Save compiled decision from validation, then insert_decision(name, decision) after DB update.

Why This Works

With incremental mutations, two concurrent creates (A and B):

  1. A writes to DB, A calls insert_decision("A", decisionA) — cache: {A}

  2. B writes to DB, B calls insert_decision("B", decisionB) — cache: \{A, B}

  3. Both succeed — the write lock is held only for the HashMap insert (microseconds)

The subscriber skips self-originated events (matched by instance_id), so the local cache is never overwritten by a stale reload_all(). For other instances, reload_all() still fires as a reconciliation step — since all DB writes have committed by the time the event propagates through RabbitMQ, the full reload sees the correct state.

Verification

  1. cargo fmt --check --all — no formatting issues

  2. cargo clippy --workspace --locked — no warnings

  3. cargo test --workspace --lib — unit tests pass

  4. cargo xtask dev reload — rebuild with new code

  5. cargo test --workspace — all integration tests pass (including previously-flaky evaluate_multiple_times_same_set)

  6. Run integration tests 3x to confirm stability

  7. cargo xtask e2e — E2E tests pass

Edit this page · latest