Skip to content

Commit 8a3d0e5

Browse files
committed
fix: address review findings from CodeRabbit + Copilot
Six real bugs + four hardening + two trivial fixes. Test coverage added for every behavioral change. Real bugs: 1. Cancelled background jobs were flipped to COMPLETED/FAILED. _buffer_background_run's finally block wrote a terminal status unconditionally; if stop_workflow set CANCELLED in parallel, the finally overwrote it. Added _finalize_job_status which re-reads the row and skips the update when the job is already CANCELLED. 2. Background scheduling failures left orphan buffers. If create_queue or start_job raised after _register_background_run, the buffer lingered with done=False forever. Wrapped scheduling in try/except, call _clear_background_run + finalize FAILED on the failure path. 3. Stop didn't abort the v2 SSE request. flowStore.stopBuilding aborts buildController, but buildFlow called runFlowAGUI without a signal. Threaded the controller's signal through runFlowAGUI and linked it to the agent's AbortController so Stop cancels the in-flight fetch. 4. runFlowAGUI set buildInfo only on RUN_FINISHED/RUN_ERROR. If the observable's complete fired without a terminal event (truncated stream, proxy timeout), buildInfo stayed null and trackFlowBuild read the run as success. Now sets a default failure buildInfo on silent complete and on abort. 5. useRunFlow's run() Promise never settled on abort. abort() and preempting run() left awaiters deadlocked because RxJS doesn't fire error/complete for those paths. Track resolver in a ref, settle on abort + preemption. 6. AGUITranslator emitted TextMessage* events with message_id="". token and add_message dropped events when the upstream payload omitted id, but still emitted START/CONTENT/END for the empty id, producing malformed AG-UI streams. Guard the emission paths. Hardening: 7. state.ts shape validation: STATE_SNAPSHOT with nodes=null or nodes as array, STATE_DELTA with non-array delta — all treated as no-ops instead of crashing the reducer. 8. _BackgroundRun.frames bounded by _MAX_FRAMES_PER_BACKGROUND_RUN (10k). Token-streaming flows used to grow without bound; _MAX_BACKGROUND_RUNS only capped the number of buffers. Oldest frames are evicted; replay tracks a base_index so re-attach with Last-Event-ID past the evicted head starts from the new head. 9. _stream_event_frames inline queue bounded by _EVENT_QUEUE_MAX_SIZE (256). Slow consumer applies backpressure to the build loop instead of letting the queue grow unbounded. 10. applyStateDelta toggles edge animation in step with node status: running -> on, success/error -> off. Mirrors the v1 build path so edges animate during the run, not just clear at the end. Trivial test tightening: 11. Last-Event-ID assertion in test_workflow_agui.py is CRLF-agnostic (split per line + check the id list). 12. Three weak "assert str(job_id) in result[...]" checks in test_workflow.py changed to exact equality.
1 parent 080bf11 commit 8a3d0e5

13 files changed

Lines changed: 624 additions & 45 deletions

File tree

src/backend/base/langflow/api/v2/agui_translator.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,12 @@ def _translate_token(self, data: dict) -> list[BaseEvent]:
117117
token boundary) is dropped: re-opening it would emit a second
118118
``TEXT_MESSAGE_START`` for an id the protocol considers closed.
119119
"""
120-
message_id = str(data.get("id", ""))
120+
message_id = str(data.get("id") or "")
121+
if not message_id:
122+
# Without a stable id the AG-UI lifecycle (START/CONTENT/END) cannot
123+
# be correlated. Dropping the event is preferable to emitting a
124+
# malformed stream with empty message_ids.
125+
return []
121126
if message_id in self._emitted_text_message_ids and self._open_message_id != message_id:
122127
return []
123128
chunk = data.get("chunk", "")
@@ -200,7 +205,10 @@ def _translate_add_message(self, data: dict) -> list[BaseEvent]:
200205
events.extend(self._close_open_message())
201206
else:
202207
text = data.get("text") or ""
203-
if text and message_id not in self._emitted_text_message_ids:
208+
# Skip text-message lifecycle emission without a stable message_id;
209+
# tool-call events above are namespaced by block/content index so
210+
# they can still ride a missing id, but TEXT_MESSAGE_* cannot.
211+
if text and message_id and message_id not in self._emitted_text_message_ids:
204212
self._emitted_text_message_ids.add(message_id)
205213
events.append(TextMessageStartEvent(message_id=message_id, role="assistant"))
206214
events.append(TextMessageContentEvent(message_id=message_id, delta=text))

src/backend/base/langflow/api/v2/workflow.py

Lines changed: 92 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -517,7 +517,10 @@ async def _stream_event_frames(
517517
playground's chat-view. A follow-up retires this once chat-view
518518
consumes the AG-UI ``TEXT_MESSAGE_*`` lifecycle directly.
519519
"""
520-
queue: asyncio.Queue = asyncio.Queue()
520+
# Bounded so a slow consumer can apply backpressure on the build loop
521+
# instead of growing without bound. The build loop awaits ``queue.put``
522+
# which yields control back to the consumer between frames.
523+
queue: asyncio.Queue = asyncio.Queue(maxsize=_EVENT_QUEUE_MAX_SIZE)
521524
event_manager = create_default_event_manager(queue)
522525
input_request = _single_input_value_request(parsed)
523526
flow_data = FlowDataRequest(**parsed.data) if parsed.data else None
@@ -667,17 +670,32 @@ class _BackgroundRun:
667670
already serialized in the protocol the original POST requested via
668671
``stream_protocol``; re-attach replays them as-is. Mixing protocols
669672
across a single run is not supported.
673+
674+
Per-run frame count is bounded by ``_MAX_FRAMES_PER_BACKGROUND_RUN`` so a
675+
long verbose run (token-by-token streams, repeated tool calls) cannot
676+
exhaust process memory while ``_MAX_BACKGROUND_RUNS`` only caps the
677+
number of buffers. When the cap is reached the oldest frames are
678+
evicted; re-attach with ``Last-Event-ID`` past that point will start
679+
from the new buffer head (replay loss is preferred over OOM).
670680
"""
671681

672682
def __init__(self, user_id: str) -> None:
673683
self.user_id = user_id
674684
self.frames: list[bytes] = []
685+
# Index of the first frame still in ``frames`` (monotonic across the
686+
# life of the buffer). Once eviction starts, ``frames[i]`` corresponds
687+
# to logical event id ``base_index + i``.
688+
self.base_index = 0
675689
self.done = False
676690
self._cond = asyncio.Condition()
677691

678692
async def append(self, frame: bytes) -> None:
679693
async with self._cond:
680694
self.frames.append(frame)
695+
overflow = len(self.frames) - _MAX_FRAMES_PER_BACKGROUND_RUN
696+
if overflow > 0:
697+
del self.frames[:overflow]
698+
self.base_index += overflow
681699
self._cond.notify_all()
682700

683701
async def finish(self) -> None:
@@ -686,27 +704,71 @@ async def finish(self) -> None:
686704
self._cond.notify_all()
687705

688706
async def replay(self, start_index: int) -> AsyncIterator[bytes]:
689-
"""Yield buffered frames from ``start_index`` and tail until done."""
707+
"""Yield buffered frames from ``start_index`` and tail until done.
708+
709+
``start_index`` is in logical event-id space (matches what was emitted
710+
on ``id:`` lines). If the caller's ``Last-Event-ID`` points before the
711+
buffer's current head (frames evicted under memory pressure), we
712+
replay from the head and the caller observes a gap.
713+
"""
690714
idx = max(start_index, 0)
691715
while True:
692716
async with self._cond:
693-
while idx >= len(self.frames) and not self.done:
717+
head = self.base_index
718+
tail = head + len(self.frames)
719+
idx = max(idx, head)
720+
while idx >= tail and not self.done:
694721
await self._cond.wait()
695-
snapshot = self.frames[idx:]
722+
head = self.base_index
723+
tail = head + len(self.frames)
724+
idx = max(idx, head)
725+
snapshot = self.frames[idx - head :]
696726
finished = self.done
697727
for frame in snapshot:
698728
yield frame
699729
idx += len(snapshot)
700-
if finished and idx >= len(self.frames):
730+
if finished and idx >= self.base_index + len(self.frames):
701731
return
702732

703733

704734
# Process-local registry of background runs keyed by job_id, bounded by
705735
# ``_MAX_BACKGROUND_RUNS`` (oldest evicted first). Re-attach reads this.
706736
_MAX_BACKGROUND_RUNS = 100
737+
# Per-run frame ceiling. Caps memory for a single long/verbose run so a
738+
# token-streaming flow can't exhaust the process. 10k frames covers minutes
739+
# of dense token streams with room to spare; beyond that we evict oldest.
740+
_MAX_FRAMES_PER_BACKGROUND_RUN = 10_000
741+
# Inline stream queue between the build loop and the SSE consumer. Bounded
742+
# so a slow consumer applies backpressure to the build loop instead of
743+
# letting frames accumulate without bound when the network is slow.
744+
_EVENT_QUEUE_MAX_SIZE = 256
707745
_BACKGROUND_RUNS: dict[str, _BackgroundRun] = {}
708746

709747

748+
async def _finalize_job_status(job_uuid: UUID, terminal_status: JobStatus) -> None:
749+
"""Update job status to a terminal value, but never overwrite CANCELLED.
750+
751+
``stop_workflow`` sets the job to CANCELLED. The buffer task runs in
752+
parallel and reaches its ``finally`` block shortly after; if it
753+
unconditionally wrote COMPLETED/FAILED it would race with the cancellation
754+
and silently overwrite the user's stop intent. Re-read the row first and
755+
skip the update if a cancellation already landed.
756+
"""
757+
job_service = get_job_service()
758+
try:
759+
job = await job_service.get_job_by_job_id(job_id=job_uuid)
760+
except Exception: # noqa: BLE001
761+
job = None
762+
if job is not None and job.status == JobStatus.CANCELLED:
763+
return
764+
with contextlib.suppress(Exception):
765+
await job_service.update_job_status(
766+
job_uuid,
767+
terminal_status,
768+
finished_timestamp=True,
769+
)
770+
771+
710772
async def _clear_background_run(job_id: str) -> None:
711773
"""Pop the background run registry entry and wake any re-attach waiters.
712774
@@ -780,12 +842,7 @@ async def _buffer_background_run(
780842
# Fire-and-forget coroutine: do not raise, the route already returned.
781843
await bg_run.finish()
782844
job_uuid = UUID(job_id) if isinstance(job_id, str) else job_id
783-
with contextlib.suppress(Exception):
784-
await get_job_service().update_job_status(
785-
job_uuid,
786-
JobStatus.FAILED,
787-
finished_timestamp=True,
788-
)
845+
await _finalize_job_status(job_uuid, JobStatus.FAILED)
789846
return
790847

791848
terminal_error_type = adapter.terminal_error_type
@@ -816,12 +873,7 @@ async def _buffer_background_run(
816873
# ``update_job_status`` queries the Job table by its UUID primary key;
817874
# passing the raw string would silently miss every row.
818875
job_uuid = UUID(job_id) if isinstance(job_id, str) else job_id
819-
with contextlib.suppress(Exception):
820-
await get_job_service().update_job_status(
821-
job_uuid,
822-
JobStatus.FAILED if errored else JobStatus.COMPLETED,
823-
finished_timestamp=True,
824-
)
876+
await _finalize_job_status(job_uuid, JobStatus.FAILED if errored else JobStatus.COMPLETED)
825877
# Fire memory-base auto-capture hook on successful runs only. Matches
826878
# the sync mode wiring above and the v1 build-pipeline wiring in
827879
# ``api/build.py``. ``fire_and_forget_task`` because we are already a
@@ -868,19 +920,29 @@ async def execute_workflow_background(
868920
bg_run = _BackgroundRun(user_id=str(current_user.id))
869921
_register_background_run(job_id_str, bg_run)
870922

871-
queue_service = get_queue_service()
872-
queue_service.create_queue(job_id_str)
873-
queue_service.start_job(
874-
job_id_str,
875-
_buffer_background_run(
876-
bg_run=bg_run,
877-
flow=flow,
878-
parsed=parsed,
879-
job_id=job_id_str,
880-
current_user=current_user,
881-
stream_protocol=stream_protocol,
882-
),
883-
)
923+
try:
924+
queue_service = get_queue_service()
925+
queue_service.create_queue(job_id_str)
926+
queue_service.start_job(
927+
job_id_str,
928+
_buffer_background_run(
929+
bg_run=bg_run,
930+
flow=flow,
931+
parsed=parsed,
932+
job_id=job_id_str,
933+
current_user=current_user,
934+
stream_protocol=stream_protocol,
935+
),
936+
)
937+
except BaseException:
938+
# If queue creation or scheduling fails after the bg_run is
939+
# registered, the buffer would stay live with ``done=False`` and
940+
# any re-attach client would block on ``_cond.wait()`` forever
941+
# (the task that would call ``finish()`` was never scheduled).
942+
# Clear the registry, mark the job FAILED, then re-raise.
943+
await _clear_background_run(job_id_str)
944+
await _finalize_job_status(job_id, JobStatus.FAILED)
945+
raise
884946
return WorkflowJobResponse(job_id=job_id_str, flow_id=parsed.flow_id, status=JobStatus.QUEUED)
885947

886948
except (WorkflowResourceError, WorkflowServiceUnavailableError, WorkflowQueueFullError):

src/backend/tests/unit/api/v2/test_agui_translator.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,41 @@ def test_end_vertex_does_not_close_open_text_message():
290290
assert more[0].message_id == "m1"
291291

292292

293+
def test_token_without_message_id_is_dropped():
294+
"""Token events with missing ``id`` cannot drive the AG-UI lifecycle.
295+
296+
A TextMessageStart/Content/End triple requires a stable id to correlate
297+
chunks. Emitting events with ``message_id=""`` produces malformed
298+
streams that some AG-UI clients reject; drop the event instead.
299+
"""
300+
t = AGUITranslator(run_id="r1", thread_id="t1")
301+
t.start()
302+
303+
no_id = t.translate("token", {"chunk": "hello"})
304+
none_id = t.translate("token", {"chunk": "hello", "id": None})
305+
empty_id = t.translate("token", {"chunk": "hello", "id": ""})
306+
307+
assert no_id == []
308+
assert none_id == []
309+
assert empty_id == []
310+
311+
312+
def test_add_message_without_message_id_skips_text_lifecycle():
313+
"""``add_message`` without an id must not emit TextMessage* events.
314+
315+
Tool-call sub-events can still ride a missing id (they namespace by
316+
block/content index), but the text lifecycle needs a stable id.
317+
"""
318+
t = AGUITranslator(run_id="r1", thread_id="t1")
319+
t.start()
320+
321+
out = t.translate("add_message", {"text": "hello"})
322+
323+
assert all(not isinstance(e, TextMessageStartEvent) for e in out)
324+
assert all(not isinstance(e, TextMessageContentEvent) for e in out)
325+
assert all(not isinstance(e, TextMessageEndEvent) for e in out)
326+
327+
293328
def test_add_message_plain_text_emits_a_text_message():
294329
t = AGUITranslator(run_id="r1", thread_id="t1")
295330
t.start()

src/backend/tests/unit/api/v2/test_workflow.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ async def test_get_workflow_status_forbidden_for_other_user_job(
391391
assert response.status_code == 404
392392
result = response.json()
393393
assert result["detail"]["code"] == "JOB_NOT_FOUND"
394-
assert str(job_id) in result["detail"]["job_id"]
394+
assert result["detail"]["job_id"] == str(job_id)
395395
finally:
396396
async with session_scope() as session:
397397
db_job = await session.get(Job, job_id)
@@ -425,7 +425,7 @@ async def test_get_workflow_status_allowed_for_own_job(
425425

426426
assert response.status_code == 200
427427
result = response.json()
428-
assert str(job_id) in result["job_id"]
428+
assert result["job_id"] == str(job_id)
429429
finally:
430430
async with session_scope() as session:
431431
db_job = await session.get(Job, job_id)
@@ -469,7 +469,7 @@ async def test_stop_workflow_forbidden_for_other_user_job(
469469
assert response.status_code == 404
470470
result = response.json()
471471
assert result["detail"]["code"] == "JOB_NOT_FOUND"
472-
assert str(job_id) in result["detail"]["job_id"]
472+
assert result["detail"]["job_id"] == str(job_id)
473473
finally:
474474
async with session_scope() as session:
475475
db_job = await session.get(Job, job_id)

src/backend/tests/unit/api/v2/test_workflow_agui.py

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,10 @@ async def test_reattach_replays_after_last_event_id(
430430
assert response.status_code == 200
431431
body = response.text
432432
# Event 0 is RUN_STARTED; with Last-Event-ID=0 it must not be replayed.
433-
assert "id: 0\n" not in body
433+
# Split per line so CRLF endings don't sneak the event through the
434+
# substring check.
435+
event_ids = [line.removeprefix("id:").strip() for line in body.splitlines() if line.startswith("id:")]
436+
assert "0" not in event_ids
434437
assert "RUN_FINISHED" in body
435438

436439
async def test_reattach_unknown_job_returns_404(
@@ -892,6 +895,70 @@ async def on_flow_output(self, **kwargs):
892895
assert call["job_id"] == _UUID(job_id)
893896

894897

898+
class TestBackgroundFinalizationGuards:
899+
"""Cancellation state + cleanup guarantees for the background path.
900+
901+
``_buffer_background_run`` and ``execute_workflow_background`` must
902+
preserve cancellation state and clean up after scheduling failures.
903+
"""
904+
905+
async def test_finalize_does_not_overwrite_cancelled_status(
906+
self,
907+
client: AsyncClient,
908+
created_api_key,
909+
chatbot_flow,
910+
):
911+
"""A run cancelled mid-flight must stay CANCELLED after the buffer ends.
912+
913+
Race: ``stop_workflow`` sets the job to CANCELLED. The buffer task's
914+
``finally`` block runs shortly after and previously wrote
915+
COMPLETED/FAILED unconditionally, silently overwriting the user's
916+
stop intent. Guarded by ``_finalize_job_status``.
917+
"""
918+
import asyncio as _asyncio
919+
from uuid import UUID as _UUID
920+
921+
from langflow.services.database.models.jobs.model import Job as _Job
922+
from langflow.services.database.models.jobs.model import JobStatus as _JobStatus
923+
924+
headers = {"x-api-key": created_api_key.api_key}
925+
start = await client.post(
926+
"api/v2/workflows",
927+
json=_agui_body(chatbot_flow, message="hi", mode="background"),
928+
headers=headers,
929+
)
930+
assert start.status_code == 200
931+
job_id = start.json()["job_id"]
932+
job_uuid = _UUID(job_id)
933+
934+
# Stop the run before it gets a chance to complete on its own. The
935+
# /stop endpoint flips the row to CANCELLED.
936+
stop = await client.post(
937+
"api/v2/workflows/stop",
938+
json={"job_id": job_id},
939+
headers=headers,
940+
)
941+
assert stop.status_code == 200
942+
943+
# Give the buffer task time to reach its finally block and call
944+
# _finalize_job_status. Poll the row for stability.
945+
for _ in range(60):
946+
async with session_scope() as session:
947+
row = await session.get(_Job, job_uuid)
948+
if row is not None and row.status in (_JobStatus.COMPLETED, _JobStatus.FAILED):
949+
break
950+
await _asyncio.sleep(0.1)
951+
952+
async with session_scope() as session:
953+
row = await session.get(_Job, job_uuid)
954+
assert row is not None
955+
assert row.status == _JobStatus.CANCELLED, (
956+
f"Buffer task overwrote the user's cancellation: got {row.status} "
957+
f"(expected CANCELLED). The finally block in _buffer_background_run "
958+
f"is racing with stop_workflow."
959+
)
960+
961+
895962
class TestBackgroundModeStreamProtocol:
896963
"""Background mode must honor ``stream_protocol`` end-to-end.
897964

0 commit comments

Comments
 (0)