fix(lr-f36626): stop idle reaper from orphaning live background subagents#344
Conversation
The idle reaper (sdk-bridge.js startIdleReaper) reaped a session that had correctly dispatched a backgrounded Task and was legitimately WAITING on it -- isProcessing is false in that state (the parent turn's own 'result' already fired), so the reaper's only two bump sites (post-pushMessage, per-turn 'result') never saw the still-running child's output and treated 30 minutes of a live dispatch as abandoned idle time. Orphaned the child's stream (queryInstance.close()), leaving its transcript frozen with zero completion record. Bump session.lastActivityAt in sdk-message-processor.js wherever genuine backgrounded-Task stream activity lands: subagent_message (tool_use/ thinking/text), tool_progress, task_started, task_progress, task_updated. Deliberately not a blanket 'never reap while any child is registered' rule -- a dead/silent child (crashed, stuck, or simply never checks back in) must still become reapable once the timeout genuinely elapses with zero activity, or a leaked registration would pin the session alive forever.
Covers both directions per the LOCKED SCOPE: - subagent stream events (subagent_message, tool_progress, task_started, task_progress, task_updated) bump session.lastActivityAt - a session with recent subagent activity survives the idle reaper past IDLE_TIMEOUT_MS even though isProcessing is false - a session whose child registration is stale (activeTaskToolIds non-empty but zero activity for the full timeout) is still reaped -- rejects a blanket never-reap-if-any-child-registered rule - a genuinely idle session (with or without a child) is reaped exactly as before -- no regression to pre-existing idle-reap behavior
Under memory pressure (lr-5e70 onCrossing), forceEvictToLimit previously
evicted the LRU-oldest loaded session regardless of whether it carried a
live registered background child (session.activeTaskToolIds non-empty).
Split into two passes: pass 1 evicts childless candidates only; pass 2
falls back to live-child candidates solely once pass 1 is exhausted and
the pressure limit is still not met -- the memory ceiling (lr-c10f6d) must
still win over deferring to a background dispatch, but only as a genuine
last resort.
Eviction itself (_evictSession) never touches queryInstance -- it only
unloads a session's in-heap history window, reloaded lazily on next
access -- so this was never the reap-and-orphan failure mode lr-f36626
diagnosed (that was always the idle reaper, fixed separately). What was
missing is visibility: forceEvictToLimit now returns
{ evicted, reclaimedLiveChild } instead of a bare count so the caller
(lib/memory-shed.js) can notify the operator per-session when pressure
reached as far as a live dispatch, instead of reclaiming it silently.
Also persists a durable pendingAutoResume marker in the session meta line
(buildMetaLine/loadSessions) so a future console-driven AUTO-RESUME (see
lib/memory-shed.js and lib/project-connection.js) survives a daemon
restart between the reclaim and the operator's next reconnect.
…d session shedMemory now consumes forceEvictToLimit's reclaimedLiveChild list (see prior commit). For each session force-evicted while it carried a live registered background child, this is the ONLY path allowed to reclaim it (the idle reaper refuses to -- see the lastActivityAt bump commits), and per spec it must never do so silently: - session.pendingAutoResume=true + pendingAutoResumeReason, persisted via sm.saveSessionFile -- the durable checkpoint the operator's next reconnect drives an AUTO-RESUME from (lib/project-connection.js). - Diagnostics panel: a project-wide diagnostic (source: 'memory-reclaim'), same delivery path as the routine pressure diagnostic. - Inline red chat marker: sendAndRecord's a type:'error' entry into the affected session's own history -- renders via the existing sys-msg.error bubble (lib/public/modules/app-messages.js's case 'error'), zero new frontend code, and replays correctly on reconnect like any other history entry. An idle-no-child shedding pass (the common case) is unaffected and emits no per-session reclaim notification -- only the routine memory diagnostic, unchanged from before.
…sion Eviction under memory pressure never kills a live child's queryInstance -- the child keeps running; only in-heap history is unloaded (see the sessions.js commit). So the console-driven 'resume' for a session flagged pendingAutoResume is: the next reconnect to that session reloads its history from disk (the durable checkpoint) as the operator would see it, and the flag is cleared so this fires exactly once rather than on every subsequent reconnect.
Covers: - forceEvictToLimit prefers childless candidates before touching a live-child session - falls back to a live-child session as last resort once childless candidates are exhausted (memory ceiling still wins) - eviction never closes queryInstance -- the child's process keeps running - shedMemory sets a durable pendingAutoResume marker only when a live child was actually reclaimed, not on every shedding pass - both UI surfaces (Diagnostics panel diagnostic + inline red chat marker) fire on an actual live-child reclaim - the durable marker round-trips through a fresh session reload (survives a daemon restart between the reclaim and the operator's reconnect) - an idle-no-child shedding pass emits no per-session reclaim diagnostic
|
PEACHES — clean (0 blocking findings, 1 nit) Idle-path fix verified. Session.lastActivityAt is bumped at line 94 (lib/sdk-message-processor.js processSubagentMessage), ONLY on genuine subagent stream activity (subagent_message, tool_progress with parentToolId, task_started/updated). A dead/silent child with no activity still times out and is reaped — no blanket exemption (test lr-f36626 line 29 validates this). Pressure path verified. forceEvictToLimit (lib/sessions.js 452–499) runs two passes: childless-only, then live-child-as-last-resort once pass 1 is exhausted. The lr-c10f6d absolute-bytes ceiling wins even over live-child sessions (test line 115 confirms live-child touched only when limit forces it). Notification path verified. lib/memory-shed.js lines 260–275 broadcast Diagnostics AND inline error marker via sendAndRecord—both fire ONLY on actual live-child reclaim (loop scoped to reclaimedLiveChild, line 242). Test line 252 confirms idle-no-child passes emit no memory-reclaim diagnostic. Durable AUTO-RESUME verified. pendingAutoResume persisted in meta line (lib/sessions.js 99–102), restored on reload (688–691), cleared exactly once on reconnect (lib/project-connection.js 208–211). Test line 235 confirms round-trip across daemon restart. Regression tests comprehensive. Both test files cover all four directions: activity bumps, not-reaped, still-reaped if stale, no regression to pre-existing idle. queryInstance never closed (test line 143). One nit. lib/memory-shed.js line 113: comment says 'lr-f36626' twice in successive lines (114 start, 123 end) without context window. Readability note only — code is correct. |
|
BOBBIE -- clean Audited PR #344 (lr-f36626: idle reaper / memory-reclaim session-lifecycle fix). Scope: base..head diff (7 files, +867/-5) -- lib/sdk-message-processor.js, lib/sdk-bridge.js, lib/sessions.js, lib/memory-shed.js, lib/project-connection.js, plus 2 new test files. No findings. Reviewed specifically:
Scanners: gitleaks clean over the PR commit range (6 commits, base 70bfd76..head a9e59bf). semgrep (auto config) against all 5 changed lib/ files: 7 findings, all on console.error format-string lines outside the base..head diff hunks in lib/sdk-bridge.js and lib/project-connection.js (pre-existing code, comment-only or unrelated-line changes in this PR) -- dropped per Pre-Report Gate criterion 1 (not inside a diff hunk). osv-scanner: dependency findings are against the pre-existing package-lock.json, which this PR does not touch -- out of base..head scope. trufflehog binary unavailable in this environment (scanner_status: unavailable); gitleaks covered the same commit range as a secret-scan backstop. review.status: clean |
|
Merged via clagentic-loadout v0.1.0
|
clagentic-merger
Bot
deleted the
fix/lr-f36626-idle-reaper-subagent-orphan
branch
What changed
Fixes lr-f36626: the idle-session reaper in lib/sdk-bridge.js was killing sessions that had correctly dispatched a background subagent (Agent-tool Task) and were legitimately WAITING on it. session.isProcessing is false in that state (the parent turn's own SDK 'result' already fired), so the reaper's only two bump sites for lastActivityAt (post-pushMessage on startQuery, per-turn 'result') never saw the still-running child's output. After 30 minutes the reaper closed queryInstance, orphaning the child mid-dispatch with zero completion record.
Reconciles against the OOM-hardening chain (lr-2ea2a7 / lr-c10f6d / lr-5e70) per the locked scope from the dispatch brief -- the lr-c10f6d absolute-bytes memory ceiling is untouched and remains a hard invariant.
Idle path (never reap a live/progressing child; reject a blanket exemption)
Pressure path (lr-5e70 onCrossing -- the ONLY path allowed to reclaim a live-child session, last resort)
Trade-off named per dispatch instruction
forceEvictToLimit's eviction never kills queryInstance (only unloads in-heap history), so the pressure path was never actually capable of the reap-and-orphan failure mode this task diagnosed -- that was always the idle reaper, fixed separately above. Given that, "AUTO-RESUME" for a pressure-reclaimed session is necessarily lighter-weight than a full durable-checkpoint-and-relaunch: it's a marker plus a reload-on-reconnect, not a console-driven cold restart of a torn-down process. If a future design intentionally needs the pressure path to also close queryInstance as a harder last resort, that would need its own decision -- this PR does not add that capability, since doing so was not required to satisfy the locked-scope acceptance criteria (memory ceiling honored, live reclaim never silent, both surfaces notified).
Not folded in
lr-255e is explicitly excluded per the dispatch brief.
Tests
npm test -- 969 tests, 965 pass. The 4 failures are pre-existing and unrelated (test/sdk-bridge-context-window-warn.test.js, lr-1f7e) -- confirmed via git stash against the pre-change baseline before starting this work.
New regression tests (both directions per LOCKED SCOPE):
Task ID: lr-f36626