Skip to content

fix(session-state): circuit-break imaging after repeated 429s - #237

Open
parziva-1 wants to merge 1 commit into
teamchong:mainfrom
parziva-1:fix/429-circuit-breaker
Open

fix(session-state): circuit-break imaging after repeated 429s#237
parziva-1 wants to merge 1 commit into
teamchong:mainfrom
parziva-1:fix/429-circuit-breaker

Conversation

@parziva-1

Copy link
Copy Markdown
Contributor

Observed in production on #234: a session got 429 on 15/15 consecutive requests,
each carrying the exact same image_count/image_bytes as the last (a client
retry of unchanged content). Imaging never got that session past the rate
limit — a 429 never populates a fresh prefix cache, so every retry re-sends
the same imaged bytes and fails the same way. Meanwhile 5 other concurrent
sessions on the same account, with different content, succeeded throughout —
so this wasn't a blanket outage, it was one session stuck resending a payload
that could not get through.

This PR does not fix whatever is rate-limiting the account (I could not
determine that from the proxy side — it's consistent with the account's
shared per-model ITPM budget documented at
platform.claude.com/docs/api/rate-limits,
where only uncached input tokens count, and imaging invalidates the
session's own cached prefix on the first request after any compression
config change). What it fixes: a session stuck behind a limit stops
re-imaging the same doomed bytes once it has failed 3 times in a row, and
falls back to plain text — smaller and more likely to get through, and the
only way that session's own prefix cache gets a chance to come back at all.

Mechanism: noteRateLimitOutcome(sessionKey, status) tracks consecutive 429s
per session, alongside the existing noteCacheOutcome bookkeeping in the same
module. isRateLimitCircuitOpen(sessionKey) reports true at 3 in a row; any
non-429 response clears the counter. transformRequest checks it right after
computing firstUserSha (the same session key noteHistoryRequest/
noteCacheOutcome already use) and falls back to { body, info } unchanged,
same shape as the existing !o.compress early-return just above it.

Verify

$ pnpm vitest run tests/rate-limit-circuit-breaker.test.ts
# before this diff (revert src/core/{session-state,transform,proxy}.ts to see it fail):
TypeError: noteRateLimitOutcome is not a function
 Test Files  1 failed (1)
      Tests  7 failed | 1 passed (8)

# with this diff:
 Test Files  1 passed (1)
      Tests  8 passed (8)

$ pnpm test
 Test Files  1 failed | 72 passed (73)
      Tests  2 failed | 1159 passed (1161)
# The 2 failures are in tests/image-byte-budget.test.ts, unrelated to this
# change, and pre-exist on a clean `main` checkout (same as reported in #236).

$ pnpm typecheck
# clean, no output

I don't have a way to force a real account-level 429 on demand to validate the
full request→429→429→429→fallback→200 loop end-to-end against the live API
without deliberately exhausting a real rate limit, so this is validated at the
level pnpm test can check: the session-state bookkeeping (unit) and
transformRequest's actual compression pipeline falling back to text once the
threshold trips (integration, no mocks on the render path). Flagging that gap
rather than claiming more than the tests show.

  • Rebased on current main
  • pnpm test and pnpm typecheck pass (the 2 pre-existing image-byte-budget.test.ts
    failures are unrelated to this change and reproduce on a clean main checkout — see Verify)
  • No raw prompts, credentials, session files, or machine identifiers

A session retrying an unchanged imaged request that gets 429 on every
attempt (teamchong#234: same image_count/image_bytes each time) never makes
progress by retrying the same way: a 429 never populates a fresh
prefix cache, so the retry re-sends the same doomed imaged bytes and
fails the same way again.

Track consecutive 429s per session (noteRateLimitOutcome, alongside
the existing noteCacheOutcome bookkeeping) and skip imaging once a
session hits 3 in a row (isRateLimitCircuitOpen), falling back to
plain text. Any non-429 response clears the counter.

This does not fix whatever is rate-limiting the account — it stops a
session from re-imaging the same bytes into the same wall, so the
request has a chance to complete, which is also the only way that
session's own prefix cache gets a chance to come back.
@teamchong

Copy link
Copy Markdown
Owner

Thanks for the detailed writeup. Concern with the mechanism:

Text fallback costs more. For the #234 payload, 5 images ≈ 8k tokens; the same 112k chars as text ≈ 28k tokens, all uncached on the first send.
Text passes in #234 because Claude Code's own text prefix is already cached upstream (cache reads are exempt from ITPM).

Any non-429 resets the counter, so it flaps:

img 429, img 429, img 429, text 200, img 429, img 429, img 429, text 200 ...

Every turn pays 3 failed retries plus a full uncached text write, and the alternation busts both prefixes, so once the limit clears the imaged cache has to be rebuilt from zero.

Likely root cause is upstream of this. I diagnosed the same symptom before: slab drift (any 1-char change in system/tools) re-renders every page, the next request needs a full cache write, the account's window can't cover it, and 429s write no cache, so every retry bills the same. #234's own log line hints at it: unknown tag(s) in static slab: cc_automode_session_rules, cc_automode_permissions, severity, category.

@parziva-1

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up — the text-fallback-cost concern is valid and matches what we're seeing locally. Sharing our full local dataset (2026-08-16 → 2026-08-18, pxpipe-proxy 0.13.1) since it now covers three full days under real Claude Code + GSD-skill traffic, including the flapping pattern you described.

Setup

  • pxpipe-proxy 0.13.1, single-user, systemd --user unit (~/.config/systemd/user/pxpipe.service), fronting Claude Code's own traffic (ANTHROPIC_BASE_URL127.0.0.1:47821).
  • Confirmed dist/node.js is the actual runtime bundle loaded by bin/cli.js — it carries its own compiled copy of KNOWN_STATIC_TAGS/DYNAMIC_BLOCK_TAGS. Editing dist/core/transform.js alone is a no-op; anyone patching locally needs to touch dist/node.js (or rebuild from src/core/transform.ts) or the change silently doesn't apply. Worth a line in the docs/CONTRIBUTING.
  • RATE_LIMIT_CIRCUIT_THRESHOLD=3 (from this PR, fix(session-state): circuit-break imaging after repeated 429s #237) applied throughout.
  • The dynamic-tag fix from fix(transform): route cc_automode_*/severity/category into dynamic tail #236 (fix(transform): route cc_automode_*/severity/category into dynamic tail, merged) applied throughout — confirms the cc_automode_* tags you cited in many 429 errors #234 are the same family we hit independently with GSD skill/subagent prompt sections.

What we tried (chronological)

  • 2026-08-16: Installed pxpipe-proxy, wired via a fish function (claude.fish) + pxpipe warp. Found pxpipe warp does not start the main daemon by itself — got 502 pxpipe warp: upstream error: connect ECONNREFUSED 127.0.0.1:47821 until we wrote a dedicated user systemd unit (Restart=always, RestartSteps=5, RestartMaxDelaySec=30, StartLimitIntervalSec=0) to keep the daemon alive across warp restarts/sleep.
  • 2026-08-17: Under load, journal showed 171 [pxpipe warn] unknown tag(s) in static slab: role, required_reading, philosophy, hypothesis_testing, investigation_techniques, verification_patterns, research_vs_reasoning, knowledge_base_protocol, debug_file_protocol, execution_flow, checkpoint_behavior, structured_returns, modes, success_criteria, session_parameters, process warnings/2h — these are static GSD skill/subagent prompt sections that never change per-turn, not per-turn dynamic content.
  • Read splitStaticDynamic: an "unknown" tag is not stripped from the static image regardless of KNOWN_STATIC_TAGS membership — classification only gates (a) the warning log and (b) whether observeStaticTagChurn tracks that tag's content-hash for churn. So we added the 16 GSD tags to KNOWN_STATIC_TAGS locally. Result: warnings 171→0 in the next 2-minute sample, but no measurable change in 429 rate (429/200 ≈ 15/11 before and after, same ballpark) — the bytes going into the cached image page are identical either way. Confirms your point: tag classification is orthogonal to slab drift, it only fixes noise + the churn-detection signal.
  • 2026-08-18: Re-checked assuming dist/core/transform.js was the live file — it wasn't; traced the real load path to dist/node.js (see Setup). Re-ran with the correct file patched.

Evidence (journal, journalctl --user -u pxpipe.service --since "2026-08-16", sanitized)

Metric Count
429 responses 484
200 responses 1034
rate_limit_circuit_open activations (breaker fired) 183

Representative lines (paths/usernames redacted):

[...17:53:09Z] POST /v1/messages → 429 (9371ms tx=24ms up=9347ms fb=9370ms) compressed 36108ch → 2img/187964B
[...17:53:49Z] POST /v1/messages → 200 (46783ms tx=551ms up=46232ms fb=12669ms) compressed 112099ch → 42img/10188853B (tr+1) tokens=2+2019 cache_read=99205
[...17:53:59Z] POST /v1/messages → 200 (47447ms tx=2ms up=47445ms fb=47445ms) skip(compress=false (rate_limit_circuit_open)) tokens=90+9 cache_read=72786

183 breaker activations against 484 total 429s (~38%) is a lot of flapping in 2 days — consistent with the img-429/text-200/img-429 cycle you described, not isolated incidents. We didn't instrument enough to separately count tokens burned per fallback, but given the ratio you quoted for #234 (5 images ≈ 8k tokens vs 112k chars as text ≈ 28k tokens uncached), 183 flips is a real cost, not a rounding error.

What did NOT help

  • Adding unrecognized GSD tags to KNOWN_STATIC_TAGS — silenced the warning entirely but did not move the 429 rate. It's a correctness/noise fix for churn tracking, not a mitigation.

What helped

  • The circuit breaker (fix(session-state): circuit-break imaging after repeated 429s #237) — confirmed live: after 3 consecutive 429s on a compressed slab it flips to compress=false (rate_limit_circuit_open), next call succeeds. Without it, every 429 loop stalled the session at 35–50s first-byte. It's the only thing keeping the proxy usable today, even with the cost concern you raised.

Open questions for the maintainer

  1. Given the counter-reset flapping you described, should the breaker use a decaying/backoff counter (e.g. don't fully reset on one 200) instead of a hard reset-on-success, to avoid the img-429/text-200/img-429 cycle?
  2. Should the breaker shrink the slab (drop older/larger static content) instead of just dropping compression outright, to avoid paying the full uncached-text cost every time it trips?
  3. Should we split this into two PRs — (a) tag classification, mechanical/safe, fixes warning noise + churn signal accuracy, no behavior change; (b) the breaker itself, so threshold/backoff/shrink-vs-drop can be reviewed and tuned independently of (a)?
  4. Long-term default policy for unrecognized top-level tags: invert the current allowlist (treat unknown-as-static-by-default with churn-based promotion to dynamic) vs. keeping an explicit allowlist that has to be updated per prompt-generator (GSD, cc_automode, etc.)?
  5. Would hashing per-block instead of per-page avoid the "any 1-char change re-renders every page" drift you flagged in many 429 errors #234, without the false-cache-hit risk of coarser granularity?
  6. Is there a way to distinguish an account-level rate-limit 429 from a cache-invalidation-triggered 429 from the response alone (headers, error body)? That would let the breaker/backoff behave differently for each case instead of treating all 429s the same.
  7. Should the docs/CONTRIBUTING call out that dist/node.js is the real runtime bundle (not dist/core/transform.js) so local patches land in the right file?
  8. Should pxpipe warp auto-start the main daemon, or should the docs explicitly require a systemd (or equivalent) unit to keep it alive?

Proposal

Split as noted in (3) above: land the tag-classification fix (mechanical, no behavior change) independently, and treat the breaker's tuning (backoff shape, shrink-vs-drop, distinguishing 429 causes) as its own follow-up now that we have three days of real flapping data showing it's not a corner case. Happy to prototype the decaying-counter or per-block-hash approach if you'd rather review a concrete diff than discuss in the abstract.

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.

2 participants