Preserve delegation work through any upstream outage, not just budget
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 52s
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 52s
An exhausted budget was a special case of something more general: the work is not at fault. A network blip, a timeout, a 5xx from the proxy, or a rate limit that did not clear all leave a delegation branch that may be perfectly good, and discarding it would throw away work for a reason that has nothing to do with the work. UpstreamUnavailable now names that family, with BudgetExhausted a member of it — distinguishable where the difference matters, since a budget will not clear within the hour while a network blip will. A delegation stopped by any of them is "interrupted": branch kept, intent left pending, and A3's resume rule picks it up. The benefit of the doubt stops at the server's own door. A 4xx that is not a rate limit means the request itself was wrong, and preserving branches on the strength of one would hide a real bug — so those still fail and clean up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
8cfb45aed3
commit
8cfa116aa1
4 files changed
+112
-17
No files matched your search
@@ -36,7 +36,7 @@ from nisse_dev.canonical import (
|
||||
ToolResult,
|
||||
)
|
||||
from nisse_dev.git import GitError, GitRepo
|
||||
from nisse_dev.llm import BudgetExhausted, LLMClient, LLMError
|
||||
from nisse_dev.llm import LLMClient, LLMError, UpstreamUnavailable
|
||||
from nisse_dev.reconcile import Reconciler
|
||||
from nisse_dev.state import Intent, StateStore
|
||||
from nisse_dev.tools import EXECUTOR_TOOLS, MUTATING_TOOLS, Workspace
|
||||
@@ -330,10 +330,10 @@ class Delegation:
|
||||
model=self._model,
|
||||
system=system,
|
||||
)
|
||||
except BudgetExhausted as exc:
|
||||
except UpstreamUnavailable as exc:
|
||||
return DelegationResult(
|
||||
"interrupted",
|
||||
f"external models are unavailable: {exc}",
|
||||
f"stopped by something upstream, not by the work: {exc}",
|
||||
branch=branch,
|
||||
artifacts=sorted(touched),
|
||||
tokens=spent,
|
||||
|
||||
@@ -54,12 +54,24 @@ def max_output_tokens(env: Mapping[str, str] = os.environ) -> int:
|
||||
return int(raw) if raw else DEFAULT_MAX_OUTPUT_TOKENS
|
||||
|
||||
|
||||
class BudgetExhausted(LLMError):
|
||||
class UpstreamUnavailable(LLMError):
|
||||
"""The proxy or provider could not be reached, or refused for reasons
|
||||
unrelated to what was asked: a network blip, a timeout, a 5xx, a rate
|
||||
limit that did not clear, an exhausted budget.
|
||||
|
||||
Worth distinguishing because the *work* is not at fault. A delegation
|
||||
stopped by one of these has a branch that may be perfectly good, so it is
|
||||
preserved and resumed rather than discarded — unlike a delegate that made
|
||||
a mess, which is cleaned up.
|
||||
"""
|
||||
|
||||
|
||||
class BudgetExhausted(UpstreamUnavailable):
|
||||
"""The proxy refused because a spend limit is gone.
|
||||
|
||||
Distinct from a rate limit, which clears in a minute: an exhausted daily
|
||||
budget takes external models away until it resets, so retrying wastes
|
||||
time and the operator needs telling rather than the loop quietly failing.
|
||||
Distinct from the rest of its family because it will not clear on its
|
||||
own within the hour: an exhausted daily budget takes external models away
|
||||
until it resets, so retrying wastes time and the operator needs telling.
|
||||
A5 keeps the budget itself at the proxy; recognising its refusal is ours.
|
||||
"""
|
||||
|
||||
@@ -174,10 +186,7 @@ class LiteLLMClient:
|
||||
self._http, "/chat/completions", body, self._sleep
|
||||
)
|
||||
if response.status_code != 200:
|
||||
if _looks_like_budget_refusal(response.text):
|
||||
raise BudgetExhausted(
|
||||
f"the proxy refused on budget grounds: {response.text[:300]}"
|
||||
)
|
||||
_raise_for_upstream(response)
|
||||
raise LLMError(
|
||||
f"LLM request returned {response.status_code}: {response.text[:500]}"
|
||||
)
|
||||
@@ -294,10 +303,7 @@ class AnthropicPassthroughClient:
|
||||
self._http, "/v1/messages", body, self._sleep
|
||||
)
|
||||
if response.status_code != 200:
|
||||
if _looks_like_budget_refusal(response.text):
|
||||
raise BudgetExhausted(
|
||||
f"the proxy refused on budget grounds: {response.text[:300]}"
|
||||
)
|
||||
_raise_for_upstream(response)
|
||||
raise LLMError(
|
||||
f"passthrough returned {response.status_code}: {response.text[:500]}"
|
||||
)
|
||||
@@ -353,7 +359,9 @@ def _post_with_rate_limit_retry(
|
||||
try:
|
||||
response = http.post(path, json=body)
|
||||
except httpx.HTTPError as exc:
|
||||
raise LLMError(f"LLM request failed: {exc}") from exc
|
||||
# Could not reach the proxy at all: nothing about the request was
|
||||
# wrong, so callers holding work in progress should keep it.
|
||||
raise UpstreamUnavailable(f"LLM request failed: {exc}") from exc
|
||||
if response.status_code != RATE_LIMIT_STATUS or attempt == 2:
|
||||
return response
|
||||
sleep(RATE_LIMIT_RETRY_S)
|
||||
@@ -383,3 +391,20 @@ def select_client(
|
||||
which is the opposite of the vendor fluidity A5 requires.
|
||||
"""
|
||||
return passthrough if has_native_adapter(model) else normalized
|
||||
|
||||
|
||||
def _raise_for_upstream(response: httpx.Response) -> None:
|
||||
"""Classify a non-200 as upstream trouble where it plainly is.
|
||||
|
||||
A 4xx that is not a rate limit means the request itself was wrong, and
|
||||
preserving work on the strength of one would hide a real bug — so the
|
||||
benefit of the doubt stops at the server's own door.
|
||||
"""
|
||||
if _looks_like_budget_refusal(response.text):
|
||||
raise BudgetExhausted(
|
||||
f"the proxy refused on budget grounds: {response.text[:300]}"
|
||||
)
|
||||
if response.status_code >= 500 or response.status_code == RATE_LIMIT_STATUS:
|
||||
raise UpstreamUnavailable(
|
||||
f"upstream returned {response.status_code}: {response.text[:300]}"
|
||||
)
|
||||
@@ -21,7 +21,7 @@ from nisse_dev.delegation import (
|
||||
delegation_reconciler,
|
||||
)
|
||||
from nisse_dev.git import GitRepo
|
||||
from nisse_dev.llm import BudgetExhausted, LLMError
|
||||
from nisse_dev.llm import BudgetExhausted, LLMError, UpstreamUnavailable
|
||||
from nisse_dev.oplog import OpLog
|
||||
from nisse_dev.state import StateStore
|
||||
from nisse_dev.tools import Workspace
|
||||
@@ -614,3 +614,36 @@ def test_an_ordinary_failure_still_discards(
|
||||
assert not result.interrupted
|
||||
assert repo.current_branch() == "main"
|
||||
assert store.pending_intents() == []
|
||||
|
||||
|
||||
def test_a_network_outage_also_preserves_the_work(
|
||||
store: StateStore, ws: Workspace, repo: GitRepo
|
||||
) -> None:
|
||||
# Same reasoning as an exhausted budget: the branch may be fine, and the
|
||||
# outage says nothing about the quality of the work on it.
|
||||
class FlakyClient(ScriptedClient):
|
||||
def complete(self, *args: Any, **kwargs: Any):
|
||||
if self.calls:
|
||||
raise UpstreamUnavailable("LLM request failed: connection refused")
|
||||
return super().complete(*args, **kwargs)
|
||||
|
||||
client = FlakyClient([[call("write_file", path="progress.py", content="real")]])
|
||||
result = build(client, store, ws, repo).run(brief="b", branch="d")
|
||||
|
||||
assert result.status == "interrupted"
|
||||
assert (repo.root / "progress.py").exists()
|
||||
assert [i.kind for i in store.pending_intents()] == [DELEGATION_INTENT]
|
||||
|
||||
|
||||
def test_a_bad_request_is_not_given_the_benefit_of_the_doubt(
|
||||
store: StateStore, ws: Workspace, repo: GitRepo
|
||||
) -> None:
|
||||
# A 4xx that is not a rate limit means we sent something wrong; treating
|
||||
# that as an outage would preserve branches and hide a real bug.
|
||||
class BadRequestClient(ScriptedClient):
|
||||
def complete(self, *args: Any, **kwargs: Any):
|
||||
raise LLMError("LLM request returned 400: malformed tool schema")
|
||||
|
||||
result = build(BadRequestClient([]), store, ws, repo).run(brief="b", branch="d")
|
||||
assert result.status == "failed"
|
||||
assert not result.interrupted
|
||||
@@ -15,10 +15,12 @@ from nisse_dev.llm import (
|
||||
DEFAULT_MAX_OUTPUT_TOKENS,
|
||||
RATE_LIMIT_RETRY_S,
|
||||
AnthropicPassthroughClient,
|
||||
BudgetExhausted,
|
||||
LiteLLMClient,
|
||||
LLMError,
|
||||
RoleMap,
|
||||
TruncatedResponse,
|
||||
UpstreamUnavailable,
|
||||
has_native_adapter,
|
||||
max_output_tokens,
|
||||
select_client,
|
||||
@@ -343,3 +345,38 @@ def test_truncation_is_an_llm_error_so_callers_already_handle_it() -> None:
|
||||
# The planner retries, the executor and delegation gate. None of them
|
||||
# need to learn a new exception type.
|
||||
assert issubclass(TruncatedResponse, LLMError)
|
||||
|
||||
|
||||
def test_a_5xx_is_upstream_trouble() -> None:
|
||||
client = make_client(lambda request: httpx.Response(503, text="bad gateway"))
|
||||
with pytest.raises(UpstreamUnavailable, match="503"):
|
||||
client.complete(EVENTS, tools=None, model="local-fast")
|
||||
|
||||
|
||||
def test_a_connection_failure_is_upstream_trouble() -> None:
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
raise httpx.ConnectError("refused")
|
||||
|
||||
with pytest.raises(UpstreamUnavailable, match="request failed"):
|
||||
make_client(handler).complete(EVENTS, tools=None, model="local-fast")
|
||||
|
||||
|
||||
def test_a_persistent_rate_limit_is_upstream_trouble() -> None:
|
||||
# It cleared for neither attempt, so the caller should keep its work
|
||||
# rather than discard it.
|
||||
client = make_client(lambda request: httpx.Response(429, text="slow down"))
|
||||
with pytest.raises(UpstreamUnavailable):
|
||||
client.complete(EVENTS, tools=None, model="local-fast")
|
||||
|
||||
|
||||
def test_a_bad_request_is_not_upstream_trouble() -> None:
|
||||
client = make_client(lambda request: httpx.Response(400, text="bad schema"))
|
||||
with pytest.raises(LLMError) as excinfo:
|
||||
client.complete(EVENTS, tools=None, model="local-fast")
|
||||
assert not isinstance(excinfo.value, UpstreamUnavailable)
|
||||
|
||||
|
||||
def test_budget_refusal_is_a_kind_of_upstream_trouble() -> None:
|
||||
# So one catch covers the family, but it can still be told apart where
|
||||
# the difference matters — it will not clear within the hour.
|
||||
assert issubclass(BudgetExhausted, UpstreamUnavailable)
|
||||
Reference in new issue
Block a user