Skip to content

Commit 5b1d976

Browse files
majdyzclaude
andauthored
fix(backend/copilot): preserve interrupted SDK partial work on final-failure exit (Significant-Gravitas#12918)
## Background [SECRT-2275](https://linear.app/autogpt/issue/SECRT-2275). User report: when a copilot ("autopilot") turn is interrupted by a usage-limit, tool-call-limit, or other run interruption, the user's recent work disappears. User described it as: "my initial message was lost 3 times and it disappeared, then when I would say 'continue' it would do a random old task." Investigation surfaced two distinct failure modes. This PR addresses both. - **Mode 1** — rate-limit (or other pre-stream rejection) at turn start: the user's text only ever lives in the optimistic `useChat` bubble; the backend rejects before the message is persisted, so the bubble is a lie and a refresh / retry would lose the text. - **Mode 2** — long-running turn interrupted mid-stream: the entire turn's progress (assistant text, tool calls, reasoning) vanishes on interruption — what users describe as "the turn is gone." ## Mode 1 — frontend: restore unsent text on 429 Backend can't recover this on its own: `check_rate_limit` raises before `append_and_save_message`, so by the time the 429 surfaces there is no DB row to roll forward. See `autogpt_platform/backend/backend/api/features/chat/routes.py:916-922` (rate-limit check) and `routes.py:945` (later append-and-save). Frontend fix in `autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotStream.ts`: when `useChat`'s `onError` reports a usage-limit error, we - drop the optimistic user bubble (DB has no record of it, so leaving it would be a phantom), - push `lastSubmittedMsgRef.current` back into the composer via the existing `setInitialPrompt` slot — the same slot URL pre-fills use, so `useChatInput`'s `consumeInitialPrompt` effect picks it up automatically, - clear `lastSubmittedMsgRef` so the dedup guard doesn't block re-send. In-memory only; surviving a hard refresh while rate-limited is a separate follow-up (would need localStorage persistence with TTL). Test: `autogpt_platform/frontend/src/app/(platform)/copilot/__tests__/useCopilotStream.test.ts` — verifies the composer is repopulated and the optimistic bubble is dropped on a 429. ## Mode 2 — backend: preserve interrupted partial in DB ### Root cause The SDK retry loop in `stream_chat_completion_sdk` always rolls back `session.messages` to the pre-attempt watermark on any exception. That rollback is correct **before a retry** so attempt #2 doesn't duplicate attempt #1's content. But it runs **before the retry decision is made**, so when retries are exhausted (or no retry is attempted) the partial work is discarded too. Three branches of the retry loop ended in a final-failure state with side effects worse than just losing the partial: - `_HandledStreamError` non-transient: rollback then add error marker — partial gone - `Exception` with `events_yielded > 0`: rollback then break — **no error marker added either**, so on refresh the chat looks like nothing happened even though the user just watched tokens stream live - `Exception` non-context-non-transient + the while-`else:` exhaustion path: same, no marker - Outer except (cancellation, GeneratorExit cleanup): didn't restore captured partial ### Fix `autogpt_platform/backend/backend/copilot/sdk/service.py`: 1. **`_InterruptedAttempt` dataclass** — holds the rolled-back `partial: list[ChatMessage]` + optional `handled_error: _HandledErrorInfo`. Three methods drive the contract: - `capture(session, transcript_builder, transcript_snap, pre_attempt_msg_count)` — slices `session.messages`, restores the transcript, strips trailing error markers to prevent duplicate markers after restore. - `clear()` — drops captured state on a successful retry so outer cleanup paths don't replay pre-retry content. - `finalize(session, state, display_msg, retryable=...) -> list[StreamBaseResponse]` — re-attaches partial, synthesizes `tool_result` rows for orphan `tool_use` blocks, appends the canonical error marker, and returns the flushed events so the caller can yield them to the client (no double-flush). 2. **`_flush_orphan_tool_uses_to_session(session, state) -> list[StreamBaseResponse]`** — synthesizes `tool_result` rows for any `tool_use` that never resolved before the error so the next turn's LLM context stays API-valid (Anthropic rejects orphan tool_use). Uses the public `adapter.flush_unresolved_tool_calls` and returns the events for the caller to yield. 3. **`_classify_final_failure(...) -> _FinalFailure | None`** — picks the display message + stream code + retryable flag for the final-failure exit. One source of truth for the in-history error marker and the client-facing `StreamError` SSE yield so they can't drift. 4. **Consolidated post-loop emit**: the former three scattered blocks (partial restore + redundant re-flush + two separate `yield StreamError` sites) collapsed to one block driven by `_classify_final_failure` → `_FinalFailure` → `finalize()` → yield events + single `StreamError`. 5. **Adapter `flush_unresolved_tool_calls`** (renamed from `_flush_unresolved_tool_calls` to drop the `# noqa: SLF001` suppressors on cross-module callers). Each retry-loop rollback site calls `interrupted.capture(...)`; the success break calls `interrupted.clear()`; the post-loop failure block calls `interrupted.finalize(...)` exactly once. The baseline service already preserves partial work via its existing finally block — no change needed there. ## Tests Backend (`backend/copilot/sdk/interrupted_partial_test.py`, new, 18 tests): - `TestInterruptedAttemptCapture` — slice semantics + stale-marker stripping - `TestInterruptedAttemptFinalize` — appends partial then marker, handles empty partial, no-op on `None` session, flushes unresolved tools between partial and marker, returns flushed events for caller to yield - `TestFlushOrphanToolUses` — synthesizes `tool_result` rows, returns events, no-op on None state / no unresolved - `TestClassifyFinalFailure` — handled_error wins, attempts_exhausted, transient_exhausted, stream_err fallback, returns None on success path - `TestRetryRollbackContract` — end-to-end: capture + finalize yields the exact content the user saw streaming live plus the error marker 1022 total SDK tests pass (baseline + new). Frontend (`useCopilotStream.test.ts`): 1 new test — `restores the unsent text and drops the optimistic user bubble on 429 usage-limit`. ## Out of scope - Frontend rendering tweaks for the interrupted-turn marker (existing error-marker rendering already works). - Refresh-survival of the unsent text in Mode 1 (would require localStorage persistence with TTL) — separate follow-up. - Hard process-kill / OOM where Python `finally` doesn't run — needs a different mechanism (pod-level checkpoint sweeper). ## Checklist - [x] My code follows the style guidelines of this project (black/isort/ruff via `poetry run format`) - [x] I have performed a self-review of my own code - [x] I have added relevant unit tests - [x] I have run lint and tests locally (1022 SDK tests pass) ## Test plan - [ ] Verify a long-running turn that hits transient-retry exhaustion preserves partial assistant text + tool results in chat history after refresh - [ ] Verify the next user message after an interrupted turn carries enough context that the model can continue the prior task instead of inventing a new one - [ ] Verify a successful retry (attempt #1 fails, attempt #2 succeeds) shows ONLY attempt #2's content (no leaked partial from #1) - [ ] Verify hitting daily usage limit at turn start re-populates the composer with the unsent text and removes the optimistic user bubble --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 10ea466 commit 5b1d976

6 files changed

Lines changed: 690 additions & 82 deletions

File tree

Lines changed: 322 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,322 @@
1+
"""Tests for partial-work preservation when an SDK turn is interrupted.
2+
3+
Covers the regression SECRT-2275 surfaced: when the SDK retry loop rolls
4+
back ``session.messages`` for a failed attempt (correct so a successful
5+
retry doesn't duplicate content) it MUST re-attach the rolled-back work on
6+
final-failure exit. Without that, the user's UI streamed tokens live then
7+
a refresh shows an empty turn — described by users as "the turn is gone".
8+
9+
Tests target the ``_InterruptedAttempt`` dataclass + the orphan-tool flush
10+
directly. Full retry-loop coverage lives in ``retry_scenarios_test.py``.
11+
"""
12+
13+
from __future__ import annotations
14+
15+
from unittest.mock import MagicMock
16+
17+
from backend.copilot.constants import (
18+
COPILOT_ERROR_PREFIX,
19+
COPILOT_RETRYABLE_ERROR_PREFIX,
20+
)
21+
from backend.copilot.model import ChatMessage, ChatSession
22+
from backend.copilot.response_model import StreamToolOutputAvailable
23+
24+
from .service import (
25+
_classify_final_failure,
26+
_FinalFailure,
27+
_flush_orphan_tool_uses_to_session,
28+
_HandledErrorInfo,
29+
_InterruptedAttempt,
30+
)
31+
32+
33+
def _make_session(messages: list[ChatMessage] | None = None) -> ChatSession:
34+
session = ChatSession.new(user_id="user-1", dry_run=False)
35+
session.messages = list(messages or [])
36+
return session
37+
38+
39+
def _tool_output(tool_call_id: str, output) -> StreamToolOutputAvailable:
40+
return StreamToolOutputAvailable(
41+
toolCallId=tool_call_id, toolName="t", output=output
42+
)
43+
44+
45+
def _adapter_with_unresolved(responses: list[StreamToolOutputAvailable]):
46+
"""Stub _RetryState whose adapter flushes the given responses."""
47+
adapter = MagicMock()
48+
adapter.has_unresolved_tool_calls = bool(responses)
49+
50+
def _flush(out: list) -> None:
51+
out.extend(responses)
52+
adapter.has_unresolved_tool_calls = False
53+
54+
adapter.flush_unresolved_tool_calls.side_effect = _flush
55+
state = MagicMock()
56+
state.adapter = adapter
57+
return state
58+
59+
60+
def _builder_stub() -> MagicMock:
61+
builder = MagicMock()
62+
builder.restore = MagicMock()
63+
return builder
64+
65+
66+
class TestInterruptedAttemptCapture:
67+
def test_keeps_partial_when_no_marker_present(self):
68+
session = _make_session(
69+
[
70+
ChatMessage(role="user", content="hi"),
71+
ChatMessage(role="assistant", content="part-1"),
72+
]
73+
)
74+
attempt = _InterruptedAttempt()
75+
attempt.capture(session, _builder_stub(), object(), pre_attempt_msg_count=1)
76+
assert [m.content for m in attempt.partial] == ["part-1"]
77+
assert [m.content for m in session.messages] == ["hi"]
78+
79+
def test_strips_trailing_error_marker(self):
80+
# _run_stream_attempt may append a marker (idle timeout, circuit
81+
# breaker) before raising _HandledStreamError. Carrying it forward
82+
# would let finalize() replay it and then add its own.
83+
marker = (
84+
f"{COPILOT_RETRYABLE_ERROR_PREFIX} The session has been idle "
85+
"for too long. Please try again."
86+
)
87+
session = _make_session(
88+
[
89+
ChatMessage(role="user", content="hi"),
90+
ChatMessage(role="assistant", content="part-1"),
91+
ChatMessage(role="assistant", content=marker),
92+
]
93+
)
94+
attempt = _InterruptedAttempt()
95+
attempt.capture(session, _builder_stub(), object(), pre_attempt_msg_count=1)
96+
assert [m.content for m in attempt.partial] == ["part-1"]
97+
98+
def test_strips_consecutive_error_markers(self):
99+
session = _make_session(
100+
[
101+
ChatMessage(role="user", content="hi"),
102+
ChatMessage(role="assistant", content="part-1"),
103+
ChatMessage(role="assistant", content=f"{COPILOT_ERROR_PREFIX} a"),
104+
ChatMessage(
105+
role="assistant", content=f"{COPILOT_RETRYABLE_ERROR_PREFIX} b"
106+
),
107+
]
108+
)
109+
attempt = _InterruptedAttempt()
110+
attempt.capture(session, _builder_stub(), object(), pre_attempt_msg_count=1)
111+
assert [m.content for m in attempt.partial] == ["part-1"]
112+
113+
def test_preserves_non_marker_assistant(self):
114+
session = _make_session(
115+
[
116+
ChatMessage(role="user", content="hi"),
117+
ChatMessage(role="assistant", content="Important note"),
118+
]
119+
)
120+
attempt = _InterruptedAttempt()
121+
attempt.capture(session, _builder_stub(), object(), pre_attempt_msg_count=1)
122+
assert [m.content for m in attempt.partial] == ["Important note"]
123+
124+
125+
class TestInterruptedAttemptFinalize:
126+
def test_appends_partial_then_marker(self):
127+
session = _make_session([ChatMessage(role="user", content="hi")])
128+
attempt = _InterruptedAttempt(
129+
partial=[
130+
ChatMessage(role="assistant", content="working"),
131+
ChatMessage(role="tool", content="result", tool_call_id="t1"),
132+
]
133+
)
134+
attempt.finalize(session, state=None, display_msg="Boom", retryable=False)
135+
roles = [m.role for m in session.messages]
136+
assert roles == ["user", "assistant", "tool", "assistant"]
137+
assert session.messages[-1].content.startswith(COPILOT_ERROR_PREFIX)
138+
# partial consumed so a follow-up finalize() is a no-op for partial.
139+
assert attempt.partial == []
140+
141+
def test_only_marker_when_partial_empty(self):
142+
session = _make_session([ChatMessage(role="user", content="hi")])
143+
attempt = _InterruptedAttempt()
144+
attempt.finalize(session, state=None, display_msg="Boom", retryable=True)
145+
assert len(session.messages) == 2
146+
assert session.messages[-1].content.startswith(COPILOT_RETRYABLE_ERROR_PREFIX)
147+
148+
def test_noop_when_session_is_none(self):
149+
attempt = _InterruptedAttempt(
150+
partial=[ChatMessage(role="assistant", content="x")]
151+
)
152+
events = attempt.finalize(None, state=None, display_msg="Boom", retryable=False)
153+
assert events == []
154+
155+
def test_flushes_unresolved_tools_between_partial_and_marker(self):
156+
session = _make_session([ChatMessage(role="user", content="hi")])
157+
attempt = _InterruptedAttempt(
158+
partial=[
159+
ChatMessage(
160+
role="assistant",
161+
content="calling",
162+
tool_calls=[
163+
{
164+
"id": "t1",
165+
"type": "function",
166+
"function": {"name": "lookup", "arguments": "{}"},
167+
}
168+
],
169+
),
170+
]
171+
)
172+
flushed = [_tool_output("t1", "interrupted")]
173+
state = _adapter_with_unresolved(flushed)
174+
events = attempt.finalize(
175+
session, state=state, display_msg="Boom", retryable=False
176+
)
177+
roles = [m.role for m in session.messages]
178+
assert roles == ["user", "assistant", "tool", "assistant"]
179+
assert session.messages[2].tool_call_id == "t1"
180+
assert session.messages[2].content == "interrupted"
181+
# The same events that were persisted to history are returned to the
182+
# caller so the caller can yield them to the client — without this
183+
# the frontend's spinner widgets stay open until refresh because the
184+
# adapter's has_unresolved_tool_calls flag is already flipped to False.
185+
assert events == flushed
186+
187+
def test_clear_drops_both_partial_and_handled_error(self):
188+
attempt = _InterruptedAttempt(
189+
partial=[ChatMessage(role="assistant", content="x")],
190+
handled_error=_HandledErrorInfo(
191+
error_msg="m", code="c", retryable=True, already_yielded=False
192+
),
193+
)
194+
attempt.clear()
195+
assert attempt.partial == []
196+
assert attempt.handled_error is None
197+
198+
199+
class TestFlushOrphanToolUses:
200+
def test_appends_synthetic_tool_results_for_unresolved(self):
201+
session = _make_session()
202+
flushed = [_tool_output("t1", "r1"), _tool_output("t2", {"ok": False})]
203+
state = _adapter_with_unresolved(flushed)
204+
events = _flush_orphan_tool_uses_to_session(session, state)
205+
assert [m.tool_call_id for m in session.messages] == ["t1", "t2"]
206+
# Dict outputs are JSON-encoded so structure survives the str-only
207+
# ChatMessage content field for the next-turn LLM read.
208+
assert session.messages[1].content == '{"ok": false}'
209+
assert events == flushed
210+
211+
def test_noop_when_state_is_none(self):
212+
session = _make_session()
213+
events = _flush_orphan_tool_uses_to_session(session, None)
214+
assert session.messages == []
215+
assert events == []
216+
217+
def test_noop_when_no_unresolved(self):
218+
adapter = MagicMock()
219+
adapter.has_unresolved_tool_calls = False
220+
state = MagicMock()
221+
state.adapter = adapter
222+
events = _flush_orphan_tool_uses_to_session(_make_session(), state)
223+
adapter.flush_unresolved_tool_calls.assert_not_called()
224+
assert events == []
225+
226+
227+
class TestClassifyFinalFailure:
228+
"""Ensures the history marker (via finalize) and the SSE StreamError yield
229+
share one source of truth for display message + stream code — any drift
230+
would let the chat bubble and the SSE event show different copy for the
231+
same failure."""
232+
233+
def test_handled_error_wins(self):
234+
interrupted = _InterruptedAttempt(
235+
handled_error=_HandledErrorInfo(
236+
error_msg="circuit tripped",
237+
code="circuit_breaker",
238+
retryable=False,
239+
already_yielded=True,
240+
)
241+
)
242+
result = _classify_final_failure(
243+
interrupted,
244+
attempts_exhausted=False,
245+
transient_exhausted=False,
246+
stream_err=RuntimeError("ignored"),
247+
)
248+
assert result == _FinalFailure(
249+
display_msg="circuit tripped",
250+
code="circuit_breaker",
251+
retryable=False,
252+
)
253+
254+
def test_attempts_exhausted(self):
255+
result = _classify_final_failure(
256+
_InterruptedAttempt(),
257+
attempts_exhausted=True,
258+
transient_exhausted=False,
259+
stream_err=RuntimeError("x"),
260+
)
261+
assert result is not None
262+
assert result.code == "all_attempts_exhausted"
263+
assert result.retryable is False
264+
265+
def test_transient_exhausted(self):
266+
result = _classify_final_failure(
267+
_InterruptedAttempt(),
268+
attempts_exhausted=False,
269+
transient_exhausted=True,
270+
stream_err=RuntimeError("x"),
271+
)
272+
assert result is not None
273+
assert result.code == "transient_api_error"
274+
assert result.retryable is True
275+
276+
def test_stream_err_fallback(self):
277+
result = _classify_final_failure(
278+
_InterruptedAttempt(),
279+
attempts_exhausted=False,
280+
transient_exhausted=False,
281+
stream_err=RuntimeError("some sdk error"),
282+
)
283+
assert result is not None
284+
assert result.code == "sdk_stream_error"
285+
assert result.retryable is False
286+
287+
def test_returns_none_when_no_failure_recorded(self):
288+
assert (
289+
_classify_final_failure(
290+
_InterruptedAttempt(),
291+
attempts_exhausted=False,
292+
transient_exhausted=False,
293+
stream_err=None,
294+
)
295+
is None
296+
)
297+
298+
299+
class TestRetryRollbackContract:
300+
"""End-to-end contract: capture on a rolled-back attempt + finalize yields
301+
the exact content the user saw streaming live, plus the error marker."""
302+
303+
def test_capture_then_finalize_matches_streamed_sequence(self):
304+
session = _make_session([ChatMessage(role="user", content="hi")])
305+
pre = len(session.messages)
306+
# Simulate incremental SDK appends during the attempt.
307+
session.messages.extend(
308+
[
309+
ChatMessage(role="assistant", content="part-1"),
310+
ChatMessage(role="assistant", content="part-2"),
311+
]
312+
)
313+
attempt = _InterruptedAttempt()
314+
attempt.capture(session, _builder_stub(), object(), pre)
315+
# Final-failure path — no retry, no success clear().
316+
attempt.finalize(session, state=None, display_msg="Boom", retryable=False)
317+
assert [m.content for m in session.messages] == [
318+
"hi",
319+
"part-1",
320+
"part-2",
321+
f"{COPILOT_ERROR_PREFIX} Boom",
322+
]

autogpt_platform/backend/backend/copilot/sdk/response_adapter.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ def convert_message(self, sdk_message: Message) -> list[StreamBaseResponse]:
166166
# are still executing concurrently and haven't finished yet.
167167
is_tool_only = all(isinstance(b, ToolUseBlock) for b in sdk_message.content)
168168
if not is_tool_only:
169-
self._flush_unresolved_tool_calls(responses)
169+
self.flush_unresolved_tool_calls(responses)
170170

171171
# After tool results, the SDK sends a new AssistantMessage for the
172172
# next LLM turn. Open a new step if the previous one was closed.
@@ -375,7 +375,7 @@ def convert_message(self, sdk_message: Message) -> list[StreamBaseResponse]:
375375
self.step_open = False
376376

377377
elif isinstance(sdk_message, ResultMessage):
378-
self._flush_unresolved_tool_calls(responses)
378+
self.flush_unresolved_tool_calls(responses)
379379
# Thinking-only final turn guard: when the model's last LLM
380380
# call after a tool result produced only a ``ThinkingBlock``
381381
# (no ``TextBlock``, no ``ToolUseBlock``) the UI has nothing
@@ -703,14 +703,20 @@ def _flush_pending_thinking(self, responses: list[StreamBaseResponse]) -> None:
703703
self._pending_thinking_delta = ""
704704
self._pending_thinking_index = None
705705

706-
def _flush_unresolved_tool_calls(self, responses: list[StreamBaseResponse]) -> None:
706+
def flush_unresolved_tool_calls(self, responses: list[StreamBaseResponse]) -> None:
707707
"""Emit outputs for tool calls that didn't receive a UserMessage result.
708708
709709
SDK built-in tools (WebSearch, Read, etc.) may be executed by the CLI
710710
internally without surfacing a separate ``UserMessage`` with
711711
``ToolResultBlock`` content. The ``PostToolUse`` hook stashes their
712712
output, which we pop and emit here before the next ``AssistantMessage``
713713
starts.
714+
715+
Callers that need to both record synthetic tool_results in history AND
716+
yield the same events to the client should call this exactly once and
717+
share the resulting list — the method mutates ``resolved_tool_calls``,
718+
so a second call returns nothing and ``has_unresolved_tool_calls``
719+
flips to ``False`` after the first invocation.
714720
"""
715721
unresolved = [
716722
(tid, info.get("name", "unknown"))

0 commit comments

Comments
 (0)