Graylog2 / Graylog2/graylog2-server

StreamRouter engine update race can silently install stale routing rules

Open
#25,730 2 comments 0 reactions 0 assignees View on GitHub
bug triaged
Dominant language
Java
Stars
8.1k
Forks
1.1k
Avg merge
1d 20h
Merged PRs (30d)
217

Description

## Summary

`StreamRouter` swaps its `StreamRouterEngine` via an `AtomicReference`, but the update path in `StreamRouterEngineUpdater.run()` is not serialized. Because the `@Named("daemonScheduler")` is a 30-thread `ScheduledThreadPoolExecutor` shared across the server, concurrent `StreamsChangedEvent`s can trigger overlapping engine rebuilds where a slower-but-older database snapshot overwrites a newer one.

## Root Cause

When a stream change event arrives, `StreamRouter` submits the same `engineUpdater` Runnable to the shared daemon scheduler (`SchedulerBindings.java:36`, pool size 30). The updater (`StreamRouter.java:124-138`) does:

```java
final StreamRouterEngine engine = getNewEngine(); // loads all streams from MongoDB — slow
if (engine.getFingerprint().equals(
routerEngine.get().getFingerprint())) {
// skip — no change
} else {
routerEngine.set(engine); // last writer wins
}
```

The fingerprint check at line 128 prevents no-op updates but does **not** prevent a stale engine from overwriting a newer one. Two threads can both pass the fingerprint check and the last `set()` wins regardless of data freshness.

### Reproduction scenario

1. Two `StreamsChangedEvent`s fire within milliseconds (common during bulk operations, stream creation with rules, or stream deletion which fires N+1 events)
2. Thread A starts `loadAllEnabled()` at T=0, Thread B starts at T=10ms
3. A stream change is committed between T=0 and T=10ms
4. Thread B's query returns fresher data, finishes first, sets the correct engine
5. Thread A finishes second with stale data, overwrites with the old engine

The stale engine persists until the next stream event where the last completing thread happens to hold the latest snapshot. If no further stream changes occur, the stale state persists **indefinitely**.

### Contributing factors

- **Duplicate events:** Several operations post multiple `StreamsChangedEvent`s for a single logical change. For example, deleting a stream with 10 rules fires 11 events (`StreamRuleServiceImpl:135` per rule + `StreamServiceImpl:418`). This increases the window for overlapping rebuilds.
- **Multi-node divergence:** On a multi-node cluster, each node runs this update independently. Different nodes can end up with different engine versions, causing the same message to route differently depending on which node processes it.

## Customer Impact

**Severity: High — silent data misrouting**

- Stream routing rules in the running engine silently diverge from what's configured in the database and displayed in the UI
- Messages may route to the wrong streams or fail to match streams they should match
- In compliance-sensitive environments (HIPAA, PCI, SOC2), this could cause regulated data to be written to unprotected indices without any visible error
- The issue is **self-masking**: the UI shows the correct rules (read from DB), but the in-memory engine uses stale rules. No error is logged, no metric is emitted, and no alert fires
- On multi-node clusters, the behavior becomes non-deterministic per message — some nodes may have the correct engine while others don't

## Suggested Fix

Several options, in order of increasing robustness:

1. **Quick fix — dedicated single-thread executor:** Replace `scheduler.submit(engineUpdater)` with a private single-threaded `ExecutorService` to serialize all engine rebuilds. This eliminates the race entirely. Add coalescing (e.g., a short delay + deduplication) to avoid redundant rebuilds from duplicate events.

2. **Better — compareAndSet with monotonic versioning:** Add a monotonic version/timestamp to each engine build. Use `AtomicReference.compareAndSet()` so a build can only install itself if the current engine is the one it was meant to replace. Stale builds fail the CAS and are discarded.

3. **Best — single-threaded executor + coalescing + fingerprint from DB:** Combine approach 1 with a mechanism to read the expected fingerprint from the database after the engine is built, to verify the build is still current before installing it.

## Effort Estimate

- **Quick fix (option 1):** Small — ~1-2 hours of code change + testing. Replace the scheduler submission with a dedicated single-thread executor, add basic coalescing. Low risk.
- **Option 2 (CAS):** Small-medium — ~2-4 hours. Requires adding versioning to the engine and changing the update logic. Slightly more invasive.
- **Option 3 (full fix):** Medium — ~4-8 hours. Includes coalescing, verification, and potentially reducing duplicate event posting in `StreamServiceImpl`/`StreamRuleServiceImpl`.

Testing should include a concurrent stream CRUD stress test that verifies the installed engine always reflects the latest database state.

## Affected Code

- `StreamRouter.java` — `StreamRouterEngineUpdater.run()` (lines 124-138)
- `SchedulerBindings.java` — `SCHEDULED_THREADS_POOL_SIZE = 30` (line 36)
- `StreamServiceImpl.java` — multiple methods posting duplicate `StreamsChangedEvent`s
- `StreamRuleServiceImpl.java` — per-rule event posting on delete

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.