fix(byoa): a turn that cannot succeed must stop retrying on both paths - #173
Merged
Merged
Conversation
The fifteen-minute operator pause was measured into existence — nine
computers whose Claude was signed out produced 1,988 failed turns in
forty minutes — but it was wired into the chat wake only.
maybeAgendaTurn() is the second path a turn ends on, and it fires on its
own every 60s whether or not anyone is talking to the agent. Its error
handling had no operator-fix branch at all: with engineError set and the
rate-limit test false, neither arm ran, so the pause was never entered
and the next tick spawned the signed-out engine again. The agents that
kept spinning were precisely the ones nobody was chatting with.
The notice those turns publish is deduplicated over the same fifteen
minutes the pause covers, so the ratio was roughly fifteen dead spawns
per agent per message the operator actually saw.
Classify once, in classifyTurnOutcome(), and assign the pause in one
method both paths end in. engineBackoffUntil now has exactly one
assignment site in the file, so the two paths cannot drift again.
Precedence is preserved exactly: a throttle still outranks an operator
fix ("insufficient quota" matches both and must stay the short,
self-clearing cooldown), a transient failure returns null rather than 0
so it cannot cancel a pause an earlier understood failure established,
and an empty error string still counts as a clean turn.
Both skip guards also printed "engine rate-limit cooldown" whatever the
cause, so an operator pause announced itself as a throttle for fifteen
minutes. They now name the real reason.
Merged
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.
The fifteen-minute operator pause was measured into existence, and the measurement is in its own doc comment: nine computers whose Claude was signed out produced 1,988 failed turns in forty minutes, with no backoff and nothing telling their operator why.
It was wired into the chat wake only.
maybeAgendaTurn()is the second path a turn ends on. It fires on its own everyAGENDA_CHECK_MS(60s) whether or not anyone is talking to the agent, and it classified the same failure differently:With
engineErrorset andagendaRateLimitedfalse, neither arm runs. The pause is never entered, so the next tick sixty seconds later spawns the signed-out engine again, and the one after that, for as long as it stays signed out.So the agents that kept spinning were precisely the ones nobody was chatting with — the pause looked like it worked because the path that demonstrated it was the path that had it.
The notice is deduplicated over
dedupeTtlSec: 900, the same fifteen minutes the pause covers. That sets the ratio: roughly fifteen dead spawns per agent per message the operator actually sees. Quietly burning the fleet's error budget is the failure mode the constant's doc comment already names.What changes
One classifier, one assignment site.
and one private method both paths end in:
this.engineBackoffUntilis now assigned in exactly one place in the file. A path can still fail to ask — that is one grep away — but the two can no longer hold different opinions about what a failure means, which is how they drifted.Three details preserved deliberately:
!rateLimited && needsOperatorFix(…);"insufficient quota"matches both, and it must stay the short self-clearing cooldown that does not post to chat. There is a test pinning that precedence.transientreturnsnull, not0. An unexplained failure must not cancel a pause an earlier, well-understood one established — otherwise the spin comes back with an extra step in it.''still counts as a clean turn, because the old code keyed on!engineErrorand changing it would pause agents that had done nothing wrong.Also, a log line that misled
Both skip guards printed
engine rate-limit cooldownregardless of cause, so an operator pause announced itself as a throttle for fifteen minutes — sending whoever reads the daemon log looking for a rate limit that was never there. This was already true on the chat path before this change. They now printengine paused (operator action needed)/(rate limit).Verification
null ≠ 0.expected one assignment, found 2;expected the chat and agenda paths, found 1), then green again on restore. Pure functions cannot catch a path that simply does not call them, so those two read the source — the same thingengine-stdin-safety.test.tsandelectron-renderer-url.test.tsdo.agents-computer-operator-fix-backoff.test.tsis untouched and still green; every string in it classifies as before.main— identical failure sets, all of them needing a live Postgres locally.tsc --noEmit,biome lint ., and all three source guards clean.computer/daemon.ts, so that suite neither exercises this nor can be disturbed by it. Saying so beats reporting a run that proves nothing.What I did not do: reproduce a live signed-out agenda turn end to end — I have no BYOA engine credentials to sign out. The reasoning above is from the source and the maintainer's own measurement; the structural assertions are what pin it.
Deliberately not in scope: widening
OPERATOR_FIX_REitself.argvRejection()already detects an engine CLI too old for the flags we send — a failure that also cannot succeed on retry — but it is only wired into the probes, and the negative cases in the existing test show the bar for that regex is intentionally high. That is a separate argument, worth making separately.