mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
The Codex provider never read the configured LLM timeout. The factory did not pass one, and CodexLLM extends LLMInterface (not the LLMProvider base that assigns self.timeout), so the class had no timeout attribute at all -- the three call sites hardcoded httpx timeout=120.0. That literal is a per-socket-read timeout, and the body was fetched with a buffering client.post(), so a backend wedged into runaway generation reset it forever: one consolidation call was read for ~830 s (~12 MB of SSE deltas for a ~340-character answer) until the backend closed the connection, holding the reserved consolidation slot for the whole time. Three such stalls cost ~1.7 h on one bank. - LLMInterface now takes and stores `timeout`, so no provider can silently drop it, and the factory threads the resolved value to the five that were missing it: codex, gemini, anthropic, fireworks and llamacpp. - Codex reads the SSE body with `client.stream()` inside an `asyncio.timeout` that covers the request *and* the parse, so the configured timeout is a total deadline rather than an idle one, plus a body-size ceiling that abandons a fast runaway stream in seconds instead of buffering it. Both surface as CodexRunawayStreamError, an httpx.RequestError, so the existing retry/backoff path handles them unchanged. - Gemini's hardcoded 90 s and Anthropic's own 300 s default become the unconfigured fallbacks rather than the only values. Codex tests move onto a shared streaming stub since the provider no longer calls client.post(). The consolidation wall-clock ceiling the issue also asks for already landed in #3746 (unreleased); it is an idle ceiling defaulting to 7200 s, so it would not have ended an 830 s stall on its own. Claude-Session: https://claude.ai/code/session_018HDqrzHgqZqsGc7EDqoTEu
This commit is contained in:
@@ -73,6 +73,7 @@ class LLMInterface(ABC):
|
||||
base_url: str,
|
||||
model: str,
|
||||
reasoning_effort: str | None = None,
|
||||
timeout: float | None = None,
|
||||
**kwargs: Any,
|
||||
):
|
||||
"""
|
||||
@@ -86,6 +87,10 @@ class LLMInterface(ABC):
|
||||
reasoning_effort: Reasoning effort level, or None when the operator
|
||||
configured none — in which case no provider sends the parameter and
|
||||
every model runs at its own default effort.
|
||||
timeout: Per-request timeout in seconds, already resolved by the caller
|
||||
from the per-operation/global config (``consolidation_llm_timeout``
|
||||
falling back to ``llm_timeout``, etc.). ``None`` means unconfigured
|
||||
and lets each provider apply its own default.
|
||||
**kwargs: Additional provider-specific parameters.
|
||||
"""
|
||||
self.provider = provider.lower()
|
||||
@@ -99,6 +104,11 @@ class LLMInterface(ABC):
|
||||
# issue #3449) and presumptuous (an unconfigured one was still transmitted).
|
||||
# An empty string is an unset environment variable, not a level.
|
||||
self.reasoning_effort: str | None = reasoning_effort or None
|
||||
# Every provider gets the resolved per-request timeout, even the ones that
|
||||
# do not use it: a provider that silently drops it (issue #3898 — Codex
|
||||
# never had the attribute at all, so a runaway response was read until the
|
||||
# backend gave up) is indistinguishable from one that honours it.
|
||||
self.timeout: float | None = timeout
|
||||
|
||||
def _warn_reasoning_effort_unsupported(self) -> None:
|
||||
"""Report, once at startup, that this provider cannot honour a configured effort.
|
||||
|
||||
@@ -525,6 +525,7 @@ def create_llm_provider(
|
||||
model=model,
|
||||
reasoning_effort=reasoning_effort,
|
||||
extra_body=extra_body,
|
||||
timeout=timeout,
|
||||
)
|
||||
|
||||
elif provider_lower == "claude-code":
|
||||
@@ -571,6 +572,7 @@ def create_llm_provider(
|
||||
base_url=base_url,
|
||||
model=model,
|
||||
reasoning_effort=reasoning_effort,
|
||||
timeout=timeout,
|
||||
vertexai_project_id=vertexai_project_id,
|
||||
vertexai_region=vertexai_region,
|
||||
vertexai_credentials=vertexai_credentials,
|
||||
@@ -589,6 +591,7 @@ def create_llm_provider(
|
||||
reasoning_effort=reasoning_effort,
|
||||
default_headers=default_headers,
|
||||
extra_body=extra_body,
|
||||
timeout=timeout,
|
||||
)
|
||||
|
||||
elif provider_lower == "litellm":
|
||||
@@ -652,6 +655,7 @@ def create_llm_provider(
|
||||
model=model,
|
||||
reasoning_effort=reasoning_effort,
|
||||
extra_body=extra_body,
|
||||
timeout=timeout,
|
||||
model_path=config.llamacpp_model_path,
|
||||
gpu_layers=config.llamacpp_gpu_layers,
|
||||
context_size=config.llamacpp_context_size,
|
||||
@@ -673,6 +677,7 @@ def create_llm_provider(
|
||||
extra_body=extra_body,
|
||||
default_headers=default_headers,
|
||||
cache_affinity=cache_affinity,
|
||||
timeout=timeout,
|
||||
)
|
||||
|
||||
elif provider_lower == "nous":
|
||||
|
||||
@@ -74,6 +74,11 @@ def _mark_last_message_for_caching(messages: list[dict[str, Any]]) -> None:
|
||||
content[-1]["cache_control"] = _EPHEMERAL_CACHE
|
||||
|
||||
|
||||
# Fallback per-request timeout when the caller resolved none (direct
|
||||
# construction, tests). Configured deployments pass one down.
|
||||
_DEFAULT_ANTHROPIC_TIMEOUT = 300.0
|
||||
|
||||
|
||||
class AnthropicLLM(LLMInterface):
|
||||
"""
|
||||
LLM provider using Anthropic's Claude models.
|
||||
@@ -89,7 +94,7 @@ class AnthropicLLM(LLMInterface):
|
||||
base_url: str,
|
||||
model: str,
|
||||
reasoning_effort: str | None = None,
|
||||
timeout: float = 300.0,
|
||||
timeout: float | None = None,
|
||||
default_headers: dict[str, str] | None = None,
|
||||
extra_body: dict[str, Any] | None = None,
|
||||
**kwargs: Any,
|
||||
@@ -103,7 +108,9 @@ class AnthropicLLM(LLMInterface):
|
||||
base_url: Base URL for the API (optional, uses Anthropic default if empty).
|
||||
model: Model name (e.g., "claude-sonnet-4-20250514").
|
||||
reasoning_effort: Reasoning effort level (not used by Anthropic).
|
||||
timeout: Request timeout in seconds.
|
||||
timeout: Per-request timeout in seconds, resolved by the caller from
|
||||
``llm_timeout`` / the per-operation override. ``None`` (direct
|
||||
construction, tests) falls back to ``_DEFAULT_ANTHROPIC_TIMEOUT``.
|
||||
default_headers: Optional custom headers passed as ``default_headers`` to
|
||||
the Anthropic SDK client. Used by operators routing through proxies
|
||||
or request-tracing middleware. Sourced from ``llm_default_headers`` in
|
||||
@@ -114,7 +121,7 @@ class AnthropicLLM(LLMInterface):
|
||||
Sourced from ``llm_extra_body`` (env: ``HINDSIGHT_API_LLM_EXTRA_BODY``).
|
||||
**kwargs: Additional provider-specific parameters.
|
||||
"""
|
||||
super().__init__(provider, api_key, base_url, model, reasoning_effort, **kwargs)
|
||||
super().__init__(provider, api_key, base_url, model, reasoning_effort, timeout=timeout, **kwargs)
|
||||
self._warn_reasoning_effort_unsupported()
|
||||
|
||||
if not self.api_key:
|
||||
@@ -133,8 +140,7 @@ class AnthropicLLM(LLMInterface):
|
||||
client_kwargs: dict[str, Any] = {"api_key": self.api_key, "max_retries": 0}
|
||||
if self.base_url:
|
||||
client_kwargs["base_url"] = self.base_url
|
||||
if timeout:
|
||||
client_kwargs["timeout"] = timeout
|
||||
client_kwargs["timeout"] = self.timeout or _DEFAULT_ANTHROPIC_TIMEOUT
|
||||
if default_headers:
|
||||
client_kwargs["default_headers"] = default_headers
|
||||
|
||||
|
||||
@@ -20,7 +20,8 @@ import json
|
||||
import logging
|
||||
import time
|
||||
import uuid
|
||||
from contextlib import AbstractAsyncContextManager, nullcontext
|
||||
from collections.abc import AsyncIterator
|
||||
from contextlib import AbstractAsyncContextManager, asynccontextmanager, nullcontext
|
||||
from pathlib import Path
|
||||
from typing import Any, Callable
|
||||
|
||||
@@ -111,6 +112,29 @@ def _repair_invalid_json_escapes(text: str) -> str:
|
||||
return "".join(result)
|
||||
|
||||
|
||||
# Fallback per-request deadline when the caller resolved no timeout (direct
|
||||
# construction, tests). Configured deployments always pass one down from
|
||||
# ``llm_timeout`` / the per-operation override.
|
||||
_DEFAULT_CODEX_TIMEOUT = 120.0
|
||||
|
||||
# Hard ceiling on one SSE response body, counted in decoded characters (what
|
||||
# ``aiter_lines`` yields — so a multi-byte body is cut off a little later than
|
||||
# the name suggests, which is fine for a backstop). The Codex backend can wedge
|
||||
# into runaway generation and emit megabytes of deltas for a request whose real
|
||||
# answer is a few hundred bytes (issue #3898); a structured-output call bounded
|
||||
# by ``max_completion_tokens`` never approaches this, so blowing past it means
|
||||
# the stream is not going to end on its own.
|
||||
_MAX_SSE_BODY_CHARS = 4 * 1024 * 1024
|
||||
|
||||
|
||||
class CodexRunawayStreamError(httpx.RequestError):
|
||||
"""The backend streamed past the deadline or the body-size ceiling.
|
||||
|
||||
Deliberately an ``httpx.RequestError``: a runaway stream is a transport-level
|
||||
failure, and both call paths already retry that class with backoff.
|
||||
"""
|
||||
|
||||
|
||||
class CodexLLM(LLMInterface):
|
||||
"""
|
||||
LLM provider using OpenAI Codex OAuth authentication.
|
||||
@@ -180,8 +204,16 @@ class CodexLLM(LLMInterface):
|
||||
self.reasoning_summary = self._map_reasoning_effort(reasoning_effort)
|
||||
self._extra_body = dict(extra_body or {})
|
||||
|
||||
# HTTP client for SSE streaming
|
||||
self._client = httpx.AsyncClient(timeout=120.0)
|
||||
# Per-request deadline. ``self.timeout`` is resolved by the caller from
|
||||
# ``llm_timeout`` / the per-operation override; the literal below is only
|
||||
# the unconfigured fallback.
|
||||
self._request_timeout = self.timeout or _DEFAULT_CODEX_TIMEOUT
|
||||
|
||||
# HTTP client for SSE streaming. httpx timeouts are per-operation (per
|
||||
# socket read), so this bounds a *silent* backend only — a stream that
|
||||
# keeps delivering bytes never trips it. The total deadline in
|
||||
# ``_stream_request`` is what bounds a talkative one.
|
||||
self._client = httpx.AsyncClient(timeout=self._request_timeout)
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Properties — delegate to _auth_manager (preserves test-visible API)
|
||||
@@ -502,18 +534,18 @@ class CodexLLM(LLMInterface):
|
||||
try:
|
||||
async with attempt_context() if attempt_context is not None else nullcontext():
|
||||
set_stage(f"llm.codex.{scope}.attempt={attempt + 1}/{max_retries + 1}")
|
||||
response = await self._client.post(url, json=payload, headers=headers, timeout=120.0)
|
||||
response.raise_for_status()
|
||||
async with self._stream_request(url, payload, headers) as response:
|
||||
response.raise_for_status()
|
||||
|
||||
# Forced-tool path: read structured output from the function-call
|
||||
# arguments (already a JSON string in a dedicated channel) rather
|
||||
# than from free-form assistant text.
|
||||
if use_forced_tool:
|
||||
text_content, tool_calls = await self._parse_sse_tool_stream(response)
|
||||
content = text_content or ""
|
||||
else:
|
||||
tool_calls = []
|
||||
content = await self._parse_sse_stream(response)
|
||||
# Forced-tool path: read structured output from the function-call
|
||||
# arguments (already a JSON string in a dedicated channel) rather
|
||||
# than from free-form assistant text.
|
||||
if use_forced_tool:
|
||||
text_content, tool_calls = await self._parse_sse_tool_stream(response)
|
||||
content = text_content or ""
|
||||
else:
|
||||
tool_calls = []
|
||||
content = await self._parse_sse_stream(response)
|
||||
|
||||
# Codex SSE carries no usage block; stash the same char/4 estimate
|
||||
# the success path traces so a later parse/validate failure records
|
||||
@@ -707,6 +739,60 @@ class CodexLLM(LLMInterface):
|
||||
logger.error(f"Unexpected Codex error: {type(e).__name__}: {e}")
|
||||
raise
|
||||
|
||||
@asynccontextmanager
|
||||
async def _stream_request(
|
||||
self,
|
||||
url: str,
|
||||
payload: dict[str, Any],
|
||||
headers: httpx.Headers,
|
||||
) -> AsyncIterator[httpx.Response]:
|
||||
"""POST and hand back the still-streaming response under a total deadline.
|
||||
|
||||
Two things this gives that ``client.post()`` did not (issue #3898):
|
||||
|
||||
* ``asyncio.timeout`` bounds the request *and* the caller's parse of the
|
||||
body. httpx's own timeout is per socket read, so a backend that keeps
|
||||
sending bytes resets it forever; only a wall-clock deadline ends that.
|
||||
* ``stream()`` means the body is consumed incrementally, so
|
||||
``_iter_sse_lines`` can abandon a runaway response instead of buffering
|
||||
megabytes of it before any parsing runs.
|
||||
|
||||
Non-200 bodies are read eagerly so callers keep using ``response.text``
|
||||
and ``raise_for_status()`` exactly as they did against a buffered response.
|
||||
"""
|
||||
try:
|
||||
async with asyncio.timeout(self._request_timeout):
|
||||
async with self._client.stream("POST", url, json=payload, headers=headers) as response:
|
||||
if response.status_code != 200:
|
||||
# Not a stream: read the body so callers keep reaching for
|
||||
# ``response.text`` / ``raise_for_status()`` unchanged.
|
||||
await response.aread()
|
||||
yield response
|
||||
except TimeoutError as e:
|
||||
raise CodexRunawayStreamError(
|
||||
f"Codex response exceeded the {self._request_timeout:g}s deadline "
|
||||
f"(HINDSIGHT_API_LLM_TIMEOUT or its per-operation override)",
|
||||
request=httpx.Request("POST", url),
|
||||
) from e
|
||||
|
||||
async def _iter_sse_lines(self, response: httpx.Response) -> AsyncIterator[str]:
|
||||
"""Yield SSE lines, abandoning the response if the body runs away.
|
||||
|
||||
The deadline in ``_stream_request`` already bounds wall time; this bounds
|
||||
volume, so a backend generating at speed is cut off in seconds rather than
|
||||
held onto until the deadline expires.
|
||||
"""
|
||||
seen_chars = 0
|
||||
async for line in response.aiter_lines():
|
||||
seen_chars += len(line) + 1 # +1 for the newline aiter_lines strips
|
||||
if seen_chars > _MAX_SSE_BODY_CHARS:
|
||||
raise CodexRunawayStreamError(
|
||||
f"Codex response exceeded {_MAX_SSE_BODY_CHARS} characters without completing; "
|
||||
"abandoning the stream",
|
||||
request=response.request,
|
||||
)
|
||||
yield line
|
||||
|
||||
async def _parse_sse_stream(self, response: httpx.Response) -> str:
|
||||
"""
|
||||
Parse Server-Sent Events (SSE) stream from Codex API.
|
||||
@@ -720,7 +806,7 @@ class CodexLLM(LLMInterface):
|
||||
full_text = ""
|
||||
event_type = None
|
||||
|
||||
async for line in response.aiter_lines():
|
||||
async for line in self._iter_sse_lines(response):
|
||||
if not line:
|
||||
continue
|
||||
|
||||
@@ -880,17 +966,17 @@ class CodexLLM(LLMInterface):
|
||||
async def _request_attempt(attempt: int) -> tuple[str | None, list[LLMToolCall]]:
|
||||
async with attempt_context() if attempt_context is not None else nullcontext():
|
||||
set_stage(f"llm.codex.tools.attempt={attempt}/2")
|
||||
response = await self._client.post(url, json=payload, headers=headers, timeout=120.0)
|
||||
if response.status_code != 200:
|
||||
# 401/403 on the first attempt may still be recovered by the
|
||||
# reactive token refresh below — don't log those as errors yet.
|
||||
detail = f"Codex API error {response.status_code}: {response.text[:500]}"
|
||||
if response.status_code in (401, 403) and not attempted_refresh_after_auth_error:
|
||||
logger.warning(f"{detail} (will attempt token refresh)")
|
||||
else:
|
||||
logger.error(detail)
|
||||
response.raise_for_status()
|
||||
return await self._parse_sse_tool_stream(response)
|
||||
async with self._stream_request(url, payload, headers) as response:
|
||||
if response.status_code != 200:
|
||||
# 401/403 on the first attempt may still be recovered by the
|
||||
# reactive token refresh below — don't log those as errors yet.
|
||||
detail = f"Codex API error {response.status_code}: {response.text[:500]}"
|
||||
if response.status_code in (401, 403) and not attempted_refresh_after_auth_error:
|
||||
logger.warning(f"{detail} (will attempt token refresh)")
|
||||
else:
|
||||
logger.error(detail)
|
||||
response.raise_for_status()
|
||||
return await self._parse_sse_tool_stream(response)
|
||||
|
||||
try:
|
||||
try:
|
||||
@@ -986,7 +1072,7 @@ class CodexLLM(LLMInterface):
|
||||
tool_calls: list[LLMToolCall] = []
|
||||
event_type = None
|
||||
|
||||
async for line in response.aiter_lines():
|
||||
async for line in self._iter_sse_lines(response):
|
||||
if not line:
|
||||
continue
|
||||
|
||||
|
||||
@@ -159,6 +159,11 @@ def _convert_messages_to_gemini(msg_list: list[dict[str, Any]]) -> _GeminiConver
|
||||
return _GeminiConversation(system_instruction=system_instruction, contents=gemini_contents)
|
||||
|
||||
|
||||
# Fallback per-request deadline when the caller resolved no timeout. A safety net
|
||||
# for network hangs; valid slow responses are well under this.
|
||||
_DEFAULT_GEMINI_TIMEOUT = 90.0
|
||||
|
||||
|
||||
class GeminiLLM(LLMInterface):
|
||||
"""
|
||||
LLM provider for Google Gemini and Vertex AI.
|
||||
@@ -184,6 +189,12 @@ class GeminiLLM(LLMInterface):
|
||||
self._client = None
|
||||
self._is_vertexai = self.provider == "vertexai"
|
||||
|
||||
# Per-request deadline, resolved by the caller from ``llm_timeout`` / the
|
||||
# per-operation override. The literal is only the unconfigured fallback —
|
||||
# it used to be hardcoded at every call site, so a configured timeout was
|
||||
# silently ignored (issue #3898).
|
||||
self._request_timeout = self.timeout or _DEFAULT_GEMINI_TIMEOUT
|
||||
|
||||
# Safety settings: None means use Gemini's defaults
|
||||
self._safety_settings: list | None = kwargs.get("gemini_safety_settings")
|
||||
self._service_tier: str | None = kwargs.get("gemini_service_tier")
|
||||
@@ -436,7 +447,7 @@ class GeminiLLM(LLMInterface):
|
||||
contents=gemini_contents,
|
||||
config=generation_config,
|
||||
),
|
||||
timeout=90.0, # Safety net for network hangs; valid slow responses are <90s
|
||||
timeout=self._request_timeout,
|
||||
)
|
||||
# Stash usage before parse/validate, which may raise locally
|
||||
# even though the provider charged for these tokens (#2387).
|
||||
@@ -815,7 +826,7 @@ class GeminiLLM(LLMInterface):
|
||||
contents=active_contents,
|
||||
config=config,
|
||||
),
|
||||
timeout=90.0, # Safety net for network hangs; valid slow responses are <90s
|
||||
timeout=self._request_timeout,
|
||||
)
|
||||
stash_response_usage(_usage_from_gemini_response(response))
|
||||
|
||||
|
||||
@@ -386,9 +386,8 @@ class GitHubCopilotLLM(LLMInterface):
|
||||
timeout: float | None = None,
|
||||
**kwargs: Any,
|
||||
) -> None:
|
||||
super().__init__(provider, api_key, base_url, model, reasoning_effort, **kwargs)
|
||||
super().__init__(provider, api_key, base_url, model, reasoning_effort, timeout=timeout, **kwargs)
|
||||
self._released = False
|
||||
self._timeout = timeout
|
||||
|
||||
if self.reasoning_effort == "none":
|
||||
self.reasoning_effort = None
|
||||
@@ -531,7 +530,7 @@ class GitHubCopilotLLM(LLMInterface):
|
||||
await self._runtime.invalidate(client, "transient session cleanup timed out or failed")
|
||||
|
||||
def _timeout_seconds(self) -> float:
|
||||
return self._timeout if self._timeout is not None else _DEFAULT_TIMEOUT_SECONDS
|
||||
return self.timeout if self.timeout is not None else _DEFAULT_TIMEOUT_SECONDS
|
||||
|
||||
@staticmethod
|
||||
async def _cleanup_session(
|
||||
|
||||
@@ -302,6 +302,7 @@ class LlamaCppLLM(LLMInterface):
|
||||
chat_format: str | None = None,
|
||||
no_grammar: bool = False,
|
||||
extra_args: str | None = None,
|
||||
timeout: float | None = None,
|
||||
**kwargs: Any,
|
||||
):
|
||||
super().__init__(
|
||||
@@ -310,6 +311,7 @@ class LlamaCppLLM(LLMInterface):
|
||||
base_url=base_url or "",
|
||||
model=model or DEFAULT_LLAMACPP_MODEL_ALIAS,
|
||||
reasoning_effort=reasoning_effort,
|
||||
timeout=timeout,
|
||||
)
|
||||
self._extra_body = extra_body
|
||||
self._model_path_str = model_path
|
||||
@@ -379,6 +381,7 @@ class LlamaCppLLM(LLMInterface):
|
||||
# rather than inventing a level for the local model.
|
||||
reasoning_effort=self.reasoning_effort,
|
||||
extra_body=self._extra_body,
|
||||
timeout=self.timeout,
|
||||
)
|
||||
|
||||
self._initialized = True
|
||||
|
||||
@@ -0,0 +1,47 @@
|
||||
"""Test stub for CodexLLM's streaming POST.
|
||||
|
||||
``CodexLLM`` reads its SSE body with ``client.stream()`` under a wall-clock
|
||||
deadline rather than a buffering ``client.post()`` (issue #3898), so tests that
|
||||
want to inspect the request or hand back a canned response patch the streaming
|
||||
call instead. The mock this returns records calls exactly as the old ``post``
|
||||
mock did — ``mock.call_args.kwargs["json"]`` / ``["headers"]`` still work —
|
||||
because ``stream()`` is called with the same keyword arguments.
|
||||
"""
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
|
||||
class _StreamContext:
|
||||
"""Async context manager yielding a canned response, like ``client.stream()``."""
|
||||
|
||||
def __init__(self, response: Any) -> None:
|
||||
self._response = response
|
||||
|
||||
async def __aenter__(self) -> Any:
|
||||
return self._response
|
||||
|
||||
async def __aexit__(self, *exc_info: Any) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
def stub_codex_stream(llm: Any, response: Any) -> Any:
|
||||
"""Patch ``llm._client.stream`` to yield ``response``; use as a context manager.
|
||||
|
||||
The patched attribute is a plain ``MagicMock`` (not an ``AsyncMock``):
|
||||
``stream()`` is a sync call returning an async context manager.
|
||||
"""
|
||||
return patch.object(llm._client, "stream", MagicMock(return_value=_StreamContext(response)))
|
||||
|
||||
|
||||
def stub_codex_stream_with(llm: Any, handler: Any) -> Any:
|
||||
"""Like ``stub_codex_stream`` but ``handler(url, **kwargs)`` builds each response.
|
||||
|
||||
For tests that vary the response per attempt (auth-refresh retries). The
|
||||
handler runs when the provider calls ``stream()``.
|
||||
"""
|
||||
return patch.object(
|
||||
llm._client,
|
||||
"stream",
|
||||
MagicMock(side_effect=lambda _method, url, **kwargs: _StreamContext(handler(url, **kwargs))),
|
||||
)
|
||||
@@ -6,6 +6,7 @@ import pytest
|
||||
|
||||
from hindsight_api.engine.llm_wrapper import create_llm_provider
|
||||
from hindsight_api.engine.providers.codex_llm import CodexLLM
|
||||
from tests.codex_stream_stub import stub_codex_stream
|
||||
|
||||
|
||||
def build_llm(extra_body: dict | None = None) -> CodexLLM:
|
||||
@@ -42,17 +43,16 @@ def test_factory_forwards_extra_body() -> None:
|
||||
@pytest.mark.asyncio
|
||||
async def test_call_merges_extra_body_into_request() -> None:
|
||||
llm = build_llm({"service_tier": "priority"})
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call(messages=[{"role": "user", "content": "hello"}], max_retries=0)
|
||||
|
||||
assert mock_post.call_args.kwargs["json"]["service_tier"] == "priority"
|
||||
assert mock_stream.call_args.kwargs["json"]["service_tier"] == "priority"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -62,14 +62,13 @@ async def test_call_with_tools_merges_extra_body_into_request() -> None:
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock, return_value=(None, [])),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call_with_tools(
|
||||
messages=[{"role": "user", "content": "hello"}],
|
||||
tools=[],
|
||||
max_retries=0,
|
||||
)
|
||||
|
||||
assert mock_post.call_args.kwargs["json"]["service_tier"] == "priority"
|
||||
assert mock_stream.call_args.kwargs["json"]["service_tier"] == "priority"
|
||||
|
||||
@@ -43,6 +43,7 @@ from hindsight_api.engine.providers.codex_llm import (
|
||||
CodexLLM,
|
||||
CodexRefreshExpiredError,
|
||||
)
|
||||
from tests.codex_stream_stub import stub_codex_stream_with
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
@@ -669,17 +670,20 @@ async def test_call_reactively_refreshes_on_401_and_retries(tmp_path: Path):
|
||||
call_count["refresh"] += 1
|
||||
return refresh_resp
|
||||
|
||||
# Async mock for the LLM's HTTP client (used for backend calls).
|
||||
async def fake_backend_post(url, **kwargs):
|
||||
fail_response.raise_for_status = MagicMock(
|
||||
side_effect=httpx.HTTPStatusError("401", request=MagicMock(), response=fail_response)
|
||||
)
|
||||
fail_response.aread = AsyncMock(return_value=b"unauthorized")
|
||||
|
||||
# Stub for the LLM's HTTP client (used for backend calls).
|
||||
def fake_backend_stream(url, **kwargs):
|
||||
call_count["post"] += 1
|
||||
sent_headers.append(httpx.Headers(kwargs["headers"]))
|
||||
if call_count["post"] == 1:
|
||||
raise httpx.HTTPStatusError("401", request=MagicMock(), response=fail_response)
|
||||
return success_resp
|
||||
return fail_response if call_count["post"] == 1 else success_resp
|
||||
|
||||
with (
|
||||
patch.object(llm._auth_manager._http_client, "post", new=fake_refresh_post),
|
||||
patch.object(llm._client, "post", new=fake_backend_post),
|
||||
stub_codex_stream_with(llm, fake_backend_stream),
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
result = await llm.call(
|
||||
@@ -720,7 +724,7 @@ async def test_call_proactively_refreshes_when_token_is_stale(tmp_path: Path):
|
||||
call_order.append("refresh")
|
||||
return refresh_resp
|
||||
|
||||
async def fake_backend_post(url, **kwargs):
|
||||
def fake_backend_stream(url, **kwargs):
|
||||
call_order.append("backend")
|
||||
# Assert that by the time the backend is called, the new token is in use.
|
||||
assert kwargs["headers"]["Authorization"] == f"Bearer {new_access}"
|
||||
@@ -728,7 +732,7 @@ async def test_call_proactively_refreshes_when_token_is_stale(tmp_path: Path):
|
||||
|
||||
with (
|
||||
patch.object(llm._auth_manager._http_client, "post", new=fake_refresh_post),
|
||||
patch.object(llm._client, "post", new=fake_backend_post),
|
||||
stub_codex_stream_with(llm, fake_backend_stream),
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
await llm.call(
|
||||
@@ -754,12 +758,12 @@ async def test_call_does_not_refresh_when_token_is_fresh(tmp_path: Path):
|
||||
|
||||
call_count = {"backend": 0}
|
||||
|
||||
async def fake_backend_post(url, **kwargs):
|
||||
def fake_backend_stream(url, **kwargs):
|
||||
call_count["backend"] += 1
|
||||
return success_resp
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new=fake_backend_post),
|
||||
stub_codex_stream_with(llm, fake_backend_stream),
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
await llm.call(
|
||||
|
||||
@@ -5,6 +5,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
||||
import pytest
|
||||
|
||||
from hindsight_api.engine.providers.codex_llm import CodexLLM
|
||||
from tests.codex_stream_stub import stub_codex_stream
|
||||
|
||||
|
||||
def build_llm(reasoning_effort: str | None = "high") -> CodexLLM:
|
||||
@@ -24,17 +25,16 @@ def build_llm(reasoning_effort: str | None = "high") -> CodexLLM:
|
||||
@pytest.mark.asyncio
|
||||
async def test_call_sends_reasoning_effort_separately_from_summary() -> None:
|
||||
llm = build_llm("high")
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call(messages=[{"role": "user", "content": "hello"}], max_retries=0)
|
||||
|
||||
assert mock_post.call_args.kwargs["json"]["reasoning"] == {
|
||||
assert mock_stream.call_args.kwargs["json"]["reasoning"] == {
|
||||
"effort": "high",
|
||||
"summary": "detailed",
|
||||
}
|
||||
@@ -48,17 +48,16 @@ async def test_call_with_tools_sends_reasoning_effort_separately_from_summary()
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock, return_value=(None, [])),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call_with_tools(
|
||||
messages=[{"role": "user", "content": "hello"}],
|
||||
tools=[],
|
||||
max_retries=0,
|
||||
)
|
||||
|
||||
assert mock_post.call_args.kwargs["json"]["reasoning"] == {
|
||||
assert mock_stream.call_args.kwargs["json"]["reasoning"] == {
|
||||
"effort": "low",
|
||||
"summary": "concise",
|
||||
}
|
||||
@@ -73,15 +72,14 @@ async def test_unconfigured_reasoning_effort_is_omitted_from_the_payload() -> No
|
||||
effort is the operator's to set (issue #3449).
|
||||
"""
|
||||
llm = build_llm(None)
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call(messages=[{"role": "user", "content": "hello"}], max_retries=0)
|
||||
|
||||
# "auto" is the neutral summary an unrecognised level already mapped to.
|
||||
assert mock_post.call_args.kwargs["json"]["reasoning"] == {"summary": "auto"}
|
||||
assert mock_stream.call_args.kwargs["json"]["reasoning"] == {"summary": "auto"}
|
||||
|
||||
@@ -6,6 +6,7 @@ import httpx
|
||||
import pytest
|
||||
|
||||
from hindsight_api.engine.providers.codex_llm import CodexLLM
|
||||
from tests.codex_stream_stub import stub_codex_stream
|
||||
|
||||
|
||||
def build_llm() -> CodexLLM:
|
||||
@@ -29,17 +30,16 @@ def assert_codex_request_identity(headers: httpx.Headers) -> None:
|
||||
@pytest.mark.asyncio
|
||||
async def test_call_sends_codex_request_identity() -> None:
|
||||
llm = build_llm()
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock, return_value="ok"),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call(messages=[{"role": "user", "content": "hello"}], max_retries=0)
|
||||
|
||||
assert_codex_request_identity(mock_post.call_args.kwargs["headers"])
|
||||
assert_codex_request_identity(mock_stream.call_args.kwargs["headers"])
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@@ -50,14 +50,13 @@ async def test_call_with_tools_sends_codex_request_identity() -> None:
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with (
|
||||
patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post,
|
||||
stub_codex_stream(llm, response) as mock_stream,
|
||||
patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock, return_value=(None, [])),
|
||||
):
|
||||
mock_post.return_value = response
|
||||
await llm.call_with_tools(
|
||||
messages=[{"role": "user", "content": "hello"}],
|
||||
tools=[],
|
||||
max_retries=0,
|
||||
)
|
||||
|
||||
assert_codex_request_identity(mock_post.call_args.kwargs["headers"])
|
||||
assert_codex_request_identity(mock_stream.call_args.kwargs["headers"])
|
||||
|
||||
@@ -0,0 +1,173 @@
|
||||
"""Regression tests for the Codex per-request deadline and stream size guard (issue #3898).
|
||||
|
||||
Before the fix ``CodexLLM`` never read the configured timeout at all — the factory
|
||||
did not pass one and the class did not have the attribute — and it fetched the SSE
|
||||
body with a buffering ``client.post()`` under a hardcoded 120 s httpx timeout. That
|
||||
timeout is per socket read, so a backend that wedges into runaway generation and
|
||||
keeps emitting deltas never trips it: the client read one such response for ~830 s
|
||||
(~12 MB) until the backend itself closed the connection, holding the consolidation
|
||||
slot for the whole time.
|
||||
|
||||
These tests run a real local SSE server rather than mocking httpx, because the whole
|
||||
defect lives in the interaction between a still-flowing socket and the timeout that
|
||||
was supposed to bound it — a mocked response cannot express "bytes keep arriving".
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import time
|
||||
from unittest.mock import patch
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
from hindsight_api.engine.providers.codex_llm import (
|
||||
_MAX_SSE_BODY_CHARS,
|
||||
CodexLLM,
|
||||
CodexRunawayStreamError,
|
||||
)
|
||||
|
||||
pytestmark = pytest.mark.asyncio
|
||||
|
||||
|
||||
def _delta_chunk(payload: str) -> bytes:
|
||||
return b'event: response.text.delta\ndata: {"delta": "' + payload.encode() + b'"}\n\n'
|
||||
|
||||
|
||||
async def _sse_server(chunks_per_second: float, body: bytes | None = None, *, finite: bool = False):
|
||||
"""Serve one endless (or fixed) SSE body; returns (server, base_url)."""
|
||||
|
||||
async def handle(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None:
|
||||
while await reader.readline() not in (b"\r\n", b"\n", b""):
|
||||
pass
|
||||
writer.write(b"HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nTransfer-Encoding: chunked\r\n\r\n")
|
||||
try:
|
||||
if finite:
|
||||
assert body is not None
|
||||
writer.write(b"%x\r\n" % len(body) + body + b"\r\n0\r\n\r\n")
|
||||
await writer.drain()
|
||||
return
|
||||
chunk = _delta_chunk("x" * 4000)
|
||||
while True:
|
||||
writer.write(b"%x\r\n" % len(chunk) + chunk + b"\r\n")
|
||||
await writer.drain()
|
||||
await asyncio.sleep(1 / chunks_per_second)
|
||||
except (ConnectionResetError, BrokenPipeError):
|
||||
return
|
||||
finally:
|
||||
writer.close()
|
||||
|
||||
server = await asyncio.start_server(handle, "127.0.0.1", 0)
|
||||
return server, f"http://127.0.0.1:{server.sockets[0].getsockname()[1]}"
|
||||
|
||||
|
||||
def _build_llm(base_url: str, timeout: float | None) -> CodexLLM:
|
||||
with (
|
||||
patch.object(CodexLLM, "_load_codex_auth", return_value=("token", "account")),
|
||||
patch.object(CodexLLM, "_load_codex_refresh_token", return_value=None),
|
||||
):
|
||||
return CodexLLM(
|
||||
provider="openai-codex",
|
||||
api_key="ignored",
|
||||
base_url=base_url,
|
||||
model="gpt-5.4-mini",
|
||||
timeout=timeout,
|
||||
)
|
||||
|
||||
|
||||
async def test_configured_timeout_bounds_a_runaway_stream():
|
||||
"""A stream that never stops delivering bytes is abandoned at the deadline.
|
||||
|
||||
The failure this guards is silent: with a per-read timeout the call simply
|
||||
never returns, so assert on elapsed wall time, not just on the exception.
|
||||
"""
|
||||
server, base_url = await _sse_server(chunks_per_second=4)
|
||||
llm = _build_llm(base_url, timeout=2.0)
|
||||
try:
|
||||
started = time.monotonic()
|
||||
with pytest.raises(CodexRunawayStreamError) as excinfo:
|
||||
await llm.call(messages=[{"role": "user", "content": "hi"}], max_retries=0)
|
||||
elapsed = time.monotonic() - started
|
||||
assert "2s deadline" in str(excinfo.value)
|
||||
assert elapsed < 10.0, f"call ran {elapsed:.1f}s despite a 2s deadline"
|
||||
finally:
|
||||
await llm.cleanup()
|
||||
server.close()
|
||||
|
||||
|
||||
async def test_deadline_also_bounds_the_tool_call_path():
|
||||
"""``call_with_tools`` is a second request path — reflect runs through it."""
|
||||
server, base_url = await _sse_server(chunks_per_second=4)
|
||||
llm = _build_llm(base_url, timeout=2.0)
|
||||
try:
|
||||
started = time.monotonic()
|
||||
with pytest.raises(CodexRunawayStreamError):
|
||||
await llm.call_with_tools(
|
||||
messages=[{"role": "user", "content": "hi"}],
|
||||
tools=[],
|
||||
max_retries=0,
|
||||
)
|
||||
elapsed = time.monotonic() - started
|
||||
assert elapsed < 10.0, f"tool call ran {elapsed:.1f}s despite a 2s deadline"
|
||||
finally:
|
||||
await llm.cleanup()
|
||||
server.close()
|
||||
|
||||
|
||||
async def test_runaway_is_retryable_transport_error():
|
||||
"""The deadline must land in the existing retry path, not escape as a hard failure."""
|
||||
assert issubclass(CodexRunawayStreamError, httpx.RequestError)
|
||||
|
||||
|
||||
async def test_oversized_body_is_abandoned_before_the_deadline():
|
||||
"""The size guard cuts off a fast runaway stream without waiting out the clock."""
|
||||
server, base_url = await _sse_server(chunks_per_second=2000)
|
||||
# Generous deadline: only the byte ceiling can end this call.
|
||||
llm = _build_llm(base_url, timeout=120.0)
|
||||
try:
|
||||
started = time.monotonic()
|
||||
with pytest.raises(CodexRunawayStreamError) as excinfo:
|
||||
await llm.call(messages=[{"role": "user", "content": "hi"}], max_retries=0)
|
||||
elapsed = time.monotonic() - started
|
||||
assert str(_MAX_SSE_BODY_CHARS) in str(excinfo.value)
|
||||
assert elapsed < 60.0, f"size guard did not fire ({elapsed:.1f}s)"
|
||||
finally:
|
||||
await llm.cleanup()
|
||||
server.close()
|
||||
|
||||
|
||||
async def test_normal_response_still_parses():
|
||||
"""The stream rewrite must not change what an ordinary short response returns."""
|
||||
body = _delta_chunk("hello ") + _delta_chunk("world") + b"data: [DONE]\n\n"
|
||||
server, base_url = await _sse_server(chunks_per_second=1, body=body, finite=True)
|
||||
llm = _build_llm(base_url, timeout=30.0)
|
||||
try:
|
||||
assert await llm.call(messages=[{"role": "user", "content": "hi"}], max_retries=0) == "hello world"
|
||||
finally:
|
||||
await llm.cleanup()
|
||||
server.close()
|
||||
|
||||
|
||||
async def test_error_body_is_readable_after_streaming():
|
||||
"""Non-200 responses still expose ``.text`` — callers log it and classify on it."""
|
||||
|
||||
async def handle(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None:
|
||||
while await reader.readline() not in (b"\r\n", b"\n", b""):
|
||||
pass
|
||||
payload = b'{"error": "bad request detail"}'
|
||||
writer.write(
|
||||
b"HTTP/1.1 400 Bad Request\r\nContent-Type: application/json\r\nContent-Length: %d\r\n\r\n" % len(payload)
|
||||
)
|
||||
writer.write(payload)
|
||||
await writer.drain()
|
||||
writer.close()
|
||||
|
||||
server = await asyncio.start_server(handle, "127.0.0.1", 0)
|
||||
base_url = f"http://127.0.0.1:{server.sockets[0].getsockname()[1]}"
|
||||
llm = _build_llm(base_url, timeout=30.0)
|
||||
try:
|
||||
with pytest.raises(httpx.HTTPStatusError) as excinfo:
|
||||
await llm.call(messages=[{"role": "user", "content": "hi"}], max_retries=0)
|
||||
assert "bad request detail" in excinfo.value.response.text
|
||||
finally:
|
||||
await llm.cleanup()
|
||||
server.close()
|
||||
@@ -23,6 +23,7 @@ from hindsight_api.engine.providers.codex_llm import (
|
||||
_repair_invalid_json_escapes,
|
||||
)
|
||||
from hindsight_api.engine.response_models import LLMToolCall
|
||||
from tests.codex_stream_stub import stub_codex_stream
|
||||
|
||||
|
||||
class _Fact(BaseModel):
|
||||
@@ -85,12 +86,11 @@ def test_repair_handles_trailing_backslash():
|
||||
@pytest.mark.asyncio
|
||||
async def test_strict_schema_uses_forced_function_tool():
|
||||
llm = build_llm()
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
tool_call = LLMToolCall(id="call-1", name="structured_response", arguments={"fact": "the sky is blue"})
|
||||
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
with patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = (None, [tool_call])
|
||||
result = await llm.call(
|
||||
@@ -99,8 +99,8 @@ async def test_strict_schema_uses_forced_function_tool():
|
||||
strict_schema=True,
|
||||
max_retries=0,
|
||||
)
|
||||
sent_payload = mock_post.call_args.kwargs["json"]
|
||||
sent_headers = mock_post.call_args.kwargs["headers"]
|
||||
sent_payload = mock_stream.call_args.kwargs["json"]
|
||||
sent_headers = mock_stream.call_args.kwargs["headers"]
|
||||
|
||||
# Forced tool wired into the request payload.
|
||||
assert sent_payload["tool_choice"] == {"type": "function", "name": "structured_response"}
|
||||
@@ -119,14 +119,13 @@ async def test_strict_schema_uses_forced_function_tool():
|
||||
@pytest.mark.asyncio
|
||||
async def test_strict_schema_skip_validation_returns_dict():
|
||||
llm = build_llm()
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
tool_call = LLMToolCall(id="c", name="structured_response", arguments={"fact": "x"})
|
||||
span_recorder = MagicMock()
|
||||
|
||||
with patch("hindsight_api.tracing.get_span_recorder", return_value=span_recorder):
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
with patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = (None, [tool_call])
|
||||
result = await llm.call(
|
||||
@@ -144,11 +143,10 @@ async def test_strict_schema_skip_validation_returns_dict():
|
||||
@pytest.mark.asyncio
|
||||
async def test_strict_schema_retries_when_forced_tool_missing():
|
||||
llm = build_llm()
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
# Model returns no tool call at all — should raise after retries exhausted.
|
||||
with patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = ("some prose", [])
|
||||
@@ -169,13 +167,12 @@ async def test_strict_schema_retries_when_forced_tool_missing():
|
||||
@pytest.mark.asyncio
|
||||
async def test_non_strict_repairs_invalid_escapes_without_retrying():
|
||||
llm = build_llm()
|
||||
response = MagicMock()
|
||||
response = MagicMock(status_code=200)
|
||||
response.raise_for_status.return_value = None
|
||||
# Escape-heavy content the model would emit as invalid JSON.
|
||||
escape_heavy = r'{"fact": "run rig-control \d serial \s command"}'
|
||||
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
with patch.object(llm, "_parse_sse_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = escape_heavy
|
||||
result = await llm.call(
|
||||
@@ -186,6 +183,6 @@ async def test_non_strict_repairs_invalid_escapes_without_retrying():
|
||||
)
|
||||
|
||||
# Parsed on the first attempt (no retry storm): the SSE stream was read once.
|
||||
assert mock_post.await_count == 1
|
||||
assert mock_stream.call_count == 1
|
||||
assert isinstance(result, _Fact)
|
||||
assert result.fact == r"run rig-control \d serial \s command"
|
||||
|
||||
@@ -16,6 +16,7 @@ import pytest
|
||||
|
||||
from hindsight_api.engine.llm_interface import LLMToolChoice
|
||||
from hindsight_api.engine.providers.codex_llm import CodexLLM
|
||||
from tests.codex_stream_stub import stub_codex_stream
|
||||
|
||||
TOOLS = [
|
||||
{
|
||||
@@ -49,8 +50,7 @@ async def test_codex_serializes_named_tool_choice_for_responses():
|
||||
response = MagicMock()
|
||||
response.status_code = 200
|
||||
response.raise_for_status.return_value = None
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
with patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = (None, [])
|
||||
await llm.call_with_tools(
|
||||
@@ -59,7 +59,7 @@ async def test_codex_serializes_named_tool_choice_for_responses():
|
||||
tool_choice=LLMToolChoice.named("recall"),
|
||||
max_retries=0,
|
||||
)
|
||||
sent_payload = mock_post.call_args.kwargs["json"]
|
||||
sent_payload = mock_stream.call_args.kwargs["json"]
|
||||
|
||||
assert sent_payload["tool_choice"] == {"type": "function", "name": "recall"}
|
||||
|
||||
@@ -71,8 +71,7 @@ async def test_codex_forced_tool_choice_still_yields_tool_calls():
|
||||
response.status_code = 200
|
||||
response.raise_for_status.return_value = None
|
||||
tool_call = {"id": "call-1", "name": "recall", "arguments": {"query": "memory"}}
|
||||
with patch.object(llm._client, "post", new_callable=AsyncMock) as mock_post:
|
||||
mock_post.return_value = response
|
||||
with stub_codex_stream(llm, response) as mock_stream:
|
||||
with patch.object(llm, "_parse_sse_tool_stream", new_callable=AsyncMock) as mock_parse:
|
||||
mock_parse.return_value = (None, [tool_call])
|
||||
result = await llm.call_with_tools(
|
||||
@@ -81,7 +80,7 @@ async def test_codex_forced_tool_choice_still_yields_tool_calls():
|
||||
tool_choice=LLMToolChoice.named("recall"),
|
||||
max_retries=0,
|
||||
)
|
||||
sent_payload = mock_post.call_args.kwargs["json"]
|
||||
sent_payload = mock_stream.call_args.kwargs["json"]
|
||||
|
||||
assert len(result.tool_calls) == 1
|
||||
assert result.tool_calls[0].name == "recall"
|
||||
|
||||
@@ -48,6 +48,82 @@ def test_openai_compatible_provider_impl_receives_timeout():
|
||||
assert llm._provider_impl.timeout == 250.0
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"provider,extra",
|
||||
[
|
||||
("openai-codex", {}),
|
||||
("anthropic", {}),
|
||||
("gemini", {}),
|
||||
("github-copilot", {}),
|
||||
("llamacpp", {}),
|
||||
],
|
||||
)
|
||||
def test_every_network_provider_receives_the_resolved_timeout(provider, extra, monkeypatch):
|
||||
"""Every provider carries the resolved timeout, whether or not it uses it (#3898).
|
||||
|
||||
Codex had no ``timeout`` at all — the factory never passed one and the class
|
||||
never read one — so a runaway response was read until the backend gave up.
|
||||
Gemini hardcoded its deadline at the call site and Anthropic fell back to its
|
||||
own 300 s default, both ignoring what the operator configured. Parametrized so
|
||||
a provider added later cannot quietly drop the value again.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
from hindsight_api.engine.providers.codex_llm import CodexLLM
|
||||
|
||||
with (
|
||||
patch.object(CodexLLM, "_load_codex_auth", return_value=("token", "account")),
|
||||
patch.object(CodexLLM, "_load_codex_refresh_token", return_value=None),
|
||||
):
|
||||
llm = LLMConfig(provider=provider, api_key="k", base_url="", model="m", timeout=222.0, **extra)
|
||||
assert llm._provider_impl.timeout == 222.0
|
||||
|
||||
|
||||
def test_gemini_uses_the_resolved_timeout_at_the_call_site():
|
||||
"""Gemini's ``asyncio.wait_for`` deadline comes from config, not a literal.
|
||||
|
||||
The 90 s literal used to be written at both call sites, so a configured
|
||||
timeout was carried and then ignored — the attribute check above would have
|
||||
passed anyway.
|
||||
"""
|
||||
llm = LLMConfig(provider="gemini", api_key="k", base_url="", model="m", timeout=45.0)
|
||||
assert llm._provider_impl._request_timeout == 45.0
|
||||
|
||||
|
||||
def test_gemini_deadline_falls_back_when_unconfigured():
|
||||
from hindsight_api.engine.providers.gemini_llm import _DEFAULT_GEMINI_TIMEOUT
|
||||
|
||||
llm = LLMConfig(provider="gemini", api_key="k", base_url="", model="m")
|
||||
assert llm._provider_impl._request_timeout == _DEFAULT_GEMINI_TIMEOUT
|
||||
|
||||
|
||||
def test_anthropic_passes_the_resolved_timeout_to_its_sdk_client():
|
||||
"""Anthropic silently used its own 300 s default because none was threaded."""
|
||||
llm = LLMConfig(provider="anthropic", api_key="k", base_url="", model="claude-sonnet-4-20250514", timeout=45.0)
|
||||
assert llm._provider_impl._client.timeout == 45.0
|
||||
|
||||
|
||||
def test_anthropic_timeout_falls_back_when_unconfigured():
|
||||
from hindsight_api.engine.providers.anthropic_llm import _DEFAULT_ANTHROPIC_TIMEOUT
|
||||
|
||||
llm = LLMConfig(provider="anthropic", api_key="k", base_url="", model="claude-sonnet-4-20250514")
|
||||
assert llm._provider_impl._client.timeout == _DEFAULT_ANTHROPIC_TIMEOUT
|
||||
|
||||
|
||||
def test_codex_deadline_falls_back_when_unconfigured():
|
||||
"""An unconfigured Codex provider keeps its historical 120 s bound."""
|
||||
from unittest.mock import patch
|
||||
|
||||
from hindsight_api.engine.providers.codex_llm import _DEFAULT_CODEX_TIMEOUT, CodexLLM
|
||||
|
||||
with (
|
||||
patch.object(CodexLLM, "_load_codex_auth", return_value=("token", "account")),
|
||||
patch.object(CodexLLM, "_load_codex_refresh_token", return_value=None),
|
||||
):
|
||||
llm = LLMConfig(provider="openai-codex", api_key="k", base_url="", model="m")
|
||||
assert llm._provider_impl._request_timeout == _DEFAULT_CODEX_TIMEOUT
|
||||
|
||||
|
||||
def test_timeout_none_falls_back_to_provider_default():
|
||||
"""No timeout passed -> provider falls back to its env/DEFAULT_LLM_TIMEOUT default.
|
||||
|
||||
|
||||
@@ -275,7 +275,7 @@ For non-English banks (especially CJK) and the language/extraction-language trad
|
||||
| `HINDSIGHT_API_LLM_MAX_RETRIES` | Max retry attempts for LLM API calls | `3` |
|
||||
| `HINDSIGHT_API_LLM_INITIAL_BACKOFF` | Initial retry backoff in seconds (exponential backoff) | `1.0` |
|
||||
| `HINDSIGHT_API_LLM_MAX_BACKOFF` | Max retry backoff cap in seconds | `60.0` |
|
||||
| `HINDSIGHT_API_LLM_TIMEOUT` | LLM request timeout in seconds | `120` |
|
||||
| `HINDSIGHT_API_LLM_TIMEOUT` | LLM request timeout in seconds. Honoured by every provider as a **total** deadline for one request, including the time spent reading a streamed response — a backend that keeps sending bytes is cut off at the deadline rather than read indefinitely. Raise it if your provider is legitimately slow; the per-operation variables below override it. | `120` |
|
||||
| `HINDSIGHT_API_LLM_REASONING_EFFORT` | Reasoning effort for providers/models that support it (for example `none`, `low`, `medium`, `high`, `xhigh`). Set it and the value is sent as given, whatever your model is called — which is how you control thinking-token volume on a self-hosted reasoning model (vLLM, Ollama, llama.cpp, TGI), where `none` is often the only value that removes the thinking block. Leave it unset and no reasoning parameter is sent at all, so each model runs at its own default effort. Honoured by `openai` and every OpenAI-compatible provider, `openai-responses`, `openai-codex`, `xai`, `llamacpp`, and `litellm`/`litellmrouter` (which translate it per target provider). The native `gemini`/`vertexai`, `anthropic` and `claude-code` providers have no reasoning-effort control and log a warning at startup if you set one — reach those models through `litellm` to apply it. | Unset (model's own default) |
|
||||
| `HINDSIGHT_API_LLM_TEMPERATURE` | Global override for the sampling temperature of internal LLM calls. Set a number in `[0.0, 2.0]`, or `none` (also `default`/`off`/empty) to **omit** the temperature parameter entirely — required for models that reject explicit temperatures, e.g. Azure `gpt-5.5`, which only accepts its default value. Per-operation variables below override this. | Per-operation defaults |
|
||||
| `HINDSIGHT_API_LLM_TEMPERATURE_VERIFICATION` | Temperature for the startup connection check. Number in `[0.0, 2.0]` or `none` to omit. Overrides `HINDSIGHT_API_LLM_TEMPERATURE`. | `0.0` |
|
||||
|
||||
@@ -275,7 +275,7 @@ For non-English banks (especially CJK) and the language/extraction-language trad
|
||||
| `HINDSIGHT_API_LLM_MAX_RETRIES` | Max retry attempts for LLM API calls | `3` |
|
||||
| `HINDSIGHT_API_LLM_INITIAL_BACKOFF` | Initial retry backoff in seconds (exponential backoff) | `1.0` |
|
||||
| `HINDSIGHT_API_LLM_MAX_BACKOFF` | Max retry backoff cap in seconds | `60.0` |
|
||||
| `HINDSIGHT_API_LLM_TIMEOUT` | LLM request timeout in seconds | `120` |
|
||||
| `HINDSIGHT_API_LLM_TIMEOUT` | LLM request timeout in seconds. Honoured by every provider as a **total** deadline for one request, including the time spent reading a streamed response — a backend that keeps sending bytes is cut off at the deadline rather than read indefinitely. Raise it if your provider is legitimately slow; the per-operation variables below override it. | `120` |
|
||||
| `HINDSIGHT_API_LLM_REASONING_EFFORT` | Reasoning effort for providers/models that support it (for example `none`, `low`, `medium`, `high`, `xhigh`). Set it and the value is sent as given, whatever your model is called — which is how you control thinking-token volume on a self-hosted reasoning model (vLLM, Ollama, llama.cpp, TGI), where `none` is often the only value that removes the thinking block. Leave it unset and no reasoning parameter is sent at all, so each model runs at its own default effort. Honoured by `openai` and every OpenAI-compatible provider, `openai-responses`, `openai-codex`, `xai`, `llamacpp`, and `litellm`/`litellmrouter` (which translate it per target provider). The native `gemini`/`vertexai`, `anthropic` and `claude-code` providers have no reasoning-effort control and log a warning at startup if you set one — reach those models through `litellm` to apply it. | Unset (model's own default) |
|
||||
| `HINDSIGHT_API_LLM_TEMPERATURE` | Global override for the sampling temperature of internal LLM calls. Set a number in `[0.0, 2.0]`, or `none` (also `default`/`off`/empty) to **omit** the temperature parameter entirely — required for models that reject explicit temperatures, e.g. Azure `gpt-5.5`, which only accepts its default value. Per-operation variables below override this. | Per-operation defaults |
|
||||
| `HINDSIGHT_API_LLM_TEMPERATURE_VERIFICATION` | Temperature for the startup connection check. Number in `[0.0, 2.0]` or `none` to omit. Overrides `HINDSIGHT_API_LLM_TEMPERATURE`. | `0.0` |
|
||||
|
||||
Reference in New Issue
Block a user