Exhaustion quarantine is a flat 30 minutes, so a weekly subscription limit is retried ~336 times #466

Open
opened 2026-09-10 13:55:17 +02:00 by ltms · 2 comments
Owner

The operator's requirement, in their words: "runtime model limit monitoring, off when subscription limit reach and on when the limit lifted". This ticket is the gap between that and what the code does today.

What the code does, measured on f5e02fe

BackendQuarantine.quarantine(String credentialId) is the whole mechanism:

public void quarantine(String credentialId) {
    Objects.requireNonNull(credentialId, "credentialId");
    if (inert) {
        return;
    }
    quarantinedUntilNanos.put(credentialId, nowNanos.getAsLong() + cooldownNanos);
}

cooldownNanos is a constructor field. There is no repeat count and no escalation: every call sets the deadline to now plus the same constant. The default is quarantineCooldownSeconds: 1800 (fleetd.example.yaml:459).

Why that is wrong for the case it exists for

The two things it is asked to handle have completely different lengths:

signal how long it really lasts what a flat 1800s does
a transient backend wobble seconds to minutes correct — 30 minutes is generous and harmless
a subscription window exhausted hours to a week retries every 30 minutes until the window resets

A weekly limit at 1800s means about 336 retry cycles. Each cycle is a spawn that either fails outright or — the worse case already recorded in this repo — spawns a member that runs and produces nothing. So "off when the limit is reached" is true for 30 minutes at a time, and "on when the limit is lifted" happens on a timer that knows nothing about whether the limit lifted.

This is live for the operator right now: their opencode weekly allowance ran out.

What #446 does and does not solve

#446 (PR #457, in review) adds the detection half and a warning that names the fix:

usage-limit fix: profile 'terra' runs model 'claude-opus-5' — set enabled: false on that model's entry under models.allow in fleetd.yaml to stop new spawns landing on it (models: is hot, no restart needed); remove the line again once the subscription window resets

That is a manual answer to a requirement stated as automatic, and it is the right first step — the operator gets told exactly which line to flip, and models: is hot so it takes effect with no restart. What it does not do is stop the 336 retries in the window before anybody reads the log.

The design question, and the part that does not depend on it

The open question — which I have asked and which is genuinely the operator's — is how the gate learns the limit lifted:

  • A. Escalating backoff. 30min, 1h, 2h, 4h, 8h, capped at 12h, reset on a success. No new signal needed. Costs one wasted spawn per probe.
  • B. Off until manually lifted. An exhaustion signal sets the model's enabled: false, and a human puts it back. Deterministic, zero wasted spawns, needs a human.
  • C. Read the reset time from the provider's error, where the error carries one, and quarantine until then. Best when available; not available everywhere.

The part worth building before that is settled: the escalation in A is an improvement under every one of the three. Under B it is the fallback for when nobody flips the switch. Under C it is the fallback for a provider whose error carries no reset time. Under A it is the whole answer. Today's flat 30 minutes is the worst of all three, so the escalation can be built now and does not pre-empt the decision.

Scope

  1. Make the cooldown a function of consecutive quarantines for the same credentialId: double each time, from quarantineCooldownSeconds, capped by a new quarantineMaxCooldownSeconds (suggest 43200, 12 hours). Reset the counter on a successful spawn on that credential.
  2. Report the current step, not only the remaining seconds. fleet_profiles/fleet_list already carry quarantinedForSeconds; add the attempt count so an operator can see "this is the 5th time" rather than inferring it.
  3. Keep the existing behaviour reachable: quarantineMaxCooldownSeconds equal to quarantineCooldownSeconds must give exactly today's flat cooldown, so an operator who liked the old shape can pin it.

A second, separable finding

quarantineCooldownSeconds is classified DEFERRED — fleetd.example.yaml:481 lists it under keys that need a restart, because CB-578 stage B bakes it once into the tracker built at startup.

That is the wrong classification for this key. It is exactly the knob an operator wants to change during an outage, and the answer today is "restart the daemon", which drops every in-flight ticket and rendezvous. Note this is the mirror of #427, which was a live-hot key documented as needing a restart; this one is genuinely deferred and ought to be hot.

Whether to fix that here or separately is the implementer's call, but do not silently reclassify it in the docs without moving the code — that is the #427 defect in the other direction.

Acceptance criteria

  1. An injected clock, not a real one. BackendQuarantine already takes a LongSupplier nowNanos, so a test can advance time exactly and there is no reason to sleep or to hunt a load level.
  2. A test that quarantines the same credential 6 times and asserts the exact deadline after each, including the cap.
  3. A test that a success resets the step back to the base cooldown.
  4. A test that quarantineMaxCooldownSeconds == quarantineCooldownSeconds reproduces today's flat behaviour.
  5. Break each of the four and paste the failure. A test that passes against both the old flat cooldown and the new escalation pins nothing.
  6. Whole suite green: mvn -B clean test in fleetd/, output redirected to a file, exit code captured with rc=$? on its own line. Never pipe mvn into tail or grep, and never use mvn -q — it deletes the Tests run: line.
The operator's requirement, in their words: **"runtime model limit monitoring, off when subscription limit reach and on when the limit lifted"**. This ticket is the gap between that and what the code does today. ## What the code does, measured on `f5e02fe` `BackendQuarantine.quarantine(String credentialId)` is the whole mechanism: ```java public void quarantine(String credentialId) { Objects.requireNonNull(credentialId, "credentialId"); if (inert) { return; } quarantinedUntilNanos.put(credentialId, nowNanos.getAsLong() + cooldownNanos); } ``` `cooldownNanos` is a constructor field. There is no repeat count and no escalation: every call sets the deadline to *now plus the same constant*. The default is `quarantineCooldownSeconds: 1800` (`fleetd.example.yaml:459`). ## Why that is wrong for the case it exists for The two things it is asked to handle have completely different lengths: | signal | how long it really lasts | what a flat 1800s does | |---|---|---| | a transient backend wobble | seconds to minutes | correct — 30 minutes is generous and harmless | | a **subscription window** exhausted | hours to **a week** | retries every 30 minutes until the window resets | A weekly limit at 1800s means about **336 retry cycles**. Each cycle is a spawn that either fails outright or — the worse case already recorded in this repo — spawns a member that runs and produces nothing. So "off when the limit is reached" is true for 30 minutes at a time, and "on when the limit is lifted" happens on a timer that knows nothing about whether the limit lifted. This is live for the operator right now: their opencode weekly allowance ran out. ## What #446 does and does not solve #446 (PR #457, in review) adds the detection half and a warning that names the fix: > `usage-limit fix: profile 'terra' runs model 'claude-opus-5' — set enabled: false on that model's entry under models.allow in fleetd.yaml to stop new spawns landing on it (models: is hot, no restart needed); remove the line again once the subscription window resets` That is a **manual** answer to a requirement stated as automatic, and it is the right first step — the operator gets told exactly which line to flip, and `models:` is hot so it takes effect with no restart. What it does not do is stop the 336 retries in the window before anybody reads the log. ## The design question, and the part that does not depend on it The open question — which I have asked and which is genuinely the operator's — is how the gate learns the limit lifted: - **A. Escalating backoff.** 30min, 1h, 2h, 4h, 8h, capped at 12h, reset on a success. No new signal needed. Costs one wasted spawn per probe. - **B. Off until manually lifted.** An exhaustion signal sets the model's `enabled: false`, and a human puts it back. Deterministic, zero wasted spawns, needs a human. - **C. Read the reset time from the provider's error**, where the error carries one, and quarantine until then. Best when available; not available everywhere. **The part worth building before that is settled: the escalation in A is an improvement under every one of the three.** Under B it is the fallback for when nobody flips the switch. Under C it is the fallback for a provider whose error carries no reset time. Under A it is the whole answer. Today's flat 30 minutes is the worst of all three, so the escalation can be built now and does not pre-empt the decision. ## Scope 1. Make the cooldown a function of consecutive quarantines for the same `credentialId`: double each time, from `quarantineCooldownSeconds`, capped by a new `quarantineMaxCooldownSeconds` (suggest 43200, 12 hours). Reset the counter on a successful spawn on that credential. 2. Report the current step, not only the remaining seconds. `fleet_profiles`/`fleet_list` already carry `quarantinedForSeconds`; add the attempt count so an operator can see "this is the 5th time" rather than inferring it. 3. Keep the existing behaviour reachable: `quarantineMaxCooldownSeconds` equal to `quarantineCooldownSeconds` must give exactly today's flat cooldown, so an operator who liked the old shape can pin it. ## A second, separable finding `quarantineCooldownSeconds` is classified **DEFERRED** — `fleetd.example.yaml:481` lists it under keys that need a restart, because CB-578 stage B bakes it once into the tracker built at startup. That is the wrong classification for this key. It is exactly the knob an operator wants to change *during* an outage, and the answer today is "restart the daemon", which drops every in-flight ticket and rendezvous. Note this is the mirror of #427, which was a live-hot key documented as needing a restart; this one is genuinely deferred and ought to be hot. Whether to fix that here or separately is the implementer's call, but do not silently reclassify it in the docs without moving the code — that is the #427 defect in the other direction. ## Acceptance criteria 1. An injected clock, not a real one. `BackendQuarantine` already takes a `LongSupplier nowNanos`, so a test can advance time exactly and there is no reason to sleep or to hunt a load level. 2. A test that quarantines the same credential 6 times and asserts the exact deadline after each, including the cap. 3. A test that a success resets the step back to the base cooldown. 4. A test that `quarantineMaxCooldownSeconds == quarantineCooldownSeconds` reproduces today's flat behaviour. 5. **Break each of the four and paste the failure.** A test that passes against both the old flat cooldown and the new escalation pins nothing. 6. Whole suite green: `mvn -B clean test` in `fleetd/`, output redirected to a file, exit code captured with `rc=$?` on its own line. Never pipe `mvn` into `tail` or `grep`, and never use `mvn -q` — it deletes the `Tests run:` line.
ltms closed this issue 2026-09-10 14:56:06 +02:00
ltms reopened this issue 2026-09-10 14:56:26 +02:00
Author
Owner

The core mechanism landed in 789b6a8 (PR #470, closed). I closed this ticket with it and then reopened it a minute later, because I was wrong: two of the three scope items were not delivered, and closing on the strength of the first one would have buried them.

Recording that plainly rather than filing a fresh ticket that hides the miss.

Item 1 — landed, with a deliberate substitution

The escalation is in and proven. Doubling per consecutive exhaustion of the same credential, capped at 12x the base, reset after a base cooldown of quiet. My own mutations on the merge killed the escalation, the ceiling and the reset separately, and a wiring test now pins that Fleetd.main actually chooses the escalating factory.

But the ticket asked for a new quarantineMaxCooldownSeconds config key, and the implementation made the multiplier (2.0) and the ceiling (12x) constants in BackendQuarantine instead. That was a considered choice, stated openly in the PR: no new YAML surface means no new ConfigRef hot/cold/deferred classification question. I accept the reasoning — but it is a substitution, not the thing asked for, and it has a consequence the next item names.

Item 3 — no longer possible

Keep the existing behaviour reachable: quarantineMaxCooldownSeconds equal to quarantineCooldownSeconds must give exactly today's flat cooldown, so an operator who liked the old shape can pin it.

With the multiplier and ceiling as constants, an operator cannot pin the flat shape at all. The flat behaviour still exists in the two-argument constructor, and the test suite exercises it — but nothing in fleetd.yaml can reach it. Production is now always escalating.

I think that is probably the right default, and I am not asking for it to be undone. But the ticket promised an escape hatch and there is none, so an operator who wants the old behaviour has no move except editing Java. That needs a decision rather than silence.

Item 2 — not delivered at all

Report the current step, not only the remaining seconds. fleet_profiles/fleet_list already carry quarantinedForSeconds; add the attempt count so an operator can see "this is the 5th time" rather than inferring it.

Nothing was added. QuarantineState tracks repeatCount internally, and it is not exposed anywhere an operator can see it.

This is the item I care about most, and it is why the ticket stays open. The whole point of this work is an operator understanding an outage while it is happening. Right now they see quarantinedForSeconds: 21600 and have to work backwards through the doubling to learn this is the fifth consecutive exhaustion — which requires knowing the base, the multiplier and the ceiling, none of which are in the output. A quarantineRepeatCount beside the seconds turns arithmetic into a fact.

There is a trap here worth stating for whoever picks it up, because this repo has been bitten by it repeatedly: the reported count must be read from the same accessor the cooldown itself uses. A second, independently-derived count can disagree with the behaviour, and a receipt that disagrees with the thing it reports on is worse than no receipt. CompositePeerLauncher.modelGateState() is the pattern to copy — its javadoc says explicitly that it reads the same accessor the gate enforces so the two can never diverge.

The separable finding — still open, correctly

quarantineCooldownSeconds is still deferred. The implementation left it that way and said so, which is the honest outcome: the ticket warned against reclassifying it in the docs without moving the code, and that trap was avoided. The argument for making it hot stands unchanged — it is exactly the knob an operator wants to turn during an outage, and today the answer is a restart, which drops every in-flight ticket and rendezvous.

What remains on this ticket

  1. Expose the repeat count where an operator can see it (fleet_profiles / fleet_list), read off the same accessor the cooldown uses.
  2. Decide whether the flat shape needs to stay reachable from config, now that it is not. Either add the escape hatch or record that it was dropped on purpose.
  3. The deferred → hot question for quarantineCooldownSeconds, separable as the ticket said.

My own error, for the record

This is the second time today that treating a ticket's state as the summary of its content cost something. Earlier I delegated #424, which was already fixed and merged, because I read "open" as "work remains". Here I nearly did the reverse: read "the main mechanism landed" as "the ticket is done". Both come from not re-reading the scope list against what actually shipped. The fix is the same in both directions — read the ticket body at the moment you change its state, not the moment you assign it.

The core mechanism landed in `789b6a8` (PR #470, closed). I closed this ticket with it and then reopened it a minute later, because I was wrong: **two of the three scope items were not delivered**, and closing on the strength of the first one would have buried them. Recording that plainly rather than filing a fresh ticket that hides the miss. ## Item 1 — landed, with a deliberate substitution The escalation is in and proven. Doubling per consecutive exhaustion of the same credential, capped at 12x the base, reset after a base cooldown of quiet. My own mutations on the merge killed the escalation, the ceiling and the reset separately, and a wiring test now pins that `Fleetd.main` actually chooses the escalating factory. But the ticket asked for a **new `quarantineMaxCooldownSeconds` config key**, and the implementation made the multiplier (2.0) and the ceiling (12x) **constants** in `BackendQuarantine` instead. That was a considered choice, stated openly in the PR: no new YAML surface means no new `ConfigRef` hot/cold/deferred classification question. I accept the reasoning — but it is a substitution, not the thing asked for, and it has a consequence the next item names. ## Item 3 — no longer possible > Keep the existing behaviour reachable: `quarantineMaxCooldownSeconds` equal to `quarantineCooldownSeconds` must give exactly today's flat cooldown, so an operator who liked the old shape can pin it. With the multiplier and ceiling as constants, **an operator cannot pin the flat shape at all.** The flat behaviour still exists in the two-argument constructor, and the test suite exercises it — but nothing in `fleetd.yaml` can reach it. Production is now always escalating. I think that is *probably* the right default, and I am not asking for it to be undone. But the ticket promised an escape hatch and there is none, so an operator who wants the old behaviour has no move except editing Java. That needs a decision rather than silence. ## Item 2 — not delivered at all > Report the current step, not only the remaining seconds. `fleet_profiles`/`fleet_list` already carry `quarantinedForSeconds`; add the attempt count so an operator can see "this is the 5th time" rather than inferring it. Nothing was added. `QuarantineState` tracks `repeatCount` internally, and it is not exposed anywhere an operator can see it. This is the item I care about most, and it is why the ticket stays open. The whole point of this work is an operator understanding an outage while it is happening. Right now they see `quarantinedForSeconds: 21600` and have to work backwards through the doubling to learn this is the fifth consecutive exhaustion — which requires knowing the base, the multiplier and the ceiling, none of which are in the output. A `quarantineRepeatCount` beside the seconds turns arithmetic into a fact. There is a trap here worth stating for whoever picks it up, because this repo has been bitten by it repeatedly: the reported count **must be read from the same accessor the cooldown itself uses**. A second, independently-derived count can disagree with the behaviour, and a receipt that disagrees with the thing it reports on is worse than no receipt. `CompositePeerLauncher.modelGateState()` is the pattern to copy — its javadoc says explicitly that it reads the same accessor the gate enforces so the two can never diverge. ## The separable finding — still open, correctly `quarantineCooldownSeconds` is still **deferred**. The implementation left it that way and said so, which is the honest outcome: the ticket warned against reclassifying it in the docs without moving the code, and that trap was avoided. The argument for making it hot stands unchanged — it is exactly the knob an operator wants to turn *during* an outage, and today the answer is a restart, which drops every in-flight ticket and rendezvous. ## What remains on this ticket 1. Expose the repeat count where an operator can see it (`fleet_profiles` / `fleet_list`), read off the same accessor the cooldown uses. 2. Decide whether the flat shape needs to stay reachable from config, now that it is not. Either add the escape hatch or record that it was dropped on purpose. 3. The deferred → hot question for `quarantineCooldownSeconds`, separable as the ticket said. ## My own error, for the record This is the second time today that treating a ticket's *state* as the summary of its *content* cost something. Earlier I delegated #424, which was already fixed and merged, because I read "open" as "work remains". Here I nearly did the reverse: read "the main mechanism landed" as "the ticket is done". Both come from not re-reading the scope list against what actually shipped. The fix is the same in both directions — read the ticket body at the moment you change its state, not the moment you assign it.
Author
Owner

Scope item 2 merged as 25ba7f1 (tree f22c392). PR #473. Verified landed with git merge-base --is-ancestor, against an unmerged control branch that reported no.

fleet_profiles and fleet_list now carry quarantineAttempt beside quarantinedForSeconds.

My own mutation battery, on the merge commit

Every cell a full mvn -B clean test on tree f22c392, the merge's own tree.

CONTROL 0: status() declared once, one quarantines.get() inside it, 2 call sites using it, 0 old remainingSeconds(credentialId) call sites left, 2 quarantineAttempt writes.

CONTROL 1, unmutated: 1629 tests, 0 failures, 0 errors, BUILD SUCCESS. This cell mattered more than usual — FleetMcp.java was auto-merged and #469 had also changed it, and a clean auto-merge is not a compiling merge.

cell mutation result
M1 report reads its own non-resetting counter, fed where the real streak is written KILLED — BackendQuarantineTest.aQuietGapResetsTheReportedAttemptCountToOneToo
M2 hardcode the reported attempt to 1 KILLED — 6 tests, including both JSON assertions
M3 drop the field from fleet_profiles only KILLED — FleetMcpTest.profilesReportsQuarantineAttemptBesideRemainingSeconds
M4 drop the field from fleet_list only KILLED — FleetMcpTest.capacityRowReportsQuarantineAttemptBesideRemainingSeconds
M5 off by one on a first occurrence KILLED — 8 tests

M1 is the ticket's own warning, built in full rather than approximated. I gave the report an independent ConcurrentHashMap counter, incremented at the same place the real streak is written, and read the report off that. Both halves look correct in isolation; they disagree only after a quiet gap clears the real streak but not the shadow copy. One test caught exactly that, and it is the one the worker named. So the "one accessor, one read" claim is real, not a comment.

M3 and M4 are the pair I care about most. Two call sites are two surfaces, and a test on one proves nothing about the other — that is the shape I have had to hand back three times recently (#446, #466 item 1, #469). Here each call site fails its own named test when its field alone is removed. Nothing had to go back to the worker.

The worker's report was lost, and it cost nothing

Its fleet_reply never arrived and fleet_poll on its inbox returned empty. fleet_status said done, which on its own looks like a member that produced nothing.

It had pushed first and put the full report in the PR body, as the brief required. So the whole report survived. That is the second time that instruction has saved a turn, and it is why I keep it in every brief rather than only the long ones. I also checked the worktree before believing the silence: git -C <worktree> log showed the commit, and the branch it committed to (worker/466-quarantine-repeatcount-report) is not the one spawn provisioned (worker/466-quarantine-escalation-5ae9c1-15) — the usual trap.

Where #466 now stands

  • Item 1 — escalating cooldown: merged in 789b6a8.
  • Item 2 — expose the attempt count: merged here in 25ba7f1. Done.
  • Item 3 — whether the flat two-argument shape should stay reachable from config: still open, and still my call to make rather than a worker's. Recording the current position so it is not lost: the flat constructor stays reachable in code and now reports a real growing attempt count even though its own cooldown does not escalate. That is the honest behaviour, because the streak is a fact whether or not the cooldown uses it. What is not decided is whether an operator should be able to choose the flat shape, and whether quarantineCooldownSeconds should become hot. Neither is urgent: nothing regresses while both stay as they are.

I am leaving this ticket open on item 3 alone. Items 1 and 2 are complete and merged.

Scope item 2 merged as `25ba7f1` (tree `f22c392`). PR #473. Verified landed with `git merge-base --is-ancestor`, against an unmerged control branch that reported `no`. `fleet_profiles` and `fleet_list` now carry `quarantineAttempt` beside `quarantinedForSeconds`. ## My own mutation battery, on the merge commit Every cell a full `mvn -B clean test` on tree `f22c392`, the merge's own tree. CONTROL 0: `status()` declared once, **one** `quarantines.get()` inside it, **2** call sites using it, **0** old `remainingSeconds(credentialId)` call sites left, **2** `quarantineAttempt` writes. CONTROL 1, unmutated: **1629 tests, 0 failures, 0 errors, BUILD SUCCESS**. This cell mattered more than usual — `FleetMcp.java` was auto-merged and #469 had also changed it, and a clean auto-merge is not a compiling merge. | cell | mutation | result | |---|---|---| | M1 | report reads its **own** non-resetting counter, fed where the real streak is written | **KILLED** — `BackendQuarantineTest.aQuietGapResetsTheReportedAttemptCountToOneToo` | | M2 | hardcode the reported attempt to 1 | **KILLED** — 6 tests, including both JSON assertions | | M3 | drop the field from `fleet_profiles` **only** | **KILLED** — `FleetMcpTest.profilesReportsQuarantineAttemptBesideRemainingSeconds` | | M4 | drop the field from `fleet_list` **only** | **KILLED** — `FleetMcpTest.capacityRowReportsQuarantineAttemptBesideRemainingSeconds` | | M5 | off by one on a first occurrence | **KILLED** — 8 tests | M1 is the ticket's own warning, built in full rather than approximated. I gave the report an independent `ConcurrentHashMap` counter, incremented at the same place the real streak is written, and read the report off that. Both halves look correct in isolation; they disagree only after a quiet gap clears the real streak but not the shadow copy. One test caught exactly that, and it is the one the worker named. So the "one accessor, one read" claim is real, not a comment. M3 and M4 are the pair I care about most. Two call sites are two surfaces, and a test on one proves nothing about the other — that is the shape I have had to hand back three times recently (#446, #466 item 1, #469). Here each call site fails its own named test when its field alone is removed. Nothing had to go back to the worker. ## The worker's report was lost, and it cost nothing Its `fleet_reply` never arrived and `fleet_poll` on its inbox returned empty. `fleet_status` said `done`, which on its own looks like a member that produced nothing. It had pushed first and put the full report in the PR body, as the brief required. So the whole report survived. That is the second time that instruction has saved a turn, and it is why I keep it in every brief rather than only the long ones. I also checked the worktree before believing the silence: `git -C <worktree> log` showed the commit, and the branch it committed to (`worker/466-quarantine-repeatcount-report`) is **not** the one spawn provisioned (`worker/466-quarantine-escalation-5ae9c1-15`) — the usual trap. ## Where #466 now stands - **Item 1** — escalating cooldown: merged in `789b6a8`. - **Item 2** — expose the attempt count: merged here in `25ba7f1`. **Done.** - **Item 3** — whether the flat two-argument shape should stay reachable from config: still open, and still my call to make rather than a worker's. Recording the current position so it is not lost: the flat constructor stays reachable in code and now reports a real growing attempt count even though its own cooldown does not escalate. That is the honest behaviour, because the streak is a fact whether or not the cooldown uses it. What is not decided is whether an operator should be able to *choose* the flat shape, and whether `quarantineCooldownSeconds` should become hot. Neither is urgent: nothing regresses while both stay as they are. I am leaving this ticket **open** on item 3 alone. Items 1 and 2 are complete and merged.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#466