Fix retain/load ordering races in stream_*_windowed setup #1

New Issue

2026-06-04T17:00:53+02:00

Brummel commented

2026-06-04 17:00:53 +02:00

Two related thread-safety races share one root cause: in the
stream_*_windowed setup path, a file is loaded into the cache
before the symbol is protected by a reference count, so a
concurrent eviction can race the load. Both are verified by reading
the source; the concrete interleavings below are flagged as
unverified — no reproducing test exists yet.

Root cause

In src/lib.rs, stream_m1_windowed (and identically
stream_tick_windowed) loads before it retains:

// src/lib.rs:215-220
self.ensure_m1_loaded(&files[0]);      // load into cache
if files.len() > 1 {
    self.prefetch_m1(&files[1]);       // spawn background load
}
self.cache.retain_symbol(&symbol_arc); // only now protected

stream_tick_windowed has the same ordering at src/lib.rs:250-255.
Between the load and the retain_symbol, the freshly loaded file is
covered by no reference count, so release_symbol ->
evict_symbol (src/cache.rs:158-181) from another thread can drop
it.

Race 1 — eviction between load and retain

Claim (unverified, deduced from the ordering above): with thread A
iterating symbol S (symbol_refs[S] == 1) and thread B entering
stream_m1_windowed(S):

B: ensure_m1_loaded(S_01)   -> S_01 now cached
A: drop(iter) -> release_symbol(S) -> refcount 1->0 -> evict_symbol(S)
   -> S_01 evicted
B: retain_symbol(S)         -> refcount 0->1 (protects an empty slot)
B: next_chunk() -> ensure_m1_loaded(S_01) -> not cached -> reloads

Consequence: redundant I/O (the file is reloaded). Delivered records
stay correct, because the iterator holds an Arc clone of any chunk
it already obtained (current_file_chunks, src/lib.rs:416).

Race 2 — prefetch insert outlives an evict

Claim (unverified, deduced from prefetch_m1 at src/lib.rs:340-356
and insert_m1 at src/cache.rs:77-84): a background prefetch thread
spawned for symbol S can call insert_m1 after evict_symbol(S)
has run:

A: evict_symbol(S)                 -> cache cleared for S, refcount gone
A's prefetch thread (still live):
   insert_m1(S_02, chunks)         -> S_02 re-inserted with refcount 0

Consequence: an entry sits in the cache with no covering reference
count. It is only reclaimed if some later iterator happens to
retain-then-release S again — otherwise it is a leak. This is sharper
than Race 1, whose only cost is redundant I/O.

Direction

Race 1: retain before load in both stream_m1_windowed and
stream_tick_windowed so symbol_refs[S] >= 1 spans the whole
setup, closing the window.
Race 2: prevent insert_* from inserting a key whose symbol is
not currently retained, or extend the reference count to cover
in-flight prefetches.
Add a regression test exercising concurrent iterate/drop on a
shared symbol to ratify the fix.

Two related thread-safety races share one root cause: in the `stream_*_windowed` setup path, a file is loaded into the cache *before* the symbol is protected by a reference count, so a concurrent eviction can race the load. Both are verified by reading the source; the concrete interleavings below are flagged as unverified — no reproducing test exists yet. ## Root cause In `src/lib.rs`, `stream_m1_windowed` (and identically `stream_tick_windowed`) loads before it retains: ```rust // src/lib.rs:215-220 self.ensure_m1_loaded(&files[0]); // load into cache if files.len() > 1 { self.prefetch_m1(&files[1]); // spawn background load } self.cache.retain_symbol(&symbol_arc); // only now protected ``` `stream_tick_windowed` has the same ordering at `src/lib.rs:250-255`. Between the load and the `retain_symbol`, the freshly loaded file is covered by no reference count, so `release_symbol` -> `evict_symbol` (`src/cache.rs:158-181`) from another thread can drop it. ## Race 1 — eviction between load and retain Claim (unverified, deduced from the ordering above): with thread A iterating symbol S (`symbol_refs[S] == 1`) and thread B entering `stream_m1_windowed(S)`: ```text B: ensure_m1_loaded(S_01) -> S_01 now cached A: drop(iter) -> release_symbol(S) -> refcount 1->0 -> evict_symbol(S) -> S_01 evicted B: retain_symbol(S) -> refcount 0->1 (protects an empty slot) B: next_chunk() -> ensure_m1_loaded(S_01) -> not cached -> reloads ``` Consequence: redundant I/O (the file is reloaded). Delivered records stay correct, because the iterator holds an `Arc` clone of any chunk it already obtained (`current_file_chunks`, `src/lib.rs:416`). ## Race 2 — prefetch insert outlives an evict Claim (unverified, deduced from `prefetch_m1` at `src/lib.rs:340-356` and `insert_m1` at `src/cache.rs:77-84`): a background prefetch thread spawned for symbol S can call `insert_m1` *after* `evict_symbol(S)` has run: ```text A: evict_symbol(S) -> cache cleared for S, refcount gone A's prefetch thread (still live): insert_m1(S_02, chunks) -> S_02 re-inserted with refcount 0 ``` Consequence: an entry sits in the cache with no covering reference count. It is only reclaimed if some later iterator happens to retain-then-release S again — otherwise it is a leak. This is sharper than Race 1, whose only cost is redundant I/O. ## Direction - [ ] Race 1: retain before load in both `stream_m1_windowed` and `stream_tick_windowed` so `symbol_refs[S] >= 1` spans the whole setup, closing the window. - [ ] Race 2: prevent `insert_*` from inserting a key whose symbol is not currently retained, or extend the reference count to cover in-flight prefetches. - [ ] Add a regression test exercising concurrent iterate/drop on a shared symbol to ratify the fix.

Brummel added the bug label 2026-06-04 17:00:53 +02:00

Brummel referenced this issue from a commit

2026-06-04 17:51:44 +02:00

Close stream setup races with SymbolGuard ownership model

Brummel closed this issue

2026-06-04 17:51:44 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Brummel/data-server#1