From 29ccb747ada4071e37fde1cd71b3000fe0d526cd Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 15 Aug 2026 10:33:44 +0200 Subject: [PATCH] CB-578 stage B: make BackendQuarantine.none() actually inert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added at merge review. none() held a clock frozen at 0 with a 1ns cooldown, so a quarantine() call on it recorded a deadline that could never pass — the credential would be locked out for the life of the daemon. Two production CompositePeerLauncher constructors default to none(), so that failure would have been silent and permanent. The implementer documented the limitation honestly rather than hiding it, but a stand-in named none() should not need the caveat. quarantine() is now a no-op on that instance, with a test asserting it. An inert value must omit the fact, never invent one. --- .../bridged/placement/BackendQuarantine.java | 22 ++++++++++++++++--- .../placement/BackendQuarantineTest.java | 19 ++++++++++++---- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/bridged/src/main/java/dev/ltms/bridged/placement/BackendQuarantine.java b/bridged/src/main/java/dev/ltms/bridged/placement/BackendQuarantine.java index 1917a58..abdb95d 100644 --- a/bridged/src/main/java/dev/ltms/bridged/placement/BackendQuarantine.java +++ b/bridged/src/main/java/dev/ltms/bridged/placement/BackendQuarantine.java @@ -27,6 +27,8 @@ public final class BackendQuarantine { private final ConcurrentHashMap quarantinedUntilNanos = new ConcurrentHashMap<>(); private final LongSupplier nowNanos; private final long cooldownNanos; + /** True only for {@link #none()}. See {@link #quarantine} for why this exists. */ + private final boolean inert; /** * @param nowNanos monotonic clock, injected for testability @@ -34,29 +36,43 @@ public final class BackendQuarantine { * must be positive */ public BackendQuarantine(LongSupplier nowNanos, long cooldownNanos) { + this(nowNanos, cooldownNanos, false); + } + + private BackendQuarantine(LongSupplier nowNanos, long cooldownNanos, boolean inert) { this.nowNanos = Objects.requireNonNull(nowNanos, "nowNanos"); if (cooldownNanos <= 0) { throw new IllegalArgumentException("cooldownNanos must be positive: " + cooldownNanos); } this.cooldownNanos = cooldownNanos; + this.inert = inert; } /** - * Inert quarantine — nothing is ever quarantined unless {@link #quarantine} is actually called on - * this instance. The explicit stand-in a caller (or a test not exercising this feature) passes + * Inert quarantine — {@link #quarantine} does nothing on this instance, so nothing is ever + * quarantined. The explicit stand-in a caller (or a test not exercising this feature) passes * instead of a defaulting overload, exactly like {@code ExhaustedPatternLookup.none()}. */ public static BackendQuarantine none() { - return new BackendQuarantine(() -> 0L, 1); + return new BackendQuarantine(() -> 0L, 1, true); } /** * Quarantine {@code credentialId} for the configured cooldown, starting now. A repeat call while * already quarantined restarts the cooldown at full length — a fresh refusal is fresh evidence the * account is still exhausted, not a reason to let an earlier, shorter wait stand. + * + *

On {@link #none()} this is a no-op. It has to be: that instance holds a clock frozen at 0, + * so recording a deadline would produce a quarantine that never expires — a credential locked out + * for the life of the daemon. Two production {@code CompositePeerLauncher} constructors default to + * {@code none()}, so the failure would be silent and permanent. An inert stand-in must omit the + * fact, never invent one. */ public void quarantine(String credentialId) { Objects.requireNonNull(credentialId, "credentialId"); + if (inert) { + return; + } quarantinedUntilNanos.put(credentialId, nowNanos.getAsLong() + cooldownNanos); } diff --git a/bridged/src/test/java/dev/ltms/bridged/placement/BackendQuarantineTest.java b/bridged/src/test/java/dev/ltms/bridged/placement/BackendQuarantineTest.java index de10c0d..5403f9e 100644 --- a/bridged/src/test/java/dev/ltms/bridged/placement/BackendQuarantineTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/placement/BackendQuarantineTest.java @@ -90,16 +90,27 @@ class BackendQuarantineTest { @Test void noneReportsNothingQuarantinedWhenNeverToldTo() { - // .none() is the stand-in for a caller whose code path never calls #quarantine at all (e.g. - // the 2/5/6-arg CompositePeerLauncher constructors) — not a guarantee that a call to - // #quarantine on it is a no-op. Left alone, as those call sites leave it, nothing is ever - // quarantined. BackendQuarantine q = BackendQuarantine.none(); assertFalse(q.isQuarantined("anything")); assertTrue(q.activeRemainingSeconds().isEmpty()); } + @Test + void noneIgnoresAQuarantineCallInsteadOfLockingTheCredentialForever() { + // .none() holds a clock frozen at 0, so if #quarantine recorded a deadline the credential + // would never expire — locked out for the life of the daemon. Two production + // CompositePeerLauncher constructors default to none(), so that failure would be silent and + // permanent. An inert stand-in must omit the fact, never invent one. + BackendQuarantine q = BackendQuarantine.none(); + + q.quarantine("shared-openai"); + + assertFalse(q.isQuarantined("shared-openai"), "none() must not quarantine anything"); + assertTrue(q.remainingSeconds("shared-openai").isEmpty()); + assertTrue(q.activeRemainingSeconds().isEmpty()); + } + @Test void aNonPositiveCooldownIsRejected() { assertThrows(IllegalArgumentException.class, () -> new BackendQuarantine(() -> 0L, 0L));