Uh oh!
There was an error while loading. Please reload this page.
fix(server): cap background-work deferral and report session-teardown losses - #5711
fix(server): cap background-work deferral and report session-teardown losses#5711gfsaaser24 wants to merge 1 commit into
Conversation
…wn losses Builds on pingdotgg#5677, which correctly stopped the reaper from killing sessions with live background work. This extends that fix along the edges where it was still lossy or unbounded: 1. Wedge cap for the deferral (pingdotgg#5677 hardening). The background-liveness skip was unconditional, so a task that never reaches a terminal state pins its session — and the provider child process — forever. Deferral now applies up to backgroundWorkMaxIdleMs (default 4h, floored strictly above the inactivity threshold so a misconfigured cap cannot silently disable deferral). The sweep reads ThreadBackgroundLivenessService directly: the projected shell's backgroundLiveness is computed from the same service at snapshot-build time, so this is the same signal minus the staleness. 2. lastSeenAt heartbeat on provider-initiated activity. The binding's lastSeenAt only refreshed on user-initiated sendTurn; turns the provider starts on its own (background task notifications waking the agent) and long-running tasks heartbeating via task.progress never routed through it, so a thread doing autonomous work read as idle from the user's last keystroke. The event pump now touches the binding on turn/task lifecycle events, throttled per thread (spent only after a successful upsert), turning the wedge cap into "no heartbeat at all for the cap window" — the definition of wedged. 3. Binding consistency on session.exited. Adapter-internal exits (stream end, session replace, stopAll) never pass through stopSession and left ghost `running` rows the reaper kept sweeping. The pump now marks the binding stopped, guarded against stale exits two ways: an exit from a different provider instance than the binding's owner is ignored (the emitting adapter's listSessions cannot see a replacement on another instance), and a same-instance exit is ignored when the adapter holds a session created strictly after the exit event (NaN-safe: an unparseable timestamp cannot count as a replacement). 4. Honest teardown loss reporting (Claude adapter). Session teardown drains liveTaskIds, emitting a terminal task.completed(stopped) per task — skipping ids a concurrently-resumed notification handler already settled — and a stream death with live tasks additionally emits a runtime.error naming the lost agents plus a structured claude.session.stream-ended-with-live-tasks log, so the previous silent kill is now visible both to the user and in the field. Diagnosed from a production incident: 13 session stops across three threads in one day, each landing 30.1-39.7 minutes after the last user message while background agents were mid-work; one session was killed while running the test suite that verified this fix. The pingdotgg#5677 test is kept, adapted to the capped deferral (its months-stale scenario now correctly reaps — that is the wedge the cap exists for, pinned by its own test). Verification on this base: ProviderSessionReaper + ProviderService + ClaudeAdapter suites 110/110; ProviderRuntimeIngestion 45/45; typecheck clean. The cross-instance guard test fails without the guard.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| ) { | ||
| return; | ||
| } | ||
| if (isSessionExit) { |
There was a problem hiding this comment.
🟡 MediumLayers/ProviderService.ts:347
syncBindingOnRuntimeEvent skips the providerInstanceId ownership check for activity events (turn.*/task.*), so after a thread moves to a different provider instance, stale events still in the old subscription's stream overwrite the new binding's lastRuntimeEvent/lastRuntimeEventAt and refresh lastSeenAt. The stale event also consumes the thread-wide 60-second throttle, which can suppress a legitimate heartbeat from the replacement session. The session-exited branch guards against this by comparing event.providerInstanceId against the persisted binding, but the activity-touch branch does not. Apply the same instance-ownership guard before activity touches, and key the throttle map by instance id so overlapping stale streams cannot suppress the replacement's heartbeats.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ProviderService.ts around line 347:
`syncBindingOnRuntimeEvent` skips the `providerInstanceId` ownership check for activity events (`turn.*`/`task.*`), so after a thread moves to a different provider instance, stale events still in the old subscription's stream overwrite the new binding's `lastRuntimeEvent`/`lastRuntimeEventAt` and refresh `lastSeenAt`. The stale event also consumes the thread-wide 60-second throttle, which can suppress a legitimate heartbeat from the replacement session. The session-exited branch guards against this by comparing `event.providerInstanceId` against the persisted binding, but the activity-touch branch does not. Apply the same instance-ownership guard before activity touches, and key the throttle map by instance id so overlapping stale streams cannot suppress the replacement's heartbeats.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR introduces substantial new runtime behavior in session lifecycle management: lost-task reporting on teardown, binding synchronization on runtime events, and reaper deferral for background work. These are significant infrastructure changes beyond a targeted bug fix, and there is an unresolved Medium-severity finding about missing instance ownership guards for activity events. You can customize Macroscope's approvability policy. Learn more. |
What Changed
Completes #5677. The background-liveness skip gets a wedge cap (
backgroundWorkMaxIdleMs, default 4h) so a task that never reaches a terminal state cannot pin its session and child process forever. The binding'slastSeenAtnow refreshes on provider-initiated turn/task lifecycle events, not just usersendTurn.session.exitedflowing through the event pump marks the persisted bindingstopped— guarded against stale exits from a replaced session, including replacements on a different provider instance. Claude session teardown drains still-live task ids with terminaltask.completed(stopped)events, and a stream death with live tasks emits aruntime.errornaming the lost agents plus a structured log.Why
#5677 correctly stopped the reaper from killing sessions with live background work, but the edges around it are still lossy or unbounded. The deferral is unconditional, so the failure mode just flips: a wedged task now pins its session forever instead of being killed too early.
lastSeenAtstill only moves on user-initiated turns, so a thread doing autonomous work reads as idle from the user's last keystroke — background liveness defers the reap, but the idle clock itself stays dishonest for anything else keyed to it. Adapter-internal exits (stream end, session replace,stopAll) never pass throughstopSessionand leave ghostrunningrows the sweep re-visits forever. And when teardown does kill live agents, the user finds out on their next message, potentially 40+ minutes later.This was diagnosed from a production day with 13 session stops across three threads, each landing 30.1–39.7 minutes after the last user message while background agents were mid-work — one session was killed while running the test suite that verifies this fix. With the heartbeat in place, the wedge cap means "no heartbeat at all for the cap window", which is the definition of wedged.
The sweep reads
ThreadBackgroundLivenessServicedirectly rather than the projected shell — the shell'sbackgroundLivenessis computed from the same service at snapshot-build time, so it is the same signal minus the snapshot staleness. #5677's test is kept and adapted to the capped deferral: its months-stale scenario now correctly reaps, and that boundary is pinned by its own wedge-cap test. The stale-exit guard's cross-instance test fails without the guard (verified by reverting and re-running).ProviderSessionReaper+ProviderService+ClaudeAdapter: 110/110 on this base;ProviderRuntimeIngestion: 45/45; typecheck clean.Bigger than the contributing guide prefers — it stays one PR because the four pieces enforce one invariant (background work is neither silently killed nor allowed to pin a session, and its loss is visible), and the heartbeat is what makes the cap meaningful. Happy to split it if you'd rather take it piecewise.
Checklist
I included before/after screenshots for any UI changes(no UI changes)I included a video for animation/interaction changes(n/a)Change authored by Claude Fable 5 via Claude Code.
Note
Medium Risk
Changes core provider session teardown, persisted binding sync, and reaper heuristics; mistakes could leave ghost running sessions or reap live autonomous work, though extensive guards and tests target the production failure modes.
Overview
Tightens background-work session lifecycle so autonomous provider work is neither silently killed nor allowed to pin processes forever, and teardown losses are visible.
Claude adapter: On SDK stream exit (clean or failed) while
liveTaskIdsremain, the adapter now logsclaude.session.stream-ended-with-live-tasks, emits aruntime.errornaming the lost agents, and drains each live task withtask.completed(stopped), including a second drain after stream-fiber interrupt to close races with in-flight handlers.Provider event pump: Adds
syncBindingOnRuntimeEventsosession.exitedmarks the persisted bindingstopped(clearing ghostrunningrows from adapter-internal exits), with guards for stale exits (newer replacement session on the same instance, or binding owned by a different provider instance). Turn/task lifecycle events refreshlastSeenAt(60s throttle) so provider-initiated work does not look idle since the user’s lastsendTurn.Session reaper: Defers reaping idle sessions while
ThreadBackgroundLivenessServicereports live work, but only up tobackgroundWorkMaxIdleMs(default 4h, floored above the inactivity threshold); beyond that, wedged “live” work is reaped anyway. Sweep uses the liveness service directly instead of the projected shell.Tests and
docs/internals/providers.mddocument the three rules (teardown reports loss, binding follows session, reaper respects background work with a cap).Reviewed by Cursor Bugbot for commit 5aeeb1b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Cap background-work deferral in session reaper and report live-task losses on session teardown
handleStreamExitnow detects background tasks still tracked as live at stream termination, emits aruntime.errordescribing the loss, and emitstask.completed(statusstopped) for each live task. A structured warning is also logged.ProviderSessionReaperdefers reaping idle sessions while background work is live, up to a configurablebackgroundWorkMaxIdleMscap (default 4h), after which the session is reaped regardless.ProviderServiceevent pump now syncs persisted bindings: marks a bindingstoppedonsession.exited(guarded against stale cross-instance or replaced-session exits), and refresheslastSeenAton turn/task lifecycle events with per-thread 60s throttling.📊 Macroscope summarized 5aeeb1b. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.