fix: handle string config values in enable_1m_context checks - #42
Open
Sam Schillace (ramparte) wants to merge 4 commits into
Open
fix: handle string config values in enable_1m_context checks#42Sam Schillace (ramparte) wants to merge 4 commits into
Sam Schillace (ramparte) wants to merge 4 commits into
Conversation
added 4 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"false"was treated as truthy_is_1m_enabled()helper with proper string normalizationllm:responseevent key names to match Usage model field namesRoot Cause
The
enable_1m_contextconfig value arrives from YAML as a string (e.g.,"false") when set insettings.yaml. Python's truthiness evaluation treats any non-empty string as truthy, causingget_info()andlist_models()to report a 1M context window even when the user explicitly setenable_1m_context: 'false'to disable it.Meanwhile, the
__init__constructor already handled this correctly with explicit string normalization, creating a dangerous mismatch:This inconsistency between what the API actually uses and what the context engine believes it can use caused silent failures—no errors, just incorrect behavior.
Changes
Extracted
_is_1m_enabled()helper — Centralizes config normalization logic with explicit handling for boolean and string values ("true","false",True,False, etc.)Updated
__init__— Replaced inline truthiness check with call to_is_1m_enabled()for consistencyUpdated
get_info()— Replaced raw truthiness check with call to_is_1m_enabled()Updated
list_models()— Replaced raw truthiness check with call to_is_1m_enabled()Additionally, this PR fixes zero-usage reporting in
llm:responsesession event logs by correcting the event key names from"input"/"output"to match the Usage model field names"input_tokens"/"output_tokens"expected by downstream consumers.Immediate Workaround
If you cannot wait for this fix, you can work around the bug by using a boolean value instead of a string in
settings.yaml: