Repository navigation
Budget breaker: close the gaps found in review before it is armed (race with a human pause, pagination, hold on a bad timestamp, lost notice, live API checks) #2251
Description
Why
"Budget breaker writer" (#2153) merged disarmed on 2026-10-11 as a44bb9bb. Its reviews (CodeAnt and the QA advisory on the pull request) found gaps that do not matter while it only logs, and do matter the moment it is armed. This issue collects them so arming ("Auto-pause companion", #1565) has a concrete checklist. Do not arm before this is closed.
Code and tests
- Narrow the window in which a human pause can be claimed. If a maintainer sets
AGENTS_PAUSED=truebetween the breaker's last read and its writes, the trip records the breaker as the owner, and a later run resumes a pause a human set. On a trip, after writing the source and until values and BEFORE setting the pause, read the state again; if the fleet is already paused, remove the two values just written, write nothing else, and warn. Test both orders of the race. - Pagination. The organisation variable list is read 30 per page and the test stub returns one page. Add a test where
AGENTS_PAUSEDis on the second page; if it goes unseen, an existing human pause reads as "not paused". Make the stub honour pagination rather than ignore the flag. - The minimum hold when
updated_atis missing or unparseable. Today the resume proceeds with no hold on that path and no test covers it. Decide the behaviour (recommended: treat the hold as not yet satisfied for one run, then resume, so a bad timestamp can delay a resume by one tick but never block it), implement it and test it. - A failed notice. A run whose notice fails goes red; the next run holds and never re-posts, so the pause is announced nowhere. Have the next run post the missing notice once (keyed on the until value), or state in
AGENTS.mdthat a red run is the only signal, and test whichever is chosen. - Arming value. A GitHub Actions expression compares strings without regard to case, so
BUDGET_BREAKER_ARMED=TRUEdelivers the write credential to the step even though the script then refuses to write and drops it. This cannot be fixed in the expression. Make the script log one clear error when the variable is set to anything other than the exact stringtrueor empty, and say inAGENTS.mdthat only lowercasetruearms.
Live checks during the disarmed period (record the results on #1565)
These are mocked in the tests and must be confirmed against the real API by the maintainer or with a throwaway organisation variable, never with the pause variables themselves:
- A value-only PATCH of an organisation variable keeps its visibility.
- A PATCH updates the variable's
updated_at. - The variable list paginates as the code expects.
- The dedicated write credential can read, create, update and delete an organisation variable, and nothing else.
- With the read credential the workflow uses, dev-lead's quota markers read as trusted (the run summary reports markers counted and dropped; confirm a real marker is counted).
Acceptance criteria
- Items 1 to 5 are implemented with tests, and the existing breaker suites pass unchanged apart from the stub's pagination support.
- Items 6 to 10 are recorded on Auto-pause companion: usage-telemetry adapter + scheduled poller that sets/clears AGENTS_PAUSED from the Claude budget #1565 with the date and what was observed.
AGENTS.mdstates the arming preconditions.
Related: #2153, #2221, #1565, and "AGENTS_PAUSED stops only dev-lead" (#2249), which should also land before a pause is relied on.
Activity
github-actions commentedon Oct 11, 2026
Thanks for filing such a thorough checklist from the review of the budget breaker writer. The issue already lists five code changes, five live API checks and the acceptance criteria. One question before work starts: for item 4 (the failed notice), do you prefer that the next run posts the missing notice once, keyed on the until value, or that
AGENTS.mdstates a red run is the only signal?fix(budget-breaker): harden the fleet pause writer before it is armed (…
don-petry commentedon Oct 11, 2026
The code half is merged; what remains here is for the maintainer
Items 1 to 5 merged on 2026-10-11 as
df06b7a4("fix(budget-breaker): harden the fleet pause writer before it is armed"), with one addition made during review: an armed run now removes the breaker's own ownership markers when it finds them on an unpaused fleet, so a leftover marker cannot make a later hand pause look like the breaker's own.BUDGET_BREAKER_ARMED=falsedisarms quietly; only lowercasetruearms. The breaker is still disarmed.This issue stays open for acceptance criterion 2. Nothing below can be done by automation: it needs the real API and, for two of them, a deliberate drill.
Live checks, from the original list (record date and result on "Auto-pause companion", #1565):
updated_at.Added by the reviews of the hardening change:
Also still blocking arming: "AGENTS_PAUSED stops only dev-lead" (#2249, in review) and "Budget breaker depends on scheduled ticks that this repository barely receives" (#2259).
don-petry commentedon Oct 11, 2026
Live checks: what was observed on 2026-10-11, and exact commands for the rest
Read-only observations below were made with the
don-petrylogin (the same identity the workflow's read credentialGH_PAT_DON_PETRYbelongs to), at about 15:20 UTC. Nothing was written and no pause variable was touched.updated_atper_page=2,gh api --paginate … --jq '.variables[] | {name, visibility, updated_at}'returned all 7 across 4 pages (2, 2, 2, 1). The filter runs once per page and every variable arrives. Not observed: a real list longer than 30.BUDGET_BREAKER_PATto existstatus=rate-limitedcomments here are pr-review's advisory marker (deliberately not counted), posted bydonpetry-bot, whoseauthor_associationreadsMEMBERto this identity. Confirm on the first breaker run after a real dev-lead rate limit:quota markers read: N trusted, M dropped as untrustedwith N ≥ 1.gh api --paginate "repos/petry-projects/.github-private/issues/1565/comments?per_page=4" --jq 'length'printed4 4 1, one count per page, which is what the notice lookup sums. Real responses were not saved as fixtures.budget-breaker.ymlhasconcurrency: {group: budget-breaker, cancel-in-progress: false}, which GitHub documents as at most one running and one pending run per group. With one run in 14 hours there has been no overlap to observe.Items 6 and 7 (any login with organisation admin; uses a throwaway variable, removes it at the end)
gh api orgs/petry-projects/actions/variables/BUDGET_BREAKER_PROBE --jq '{value, visibility, updated_at}'sleep 5; gh api -X PATCH orgs/petry-projects/actions/variables/BUDGET_BREAKER_PROBE -f value=bgh api orgs/petry-projects/actions/variables/BUDGET_BREAKER_PROBE --jq '{value, visibility, updated_at}'Pass: the second read shows
value: b,visibility: privateunchanged (item 6), and a laterupdated_at(item 7).Item 9 (with the dedicated credential; do not paste its value anywhere)
Put the token in the shell without echoing it or writing it to history, run the same five commands as above with it, then two calls that must be refused:
unset BBWPass: the variable list (and the create, PATCH, DELETE of
BUDGET_BREAKER_PROBErun withGH_TOKEN="$BBW") succeed; the secrets list and the repository variable list are refused (403 or 404). The token's own settings page is the authority for "nothing else": it should list organisation Variables read and write, and no repository permission.Item 12: the drill needs a test organisation
The three variable names are fixed in
scripts/lib/budget-breaker-writer.shand cannot be overridden from the environment, which is deliberate: nothing can point the writer at another name by accident. So a drill "with throwaway variables" is not possible without a code change. The organisation can be overridden (BUDGET_BREAKER_ORG), so the drill has to runscripts/budget_breaker.shlocally against a throwaway organisation, with a credential for that organisation only. I recommend that route over adding a test-only override of the names. I can write the step-by-step procedure for it once a test organisation exists; I have not, because the commands depend on its name and on how quota markers are seeded there.Still open after this comment
6, 7, 9 and 12 are the maintainer's. 10 waits for a real dev-lead rate limit. 8, 11 and 13 are recorded as observed with the limits stated; say if a real response longer than 30 variables or an observed overlap is required before arming.
don-petry commentedon Oct 12, 2026
Item 12 (the hand-pause drill): replaced, by maintainer decision, 2026-10-11
There is no throwaway organisation to run the drill in, and the variable names are fixed in the writer on purpose. Instead of creating an organisation or adding a way to rename the variables, the two races are closed by procedure and the one assumption behind it becomes a live check. Documentation change: #2314.
The procedure. A pause set by hand is three calls, in this order: delete
AGENTS_PAUSE_SOURCE, setAGENTS_PAUSED=true, deleteAGENTS_PAUSE_SOURCEagain. The first delete removes a leftover source (the second race); the last covers a trip that had already passed its final check (the first race). A two-call version (set, then delete) is not enough: a breaker run landing between the two calls on a leftover source can lift the pause.The new live check, replacing item 12. A read of the variable list must see a write made just before it. Create the throwaway variable with the item 6 command above, run this, then delete it with the last command of that block:
Pass:
finishedwith noSTALE READline. A stale read means the breaker's guard can miss a pause set a moment earlier, and a real drill is needed after all.Also found while writing it:
gh variable set --orgdefaults to visibilityprivateand sends it on updates too. Every repository here is public, so a pause set that way would reach none of them. The procedure uses value-only API calls, and the existing fleet variableAI_ENGINEShas visibilityall.Remaining for the maintainer on this issue: checks 6, 7 and 9, and the loop above. Check 10 waits for a real dev-lead rate limit.
docs(fleet-pause): the hand pause forces visibility, ends with a chec…
don-petry commentedon Oct 14, 2026
Update to the live check that replaces item 12 (use this loop, not the one above)
Review of #2314 found the earlier loop tested updates only, while the hand-pause guarantee also depends on a deleted variable being gone from the next read. This one creates, updates and deletes. It needs no setup and leaves nothing behind:
Pass:
finishedwith noSTALEline. If the loop is interrupted, remove the variable withgh api -X DELETE orgs/petry-projects/actions/variables/BUDGET_BREAKER_PROBE. OnceBUDGET_BREAKER_PATexists, a second pass with the reads made by that credential is worth doing, since it is the breaker's reader.The procedure itself also changed in review: it now ends with a check a minute later (repeat if the fleet is not paused), because a resume the breaker has already decided on can undo a hand pause and no ordering of calls prevents that; and the pause call sets
visibility=allevery time.docs(fleet-pause): wait for breaker runs under way before the check, …
docs(fleet-pause): the wait fails closed, the check reads each variab…
don-petry commentedon Oct 14, 2026
Correction to the loop posted above, from review of #2314: the reads should paginate the way the breaker reads (
gh api --paginateon?per_page=30), so a page boundary cannot hide the variable. Use this one:Pass:
finishedwith noSTALEline. If interrupted:gh api -X DELETE orgs/petry-projects/actions/variables/BUDGET_BREAKER_PROBE.docs(fleet-pause): only a 404 counts as no pause source in the check