mirror of
https://github.com/block/buzz.git
synced 2026-11-01 06:51:17 +00:00
fd9e1e29ce
Two defects in the ACP client's backoff jitter, one arithmetic and one
structural.
The arithmetic defect: jitter divided subsec_nanos() (max 999,999,999)
by u32::MAX (4,294,967,295), a value 4.295x larger than the input domain
can ever reach. The "+/-20%" factor was therefore confined to
[0.8, 0.8931] and never reached 1.0. Two consequences: every backoff was
shortened, never lengthened, contradicting the comment on its own line;
and the anti-thundering-herd stagger spanned 9.31% of base instead of
the intended 40%, so retrying clients bunched 4.295x more tightly than
designed. lib.rs carried a third hand-rolled copy with the same shape
and comment but the correct /1e9 divisor -- the typo is proven by its
own sibling.
Rather than patch the two broken copies in place, the three hand-rolled
implementations are consolidated into one backoff module. The divergence
between them was the root cause; a single definition is what stops it
recurring.
The structural defect: the rate-limit gate applied symmetric jitter to
the relay's "retry in {N}s" hint. That hint is the relay's authoritative
window TTL, so shortening it wakes the client inside a window the relay
has already declared closed -- earning a fresh denial and burning a
counter increment for no chance of success. The gate now uses a
one-sided factor in [1.0, 1.2), so it can only ever arm at or beyond the
hint. This is enforced by construction rather than by a check at the
call site: an early gate is unrepresentable.
Testing notes, since two of these tests exist to kill specific mutants:
The gate computation is extracted into a pure gate_delay(retry_secs,
jitter_nanos), with the wall clock acquired by the caller. Entropy as a
parameter is what allows the jitter endpoints to be driven rather than
sampled. An earlier version of this change injected the sample only at
the call site and swept it in a loop; that test was flaky against a
symmetric-jitter mutant, because a mutant that ignores the parameter
reads the clock instead, and a tight loop re-reads the same instant
(measured: 101 iterations span 1-4 distinct subsec_nanos values). The
sweep was a single draw wearing a sweep's costume.
The discriminating assertion checks the ceiling at nanos = 999_999_999,
not a range. A bound of the form 0.8*base <= d < 1.2*base is satisfied
by both the correct and the broken divisor, because the broken range is
a strict subset of the asserted one. The upper bound is inclusive:
Duration is nanosecond-resolution, so 2s * 1.1999999998 rounds to
exactly 2.4s and a strict < would be unsatisfiable.
Mutation results: reintroducing the u32::MAX divisor is killed 3/3 by
the backoff ceiling tests. Swapping the gate's one-sided factor for the
symmetric one, inside the seam, is killed 10/10 by the endpoint test.
A mutation that bypasses the seam entirely is a structural change these
unit tests do not claim to kill; unused_variables on the parameter and
the integration test cover that case.
Co-authored-by: Tyler Longwell <tlongwell@squareup.com>
Signed-off-by: Tyler Longwell <tlongwell@squareup.com>