# 17. SegmentBudget over-admission, and a wait-list to wake denied tasks Status: accepted ## Context `core/docs/m7-baseline.md`'s first pass measured ~70 MB RSS for the M1/M7 DoD's "20 active downloads, `max_active_segments=32`" scenario, against a 60 MB target `docs/adr/0012` estimated would hold with ~45–50 MB of margin. `tools/bench heap-profile` (added to chase this down — no massif/heaptrack in this environment) found the actual cause directly: `Engine::segment_budget().budget().active` read 56–86 against a `total` of 32. The engine was not over budget by a measurement artifact or an ADR 0012 arithmetic gap; it was genuinely running more concurrent segments — and their 1 MiB ring buffers — than `max_active_segments` allows. `SegmentBudget::confirm_slot(id)` checked only `t.held < t.target` — a per-task check. `reallocate_locked()`'s two-pass fairness allocation does correctly bound `sum(target) <= max_active_` at the moment it computes a plan, but that bound says nothing about `sum(held)`: a task can be legitimately holding more than its own just-lowered target for a while, because yield is deliberately deferred to a segment boundary, never mid-segment (ADR 0011 A1). When that happens, another task's target can correctly rise to claim the capacity the first task is *about* to give back, and both `confirm_slot()` calls can succeed against their own, individually-correct targets while the sum of what's *physically held* exceeds `max_active_`. Reproduced deterministically in `core/tests/segment/budget_test.cpp` (`budget_active_never_exceeds_max_active_segments_ under_concurrent_load`, `budget_wait_list_wakes_a_task_whose_target_never_changed`) independent of any real network I/O. ## Decision **`confirm_slot()` also checks `active_ < max_active_`, unconditionally**, as a backstop that doesn't depend on any task's target bookkeeping being perfectly in sync with what every other task currently holds. This is the fix for the invariant itself. That backstop creates a liveness question the original design never had to answer: a task denied *only* by this new check has a target that's already correct — it doesn't change again on its own, so `reallocate_locked()`'s plain "fire a callback when a task's target changes" mechanism never revisits it. Nothing was watching for "this specific task is owed a slot"; **`Task` gained a `waiting_for_slot` flag**, set by `confirm_slot()` on exactly this denial and cleared on the task's next successful confirm however that retry was triggered. `release_slot()` and `deregister_task()` — the only two places that can free real capacity — now call `wake_one_waiter_locked()`, which hands the newly-freed slot to the highest-priority `waiting_for_slot` task (other than the one that just released) via a direct retry hint, if `reallocate_locked()`'s own plan didn't already produce one. On the `download_task.cpp` side, a woken task's `apply_slot_target()` → `fill_slots_locked()` also needed a fix: a segment that had backed off mid-retry (`SegState::stalled`, its `release_slot()` already called, a `retry_worker()` timer already scheduled and possibly already denied once) has no live worker and never surfaces through `Segmenter::assign_slot()` — it stays *assigned*, just not running, and assign_slot() only ever hands out unassigned or fresh ranges. `fill_slots_locked()` now restarts any stalled segment with no live worker directly (bounded by `slot_target`, same as its `assign_slot()` loop) before looking for new work; a segment it doesn't get to keeps its own scheduled `retry_worker()` timer as a second chance, so this is additive, not a replacement for that path. ### What this isn't Production's real `on_target` callback (`register_task()`'s lambda, `download_task.cpp`) never runs synchronously with whatever budget call triggered it — it posts through `host.schedule()` (`engine.cpp`'s single timer thread), so a task's own `mu` can never be re-entered on the same call stack, and two tasks' callbacks can never race each other into an AB-BA lock order either (only one ever runs at a time, on one thread). An earlier version of this fix tried to defend against a same-thread reentrancy hazard that, diagnosed correctly, doesn't exist in production at all — it exists only if a *test double* calls back into the budget synchronously from inside `on_target`, which none of production does. `core/tests/segment/budget_test.cpp`'s `FakeTask` does exactly that (by design — it's simple and every other test in the file drives the budget from one thread at a time, where that's harmless); the one test that drives it from *multiple* concurrent threads (`budget_concurrent_confirm_release_stays_consistent`) uses a separate `AsyncFakeTask` that posts through a small `TestTimer`, mirroring `host.schedule()` / the engine's timer thread for real, rather than adding synchronization machinery to `SegmentBudget` itself to paper over a test double being unlike production. `wake_one_waiter_locked()`'s `exclude` parameter is the one piece of that defense that's independently justified either way — a task doesn't need to be told to retry a slot it just gave back itself — and is the only piece that stayed. ## Consequences - The M7 RSS number now clears the DoD line: `heap-profile` reports 45.41 MiB for the 20-task/8-segment/32-cap scenario (`core/docs/m7-baseline.md`), and `budget.active` never exceeds `budget.total` regardless of contention. - `docs/adr/0016`'s TSan-only load-test straggler (a *different* subsystem — `rate::RateLimiter`'s byte-pacing, not `SegmentBudget`'s segment-admission) is now suspected to have been this same root cause rather than the `CURLOPT_LOW_SPEED_TIME` guess offered there, since the symptom (a task that simply never resumes) matches exactly. Not reverified at the DoD's full shape under `--preset tsan` in this change — `tools/bench`'s sanitizer-preset `load` registration still runs at reduced concurrency. Worth a follow-up run before closing that ADR's postscript. - `docs/adr/0016`'s actual subject — `rate::RateLimiter::TokenBucket`'s own peek/commit race, unrelated to `SegmentBudget` — is untouched by this change and remains open. - `wake_one_waiter_locked()` wakes exactly one task per actual release, by priority order. Under sustained heavy oversubscription (this bench's 20 tasks × 8 segments = 160 wanted against 32 available) a low-priority task can still wait a long time for its fair share — that's the two-pass allocator's own fairness policy working as designed, not a liveness bug: it will get there, just not fast, and every fresh `reallocate_locked()` call (any task's `set_want`, registration, or departure) reconsiders everyone from scratch. No test in this change measures *how* long; if that ever needs a stronger guarantee (bounded wait time, not just eventual service), it's a fairness-policy change, not a wiring fix. Co-Authored-By: Claude Sonnet 5