alishahryar1--free-claude-code
af12e7b2bb
## Problem Concurrent transient failures could start independent retry, replay, continuation, and repair loops while holding provider concurrency slots. This multiplied upstream attempts and could delay or strand terminal errors under fan-out. ## Changes | Before | After | | --- | --- | | Retry paths owned separate attempt budgets. | One logical-execution session caps all upstream work at five attempts. | | Concurrent failures backed off independently. | One provider-owned recovery episode elects a single half-open probe while followers coalesce. | | Backoff occupied stream concurrency. | Concurrency is held only while an upstream operation or stream is active. | | Provider catalog calls and stream creation used separate admission paths. | Every upstream operation uses one provider-generation admission controller. | | Cancellation could leave recovery ownership or follower state unresolved. | Cancellation releases permits, transfers probe ownership, and unregisters waiting followers. | | Late in-flight failures could cross an exhausted episode boundary. | Every coalesced execution retains that generation's terminal outcome. | | Replay tests allowed loose lifecycle assertions. | Exact SSE contracts prove retries and continuations emit one unduplicated response. | | Recovery wrappers could mask final diagnostics. | Final responses and traces retain the raw provider failure and request ID. | <!-- greptile_comment --> <details open><summary><h3>Greptile Summary</h3></summary> This PR coordinates provider recovery and retry work under concurrent load. The main changes are: - One five-attempt budget for each logical execution. - Provider-wide recovery episodes with one elected probe. - Shared admission for streams, catalog calls, rate limits, and concurrency. - Concurrency permits held only during active upstream work. - Cancellation-safe probe ownership and preserved final diagnostics. </details> <h3>Confidence Score: 5/5</h3> This looks safe to merge. No blocking issues found in the changed code. <details><summary><h3><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="20" align="absmiddle"></a> T-Rex Logs</h3></summary> **What T-Rex did** - Reviewed the coordinated-recovery-01-before.log to understand how the exhausted generation outcome was not preserved in a late in-flight failure. - Reviewed the coordinated-recovery-02-after.log to confirm that the updated implementation preserves the exhausted generation outcome for the same focused contract set. - Validated that the provider-admission-full-current.log shows the complete requested test file passed under Python 3.14 with uv run pytest -n 0. <a href="https://app.greptile.com/trex/runs/15050270/artifacts"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifactsDark.svg?v=4"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"><img alt="View all artifacts" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"></picture></a> <sub><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="14" align="absmiddle"></a> Ran code and verified through T-Rex</sub> </details> <details open><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | src/free_claude_code/providers/admission.py | Adds shared admission, retry budgets, recovery episodes, probe election, and cancellation handling. | | src/free_claude_code/providers/openai_chat/provider.py | Moves stream creation, replay, continuation, and repair onto one admission-owned retry session. | | src/free_claude_code/providers/stream_recovery.py | Selects replay, continuation, repair, or final failure using the remaining shared attempt budget. | | src/free_claude_code/providers/failure_policy.py | Adds recovery exhaustion handling and preserves the underlying provider error for final classification. | | src/free_claude_code/providers/runtime/factory.py | Creates one admission controller per provider generation and passes it through provider factories. | </details> <details open><summary><h3>Sequence Diagram</h3></summary> <a href="#gh-light-mode-only"> ```mermaid %%{init: {'theme': 'neutral'}}%% sequenceDiagram participant E as Execution participant A as Admission controller participant P as Provider participant F as Concurrent follower E->>A: Open attempt A->>P: Send upstream request P-->>E: Retryable failure E->>A: Open recovery episode F->>A: Request admission A-->>F: Coalesce and wait E->>A: Claim probe A->>P: Send half-open probe alt Probe succeeds P-->>E: Valid response E->>A: Close recovery episode A-->>F: Release waiter else Probe fails P-->>E: Retryable failure E->>A: Schedule next probe or finalize error end ``` </a> <a href="#gh-dark-mode-only"> ```mermaid %%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant E as Execution participant A as Admission controller participant P as Provider participant F as Concurrent follower E->>A: Open attempt A->>P: Send upstream request P-->>E: Retryable failure E->>A: Open recovery episode F->>A: Request admission A-->>F: Coalesce and wait E->>A: Claim probe A->>P: Send half-open probe alt Probe succeeds P-->>E: Valid response E->>A: Close recovery episode A-->>F: Release waiter else Probe fails P-->>E: Retryable failure E->>A: Schedule next probe or finalize error end ``` </a> </details> <sub>Reviews (2): Last reviewed commit: ["Harden coordinated retry lifecycle invar..."](https://github.com/alishahryar1/free-claude-code/commit/2e871c8649d148b5eb71d21f80bf870ae2d11708) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=45554917)</sub> <!-- /greptile_comment -->
265 行
8.0 KiB
Python
265 行
8.0 KiB
Python
"""Provider streams raise canonical failures after closing committed blocks."""
|
|
|
|
from collections import deque
|
|
from collections.abc import AsyncIterator
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from free_claude_code.config.nim import NimSettings
|
|
from free_claude_code.core.anthropic.stream_contracts import parse_sse_text
|
|
from free_claude_code.core.async_iterators import AsyncCloseable
|
|
from free_claude_code.core.failures import ExecutionFailure, FailureKind
|
|
from free_claude_code.providers.base import ProviderConfig
|
|
from free_claude_code.providers.http import close_provider_stream
|
|
from free_claude_code.providers.nvidia_nim import NvidiaNimProvider
|
|
from tests.providers.request_factory import make_messages_request
|
|
from tests.providers.support import immediate_admission
|
|
|
|
|
|
class _FailingStream:
|
|
def __init__(
|
|
self,
|
|
chunks: list[object],
|
|
error: Exception | None,
|
|
*,
|
|
close_error: Exception | None = None,
|
|
) -> None:
|
|
self._chunks = chunks
|
|
self._error = error
|
|
self._close_error = close_error
|
|
self.close_calls = 0
|
|
|
|
def __aiter__(self) -> AsyncIterator[object]:
|
|
return self._iterate()
|
|
|
|
async def _iterate(self) -> AsyncIterator[object]:
|
|
for chunk in self._chunks:
|
|
yield chunk
|
|
if self._error is not None:
|
|
raise self._error
|
|
|
|
async def aclose(self) -> None:
|
|
self.close_calls += 1
|
|
if self._close_error is not None:
|
|
raise self._close_error
|
|
|
|
|
|
def _chunk(*, content: str | None = None, finish_reason: str | None = None) -> object:
|
|
delta = MagicMock(content=content, tool_calls=None, reasoning_content=None)
|
|
choice = MagicMock(delta=delta, finish_reason=finish_reason)
|
|
return MagicMock(choices=[choice], usage=None)
|
|
|
|
|
|
def _provider() -> NvidiaNimProvider:
|
|
return NvidiaNimProvider(
|
|
ProviderConfig(
|
|
api_key="test_key",
|
|
base_url="https://test.api.nvidia.com/v1",
|
|
rate_limit=10,
|
|
rate_window=60,
|
|
),
|
|
nim_settings=NimSettings(),
|
|
admission=immediate_admission(),
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_committed_provider_failure_closes_block_then_raises_canonical_value() -> (
|
|
None
|
|
):
|
|
provider = _provider()
|
|
request = make_messages_request(
|
|
"test-model",
|
|
messages=[],
|
|
max_tokens=32,
|
|
)
|
|
# Crossing the recovery buffer's byte threshold makes the content block
|
|
# downstream-visible before the failure, so its close prelude must escape.
|
|
stream = _FailingStream(
|
|
[_chunk(content="x" * 65_536)],
|
|
RuntimeError("connection lost after commit"),
|
|
)
|
|
emitted: deque[str] = deque()
|
|
|
|
with (
|
|
patch.object(
|
|
provider._client.chat.completions,
|
|
"create",
|
|
new_callable=AsyncMock,
|
|
return_value=stream,
|
|
),
|
|
pytest.raises(ExecutionFailure) as exc_info,
|
|
):
|
|
async for event in provider.stream_response(
|
|
request,
|
|
request_id="req_committed_failure",
|
|
):
|
|
emitted.append(event)
|
|
|
|
events = parse_sse_text("".join(emitted))
|
|
assert [event.event for event in events][-1] == "content_block_stop"
|
|
assert not any(event.event in {"error", "message_stop"} for event in events)
|
|
assert exc_info.value.kind is FailureKind.UPSTREAM
|
|
assert exc_info.value.status_code == 502
|
|
assert exc_info.value.retryable is False
|
|
assert "connection lost after commit" in exc_info.value.message
|
|
assert "Request ID: req_committed_failure" in exc_info.value.message
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_openai_stream_close_failure_cannot_mask_execution_failure() -> None:
|
|
provider = _provider()
|
|
request = make_messages_request(
|
|
"test-model",
|
|
messages=[],
|
|
max_tokens=32,
|
|
)
|
|
stream = _FailingStream(
|
|
[],
|
|
RuntimeError("original provider failure"),
|
|
close_error=RuntimeError("cleanup api_key=SECRET"),
|
|
)
|
|
|
|
with (
|
|
patch.object(
|
|
provider._client.chat.completions,
|
|
"create",
|
|
new_callable=AsyncMock,
|
|
return_value=stream,
|
|
),
|
|
patch("free_claude_code.providers.http.trace_event") as trace_event,
|
|
pytest.raises(ExecutionFailure) as exc_info,
|
|
):
|
|
[
|
|
event
|
|
async for event in provider.stream_response(
|
|
request,
|
|
request_id="req_close_failure",
|
|
)
|
|
]
|
|
|
|
assert stream.close_calls == 1
|
|
assert exc_info.value.status_code == 502
|
|
assert "original provider failure" in exc_info.value.message
|
|
assert "cleanup" not in exc_info.value.message
|
|
assert "SECRET" not in exc_info.value.message
|
|
trace_event.assert_called_once_with(
|
|
stage="provider",
|
|
event="provider.stream.close_failed",
|
|
source="provider",
|
|
provider="NIM",
|
|
request_id="req_close_failure",
|
|
close_exc_type="RuntimeError",
|
|
preserved_exc_type="ExecutionFailure",
|
|
)
|
|
assert "SECRET" not in repr(trace_event.call_args)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_stream_close_failure_without_active_error_is_observability_only() -> (
|
|
None
|
|
):
|
|
stream = _FailingStream(
|
|
[],
|
|
RuntimeError("unused"),
|
|
close_error=RuntimeError("normal close failed"),
|
|
)
|
|
|
|
with patch("free_claude_code.providers.http.trace_event") as trace_event:
|
|
await close_provider_stream(
|
|
stream,
|
|
active_error=None,
|
|
provider_name="TEST",
|
|
request_id="req_normal_close",
|
|
)
|
|
|
|
assert stream.close_calls == 1
|
|
trace_event.assert_called_once_with(
|
|
stage="provider",
|
|
event="provider.stream.close_failed",
|
|
source="provider",
|
|
provider="TEST",
|
|
request_id="req_normal_close",
|
|
close_exc_type="RuntimeError",
|
|
preserved_exc_type=None,
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_completed_stream_close_failure_preserves_success_lifecycle() -> None:
|
|
provider = _provider()
|
|
request = make_messages_request(
|
|
"test-model",
|
|
messages=[],
|
|
max_tokens=32,
|
|
)
|
|
stream = _FailingStream(
|
|
[_chunk(content="complete", finish_reason="stop")],
|
|
None,
|
|
close_error=RuntimeError("cleanup api_key=SECRET"),
|
|
)
|
|
|
|
with (
|
|
patch.object(
|
|
provider._client.chat.completions,
|
|
"create",
|
|
new_callable=AsyncMock,
|
|
return_value=stream,
|
|
),
|
|
patch("free_claude_code.providers.http.trace_event") as trace_event,
|
|
):
|
|
emitted = [
|
|
event
|
|
async for event in provider.stream_response(
|
|
request,
|
|
request_id="req_successful_close_failure",
|
|
)
|
|
]
|
|
|
|
events = parse_sse_text("".join(emitted))
|
|
assert events[-1].event == "message_stop"
|
|
assert sum(event.event == "message_stop" for event in events) == 1
|
|
assert not any(event.event == "error" for event in events)
|
|
assert stream.close_calls == 1
|
|
trace_event.assert_called_once_with(
|
|
stage="provider",
|
|
event="provider.stream.close_failed",
|
|
source="provider",
|
|
provider="NIM",
|
|
request_id="req_successful_close_failure",
|
|
close_exc_type="RuntimeError",
|
|
preserved_exc_type=None,
|
|
)
|
|
assert "SECRET" not in repr(trace_event.call_args)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_closing_public_openai_stream_closes_raw_stream_once() -> None:
|
|
provider = _provider()
|
|
request = make_messages_request(
|
|
"test-model",
|
|
messages=[],
|
|
max_tokens=32,
|
|
)
|
|
raw_stream = _FailingStream(
|
|
[
|
|
_chunk(content="x" * 65_536),
|
|
_chunk(content="done", finish_reason="stop"),
|
|
],
|
|
None,
|
|
)
|
|
|
|
with patch.object(
|
|
provider._client.chat.completions,
|
|
"create",
|
|
new_callable=AsyncMock,
|
|
return_value=raw_stream,
|
|
):
|
|
stream = provider.stream_response(request, request_id="req_early_close")
|
|
await anext(stream)
|
|
assert isinstance(stream, AsyncCloseable)
|
|
await stream.aclose()
|
|
|
|
assert raw_stream.close_calls == 1
|