add jitter and decorrelated backoff - #2
Merged
Merged
Conversation
Retrying on a fixed schedule means every client that failed together retries together, so a dependency coming back up gets a synchronized wave. Jitter is the fix. Two shapes ship here. `Jitter::Full` / `Jitter::Equal` with the `Jittered<B>` wrapper and the `Backoff::jittered(mode)` combinator randomize any strategy's delays. Opt-in; retry's default stays deterministic. `DecorrelatedBackoff` is the AWS formula, `min(cap, rand(base, prev*3))`. It can't be a `Jitter` mode because the randomness is in the recurrence: each range is set by the delay actually drawn last time, so there's no deterministic sequence underneath to wrap. Validated config, `max_retries` give-up, and the same `BackoffConfigError` as the exponential strategy. Neither randomized type is `Clone`. Copying the RNG state would make every copy replay identical delays, which puts concurrent requests on one host into lockstep, which is the herd again. Hold the config, which is `Clone`, and build a fresh strategy from it. Both are seedable (`with_seed`) so jittered retries stay reproducible. Tests are mutation-checked: a no-op `Equal` mode, a dropped `mode` argument, a fixed seed in place of entropy, the raw draw fed back instead of the capped one, a smaller multiplier, and the cap applied before the draw all fail the suite. ADR004 records the `fastrand` dependency (measured against the cost of the `tracing` tree we already take), why decorrelated is a strategy rather than a mode, and why the randomized types aren't `Clone`.
The backoff tests cover the delay sequences in isolation, but nothing exercised a strategy carrying its own RNG through the pin-projected state machine. Seeded, so the delays stay fixed. Blocking twin skipped: its driver is a plain loop, so it can't have the class of bug these cover (state surviving a move into a pinned future).
`ConstantBackoff` was on the 0.3.0 list, but `ExponentialBackoff` with `factor: 1` already produces exactly that: the delay never grows, so it stays at `base`. A separate type would be a permanent public commitment for something already expressible, so this is one doc line and a test pinning the behaviour instead.
Jitter only. The RetryError<E> work is breaking and ships separately, so this release is purely additive: existing code compiles unchanged and the default backoff stays deterministic. Bumps the version because cargo-semver-checks compares against the published 0.2.0, and adds a README section since jitter is user-facing.
Equal jitter's floor is delay/2. With the default factor of 2 that is also the previous ladder value, so the two readings give the same number and the existing tests -- which all use factor 2 -- cannot tell them apart. This is documentation rather than coverage, and the comment says so. Jitter::apply takes one Duration and keeps no history, so the previous-step reading is not implementable without a structural change; the test exists to stop that change happening by accident and to state the rule at a factor where the two numbers differ.
Equal jitter is dominated. AWS's own post says it "does slightly more work than Full Jitter, and takes much longer", and their SDK agrees: full jitter is the default for STANDARD and ADAPTIVE retry modes while equal survives only on the LEGACY throttling path. Simulating both against mettle's actual implementations under a contended resource reproduced it. Equal lost to full on total calls AND completion time in all five configurations tried, across client counts and base delays. An option nobody should pick is a trap rather than a choice, and "equal" reads as the safe middle option, which is exactly backwards. With one mode left the enum was ceremony, so `jittered()` now takes no argument and the `Jitter` type is gone. The cost is the extension point: `Jitter` was #[non_exhaustive] so a fourth mode would have been free to add, and now it would need a separate `jittered_with(mode)`. Accepted -- the literature defines three, we ship the one that measures best, and decorrelated cannot be a mode at all. Docs now say which to reach for: `.jittered()` composes with any strategy and spreads widest; `DecorrelatedBackoff` gives a floor, at the price of never retrying sooner than `base`.
Two papercuts found by writing the API from a user's side rather than from inside the crate. `.jittered()` reads well but couldn't be seeded, and seeding meant `Jittered::with_seed(ExponentialBackoff::default(), 42)` -- so a test and the production config it was supposed to be testing looked nothing alike. `jittered_with_seed(seed)` closes that. And `Clock` wasn't implemented for references, so `.clock(&mock)` was a compile error (E0599) and every mock had to wrap its own state in Rc or Arc before it could be passed at all. That is friction on the exact path this crate advertises. Added for `&C` and `Arc<C>` on both Clock traits; a mock is now a plain struct with a RefCell. Both additive. Verified from a downstream crate: a mock with no interior sharing now works through &clock, through Arc, and through the seeded combinator, all three producing identical delays.
The release is additive at the signature level, but adding `impl Clock for &C` collides with the same impl if a user already wrote it themselves -- most likely as a workaround for it being missing. Verified: a crate with both `impl Clock for MyClock` and `impl Clock for &MyClock` compiles against 0.2.0 and fails with E0119 against this branch. Blast radius is small and the fix is deleting a line that this release makes redundant, so the impls stay. But cargo-semver-checks does not flag added impls, so nothing else would have told anyone. It goes in the CHANGELOG.
Every gate was green and every one of these was still wrong, because nothing checks whether prose is current. The crate-level docs -- the docs.rs landing page, the first thing anyone reads -- never mentioned jitter at all. The ADR index never listed ADR004. And examples/retry.rs, which the README calls the place to start, showed none of the release's headline feature. Also corrected the 'available now' line, which still described 0.2.0.
A changelog answers 'what changed and what must I do'. Rationale is a different question with a different audience and a different lifetime: it belongs where someone goes looking for why, not where someone goes looking for whether to upgrade. The 0.3.0 entry had grown three paragraphs of argument about AWS's jitter measurements, why decorrelated is a strategy rather than a mode, and why the reference impls exist. That is ADR material. The entry is now the change list, the new dependency, and the one thing that can break a build, with a link out for the why. ADR004 gains the two decisions that had no home: why both a readable and a seeded jitter path exist, and why Clock and Now are implemented for references -- including the honest note that our own tests hid that gap for two releases by working around it on day one.
ADR004 was carrying two things that are not about jitter: why both a readable and a seeded jitter path exist, and why Clock and Now are implemented for references. Both came out of shipping jitter, but neither is a jitter decision -- they are about whether the deterministic path this crate advertises is actually usable. ADR005 also records the practice that found them, which is the part worth keeping: build a throwaway crate against a path dependency and write the code a first-time reader would write. Our own tests hid the reference-impl gap for two releases because they worked around it on day one, so a suite that lives inside the crate cannot tell you whether the front door opens. Numbering: retry-error and circuit-breaker will shift their ADRs to 006 and 007 when they rebase.
Fair question to get asked: the crate injects time through a trait, so why is randomness a bare fastrand::Rng? Because the two problems are not the same shape. You cannot seed the system clock, so the only way a test controls time is to replace the source, which is what the Clock trait is for. Randomness has a cheaper answer: seeding IS the determinism mechanism, and with_seed already gives full reproducibility without replacing anything. A trait would buy the ability to swap the algorithm, which nobody has asked for and which costs a second type parameter on two public types forever. The coupling is three functions (Rng::new, Rng::with_seed, rng.u64) and fastrand appears in no public signature, so the abstraction is not needed to keep the option of swapping it either. Also fixes an inconsistency the question surfaced: DecorrelatedBackoff's seeded constructor warned that exact values are not an API contract, and the two jitter seed paths -- the ones people reach for first -- did not.
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
1 participant
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.
No description provided.