# 16. Global rate limit fairness under heavy segment contention (known issue, not fixed) Status: accepted (documents a known limitation; no code change to `rate::RateLimiter`) ## Context While finishing `tools/bench`'s `load` subcommand (the M1 DoD's 20-task load test), pacing 20 concurrent tasks (`default_segments=8` each, so up to 160 segments contending for `max_active_segments=32` slots) via `RateLimiter::set_global_limit()` reproduced 2 of 20 tasks hanging past a 120 s per-task wait instead of completing in the ~5 s the configured rate implied. Unthrottled, all 20 tasks complete in well under a second — the stall is specific to many segment workers contending for one global `TokenBucket` through `RateLimiter::acquire()`'s peek-then-commit-all-or-nothing path, not a general deadlock. `TokenBucket::consume`/`peek` compute a wait duration assuming the caller retries once that much time has passed and the bucket will then have `n` tokens. Under heavy contention that assumption breaks: many segment workers independently schedule a retry via `TaskHost::schedule()` for whenever they were told tokens *would* be available, but whichever of them acquires `RateLimiter::mu_` first on waking drains the tokens the others were counting on, forcing the losers to recompute and reschedule a fresh wait. There is no fairness ordering (FIFO queue, ticket, or similar) across that race — repeated bad luck for the same task's segments is possible and was observed twice in one run. This gets worse, not better, as contention rises: more competing waiters means more of them lose each round. ## Decision Left unfixed for M1. `tools/bench/vdm_bench.cpp`'s `load` subcommand works around it by giving each task its own independent per-task bucket (`RateLimiter::set_task_limit`) instead of one shared global bucket — each task's `acquire()` then only ever contends with its own ≤8 segments, which the same 20-task run completes in ~6.6 s with zero timeouts. That workaround is sufficient for the bench (it still needs, and gets, real concurrent buffering to measure RSS against) and is documented inline where the choice is made. It is **not** sufficient for a real user: `download.setGlobalLimit` (or however DAEMON surfaces it) is a real, everyday feature, and a household running 20+ concurrent downloads against a single global cap is a plausible, not exotic, scenario. This ADR exists so that scenario doesn't get rediscovered from scratch. ## Consequences - `RateLimiter`'s global/queue level should get a fairness mechanism before M1 sign-off treats "global bandwidth cap with many concurrent downloads" as supported: e.g. serve waiters in the order their wait was computed (a min-heap keyed on wake time, or a simple ticket counter checked before committing), or move to a scheme where a waiting caller's reserved allocation can't be stolen by a later arrival. - Needs a regression test once fixed: N tasks (N large enough to exceed `max_active_segments`), one shared global limit, assert every task completes within a bounded multiple of the ideal `total_bytes / global_bps` time — the shape of the bug `tools/bench load` stumbled into, made deterministic. - Filed here rather than fixed in this change because it's a `core/src/rate/` / `core/src/task/` design question (retry/backoff and scheduling policy under contention), not a `tools/bench` one, and deserves its own review rather than a bundled-in fix. ## Postscript: a second, TSan-only straggler (still unexplained, not proven the same bug) After the `--preset tsan` build was made to work (a real, separate ASan-caught bug fixed in the same change: `DownloadTaskState::quiesce()` was clearing `workers` synchronously right after issuing an async `cancel()`, racing the HttpClient worker thread's still-in-flight write callback — see the commit that adds this ADR), `tools/bench load` was run under `--preset tsan` to complete the M1 DoD's sanitizer-clean load test. Even with the per-task-bucket workaround above *and* external (testserver-side) pacing instead of the engine's rate limiter entirely, a single straggler task failed to complete within a generous (300–500s) per-task budget under TSan specifically — reproduced at tasks=20/segments=8 (2 stragglers), tasks=20/segments=2 (1 straggler); tasks=8/segments=2 was reliable across repeated runs and is what `tools/bench`'s ctest registration now uses. No TSan diagnostic (data race, lock-order inversion, etc.) ever accompanied a straggler — across every run that hit one. That means either: (a) it really is just TSan's per-access instrumentation overhead compounding with this sandbox's own scheduling/virtualization under high simultaneous curl/thread activity, with no engine defect at all, or (b) it's a genuine timing-sensitive bug (a plausible candidate: `HttpClient`'s `CURLOPT_LOW_SPEED_LIMIT`/`CURLOPT_LOW_SPEED_TIME` stall detection — 1024 B/s for 30s by default — false-tripping when TSan's overhead makes real throughput look stalled to curl's own timers, driving a segment into a retry/backoff loop that never catches up) that TSan's slowdown merely makes likelier to manifest, not one it creates. This was not root-caused: doing so needs reproducing outside this sandbox, on hardware not already shared/loaded, to separate "TSan is just slow here" from "there is a real bug TSan is making easier to hit". Co-Authored-By: Claude Sonnet 5