fix(gateway): tombstone deleted sessions to stop trace-store leak#797
Draft
jeffreysijuntan wants to merge 2 commits into
Draft
fix(gateway): tombstone deleted sessions to stop trace-store leak#797jeffreysijuntan wants to merge 2 commits into
jeffreysijuntan wants to merge 2 commits into
Conversation
Traces persist fire-and-forget (sync_traces=False), so an LLM completion still in flight when a rollout ends can call store_trace *after* the trainer's delete_session. MemoryTraceStore then resurrected the session via its defaultdict session index -- and since no reader ever deletes that uid again, the session leaked for the process lifetime. Observed live on a Fireworks async run: a steady ~157 orphaned single-trace sessions/hr (each a large mid-episode straggler), unbounded (the store has no TTL/eviction). Record a tombstone (session_id -> deletion time) in delete_session and drop any store_trace for a tombstoned session within the TTL window (default 1800s, comfortably longer than the 600s request timeout). Tombstones are purged on the delete cold path so the map stays bounded to one TTL window; ttl<=0 disables the guard (legacy behaviour). SqliteTraceStore shares the same INSERT-OR-REPLACE resurrection pattern and would take the same guard -- left as a follow-up since the observed leak is on the in-use memory backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Session ids are unique uuids that are never reused, so a deleted session must be rejected permanently, not for a time window. A TTL also has a real failure mode: if a request outlives the TTL (the heartbeat budget allows up to 3600s, longer than the prior 1800s default), a very-late straggler could still resurrect the session. Replace the time-based tombstone with a permanent set of deleted session ids, FIFO-capped (default 1M) purely as a memory backstop for long-lived multi-run gateways -- far larger than the number of sessions deletable while any request is still in flight, so eviction never exposes a live straggler. max_tombstones=0 disables. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The gateway's in-memory trace store (
--store memory) leaks sessions during async RL training. Observed live on a Fireworks run: ~157 orphaned sessions/hr, each exactly 1 trace, at old weight versions, with large mid-episode prompts — accumulating unbounded (the store has no TTL/eviction).Root cause — a persist-after-delete race
Every rollout correctly deletes its own gateway session in a
finally(so filtering, staleness, and errors are all cleaned up — those were not the leak). The leak is a lifecycle race:sync_traces=False): the proxy doesasyncio.create_task(store_trace(...))after each completion returns.flush()thendelete_session(uid).flush()only drains persist tasks that already exist — an LLM call still in flight at the provider hasn't reached the persist line yet, so it can't be flushed.store_traceruns → andMemoryTraceStoreresurrects the session becauseself._session_index[session_id]is adefaultdict(list)with no tombstone. Nothing deletes thatuidagain → permanent orphan with exactly 1 trace.Signature match: single trace (the lone straggler), large prompt (slow
_build_trace_datawidens the window under high concurrency), grouped by task-uuid (a hard prompt makes several group members straggle together), early weight version (early policy aborts right after big tool-call turns).Fix — a permanent tombstone
Session ids are unique uuids (
uid = f"{uuid4()}:{rollout_idx}") that are never reused, so a deleted session must be rejected permanently, not for a time window:delete_sessionrecords thesession_idin a tombstone set.store_tracedrops any write for a tombstoned session instead of resurrecting it — always correct, since a deleteduidis never legitimately written again.max_tombstones, default 1,000,000) purely as a memory backstop for long-lived multi-run gateways. The cap is far larger than the number of sessions deletable while any request is still in flight, so eviction never exposes a live straggler.max_tombstones=0disables the guard (legacy behaviour).Why not require pre-creation instead? Sessions are implicitly created on first request (a documented gateway feature for captured/external sessions), and the persist path (
ReverseProxy) doesn't hold theSessionManager's liveness state — so remembering deletions (a tombstone) is the correct store-level mechanism.Why not a TTL? A time window has a real failure mode — if a request outlives it (the heartbeat budget allows up to 3600s), a very-late straggler could still resurrect the session. A permanent block has no such edge.
Cost: one interned
session_idper deleted session (~tens of MB worst-case for an entire multi-day run, hard-capped); no change to the read/serialization path.Tests
tests/unit/test_store.py— newTestTombstone: straggler-after-delete is dropped (not resurrected), the block is permanent across repeated late writes, unrelated sessions unaffected, FIFO cap evicts the oldest tombstone, andmax_tombstones=0restores legacy behaviour. Full store suite: 37 passed.Scope / follow-up
SqliteTraceStoreshares the same resurrection pattern (INSERT OR REPLACE/IGNOREon write, plainDELETE) and would take the identical guard — left as a follow-up since the observed leak is on the in-usememorybackend.This is a separate fix from the array-packing perf change (#787, already merged into this branch); the leak is a distinct, slower, unbounded issue on top of the O(N²) working-set growth.
🤖 Generated with Claude Code