Skip to content

fix: retry transient Anthropic server errors (HTTP 200 with error body) - #43

Open
Sam Schillace (ramparte) wants to merge 6 commits into
microsoft:mainfrom
ramparte:fix/retry-transient-server-errors
Open

fix: retry transient Anthropic server errors (HTTP 200 with error body)#43
Sam Schillace (ramparte) wants to merge 6 commits into
microsoft:mainfrom
ramparte:fix/retry-transient-server-errors

Conversation

@ramparte

Copy link
Copy Markdown
Contributor

Problem

Transient Anthropic API failures kill sessions instead of being retried.

Anthropic returned HTTP 200 with an error body:

```json
{
"type": "error",
"error": {
"type": "api_error",
"message": "Internal server error",
"details": null
}
}
```

The SDK didn't raise an exception (HTTP status was 200), the response wasn't validated, and the session halted immediately with `retryable=false`. The failing request was small (~4.5k tokens) with no tool calls — three earlier, larger requests with tool calls all succeeded fine. Textbook transient server-side failure.

Root Cause

Two gaps in the error handling:

  1. No response validation — after the API call returns, the response is passed directly to `_convert_to_chat_response()` without checking if it's actually a valid message. HTTP 200 with an error body slips through.

  2. Base `APIError` not caught — the exception chain catches specific `APIStatusError` subclasses (`RateLimitError`, `BadRequestError`, etc.) and `APIStatusError` itself, but not the base `APIError`. The SDK can raise `APIConnectionError`, `APITimeoutError`, or other non-status errors that fall through to the generic `Exception` handler.

Fix

Two defensive layers, both feeding into the existing `retry_with_backoff` loop:

1. Response validation (belt-and-suspenders)

After the API call, before returning the response, validate `response.type == "message"`. If not, extract error details and raise `KernelProviderUnavailableError(retryable=True)`.

2. Base `AnthropicAPIError` catch

New exception handler between `AnthropicAPIStatusError` and `asyncio.TimeoutError` that catches the SDK's base `APIError` class. Treats all as transient/retryable.

Testing

  • All 298 existing tests pass
  • Syntax verified via `ast.parse()`
  • Pure additions — no existing code modified or removed
  • Patch tested locally via source override

Sam Schillace added 6 commits March 3, 2026 12:11
## Problem
Cloudflare interposes HTML challenge pages (HTTP 403) in front of
api.anthropic.com when bot-detection triggers (common during concurrent
delegation requests). The provider-anthropic module was treating ALL 403s
as permanent API failures (KernelAccessDeniedError, non-retryable), causing
sessions to die immediately instead of retrying.

## Root Cause
No distinction between two types of 403 responses:
- **Cloudflare 403**: HTML body with challenge, transient bot challenge
- **API 403**: JSON body from Anthropic, permanent access denial

Both hit the same error path and immediately fail.

## Solution
Added _is_cloudflare_challenge() static method that detects Cloudflare
responses by checking:
- response.body is None (HTML parse failure) or 'content-type: text/html'
- Presence of Cloudflare markers ('Just a moment', 'Ray ID', etc.)

Modified _do_complete() 403 handling:
- Cloudflare challenges now raise KernelProviderUnavailableError (retryable)
- API 403s remain KernelAccessDeniedError (non-retryable)
- retry_with_backoff() wrapper automatically handles retryable errors

## Testing
- 10 new tests in test_cloudflare_retry.py covering detection logic and retry integration
- All 257 tests pass (10 new + 247 existing)
- Verified against both HTML and JSON 403 responses

## Impact
Sessions using delegation no longer hard-fail on transient Cloudflare
challenges. Retryable backoff allows time for Cloudflare bot-challenge
window to close.
Add two features to address Cloudflare bot-detection triggered by concurrent
API calls from multiple delegated sessions:

Feature 1 — Process-wide concurrency semaphore
- New module-level _process_semaphore (asyncio.Semaphore) shared across ALL
  AnthropicProvider instances in the same process (parent + child sessions).
- New config key max_concurrent_requests (default: 5). Set to 0 to disable.
- _do_complete_guarded() wraps each _do_complete() attempt with semaphore
  acquire/release so that at most N API calls are in-flight simultaneously.
  Semaphore is released between retry attempts (during backoff sleep) so
  other requests can proceed while a CF-blocked call waits to retry.
- Semaphore is keyed by running event loop so asyncio.run()-based tests each
  get a fresh semaphore without 'Future attached to different loop' errors.

Feature 2 — Concurrency diagnostic logging
- provider:concurrency event emitted before each API call attempt with:
    active_requests, waiting_requests, max_concurrent, process_id, model
  Allows post-mortem correlation in events.jsonl: was request volume the cause?
- provider:cloudflare_challenge event emitted on every Cloudflare 403 with
  the same concurrency fields plus a timestamp.  One event per attempt,
  so retry storms are visible in the event log.

Tests (tests/test_concurrency.py — 24 new tests, 281 total):
- Semaphore config: default=5, override, zero-disables, idempotent, loop-refresh
- Concurrency enforcement: peak in-flight <= limit, limit=1 serializes,
  disabled allows full concurrency, all requests complete
- Event emission: fields present, max_concurrent matches config, process_id
  correct, emitted with semaphore disabled, active_requests >= 1, no crash
  without coordinator
- Cloudflare challenge event: emitted per attempt (incl. retries), all fields
  present, process_id correct, NOT emitted for real JSON-body 403s
After each successful API response, write rate-limit header data to a
shared JSON file at ~/.amplifier/rate-limit-state.json using an atomic
tmp→rename pattern. Before each pre-emptive throttle check, read that
file and merge in the lower remaining values so that sibling processes
(parallel sessions, Docker containers sharing a home dir) can see how
much capacity other processes have consumed.

Changes:
- _write_shared_rate_limit_state(): atomic JSON write with debounce
  (skip if data unchanged) and full try/except safety
- _read_shared_rate_limit_state(): read+merge with 1s cache, 120s
  staleness window, and conservative min() merge for remaining fields
- __init__: three new instance attrs (shared_state_path, last_read ts,
  last_written dict); configurable via rate_limit_state_path config key
- complete(): read hook before throttle check; write hook after
  update_from_headers on every successful response
- tests/test_shared_state.py: 17 new tests covering all 7 spec cases
- tests/test_throttle.py: disable shared state in provider factory so
  unit tests that seed local _RateLimitState directly are not affected
  by a real ~/.amplifier/rate-limit-state.json on the test machine

All 298 tests pass.
The `enable_1m_context` config value arrives as a YAML string (e.g.,
"false") when set in settings.yaml. Python's truthiness treats the
string "false" as truthy, causing get_info() and list_models() to
report a 1M context window even when the user intended to disable it.

Meanwhile, the __init__ constructor already handled this correctly
with explicit string normalization. The mismatch meant:
- API calls used 200K context (no beta header sent — correct)
- context-simple believed 1M was available (incorrect)
- Compaction threshold set at ~745K, never reachable
- Sessions hit the 200K hard limit without any compaction

This fix:
- Extracts _is_1m_enabled() helper with proper string normalization
- Replaces raw truthiness checks in get_info(), list_models(), and __init__
- Fixes llm:response event key names ("input"/"output" → "input_tokens"/"output_tokens")
  to match the Usage model field names expected by consumers
Adds two defensive layers for transient Anthropic API failures that
currently kill sessions instead of being retried:

1. Response validation after API call: checks response.type — if the
   SDK returns a response that isn't type "message" (e.g., HTTP 200
   with an error body like {"type": "error", "error": {"type":
   "api_error", "message": "Internal server error"}}), raises
   KernelProviderUnavailableError(retryable=True) so the retry loop
   handles it.

2. Base AnthropicAPIError handler: catches the SDK's base APIError
   class (not just APIStatusError subclasses). This covers
   APIConnectionError, APITimeoutError, and any future SDK error
   types that don't inherit from APIStatusError. All treated as
   transient/retryable.

Root cause: Session cab8071e died when Anthropic returned HTTP 200
with body {"type": "error", "error": {"type": "api_error", "message":
"Internal server error", "details": null}}. The SDK didn't raise an
exception (HTTP status was 200), the response wasn't validated, and
the session halted with retryable=false. The request that failed was
small (~4.5k tokens) and had no tool calls — three earlier, larger
requests with tool calls all succeeded. Classic transient server-side
failure that should have been retried.

Both new code paths raise KernelProviderUnavailableError(retryable=True),
feeding into the existing retry_with_backoff loop (up to 5 retries
with exponential backoff).

All 298 existing tests pass.
The previous fix (04a73cb) added two guards that turned out to be
unreachable for the actual error path:
- Fix 1 (response type check) guards get_final_message() return,
  but the SDK raises during streaming iteration before returning.
- Fix 2 (base APIError catch) is pre-empted by the more specific
  AnthropicAPIStatusError handler above it.

The real path: SDK raises APIStatusError(status_code=200) when the
streaming response sends an SSE "event: error". This is caught by
the AnthropicAPIStatusError handler where status=200 falls through
to the retryable=False catch-all (200 < 500).

Add `if status < 400` guard before the fallthrough to treat 2xx/3xx
APIStatusErrors as retryable transient failures.

Add 18 regression tests covering HTTP 200 errors, other 2xx/3xx
status codes, and verification that 4xx behavior is unchanged.

Fixes: session 897eb0d4 (HTTP 200 error killed session)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant