fix(foreground-fallback): protect managed child sessions and skip replay after streamed output - #1011
Conversation
Greptile SummaryThe PR narrows foreground fallback so background-task child sessions remain under their owning task lifecycle and user turns are not replayed after partial assistant text has already streamed.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified. The ownership predicate is applied before both abort and replay, task-session events update ownership before fallback handles the same event, and the replay guard is scoped to meaningful assistant output after the latest replayable user turn. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
E[Retryable session error] --> O{Managed task child?}
O -->|Yes| T[Leave recovery to task lifecycle]
O -->|No| M[Load session messages]
M --> A{Meaningful assistant text after last user turn?}
A -->|Yes| K[Keep partial output and skip replay]
A -->|No| F[Select fallback model]
F --> R[Replay user turn]
Reviews (1): Last reviewed commit: "fix(foreground-fallback): protect backgr..." | Re-trigger Greptile |
…te leaks, and stream races - Replay guard now treats tool-call parts as attempt activity: replaying a turn whose attempt issued tools would re-execute them with duplicate side effects (hasAssistantActivity replaces hasMeaningfulAssistantOutput). - Fallback model (sessionTried) and retry budget are only consumed when the replay actually happens; a skipped replay no longer marks the next model as tried or resets retries, so a later failure can still use it. - Stream-settlement hardening: when the failed attempt's message is still open (time.created without time.completed), wait once for the stream to settle and re-check before replaying, closing the race where partial output lands after the guard reads session.messages(). - Restart recovery: seed managed child-session ownership from session.list() at init so pre-existing child sessions (no session.created re-emission after a plugin reload) stay under their task lifecycle (collectManagedChildSessionIDs). - Tests: sequential-event (skipped replay does not consume the next model), tool-only replay skip, settle-wait land/replay paths, wiring/order predicate gating, and restart ownership collection/removal.
|
|
@adevwithpurpose can you please resolve the conflicts and we can merge this in cc @alvinunreal |
f3997fa to
346e6b3
Compare
|
Done — rebased both commits onto master (f47b1c2) and resolved the conflicts:
Verified after rebase: tsc clean, 113 foreground-fallback + 144 task-session-manager tests passing, biome check clean, CI green on this branch. Ready to merge from our side. |
What changed, and why was it needed?
Relates to #595 and #560 (orphaned/resumed background tasks). Two failure modes remain when a managed child session errors:
This change narrows the fallback's blast radius. It does not claim to fully fix orphan-result handling — that remains owned by the background-task lifecycle (#595/#560):
ForegroundFallbackManagernow accepts ashouldHandleSessionpredicate. Sessions owned by the background-task lifecycle (registered onsession.createdwith aparentID, present on the background job board, or seeded fromsession.list()at init) are skipped by both the abort path and the replay path, so fallback no longer detaches managed child sessions. After a plugin reload, pre-existing child sessions do not re-emitsession.created, so ownership is also seeded from the session list snapshot at init (collectManagedChildSessionIDs) — a restart can no longer leave an in-flight child unprotected.hasAssistantActivity) blocks re-queueing the user turn when the failed attempt already produced activity after the last user message: meaningful text (v1 and v2 shapes) or tool-call parts. Reasoning/retry parts do not count as activity. Fallback still applies when the attempt failed before producing any activity (first-token rate limits, pre-stream errors).time.createdwithouttime.completed), the guard waits once (150 ms) and re-checkssession.messages()before deciding to replay. No fixed delay is added when nothing is in flight.Verification
bun test src/hooks/foreground-fallback/index.test.ts: 109 pass, 0 fail (includes managed-child detachment guards, replay-guard text/tool/settlement tests, sequential-event state-consumption regression, and predicate wiring/order tests)bun test -t "restart recovery|tracks background child sessions" src/hooks/task-session-manager/index.test.ts: 3 pass, 0 failbun run typecheck: passbun run check:ci: only pre-existing failures (see below) — changed files are Biome-cleangit diff --check: passKnown pre-existing failures (present at base, not introduced here; both are Windows-only path-separator issues and pass on Linux CI):
task-session-manager"reads before and after launch attach with unique-line counts and caps" hard-codes a forward-slash path (src/large.tsvssrc\large.ts).cache-safety"every hook module defining a message transform is covered here" comparesBun.Globresults against forward-slash expectations and sees backslash paths on Windows.