Files
vdm/core/src/segment/budget.cpp
T
samiandClaude Sonnet 5 322a20efa5 core: fix SegmentBudget over-admission and add a wait-list wakeup
Root cause of the M7 RSS gap (core/docs/m7-baseline.md: ~70 MB measured
against a 60 MB target): SegmentBudget::confirm_slot() only checked a task's
own held count against its own target -- never the engine-wide active_ sum.
reallocate_locked()'s two-pass fairness allocation does 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 (yield is deferred to a segment
boundary, never mid-segment -- ADR 0011 A1), and another task's target can
correctly rise to claim that capacity before the first task has physically
released it. Both confirm_slot() calls could then succeed against their own,
individually-correct targets while sum(held) exceeded max_active_ --
tools/bench heap-profile caught this directly: budget.active reading 56-86
against a total of 32.

confirm_slot() now also checks active_ < max_active_, unconditionally, as a
backstop that doesn't depend on any task's target bookkeeping being in sync
with what every other task holds. That creates a liveness question the
original design never answered: a task denied only by this new check has a
target that's already correct, so it never changes again and
reallocate_locked()'s plain "fire a callback when a task's target changes"
mechanism never revisits it. Task gained a waiting_for_slot flag, set on
exactly this denial; release_slot()/deregister_task() (the only two places
that free real capacity) now hand a freed slot directly to the
highest-priority waiting task via wake_one_waiter_locked(), if
reallocate_locked()'s own plan didn't already produce a callback for anyone.

download_task.cpp's fill_slots_locked() needed a matching fix: a woken
task's stalled segments (SegState::stalled -- backed off mid-retry, its own
release_slot() already called) have no live worker and never surface
through Segmenter::assign_slot(), which only 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.

Also fixes a real TSan-caught data race this work surfaced: SegWorker::
speed_bps was written only by its own segment's curl callback and, before
Progress.speed_bps's polled-path fix, only ever read from that same thread
-- safe without synchronization. snapshot_progress() reading it from
whatever thread calls DownloadHandle::progress() broke that invariant
(workers_mu's shared_lock protects the workers map's structure, not an
individual SegWorker's fields). Now std::atomic<double> with relaxed
ordering -- an informational EMA, nothing synchronizes real state on it --
rather than adding a lock to the write side.

core/tests/segment/budget_test.cpp adds two tests reproducing the actual
gap (budget_active_never_exceeds_max_active_segments_under_concurrent_load,
budget_wait_list_wakes_a_task_whose_target_never_changed) plus a sanity
baseline (budget_release_wakes_a_denied_waiter), and introduces AsyncFakeTask
+ TestTimer for the one existing test that drives the budget from multiple
concurrent threads -- mirroring production's real dispatch (register_task()'s
on_target lambda posts through host.schedule(), download_task.cpp, never a
synchronous call) rather than adding reentrancy-guarding machinery to
SegmentBudget itself to compensate for a synchronous test double being
unlike production. See docs/adr/0017 for the full writeup, including what an
earlier version of this fix got wrong chasing a same-thread reentrancy
hazard that doesn't actually exist in production.

core/docs/m7-baseline.md updated: the RSS number now clears the DoD line
(45.41 MiB via heap-profile), root-caused rather than just re-measured.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ
2026-09-12 10:58:14 +04:00

344 lines
11 KiB
C++

// vdm/segment/budget.cpp
#include "vdm/segment/budget.hpp"
#include <algorithm>
namespace vdm::segment {
SegmentBudget::SegmentBudget() : SegmentBudget(Options{}) {}
SegmentBudget::SegmentBudget(Options opts)
: max_active_(opts.max_active_segments ? opts.max_active_segments : 1),
notify_period_(opts.notify_period) {
notifier_ = std::jthread([this](std::stop_token st) { notifier_loop(st); });
}
SegmentBudget::~SegmentBudget() {
notifier_.request_stop();
notify_cv_.notify_all();
}
// --- allocation -----------------------------------------------------------------------
std::uint32_t SegmentBudget::effective_cap_locked(const Task &t) const {
std::uint32_t base = t.resumable ? t.per_task_cap : 1;
base = std::clamp<std::uint32_t>(base, 1, 32);
if (auto it = host_caps_.find(t.host); it != host_caps_.end() && it->second > 0)
base = std::min(base, it->second);
return base;
}
SegmentBudget::EngineBudget SegmentBudget::snapshot_locked() const {
std::uint32_t starved = 0;
for (const auto &[id, t] : tasks_)
if (t.want >= 1 && t.held == 0)
++starved;
return EngineBudget{max_active_, active_, starved};
}
// DAEMON's list first, then any registered task not in it (defensive; "a running task
// absent from the list sorts last"). Shared by reallocate_locked() and
// wake_one_waiter_locked(), which need the same priority ordering.
std::vector<TaskId> SegmentBudget::priority_order_locked() const {
std::vector<TaskId> order;
order.reserve(tasks_.size());
for (TaskId id : order_)
if (tasks_.count(id))
order.push_back(id);
for (const auto &[id, _] : tasks_)
if (std::find(order.begin(), order.end(), id) == order.end())
order.push_back(id);
return order;
}
// The two-pass fairness allocation. Recomputes every task's target from scratch (so a
// live cap cut naturally produces target < held -> yield), diffs against the last
// published target, and collects the callbacks to fire once mu_ is released.
SegmentBudget::Plan SegmentBudget::reallocate_locked() {
std::vector<TaskId> order = priority_order_locked();
std::unordered_map<TaskId, std::uint32_t> target;
target.reserve(order.size());
std::uint32_t pool = max_active_;
auto capped_want = [&](TaskId id) {
const Task &t = tasks_.at(id);
return std::min(t.want, effective_cap_locked(t));
};
// Guarantee pass: one slot each, in priority order, to anyone who wants one.
for (TaskId id : order) {
if (pool == 0)
break;
if (capped_want(id) >= 1) {
target[id] = 1;
--pool;
}
}
// Growth pass: round-robin the remainder, up to each task's effective cap.
while (pool > 0) {
bool granted = false;
for (TaskId id : order) {
if (pool == 0)
break;
std::uint32_t &tv = target[id];
if (tv < capped_want(id)) {
++tv;
--pool;
granted = true;
}
}
if (!granted)
break;
}
const SteadyTime now = std::chrono::steady_clock::now();
Plan plan;
for (auto &[id, t] : tasks_) {
std::uint32_t nt = target.count(id) ? target[id] : 0;
if (nt != t.target) {
t.target = nt;
if (t.on_target)
plan.targets.emplace_back(t.on_target, nt);
}
// starvation timestamp bookkeeping
bool starved_now = t.want >= 1 && t.held == 0;
if (starved_now && !t.starved_since)
t.starved_since = now;
if (!starved_now)
t.starved_since.reset();
}
EngineBudget eb = snapshot_locked();
bool starved_edge = (eb.tasks_starved == 0) != (last_starved_ == 0);
if (eb != last_notified_)
dirty_ = true;
last_starved_ = eb.tasks_starved;
if (starved_edge && on_changed_) {
plan.notify_now = std::make_pair(on_changed_, eb);
last_notified_ = eb;
dirty_ = false;
}
if (dirty_)
notify_cv_.notify_one();
return plan;
}
void SegmentBudget::run(Plan &p) {
for (auto &[fn, n] : p.targets)
if (fn)
fn(n);
if (p.notify_now && p.notify_now->first)
p.notify_now->first(p.notify_now->second);
}
// --- task-facing --------------------------------------------------------------------
void SegmentBudget::register_task(TaskId id, const TaskParams &params, SlotTargetFn on_target) {
Plan plan;
{
std::lock_guard lk(mu_);
Task t;
t.host = params.host;
t.per_task_cap = params.per_task_cap ? params.per_task_cap : 1;
t.resumable = params.resumable;
t.on_target = std::move(on_target);
tasks_[id] = std::move(t);
plan = reallocate_locked();
}
run(plan);
}
void SegmentBudget::deregister_task(TaskId id) {
Plan plan;
{
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
if (it == tasks_.end())
return;
active_ -= it->second.held;
tasks_.erase(it);
plan = reallocate_locked();
wake_one_waiter_locked(plan, id); // a departing task frees real slots too
}
run(plan);
}
void SegmentBudget::set_want(TaskId id, std::uint32_t want) {
Plan plan;
{
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
if (it == tasks_.end())
return;
if (it->second.want == want)
return;
it->second.want = want;
plan = reallocate_locked();
}
run(plan);
}
bool SegmentBudget::confirm_slot(TaskId id) {
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
if (it == tasks_.end())
return false;
Task &t = it->second;
if (t.held >= t.target)
return false; // target was cut in the race
// reallocate_locked()'s pool math bounds sum(target) <= max_active_ *as computed*, but
// that doesn't bound sum(held): a task can be legitimately over its own just-lowered
// target for a while (yield deferred to a segment boundary, ADR 0011 A1), and another
// task's target can correctly rise to claim that capacity before the first task has
// physically released it. active_ is the one number that's always true regardless of
// any task's target bookkeeping, so it's the backstop. Denials here are remembered
// (waiting_for_slot) rather than left for the caller to somehow ask again at the right
// moment -- see release_slot()'s wake_one_waiter_locked() call.
if (active_ >= max_active_) {
t.waiting_for_slot = true;
return false;
}
t.waiting_for_slot = false;
++t.held;
++active_;
if (snapshot_locked() != last_notified_) {
dirty_ = true;
notify_cv_.notify_one();
}
return true;
}
// Appends a retry hint for the highest-priority task with waiting_for_slot set (other
// than `exclude`) to `plan`. Only ever wakes a task confirm_slot() actually turned away --
// not just anyone below its target, which would also fire for tasks that are fairly,
// correctly not entitled to more right now (see reallocate_locked()'s own target math).
void SegmentBudget::wake_one_waiter_locked(Plan &plan, TaskId exclude) const {
for (TaskId id : priority_order_locked()) {
if (id == exclude)
continue;
auto it = tasks_.find(id);
if (it == tasks_.end())
continue;
const Task &t = it->second;
if (t.waiting_for_slot && t.on_target) {
plan.targets.emplace_back(t.on_target, t.target);
return; // exactly one freed slot, exactly one retry hint
}
}
}
void SegmentBudget::release_slot(TaskId id) {
Plan plan;
{
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
if (it == tasks_.end() || it->second.held == 0)
return;
--it->second.held;
--active_;
plan = reallocate_locked();
// reallocate_locked() only fires a callback for a task whose *target* changed.
// The task this freed slot is actually owed to (see confirm_slot()) may have a
// target that was already correct and hasn't moved -- nothing else will ever ask
// it to retry, so the budget has to remember and hand this slot to it directly.
wake_one_waiter_locked(plan, id);
}
run(plan);
}
// --- DAEMON-facing ---------------------------------------------------------------------
void SegmentBudget::set_max_active_segments(std::uint32_t n) {
Plan plan;
{
std::lock_guard lk(mu_);
n = n ? n : 1;
if (n == max_active_)
return;
max_active_ = n;
plan = reallocate_locked();
}
run(plan);
}
void SegmentBudget::set_host_segment_cap(std::string host, std::uint32_t cap) {
Plan plan;
{
std::lock_guard lk(mu_);
if (cap == 0)
host_caps_.erase(host);
else
host_caps_[std::move(host)] = cap;
plan = reallocate_locked();
}
run(plan);
}
void SegmentBudget::set_task_order(std::span<const TaskId> priority_order) {
Plan plan;
{
std::lock_guard lk(mu_);
order_.assign(priority_order.begin(), priority_order.end());
plan = reallocate_locked();
}
run(plan);
}
SegmentBudget::EngineBudget SegmentBudget::budget() const {
std::lock_guard lk(mu_);
return snapshot_locked();
}
std::uint32_t SegmentBudget::segments_active(TaskId id) const {
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
return it == tasks_.end() ? 0 : it->second.held;
}
std::vector<TaskId> SegmentBudget::starved_tasks() const {
std::lock_guard lk(mu_);
std::vector<TaskId> out;
for (const auto &[id, t] : tasks_)
if (t.want >= 1 && t.held == 0)
out.push_back(id);
return out;
}
std::optional<SteadyTime> SegmentBudget::starved_since(TaskId id) const {
std::lock_guard lk(mu_);
auto it = tasks_.find(id);
return it == tasks_.end() ? std::nullopt : it->second.starved_since;
}
void SegmentBudget::on_budget_changed(std::function<void(EngineBudget)> cb) {
std::lock_guard lk(mu_);
on_changed_ = std::move(cb);
}
// --- notifier thread: coalesced <=4 Hz -----------------------------------------------
void SegmentBudget::notifier_loop(std::stop_token st) {
std::unique_lock lk(mu_);
while (!st.stop_requested()) {
notify_cv_.wait_for(lk, notify_period_, [&] { return dirty_ || st.stop_requested(); });
if (st.stop_requested())
break;
if (!dirty_)
continue;
EngineBudget eb = snapshot_locked();
auto cb = on_changed_;
last_notified_ = eb;
last_starved_ = eb.tasks_starved;
dirty_ = false;
lk.unlock();
if (cb)
cb(eb);
lk.lock();
}
}
} // namespace vdm::segment