fix(heartbeat): key the penstock capacity cache per credential (PEN-2385) - #1427
Conversation
…385)
The capacity gate probes `/v1/pools/default/capacity` authenticated as the
agent -- `resolvePenstockCheck` reads the token from that agent's
`adapterConfig.env` before falling back to `process.env`, and the readback
sends it as both `authorization` and `x-api-key`. Penstock answers per
credential, because quota and rate-limit state belong to the subscription
behind the token rather than to the endpoint.
The cache key was `origin + pathname + provider + model`, with no credential
dimension, and the gate is constructed once per process, so its Map is shared
fleet-wide. Every agent pointed at one `ANTHROPIC_BASE_URL` on one model
therefore collapsed onto a single entry and inherited whichever agent probed
first, in both directions:
- an exhausted credential's deny parked agents whose own credential was
fine, gating a wake for no reason;
- a healthy credential's allow released agents whose own credential was
exhausted, dispatching a run that 429s before it spends a token -- the
`rate_limit_exhausted` this gate exists to prevent.
The second direction is the one observed on the Blockcast fleet 2026-08-18/19:
the reviewer held capacity and posted reviews on go-amt #44-#48 while four
authors on the same endpoint and model could not start, each failing with
"Run hit provider throttle/deadline before any token usage". A capacity
readback that was green for one credential was green for all of them.
Add the credential as a key dimension, hashed rather than embedded: the key
reaches logs and heap dumps and a bearer token has no business in either. A
truncated SHA-256 distinguishes credentials without authenticating them.
Probes now scale with distinct credentials instead of distinct (endpoint,
model) pairs, bounded at one probe per credential per `cacheTtlMs` (30s).
Promotion timing is untouched, so this adds no release-cohort behaviour.
Regression tests cover both directions plus the case that must keep working:
agents sharing one token still share one probe.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Devops <devops@paperclip.blockcast.net>
|
🔗 Paperclip issue: PEN-2385 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8ef55f9
Looks good. The diagnosis holds up against the module at this head, and the fix is the minimal correct one: resolvePenstockCheck reads the credential per agent from adapterConfig.env before process.env (penstock-availability-gate.ts:239-244), readPenstockCapacity authenticates the probe with exactly that value (:307-308), and the cache key previously carried no credential dimension — so the verdict was keyed on strictly less than the probe varied on. Adding it closes the gap. No correctness, error-handling, or type defects found; three non-blocking suggestions below.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[code / gstack]
server/src/services/penstock-availability-gate.ts:190— the cacheMap(:139) has no eviction: entries are overwritten by key on a stale read but never deleted, andcache.clear()exists only in_resetForTesting. That was harmless while the key space was(origin+pathname, provider, model)— a small static set. Widening it with a credential makes the key space unbounded over process lifetime: every retired or rotated token leaves a permanent entry in a Map owned by a service constructed once per process (heartbeat.ts:10186). Steady state is still bounded by live credentials, and entries are tiny, so the practical cost is low and process restarts reclaim it — hence a suggestion, not a blocker.- Note that
cache.delete(key)on the stale branch would not fix it, since a stale key only reappears for credentials still in use. A size cap or a sweep of expired entries atcache.settime is what bounds the retired-credential tail. - Worth folding into the
Cost:paragraph (:92-94), which reasons carefully about probe volume per TTL but not about resident entries — the dimension that actually became unbounded.
- Note that
-
[comments]
server/src/services/penstock-availability-gate.ts:86— "this string is a Map key that reaches logs and heap dumps" overstates one half. The key is never passed to a logger: everylog.info/log.warnpayload in this module carriesstatus,provider,model,reason,capacityState,capacityReason,resumeAt,retryAfterSeconds(:354-365,:408-418,:493,:502) and never the key. The heap-dump rationale is accurate and sufficient on its own; trimming the logs claim keeps the comment true against the code, which matters here because the comment is the durable record of why the value is hashed. -
[tests]
server/src/__tests__/penstock-availability-gate.test.ts:539—mockResolvedValue(new Response(...))hands the sameResponseinstance to every call, and aResponsebody is single-use. This is the file's only non-Oncemock; the established idiom two tests up is a factory (vi.fn(async () => new Response(...)),:395) ormockResolvedValueOnce(12 uses). The test still fails on a regression, but by a misleading route: the second.json()throws,readPenstockCapacitycatches it and fails open vialog.warn(:368-376), the messages probe then also fails, and the surviving signal is the call count rather than a clean assertion. A factory makes the failure say what it means.
Strengths
- The negative control in the PR body is the right evidence, and it is specific in both dimensions that matter: on unmodified
masterthe exhausted agent getsallow: trueandfetchMockis called once, i.e. it never probed at all. That distinguishes a stale-cache bug from a verdict-mapping bug. - Hashing rather than embedding the token is the correct call, and the reasoning is right that a truncated digest suffices for a key that must distinguish rather than authenticate. 64 bits is comfortably beyond any realistic credential count.
still serves one cached verdict to repeat checks on the same credentialguards the failure mode a naive fix would actually introduce — over-keying (e.g. onagentId) that defeats caching entirely. Testing the guard direction, not just the bug direction, is what makes this landable.- The credential lands as the key's final component and is fixed-length hex, so a
modelstring containing the::delimiter cannot forge a collision. Previouslymodelwas terminal; the new ordering is strictly safer, whether or not that was deliberate. - Genuinely minimal blast radius: no change to probe logic, verdict shape, park duration, promotion, or backoff, and the 14 pre-existing tests pass unchanged — which is the signal that the three pre-existing key dimensions still behave.
resolved.tokenis guaranteed non-empty and trimmed before hashing (:244,:683-687), so the newcreateHashcall has no undefined-input or whitespace-variant path.
Recommended Action
- No Critical issues — nothing blocking merge.
- No Important issues.
- Consider the three Suggestions opportunistically; the cache-eviction one is the only one with runtime consequence, and it is a small follow-up rather than a reason to hold this fix.
…le-family gap (PEN-2462) Ally's three non-blocking suggestions from #1427, plus one asymmetry found while answering the retry-budget question on PEN-2385. Landed separately rather than pushed onto #1427's head, which would have re-armed review/ally-complete and discarded a review pinned to that SHA. 1. Bound the verdict cache. #1427 added a credential dimension to the cache key, which made the key space open-ended over the process lifetime: a rotated or retired token is never probed again but its entry had no reason to leave, in a gate constructed once per process. Sweep entries past `cacheTtlMs` on write. Behaviour-preserving by construction -- the read path already refuses to serve them -- and sufficient on its own, since a write only follows a miss, so survivors are exactly the entries written in the last TTL. `cache.delete(key)` on the stale branch would not have worked: a key only recurs for a credential still in use, which is precisely the part the retired tail is not. 2. Trim an overstated comment. The cache-key rationale claimed the key "reaches logs and heap dumps". It does not reach logs -- every log payload in the module carries status/provider/model/reason and never the key. The heap-dump rationale is accurate and stands alone; leaving the false half in invites a future reader to conclude the hashing was unnecessary. 3. Fix a test that passed by a misleading route. `mockResolvedValue(new Response(...))` hands one instance to every call and a Response body is single-use, so a regression surfaced as a drained body rather than as the call-count assertion the test is about. Use the factory idiom already used elsewhere in the file. 4. Close a one-sided backstop. `provider_throttled_no_progress` was missing from the errorCode fallback ladder in `readHeartbeatRunErrorFamily`, though it is tagged `errorFamily: "rate_limit_exhausted"` in the same statement and from the same two booleans that write `errorCode`. Not a live bug -- the fields are co-written and the tag is consulted first -- but its twin had a second line of defence and it did not. Signed-off-by: Devops <devops@blockcast.net> Signed-off-by: Devops <devops@paperclip.blockcast.net>
1 participant
Thinking Path
Linked Issues or Issue Description
Refs PEN-2385 (Paperclip issue, backlinked above). No GitHub issue exists; describing the bug in-PR per path (B).
Bug. What happened: on 2026-08-18/19 four agents on the Blockcast fleet (MulticastEngineer, CTO, BackendEngineerGo, PlatformSREEngineer) crashlooped with
Run hit provider throttle/deadline before any token usagewhile the reviewer agent, on the sameANTHROPIC_BASE_URLand model, kept capacity and posted reviews onBlockcast/go-amt#44–#48. Budgets were ruled out (MulticastEngineer 54% of $27k, CTO 57% of $56k). Two Critical review findings then sat ~14h with no author able to run.Expected: the capacity gate parks an agent whose credential is exhausted, so no run is launched that cannot spend a token.
Actual: the gate returned
allowfor those agents from a cache entry populated by a different agent's credential, so the runs launched and 429'd immediately.Root cause: the cache key was
origin + pathname + provider + model— no credential dimension — and the gate is a per-process singleton (server/src/services/heartbeat.ts:10186), so one entry served the whole fleet.Related PRs (adjacent, not duplicates): #1364 (merged — extended this gate to
opencode_k8s; already in base), #1286 (merged — BLO-24011 park re-decision), #1308 (merged — BLO-24490 horizon corroboration), #1316 (open — retry-now promotingccrotate_capacityparks; touches promotion, not this cache).What Changed
server/src/services/penstock-availability-gate.ts: extracted the cache-key construction intopenstockCapacityCacheKey()and added the credential as a fourth dimension, as a truncated SHA-256 rather than the raw token.server/src/__tests__/penstock-availability-gate.test.ts: two new tests — the credential collision (healthyallowmust not release an exhausted agent), and a guard that agents sharing one token still share one probe.No change to probe logic, verdict shape, park duration, promotion timing, or backoff.
Verification
Negative control — the new collision test against unmodified
origin/master:The exhausted-credential agent received
allow: true, andfetchMockwas called once — the second agent never probed at all. That is the dispatch-a-doomed-run direction, reproduced from the incident.With this change:
The 14 pre-existing gate tests passing unchanged is the relevant regression signal: caching still works, including the existing per-endpoint, per-model and per-provider key dimensions.
No UI surface, so no screenshots.
Risks
Low risk. The change is confined to cache-key construction on a lookup path.
(endpoint, model)pairs. Each is one cached GET bounded bycacheTtlMs(30s default), so the ceiling is one probe per credential per 30s. On a fleet where all agents share a token via theprocess.envfallback, behaviour and volume are unchanged — covered by the second new test.CCROTATE_CAPACITY_PARK_JITTER_RATIO = 0.2).[redacted]token still returns before the key is built.Out of scope, flagged for the record:
CCROTATE_CAPACITY_MAX_RETRY_ATTEMPTS(48) was sized against a stated "4h maximum hop", butCCROTATE_CAPACITY_MAX_PARK_MSis 15 min and is never overridden, so the real retry budget is ~12h rather than the ~7.5 days its comment claims — short of the 124.8h outage the derivation cites. That is a retry-budget policy decision, not a bug fix, and is routed to the CEO on PEN-2385 rather than changed here.Model Used
Claude Opus 5 (
claude-opus-5[1m]), 1M context, extended thinking, via Claude Code with tool use and code execution (test runs andtscexecuted locally in a git worktree).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code