mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-17 02:13:37 +00:00
fix: little more tool call hardening
This commit is contained in:
1 parent
4a4f485781
commit
114585cd7d
14 files changed
+961
-22
No files matched your search
@@ -68,9 +68,22 @@ class ToolActionParser:
|
||||
# along with it. Only genuinely malformed arguments fail here.
|
||||
call_args = self._decode_arguments(call.arguments)
|
||||
if call_args is _MALFORMED_ARGUMENTS:
|
||||
# Quote a bounded prefix of the payload: the shape is the only
|
||||
# thing that tells "we concatenated two complete payloads"
|
||||
# (``{}{}`` — the per-index merge in llm/handlers/base.py
|
||||
# appends, so a provider that restates complete arguments on a
|
||||
# second frame produces invalid JSON) apart from "the model
|
||||
# emitted garbage". Both look identical without it, which cost
|
||||
# a day of triage on 2026-08-19. Truncated because tool
|
||||
# arguments carry user content.
|
||||
raw = call.arguments
|
||||
preview = raw[:120] if isinstance(raw, str) else repr(raw)[:120]
|
||||
logger.error(
|
||||
"Error parsing OpenAI LLM call: arguments are not decodable JSON (%s)",
|
||||
"Error parsing OpenAI LLM call: arguments are not decodable "
|
||||
"JSON (%s); length=%s prefix=%r",
|
||||
getattr(call, "name", "<unknown>"),
|
||||
len(raw) if isinstance(raw, (str, bytes)) else "n/a",
|
||||
preview,
|
||||
)
|
||||
return None, None, None
|
||||
|
||||
|
||||
@@ -8,6 +8,9 @@ from application.api import api
|
||||
|
||||
from application.api.answer.routes.base import answer_ns, BaseAnswerResource
|
||||
|
||||
from application.api.answer.services.continuation_service import (
|
||||
ResumeInProgressError,
|
||||
)
|
||||
from application.api.answer.services.persistence_policy import resolve_persistence
|
||||
from application.api.answer.services.stream_processor import StreamProcessor
|
||||
from application.streaming.sse_keepalive import with_sse_keepalive
|
||||
@@ -175,6 +178,24 @@ class StreamResource(Resource, BaseAnswerResource):
|
||||
),
|
||||
mimetype="text/event-stream",
|
||||
)
|
||||
except ResumeInProgressError as e:
|
||||
# Another request already owns this conversation's continuation
|
||||
# claim — an expected concurrency outcome, not a bad request. It
|
||||
# must be caught ahead of ``ValueError`` (its base class), or it
|
||||
# lands in the handler below and is reported to the user as a
|
||||
# malformed body while filling the ERROR channel with tracebacks.
|
||||
# ``/v1/chat/completions`` has answered 409 for this since
|
||||
# 2026-07-13; this is the same contract on the primary route.
|
||||
logger.warning(
|
||||
"/stream - resume already in progress for conversation %s",
|
||||
data.get("conversation_id"),
|
||||
extra={"error": str(e)},
|
||||
)
|
||||
return Response(
|
||||
self.error_stream_generate(str(e)),
|
||||
status=409,
|
||||
mimetype="text/event-stream",
|
||||
)
|
||||
except ValueError as e:
|
||||
message = "Malformed request body"
|
||||
logger.error(
|
||||
|
||||
@@ -60,6 +60,28 @@ def run_reconciliation() -> Dict[str, Any]:
|
||||
# ``(user_id, event_type, payload, scope)``.
|
||||
events: list[tuple] = []
|
||||
|
||||
# Each sweep below commits in its own transaction, but the queued
|
||||
# user-facing events are fanned out once, at the end. Without the
|
||||
# ``finally``, a transient DB error in a later sweep discards events whose
|
||||
# writes already landed — and the loss is permanent, because every sweep
|
||||
# filters out the rows it just made terminal (``find_and_lock_stalled_
|
||||
# ingests`` looks only at ``status='active'``, ``find_and_lock_stuck_
|
||||
# messages`` only at pending/streaming), so no later tick re-finds them to
|
||||
# re-emit. The user is left with an upload toast spinning on "Training..."
|
||||
# forever. Seen live on 2026-08-21 when an Azure DNS blip took Postgres
|
||||
# unreachable mid-tick.
|
||||
try:
|
||||
return _run_sweeps(engine, summary, events)
|
||||
finally:
|
||||
_publish_events(events)
|
||||
|
||||
|
||||
def _run_sweeps(engine, summary: Dict[str, Any], events: list[tuple]) -> Dict[str, Any]:
|
||||
"""Run every reconciler sweep, appending user-facing events to ``events``.
|
||||
|
||||
Split out of :func:`run_reconciliation` so the caller can guarantee the
|
||||
queued events are published even when a sweep raises partway through.
|
||||
"""
|
||||
with engine.begin() as conn:
|
||||
repo = ReconciliationRepository(conn)
|
||||
pt_repo = PendingToolStateRepository(conn)
|
||||
@@ -121,8 +143,11 @@ def run_reconciliation() -> Dict[str, Any]:
|
||||
repo.mark_tool_call_failed(
|
||||
row["call_id"],
|
||||
error=(
|
||||
"reconciler: stuck in 'proposed' for >5 min; "
|
||||
"side effect status unknown"
|
||||
"reconciler: never advanced past 'proposed' for >5 min. "
|
||||
"The row is written immediately before the tool runs, so "
|
||||
"either the tool is still executing or the stream was "
|
||||
"abandoned before it reported back; in the abandoned case "
|
||||
"no side effect was committed."
|
||||
),
|
||||
)
|
||||
summary["tool_calls_failed"] += 1
|
||||
@@ -142,8 +167,12 @@ def run_reconciliation() -> Dict[str, Any]:
|
||||
repo.mark_tool_call_failed(
|
||||
row["call_id"],
|
||||
error=(
|
||||
"reconciler: executed-not-confirmed; side effect "
|
||||
"assumed committed, manual cleanup required"
|
||||
"reconciler: executed but not confirmed within 15 min. "
|
||||
"The tool SUCCEEDED and its side effect is committed; "
|
||||
"confirmation only lands when the whole turn finalizes, "
|
||||
"so a long-running turn reaches this state while healthy. "
|
||||
"Check the parent message before assuming cleanup is "
|
||||
"needed."
|
||||
),
|
||||
)
|
||||
summary["tool_calls_failed"] += 1
|
||||
@@ -328,7 +357,6 @@ def run_reconciliation() -> Dict[str, Any]:
|
||||
},
|
||||
)
|
||||
|
||||
_publish_events(events)
|
||||
return summary
|
||||
|
||||
|
||||
|
||||
@@ -28,12 +28,24 @@ logger = logging.getLogger(__name__)
|
||||
# durability story is grep-able next to the body. Combined with
|
||||
# ``autoretry_for=(Exception,)`` and a bounded ``max_retries`` so a poison
|
||||
# message can't loop forever.
|
||||
# ``retry_backoff`` is the factor, NOT a boolean toggle: celery's
|
||||
# ``add_autoretry_behaviour`` OVERWRITES ``retry_kwargs["countdown"]`` whenever
|
||||
# it is truthy (celery/app/autoretry.py). With the old ``retry_backoff=True``
|
||||
# the factor was ``int(max(1.0, True)) == 1``, so the declared 60 s countdown
|
||||
# never reached ``task.retry`` and the three waits were jittered 0-1 s, 0-2 s
|
||||
# and 0-4 s — a whole retry envelope of at most 7 seconds. A 3.5-minute network
|
||||
# blip (2026-08-21) therefore exhausted every attempt and published a terminal
|
||||
# ``source.ingest.failed`` / ``attachment.failed`` to the user.
|
||||
# Factor 30 gives 30 s / 60 s / 120 s (full-jittered, capped by celery's
|
||||
# ``retry_backoff_max`` default of 600 s), which actually spans an outage of
|
||||
# that length. ``countdown`` is deliberately absent: leaving it in only
|
||||
# documents a value the library discards.
|
||||
DURABLE_TASK = dict(
|
||||
bind=True,
|
||||
acks_late=True,
|
||||
autoretry_for=(Exception,),
|
||||
retry_kwargs={"max_retries": 3, "countdown": 60},
|
||||
retry_backoff=True,
|
||||
retry_kwargs={"max_retries": 3},
|
||||
retry_backoff=30,
|
||||
)
|
||||
|
||||
|
||||
@@ -681,10 +693,24 @@ def cleanup_idempotency_dedup(self):
|
||||
|
||||
@celery.task(bind=True, acks_late=False)
|
||||
def reconciliation_task(self):
|
||||
"""Sweep stuck durability rows and escalate them to terminal status + alert."""
|
||||
"""Sweep stuck durability rows and escalate them to terminal status + alert.
|
||||
|
||||
Never retries: the task is on a 30 s beat, so the next tick IS the retry,
|
||||
and ``acks_late=False`` keeps a failing run from being redelivered. It only
|
||||
guards itself so a transient connectivity error emits one WARNING instead
|
||||
of a full traceback on the same ERROR channel ``_emit_alert`` uses for real
|
||||
reconciler findings — at 30 s cadence a multi-minute outage would otherwise
|
||||
bury genuine alerts under dozens of identical stack traces.
|
||||
"""
|
||||
from application.api.user.reconciliation import run_reconciliation
|
||||
|
||||
return run_reconciliation()
|
||||
try:
|
||||
return run_reconciliation()
|
||||
except Exception as exc: # noqa: BLE001 - housekeeping must never crash the beat loop
|
||||
logging.getLogger(__name__).warning(
|
||||
"reconciliation_task failed; the next beat retries: %s", exc,
|
||||
)
|
||||
return {"messages_failed": 0, "tool_calls_failed": 0, "error": True}
|
||||
|
||||
|
||||
@celery.task(bind=True, acks_late=False)
|
||||
|
||||
@@ -1165,9 +1165,18 @@ class ConversationsRepository:
|
||||
|
||||
Deleting is still correct — the user explicitly asked to replace these
|
||||
turns — so the rows go, but any non-terminal one is first stamped
|
||||
terminal. That makes the owning stream's late ``finalize_message``
|
||||
report ``ALREADY_FAILED`` (a known, quiet outcome) rather than
|
||||
``NOT_FOUND``, and leaves an audit trail of what was superseded.
|
||||
terminal, which leaves an audit trail of what was superseded.
|
||||
|
||||
That stamp does NOT survive: the UPDATE and the DELETE below run the
|
||||
same ``position > :pos`` predicate in one transaction, so every stamped
|
||||
row is removed microseconds later and the owning stream's late
|
||||
``finalize_message`` correctly reports ``NOT_FOUND``, not
|
||||
``ALREADY_FAILED``. Do not rely on the stamp for anything but the
|
||||
``RETURNING id`` log line. A stream superseded less than
|
||||
``STREAM_HEARTBEAT_INTERVAL`` before it finishes is never cancelled
|
||||
(the ticker in ``api/answer/routes/base.py`` is the only thing that
|
||||
sets the cancel flag), so it runs to completion and raises
|
||||
``answer_persist_failed`` with an answer nothing can save.
|
||||
|
||||
Args:
|
||||
conversation_id: Conversation whose tail is being trimmed.
|
||||
|
||||
Reference in new issue
Block a user