Skip to content

Commit 9d8aa7b

Browse files
committed
fix(viz): evict idle LogStream entries without requiring a later release
LogStreamRegistry only ran _sweep_idle_streams_locked() from inside release(), so a stream whose refcount dropped to zero without EOF stayed resident for the process lifetime whenever no subsequent release() fired after the idle TTL. Long-lived dashboards with low-churn traffic leaked every LogStream that ever had a one-off disconnect, together with its retained event buffer. Piggyback the sweep onto acquire() and streams_in_cache_dir() too. New SSE connections anywhere on the dashboard reclaim stale retention deques from unrelated sessions, and the cache watcher callback (which calls streams_in_cache_dir on every observed write) keeps eviction driven by ongoing activity instead of only by balanced refcount transitions. Add a streaming regression that rewinds _idle_since and verifies both call-sites evict idle streams.
1 parent 66a8d06 commit 9d8aa7b

2 files changed

Lines changed: 61 additions & 0 deletions

File tree

tests/test-streaming.sh

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,54 @@ else
471471
_fail "out-of-window reconnect wrong: $OUTPUT"
472472
fi
473473

474+
# ─── Idle stream eviction without follow-up release ───
475+
# Regression: a stream whose refcount drops to zero without EOF should
476+
# not survive forever when no subsequent release() ever fires. Sweeps
477+
# must also run on other registry interactions (acquire,
478+
# streams_in_cache_dir) so idle retention deques are reclaimed under
479+
# low-churn traffic.
480+
SWEEPLOG="$CACHE_DIR/round-9-codex-run.log"
481+
: > "$SWEEPLOG"
482+
483+
OUTPUT="$(_run_py "
484+
import time
485+
from log_streamer import LogStreamRegistry
486+
# Use a non-zero TTL and rewind the recorded idle timestamp so the
487+
# next sweep observes the TTL as elapsed without real waiting. Reaching
488+
# into a private dict is acceptable in a white-box regression test:
489+
# the point is to verify which call-sites sweep, not real-time timing.
490+
reg = LogStreamRegistry(idle_ttl_seconds=60.0)
491+
# Stream A: one-off disconnect, no EOF, no further release on the same key.
492+
reg.acquire('$CACHE_DIR', 'sid-sweep-A', 'round-9-codex-run.log')
493+
reg.release('sid-sweep-A', 'round-9-codex-run.log')
494+
print('A_PRESENT_BEFORE_SWEEP:', ('sid-sweep-A', 'round-9-codex-run.log') in reg)
495+
# Force A's idle_since far in the past so any subsequent sweep evicts it.
496+
reg._idle_since[('sid-sweep-A', 'round-9-codex-run.log')] = time.monotonic() - 1e6
497+
# New acquire on a different session must trigger the sweep.
498+
reg.acquire('$CACHE_DIR', 'sid-sweep-B', 'round-9-codex-run.log')
499+
print('A_EVICTED_BY_ACQUIRE:', ('sid-sweep-A', 'round-9-codex-run.log') not in reg)
500+
print('B_PRESENT:', ('sid-sweep-B', 'round-9-codex-run.log') in reg)
501+
502+
# Independent registry: verify streams_in_cache_dir() (invoked by the
503+
# cache watcher callback on every observed write) also evicts idle
504+
# streams even when no release() follows.
505+
reg2 = LogStreamRegistry(idle_ttl_seconds=60.0)
506+
reg2.acquire('$CACHE_DIR', 'sid-sweep-C', 'round-9-codex-run.log')
507+
reg2.release('sid-sweep-C', 'round-9-codex-run.log')
508+
reg2._idle_since[('sid-sweep-C', 'round-9-codex-run.log')] = time.monotonic() - 1e6
509+
_ = reg2.streams_in_cache_dir('$CACHE_DIR', 'round-9-codex-run.log')
510+
print('C_EVICTED_BY_STREAMS_LOOKUP:', ('sid-sweep-C', 'round-9-codex-run.log') not in reg2)
511+
")"
512+
513+
if grep -q '^A_PRESENT_BEFORE_SWEEP: True$' <<<"$OUTPUT" && \
514+
grep -q '^A_EVICTED_BY_ACQUIRE: True$' <<<"$OUTPUT" && \
515+
grep -q '^B_PRESENT: True$' <<<"$OUTPUT" && \
516+
grep -q '^C_EVICTED_BY_STREAMS_LOOKUP: True$' <<<"$OUTPUT"; then
517+
_pass "idle streams are evicted by acquire() and streams_in_cache_dir(), not only by a follow-up release()"
518+
else
519+
_fail "idle-stream sweep regression: $OUTPUT"
520+
fi
521+
474522
# ─── Summary ───
475523
echo
476524
echo "========================================"

viz/server/log_streamer.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,13 @@ def acquire(self, cache_dir: str, session_id: str, basename: str) -> LogStream:
398398
"""
399399
key = (session_id, basename)
400400
with self._lock:
401+
# Every new acquire is also a chance to drop OTHER entries
402+
# whose idle TTL has elapsed without a follow-up release.
403+
# Without this, a refcount=0 stream that is never released
404+
# again (one-off disconnect on a long-lived session) would
405+
# stay resident for the process lifetime and leak its
406+
# retention deque.
407+
self._sweep_idle_streams_locked()
401408
stream = self._streams.get(key)
402409
if stream is None:
403410
stream = LogStream(cache_dir, basename)
@@ -475,6 +482,12 @@ def get(self, session_id: str, basename: str) -> Optional[LogStream]:
475482
def streams_in_cache_dir(self, cache_dir: str, basename: str) -> List[LogStream]:
476483
"""Return all streams that observe a specific cache file."""
477484
with self._lock:
485+
# Piggyback a sweep: this method is invoked from the cache
486+
# watcher callback on every observed write, so leveraging
487+
# it keeps idle eviction driven by ongoing activity rather
488+
# than only by the next ``release()`` call, which may
489+
# never happen on long-lived dashboards with low churn.
490+
self._sweep_idle_streams_locked()
478491
return [
479492
s for s in self._streams.values()
480493
if s.cache_dir == cache_dir and s.basename == basename

0 commit comments

Comments
 (0)