fix: reap dead-child sessions even when --session-idle-ttl is unset - #425
Merged
Conversation
Closes #385. SessionRegistry.reap_idle() already reaps two independent things — a dead child (backend.closed, unconditionally, TTL or not) and one idle past the TTL (only when idle_ttl > 0) — but start_reaper() gated STARTING THE THREAD AT ALL on the TTL alone. In the default deployment (--session-idle-ttl unset, 0) the thread never started, so the unconditional dead-child sweep had no production caller: a long-lived gateway on default settings accumulated dead-child sessions with no way to shed them, eating into --max-sessions until new initialize calls got 503 against slots that were all corpses. start_reaper() now always starts (no-op only on a second call): the tick interval favors the idle TTL when one is configured, and falls back to sweeping for dead children alone, at least once a minute (_MAX_REAP_INTERVAL_SECS), when it is not. Mirrors the identical fix already applied to the modern pool's analogous reaper (#390 R3F1, ModernBackendPool.start_reaper) — separate "why does the thread run" from "what does each tick evict", since reap_idle was always correct on the latter. The modern pool's own reaper still has a residual version of this gap (its start condition also needs --modern-idle-ttl or the MRTR bridge) — out of this issue's scope, left for a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Yjo4vhXdQAbA6xHPW9NdS
ai-review found two advisory issues: - R1F1: the docstring claimed this "mirrors the identical fix" applied to the modern pool's reaper, but the modern pool's start_reaper still gates on modern_idle_ttl > 0 or the MRTR bridge, so a pure-default deployment still never starts that thread — the legacy fix here has no equivalent second gate. Reworded to distinguish "same pattern" from "same guarantee": only the former is true. - R1F2: test_reaper_thread_does_not_evict_idle_alive_session_when_ttl_ disabled only checked that an idle-alive session's count stayed at 1 — which would pass just the same if the reaper's daemon thread had silently died before ever ticking, proving nothing about the sweep logic itself. Now creates a SEPARATE dead-child session alongside it and waits for that one to actually be reaped before asserting the alive one survived, so the test can only pass if the reaper genuinely ran and correctly distinguished between the two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Yjo4vhXdQAbA6xHPW9NdS
ai-review round 2 (PR #425) found R2F1 (advisory): test_reaper_thread_does_not_evict_idle_alive_session_when_ttl_disabled started the reaper (ticking every 0.2s) BEFORE creating both sessions, so on a slow enough runner it could reap the dead-child session before the count == 2 sanity check ran, making the test spuriously fail. Both sessions are now created and the dead one killed first; start_reaper() runs only after the count == 2 assertion, so the very first tick always has a stable starting point to sweep from. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Yjo4vhXdQAbA6xHPW9NdS
Contributor
AI review (gpt-5.6-sol)Verdict: no blocking findings. Scope: new commits since No findings clear the reporting bar this round. Findings ledger (all rounds)
Advisory per-push review (round 3) generated by ai-review — verify findings before acting. |
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.
Summary
Closes #385.
SessionRegistry.reap_idle()already reaps two independent things — a dead child (backend.closed, unconditionally, TTL or not) and one idle past the TTL (only whenidle_ttl > 0) — butstart_reaper()gated starting the thread at all on the TTL alone. In the default deployment (--session-idle-ttlunset,0) the thread never started, so the unconditional dead-child sweep had no production caller: a long-lived gateway on default settings accumulated dead-child sessions with no way to shed them, eating into--max-sessionsuntil newinitializecalls got503against slots that were all corpses.Fix
start_reaper()now always starts (no-op only on a second call): the tick interval favors the idle TTL when one is configured, and falls back to sweeping for dead children alone, at least once a minute (_MAX_REAP_INTERVAL_SECS), when it is not.This mirrors the identical fix already applied to the modern pool's analogous reaper (#390 R3F1,
ModernBackendPool.start_reaper) — separate "why does the thread run" from "what does each tick evict", sincereap_idlewas always correct on the latter.Deliberately out of scope
The modern pool's own reaper still has a residual version of this gap — its start condition needs
--modern-idle-ttlor the MRTR bridge enabled, so a pure-default deployment (neither set) still never starts it. That's the same class of bug, just not this issue's literal scope (#385 was explicitly split out of #383 to stay a narrow, provable fix). Happy to file a follow-up if wanted.Test plan
pytest tests/ -v— 1993 passed, 3 skippedpytest tests_integration/ -v— 42 passedruff check ./ruff format --check .— cleanidle_ttl=0; it sweeps a dead-child session even with no TTL configured (via a monkeypatched fast tick interval, so the test doesn't wait out the real 60s fallback); a merely-idle-but-alive session is left alone in that same configuration, proving this didn't silently enable full idle evictionhttps://claude.ai/code/session_019Yjo4vhXdQAbA6xHPW9NdS