Graylog2 / Graylog2/graylog2-server
LookupTableService: unserialized event handlers on shared thread pool cause stale or broken lookup tables
- Dominant language
- Java
- Stars
- 8.1k
- Forks
- 1.1k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 217
Description
## Summary
`LookupTableService` manages in-memory lookup state across 5 `ConcurrentHashMap`s, but all 7 `@Subscribe` event handlers dispatch work to the shared `@Named("daemonScheduler")` — a 30-thread `ScheduledThreadPoolExecutor`. Concurrent events (adapter update + cache update, bulk import, content pack install) execute overlapping handlers on different threads with no serialization, leading to permanently broken lookup tables that reference stopped services.
## Root Cause
Each event handler (`handleAdapterUpdate`, `handleCacheUpdate`, `handleLookupTableUpdate`, etc.) follows this pattern:
```java
@Subscribe
public void handleAdapterUpdate(DataAdaptersUpdated updated) {
scheduler.schedule(() -> {
// 1. Create new adapter, start it, wait for RUNNING
// 2. Put into idToAdapter + liveAdapters (two separate ConcurrentHashMap puts — not atomic)
// 3. createLookupTable() for all tables using this adapter
// - reads from idToCache (may see old or new cache depending on timing)
// - reads from idToAdapter
// - liveTables.put() — last writer wins
// 4. Stop old adapter
}, 0, TimeUnit.SECONDS);
}
```
The `daemonScheduler` has a pool size of 30 (`SchedulerBindings.java:36`), shared across the entire server. When multiple lookup-related events arrive concurrently, their handlers run on different threads and modify overlapping state without coordination. The 5 `ConcurrentHashMap`s provide per-operation atomicity but no compound-operation guarantees across maps.
### Why ConcurrentHashMap is insufficient
The handlers perform multi-step sequences that must be atomic:
1. **Cross-map consistency:** `idToAdapter.put()` and `liveAdapters.put()` happen in separate calls (`DataAdapterListener.running()`, lines 203-204). Between them, a concurrent `createLookupTable()` can see the new adapter in one map and the old in the other.
2. **Read-then-build-then-write in `createLookupTable()`:** Reads `idToCache.get()` (line 527) and `idToAdapter.get()` (line 533), builds a `LookupTable`, then writes to `liveTables.put()` (line 571). A concurrent delete or update can invalidate the read values before the write completes.
3. **Last writer wins across handlers:** When both an adapter update and a cache update affect the same lookup table, both handlers call `createLookupTable()`. Each builds a `LookupTable` from whatever it sees in the maps at that moment. The last `liveTables.put()` wins — potentially installing a table built from a stale snapshot.
4. **Old service stopped while still referenced:** After replacing the table in `liveTables`, the handler calls `stopAsync()` on the old adapter/cache. But message processing threads that already obtained a reference to the old `LookupTable` are still using the old (now stopping) service, causing `IllegalStateException` from `checkState(isRunning())` in `LookupDataAdapter.get()` (line 141).
## Customer Impact
**Severity: High — lookup tables can become permanently broken**
### Hard failures on the message processing hot path
When a `LookupTable` ends up referencing a stopped adapter, every pipeline rule calling `lookup()` on that table throws `IllegalStateException`. This causes:
- Pipeline rule evaluation failures logged as errors
- Messages processed without enrichment data (missing GeoIP, threat intel, asset lookups)
- Depending on pipeline error handling configuration, messages may be dropped entirely
### Permanently broken state
Unlike transient errors, this state does **not self-heal**. The `liveTables` map holds a reference to a stopped adapter. The correct adapter is running and registered in `idToAdapter`/`liveAdapters`, but never wired into the lookup table. Recovery requires:
- Manually re-saving the affected adapter or cache config (triggers a new event)
- Restarting the Graylog node (full rebuild from DB)
A customer may not realize the state is broken, since the UI reads from MongoDB (which has the correct config) while the runtime uses the stale in-memory state.
### Non-deterministic across cluster nodes
On a multi-node cluster, the race depends on per-node thread scheduling. Different nodes can end up in different states — some working, some broken — for the same lookup table. This makes the issue extremely difficult to diagnose: lookups succeed on some nodes and fail on others, with the outcome depending on which node processes each message.
### Triggered by common operations
The race window opens during any concurrent lookup-related changes:
- **Content pack / Illuminate pack installation** — creates multiple adapters, caches, and tables in rapid succession, firing 10+ events
- **Admin updating adapter + cache for the same table** — two clicks in the UI
- **API automation** — scripts that configure multiple lookup entities
- **Bulk operations** — any operation that touches multiple lookup entities
### Cascade impact through pipelines
A broken lookup table affects every pipeline rule that references it. In environments where lookup tables provide enrichment (threat intel, GeoIP, asset mapping), a single broken table can degrade enrichment for all messages passing through that pipeline stage.
## Suggested Fix
### Recommended: Dedicated single-threaded executor with coalescing
Replace `scheduler.schedule(() -> {...}, 0, TimeUnit.SECONDS)` in all 7 event handlers with submission to a **private single-threaded executor** dedicated to lookup state mutations.
```java
private final ExecutorService lookupStateExecutor = Executors.newSingleThreadExecutor(
new ThreadFactoryBuilder().setNameFormat("lookup-state-updater-%d").setDaemon(true).build());
```
This serializes all state mutations, eliminating every compound-operation race without any need for fine-grained locking across the 5 maps.
**Additionally**, add coalescing to batch rapid events. When 10 events arrive in 50ms (content pack install), they should trigger a single rebuild pass rather than 10 overlapping ones. A simple approach: accumulate event IDs for a short window (e.g., 100ms), then process the batch.
### For the old-adapter-still-referenced issue (Issue 4 above)
Delay `stopAsync()` on old adapters/caches. Instead of stopping immediately after replacement, schedule the stop with a short delay (e.g., 5 seconds) to allow in-flight lookups to complete. Or use reference counting to stop only when no thread holds the old reference.
## Effort Estimate
- **Single-threaded executor (core fix):** Small — ~2-4 hours. Replace the scheduler dispatch in all 7 handlers with the dedicated executor. This is the same pattern needed in several other places in the codebase that use the shared `daemonScheduler` for state mutations.
- **Coalescing:** Small — ~2-4 hours. Add a short batching window to reduce redundant rebuilds during bulk operations. This is a performance improvement on top of the core fix.
- **Delayed stop for old services:** Small — ~1-2 hours. Add a scheduled delay before calling `stopAsync()` on replaced adapters/caches.
- **Testing:** Medium — ~4-8 hours. Requires a concurrent stress test: multiple threads firing adapter/cache/table update events while lookup threads perform continuous lookups. Verify that `liveTables` always reflects the latest state and that no `IllegalStateException` is thrown.
**Total estimate: 1-2 days** including testing.
## Affected Code
- `LookupTableService.java` — all 7 `@Subscribe` handlers (lines 300-411), `DataAdapterListener.running()` (lines 200-212), `CacheListener.running()` (lines 260-272), `createLookupTable()` (lines 526-576)
- `SchedulerBindings.java` — `SCHEDULED_THREADS_POOL_SIZE = 30` (line 36) — the shared pool that enables concurrent execution
- `LookupDataAdapter.java` — `get()` (line 137-145) — `checkState(isRunning())` that throws when a stopped adapter is called
Contributor guide
Assessment
This issue has not been assessed yet.