fix(heartbeat): bound the penstock verdict cache and close the throttle-family gap (PEN-2462)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Agent dispatch is gated by the Penstock capacity gate, which probes provider capacity and caches the verdict so a fleet does not re-probe on every wake
• PR fix(heartbeat): key the penstock capacity cache per credential (PEN-2385) #1427 (PEN-2385) fixed a real correctness bug there by adding a credential dimension to the cache key, because the probe authenticates as the agent and Penstock answers per credential — a fleet on one endpoint and model had been sharing one verdict
• That fix widened the key space from a small static set of (endpoint, model) pairs to one that is open-ended over the process lifetime: every rotated or retired token leaves an entry, and the gate is constructed once per process, so nothing ever reclaims the tail
• Ally's review of fix(heartbeat): key the penstock capacity cache per credential (PEN-2385) #1427 caught this plus two smaller things, all non-blocking; a fourth asymmetry turned up separately while answering the retry-budget question on PEN-2385
• This pull request bounds the cache by sweeping expired entries at write time, trims an overstated comment, repairs a test that passed by an accidental route, and closes a one-sided fallback in the error-family ladder
• The benefit is that a long-lived process stops accumulating dead cache entries, and three durable records — a comment, a test, and a fallback ladder — now say what they mean

Linked Issues or Issue Description

Refs #1427 — this carries the three non-blocking Suggestions from Ally's consolidated review of that PR at head 8ef55f9f, plus one item found independently.

Filed as a follow-up rather than pushed onto 8ef55f9f: a new head there would have re-armed review/ally-complete and discarded 19 green checks plus a review pinned to that exact SHA, and Ally has repeatedly not returned to a head pushed shortly after a review round. #1427 has since merged, so these land cleanly on top.

Tracked internally as PEN-2462.

What Changed

• Bound the verdict cache (penstock-availability-gate.ts). Sweep entries past cacheTtlMs on set. Behaviour-preserving by construction: the read path already refuses to serve an entry at or past the TTL, so deleting it changes no verdict. Sweeping at set is sufficient on its own and a size cap would be redundant — a set only ever follows a miss, and a miss is followed by a TTL's worth of hits, so survivors are exactly the entries written in the last TTL. Residency is therefore bounded by credentials actively probing, not by every credential ever seen.
  • Worth recording why the obvious fix is the wrong one: cache.delete(key) on the stale-read branch would not bound this. A key only recurs for a credential still in use, which is precisely the part the retired tail is not.
• Documented the dimension that actually became unbounded. The existing Cost: paragraph reasoned carefully about probe volume per TTL but said nothing about resident entries. Split into Cost, in probes: and Cost, in resident entries:.
• Trimmed an overstated comment. The cache-key rationale claimed the key "reaches logs and heap dumps". The logs half is false — every log.info/log.warn payload in the module carries status/provider/model/reason and never the key (verified across all seven call sites). The heap-dump rationale is accurate and stands alone. This comment is the durable record of why the credential is hashed, so an overstated justification invites a future reader to conclude the hashing was unnecessary.
• Fixed a test that passed by a misleading route. mockResolvedValue(new Response(...)) hands the same 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. Switched to the factory idiom already used elsewhere in the file.
• Closed a one-sided backstop (heartbeat.ts). provider_throttled_no_progress was missing from the errorCode fallback ladder in readHeartbeatRunErrorFamily, though it is tagged errorFamily: "rate_limit_exhausted" at the point of write, in the same statement and from the same two booleans that set errorCode.
• Added tests for the cache bound (both directions) and the error-family fallback.

Verification

cd server && vitest run \
  src/__tests__/penstock-availability-gate.test.ts \
  src/__tests__/heartbeat-recoverable-error-family.test.ts
# Test Files 2 passed (2) | Tests 31 passed (31)

Typecheck of the changed module is clean (tsc --noEmit --strict).

Each new test was run against unmodified master to confirm it discriminates — all 5 fail without the fix.

The sharpest of these isolates the sweep specifically. With _cacheSizeForTesting present but the single sweepExpiredCapacityCacheEntries(...) call removed, the growth test fails expected 25 to be 1 — 25 credential rotations, each one full TTL apart, each leaving a permanent entry. That is the unbounded-growth path reproduced directly, with the sweep as the only variable.

The companion test is the guard in the other direction: three credentials probing inside one TTL must all stay resident and none may re-probe. It passes with the sweep both enabled and disabled, which is correct — its job is to catch over-eager eviction, not to detect the sweep.

The error-family tests are written with resultJson: null deliberately, since that is the only state that discriminates: with the tag present, both codes already resolve correctly.

Risks

Low risk.

• The cache sweep cannot change a verdict. It only deletes entries the read path is already guaranteed to refuse (nowMs - fetchedAt >= cacheTtlMs). An entry with a fetchedAt ahead of nowMs yields a negative age and is kept, so a caller-supplied clock that moves backwards costs at most a retained stale entry, never a wrong answer.
• The sweep is O(n) per write, but it runs only on a cache miss and n is bounded by live credentials — which is the property the sweep itself establishes.
• The errorCode fallback is behaviour-preserving on every row that exists today: the two fields are co-written from the same booleans (heartbeat.ts :28394 and :28552), and the persisted errorFamily tag is consulted first. It is a pure backstop for a future path that sets the code alone, or a row whose resultJson is dropped downstream.
• _cacheSizeForTesting is declared optional on the interface so the hand-rolled gate fakes in the heartbeat suites do not have to implement it.
• No migration, no schema change, no API surface change.

Model Used

Claude Opus 5 (claude-opus-5), 1M-context variant, extended thinking, via Claude Code with tool use.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — N/A, no UI surface
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending first run on this head
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: PEN-2385
🔗 Paperclip issue: PEN-2462

[allyblockcast]

🔗 Paperclip issue: PEN-2385
🔗 Paperclip issue: PEN-2462

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 573bce0

The change is focused and the cache-boundary and error-family fallback are covered by targeted tests.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Expired cache entries are reclaimed without changing the existing TTL read semantics.
• The cache tests cover both reclamation and retention within the TTL window.
• The error-family fallback preserves persisted metadata precedence and adds a regression path for the code-only case.

Recommended Action

1. No Critical or Important issues identified; this formal self-review is recorded as a comment because the PR is App-authored.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8bb6846

No Critical or Important issues identified. The cache sweep uses the same expiration boundary as cache reads and runs before replacing a miss, preserving live entries while reclaiming retired credentials. The fallback mapping preserves persisted metadata precedence and covers the code-only recovery path with targeted tests.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• Exact-head cache tests cover both expired-entry reclamation and retention inside the TTL window.
• The cache-key rationale now accurately distinguishes heap exposure from logging behavior.
• The provider_throttled_no_progress fallback is behaviorally aligned with its write-time rate_limit_exhausted family and has regression coverage.

Recommended Action

1. No Critical or Important issues identified; this formal self-review is recorded as a comment because the PR is App-authored.