PR 9 — Advanced budget hardening - #31
Merged
Merged
Conversation
Delivers the advanced tier of the request budget (IMPLEMENTATION_PLAN.md §13): runtime source allocation, shared-IP reconciliation edge cases, ledger bootstrap edge cases, a threaded stress suite, budget observability, and the configuration validation the boot-time check could not see. Two defects found while reading the shipped code are fixed here. Allowances#clamped could derive a poll allowance of zero at an observed limit below the reserve, which nothing recovers from — only a poll can observe a new limit, and rollover re-derives from the stored one. And ENABLED_LIVE_SOURCE_COUNT could disagree with event_sources silently, under- or over-provisioning the poll allowance with nothing in the logs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Linked issue
Closes #19 —
IMPLEMENTATION_PLAN.md§13, PR 9 — Advanced budget hardening.Problem
The core budget system shipped in PRs 4–7 and is never cut by the descope ladder. What it
deliberately left is the advanced tier: the allowance formula's source count came from an
environment variable that nothing checked against reality, GitHub's
x-ratelimit-usedwasparsed and read by nobody, the ledger's row lock was asserted but never proved under
contention, and several budget conditions were enforced silently.
Reading the shipped code for those items surfaced two defects, both fixed here:
Allowances#clampedcould strand the ledger permanently. With an observedx-ratelimit-limitat or belowRATE_LIMIT_RESERVE,spendableis zero andpoll_allowancebecame zero — so every poll is denied:class_allowance_exhausted,forever. Only a poll can observe a new limit, and rollover re-derives from the stored one,
which can now never change. Reachable from any limit ≤ 8.
ENABLED_LIVE_SOURCE_COUNTandevent_sourcescould disagree in silence. A secondenabled source with the variable left at 1 means twelve poll attempts an hour split
between two sources; the mirror case reserves 24 attempts for one source and takes 12 away
from enrichment. Nothing said so. ADR 0004
had already assigned the fix to this PR by name.
Scope
Included — the issue's six checklist items:
Github::SourceAllocationcounts theenabled, in-service
event_sourcesrows of the running mode and feeds the formula, atwindow initialization and rollover only. Drift against the environment logs
budget.source_allocation_drift.budget_ledger_shared_ip_spec.rb: a co-tenantdraining the window and recovering only at the boundary, an observed limit changing
mid-window, usage this application did not make, a rollover under co-tenant pressure, and
the global block a reserve breach produces.
budget_ledger_bootstrap_spec.rb: partial headers, noresponse at all, a window that opens with nothing left in it, a reset instant already in
the past, and a row created under a different configuration.
spec/stress/budget_ledger_spec.rb— real threads on realsessions over the class allowance, the fairness shares and borrowing, the reserve, a
window rolling under load, and a cold-start bootstrap race.
GithubApiBudget#to_log, plusbudget.co_tenant_usage,budget.co_tenant_pressure,budget.allowances_clamped,budget.window_reset_in_past,config.budget_resolved.Configuration#worst_case_reservations_per_polland the
config.amplificationboot warning.Deliberately not included:
/statusand the inspection endpoints (PR 10); crash-window,multi-poller, container-kill and fixture-mode Docker e2e (PR 11); any schema change, since
§7 fixes
github_api_budget's column list; any new dependency.IMPLEMENTATION_PLAN.mdisfrozen and untouched.
Technical decisions
Recorded in full in
ADR 0009. The four
that carry a tradeoff:
Boot validation still reads no database. ADR 0004 requires it — that is what makes
db:prepare,rails runnerand CI's schema load work before any table exists — so theruntime count lives in
Github::SourceAllocationand is consulted only where ADR 0004already re-derives allowances.
#bootstrap!keeps the configured arithmetic, because itruns ahead of every reservation and the row it writes is overwritten by the first response.
The count runs inside the ledger's row lock and cannot deadlock there: it takes only
ACCESS SHAREonevent_sources, which does not conflict withSourceProvisioner'sSHARE ROW EXCLUSIVE, and no session can hold anevent_sourceslock and then reach forthe ledger row, because
assert_committable!forbids reserving inside a transaction.The clamp floor grants one poll attempt in a window the reserve already forbids. That is
one request an hour, spent only while
remainingis unknown, and it is the price of thestate being recoverable at all. Once
remainingis known,remaining <= reservedenies iton the ledger's own terms.
Co-tenant divergence is reported, never applied. GitHub's
usedcarries no requestclass, so folding it into
poll_usedorenrichment_usedwould have to guess — and a guessbreaks the invariant that the two shares sum to
enrichment_used. Storing it was rejectedtoo: §7 fixes the column list, and an
observed_usedcolumn is not on it. Consequence: "firstdivergence this window" is not knowable, so the INFO line keys on the reserve threshold.
config.amplificationwarns rather than refusing. The existing rejection covers acertainty —
poll_allowance + reservereaching the limit leaves no enrichment capacity everyhour whatever GitHub does. This is a worst case a healthy endpoint never reaches, §7 already
accepts that these are request-attempt allowances, and §10 requires runtime conditions to
degrade rather than crash-loop.
Rejected: keeping the stress specs in the main suite. They open genuine concurrent
PostgreSQL sessions, and
spec/support/advisory_lock_helpers.rbdocuments the assumptionthat breaks — with transactional fixtures the pool is pinned, so every thread shares one
session and both advisory locks are re-entrant. Growing the pool changes that for the whole
process, not just for the group that did it: cleaning up afterwards left the suite green at
most seeds and deadlocking at others (
ExclusiveLockon the request gate against a ledgerrow lock). They now run as their own
rspecinvocation —spec/stress/README.md— wired into both the composetestserviceand CI, so it cannot be forgotten.
Testing performed
1453 examples, 0 failuresin the main suite and10 examples, 0 failuresinspec/stress, verified at seeds 63391, 11111, 22222, 33333 and 77777.rubocop: 205 files,no offenses.
Each new guard was verified by removing it and watching the specs fail, the standard ADR
0004 already applies to the two PostgreSQL
NULLbehaviours:.lockfromreserve!'sSELECT … FOR UPDATELedgerInvariantViolationruntime source countderivation examples failNew coverage: 15 bootstrap edge cases, 18 shared-IP cases, 10 stress cases, 13 allocation
cases, plus
#to_log,.pollableand the amplification arithmetic.Docker verification
The one-shot logs
config.budget_resolvedat boot with the resolved formula, thenbudget.window_initializedon the first poll, and reportsPersisted push events: 4/Budget remaining (core): 59— no co-tenant line, correctly, because observedusedandthe ledger's own count agree at 1.
Documentation updates
New ADR 0009; new
spec/stress/README.md; README gains the two-invocation test note, thereworded
ENABLED_LIVE_SOURCE_COUNTrow, the new log events and a "Sharing the outbound IP"section;
.env.examplegains the amplification and allocation notes.IMPLEMENTATION_PLAN.mdis unchanged, and ADR 0004 is left as written — ADRs are append-only, and 0009 references it.
Known limitations
Another container behind the same address still spends the same sixty requests an hour,
and no count of
event_sourcescan see it. This narrows one failure mode; the ledger isstill not authoritative over the window.
ADR 0004's existing trade, applied to a new input.
budget.window_reset_in_pastreports a condition it does not fix. Under clock skew thewindow initializes and rolls straight back to
uninitialized, so enrichment staysineligible for as long as the skew lasts. Refusing to initialize was considered and
rejected: it starves enrichment identically while removing the only signal naming the
cause. Polling is unaffected either way.
log stream and not queryable.
🤖 Generated with Claude Code