Skip to content

Commit 87a6ad0

Browse files
eberki-scaleclaude
andcommitted
fix(streaming): mark _is_closing in close() and roll back on terminal failure
Greptile follow-ups on the _is_closing guard: - close() sets _is_closing before reaping the buffer, so a delta racing a direct close() (e.g. via __aexit__) can't slip past the None buffer and publish after DONE. - A failed terminal write (e.g. messages.update raising) rolls _is_closing back to False, so the still-open context doesn't silently drop later deltas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b462ec0 commit 87a6ad0

2 files changed

Lines changed: 108 additions & 55 deletions

File tree

src/agentex/lib/core/services/adk/streaming.py

Lines changed: 71 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -439,41 +439,51 @@ async def close(self) -> TaskMessage:
439439
if not self.task_message:
440440
raise ValueError("Context not properly initialized - no task message")
441441

442-
# Reap the buffer (stopping its ticker) before the _is_closed
443-
# short-circuit, so a context already marked done by a Full update can't
444-
# leave the ticker orphaned. Draining here also lets consumers see the
445-
# full delta sequence in order before DONE.
446-
await self._reap_buffer()
442+
# Mark closing before reaping so a concurrent coalesced delta can't slip
443+
# past the now-None buffer and publish after DONE. Rolled back below if
444+
# the terminal write fails, so a failed close doesn't wedge the context
445+
# into silently dropping later deltas.
446+
self._is_closing = True
447+
try:
448+
# Reap the buffer (stopping its ticker) before the _is_closed
449+
# short-circuit, so a context already marked done by a Full update
450+
# can't leave the ticker orphaned. Draining here also lets consumers
451+
# see the full delta sequence in order before DONE.
452+
await self._reap_buffer()
447453

448-
if self._is_closed:
449-
return self.task_message # Already done (buffer reaped above)
454+
if self._is_closed:
455+
return self.task_message # Already done (buffer reaped above)
450456

451-
# Send the DONE event
452-
done_event = StreamTaskMessageDone(
453-
parent_task_message=self.task_message,
454-
type="done",
455-
)
456-
await self._streaming_service.stream_update(done_event)
457+
# Send the DONE event
458+
done_event = StreamTaskMessageDone(
459+
parent_task_message=self.task_message,
460+
type="done",
461+
)
462+
await self._streaming_service.stream_update(done_event)
457463

458-
# Update the task message with the final content
459-
has_deltas = (
460-
self._delta_accumulator._accumulated_deltas
461-
or self._delta_accumulator._reasoning_summaries
462-
or self._delta_accumulator._reasoning_contents
463-
)
464-
if has_deltas:
465-
self.task_message.content = self._delta_accumulator.convert_to_content()
464+
# Update the task message with the final content
465+
has_deltas = (
466+
self._delta_accumulator._accumulated_deltas
467+
or self._delta_accumulator._reasoning_summaries
468+
or self._delta_accumulator._reasoning_contents
469+
)
470+
if has_deltas:
471+
self.task_message.content = self._delta_accumulator.convert_to_content()
466472

467-
await self._agentex_client.messages.update(
468-
task_id=self.task_id,
469-
message_id=self.task_message.id,
470-
content=self.task_message.content.model_dump(),
471-
streaming_status="DONE",
472-
)
473+
await self._agentex_client.messages.update(
474+
task_id=self.task_id,
475+
message_id=self.task_message.id,
476+
content=self.task_message.content.model_dump(),
477+
streaming_status="DONE",
478+
)
473479

474-
# Mark the context as done
475-
self._is_closed = True
476-
return self.task_message
480+
# Mark the context as done
481+
self._is_closed = True
482+
return self.task_message
483+
except Exception:
484+
if not self._is_closed:
485+
self._is_closing = False
486+
raise
477487

478488
async def stream_update(self, update: TaskMessageUpdate) -> TaskMessageUpdate | None:
479489
"""Stream an update to the repository.
@@ -512,32 +522,38 @@ async def stream_update(self, update: TaskMessageUpdate) -> TaskMessageUpdate |
512522

513523
# From here the update is terminal. Mark closing so a concurrent delta
514524
# can't fall through and publish after the terminal once the buffer is
515-
# reaped below.
516-
if isinstance(update, (StreamTaskMessageFull, StreamTaskMessageDone)):
525+
# reaped below. Rolled back if the terminal path fails, so a failed
526+
# terminal doesn't wedge the context into silently dropping later deltas.
527+
is_terminal = isinstance(update, (StreamTaskMessageFull, StreamTaskMessageDone))
528+
if is_terminal:
517529
self._is_closing = True
518-
519-
# A Full ends the stream and supersedes buffered deltas. Drain and stop
520-
# the buffer BEFORE publishing the Full, so leftover deltas land in order
521-
# (deltas -> Full) instead of trailing the terminal Full as a stale
522-
# duplicate tail. This also stops the ticker, which would otherwise be
523-
# orphaned when __aexit__'s close() short-circuits on _is_closed.
524-
if isinstance(update, StreamTaskMessageFull):
525-
await self._reap_buffer()
526-
527-
result = await self._streaming_service.stream_update(update)
528-
529-
if isinstance(update, StreamTaskMessageDone):
530-
await self.close()
531-
return update
532-
elif isinstance(update, StreamTaskMessageFull):
533-
await self._agentex_client.messages.update(
534-
task_id=self.task_id,
535-
message_id=update.parent_task_message.id, # type: ignore[union-attr]
536-
content=update.content.model_dump(),
537-
streaming_status="DONE",
538-
)
539-
self._is_closed = True
540-
return result
530+
try:
531+
# A Full ends the stream and supersedes buffered deltas. Drain and
532+
# stop the buffer BEFORE publishing the Full, so leftover deltas land
533+
# in order (deltas -> Full) instead of trailing the terminal Full as
534+
# a stale duplicate tail. This also stops the ticker, which would
535+
# otherwise be orphaned when __aexit__'s close() short-circuits.
536+
if isinstance(update, StreamTaskMessageFull):
537+
await self._reap_buffer()
538+
539+
result = await self._streaming_service.stream_update(update)
540+
541+
if isinstance(update, StreamTaskMessageDone):
542+
await self.close()
543+
return update
544+
elif isinstance(update, StreamTaskMessageFull):
545+
await self._agentex_client.messages.update(
546+
task_id=self.task_id,
547+
message_id=update.parent_task_message.id, # type: ignore[union-attr]
548+
content=update.content.model_dump(),
549+
streaming_status="DONE",
550+
)
551+
self._is_closed = True
552+
return result
553+
except Exception:
554+
if not self._is_closed:
555+
self._is_closing = False
556+
raise
541557

542558

543559
class StreamingService:

tests/lib/core/services/adk/test_streaming.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,3 +621,40 @@ async def test_late_delta_during_close_window_is_not_published_after_full(self)
621621
await ctx.stream_update(_text(tm, "late"))
622622

623623
assert svc.stream_update.await_count == 0, "late delta published after the terminal Full"
624+
625+
@pytest.mark.asyncio
626+
async def test_close_marks_closing_before_reaping_buffer(self) -> None:
627+
"""close() must set _is_closing before reaping, so a concurrent delta in
628+
the post-reap / pre-_is_closed window is dropped, not published after DONE."""
629+
ctx, _svc, tm = await _make_context("coalesced")
630+
await ctx.stream_update(_text(tm, "hi"))
631+
632+
closing_at_reap = {}
633+
orig_reap = ctx._reap_buffer
634+
635+
async def spy() -> None:
636+
closing_at_reap["value"] = ctx._is_closing
637+
await orig_reap()
638+
639+
ctx._reap_buffer = spy # type: ignore[method-assign]
640+
await ctx.close()
641+
assert closing_at_reap.get("value") is True, "close() must mark _is_closing before reaping"
642+
643+
@pytest.mark.asyncio
644+
async def test_failed_terminal_rolls_back_is_closing(self) -> None:
645+
"""If the terminal persistence fails, _is_closing must roll back so the
646+
still-open context doesn't silently drop later coalesced deltas."""
647+
ctx, _svc, tm = await _make_context("coalesced")
648+
await ctx.stream_update(_text(tm, "hi"))
649+
ctx._agentex_client.messages.update.side_effect = RuntimeError("persist boom") # type: ignore[attr-defined]
650+
651+
full = StreamTaskMessageFull(
652+
parent_task_message=tm,
653+
content=TextContent(author="agent", content="hi", format="markdown"),
654+
type="full",
655+
)
656+
with pytest.raises(RuntimeError):
657+
await ctx.stream_update(full)
658+
659+
assert ctx._is_closed is False
660+
assert ctx._is_closing is False, "failed terminal left _is_closing stuck on"

0 commit comments

Comments
 (0)