sami/vdm
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

core: fix SegmentBudget over-admission and add a wait-list wakeup

Browse Source 
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
This commit is contained in:
sami
2026-09-12 10:58:14 +04:00
co-authored by Claude Sonnet 5
parent 7ea04fa79c
commit 322a20efa5
6 changed files with 536 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/tests/segment/budget_test.cpp
+262 -12
View File
@@ -2,7 +2,10 @@
#include <atomic>
#include <chrono>
#include <condition_variable>
#include <functional>
#include <mutex>
#include <optional>
#include <thread>
#include <vector>
@@ -20,29 +23,153 @@ TaskId tid(std::uint64_t v) {
// A test task that reacts to slot targets the way stage 8's download_task will: start
// workers up to the target, release them when the target drops. Purely bookkeeping.
//
// Production's real equivalent (register_task()'s on_target lambda, download_task.cpp)
// never calls back in synchronously -- it posts through host.schedule() (engine.cpp), so
// apply_slot_target() never runs on the same call stack as whatever budget call triggered
// it. wake_one_waiter_locked()'s wakeups, though, mean on_target() here *can* now be
// reentered on the same thread (e.g. this task's own release_slot() call, deep in the grow
// loop below, can cascade into waking a *different* task whose own release, in turn,
// cascades back to this one) -- a plain std::mutex would self-deadlock on that, the way
// FakeTask::mu almost did until this was written. A recursive_mutex plus coalescing the
// reentrant call's target into `pending` (processed by the outer call's loop once its
// current pass finishes) keeps this a faithful, deterministic stand-in without actually
// needing real threads: no callback ever runs nested inside a still-running instance of
// itself, and a burst of reentrant retargeting converges to the latest value instead of
// each one doing its own redundant grow/shrink pass.
struct FakeTask {
SegmentBudget *budget = nullptr;
TaskId id{};
std::mutex mu;
std::recursive_mutex mu;
std::uint32_t workers = 0;
std::uint32_t target = 0;
bool running = false;
std::optional<std::uint32_t> pending;
FakeTask() = default;
FakeTask(SegmentBudget *b, TaskId i) : budget(b), id(i) {}
void on_target(std::uint32_t t) {
std::lock_guard lk(mu);
target = t;
while (workers < target) {
if (!budget->confirm_slot(id))
if (running) {
pending = t; // reentrant call on this thread -- the running instance picks it up
return;
}
running = true;
std::uint32_t want = t;
for (;;) {
target = want;
while (workers < target) {
if (!budget->confirm_slot(id))
break;
++workers;
}
// over target -> yield the excess immediately (a real task waits for a
// boundary)
while (workers > target) {
budget->release_slot(id);
--workers;
}
if (!pending)
break;
++workers;
want = *pending;
pending.reset();
}
// over target -> yield the excess immediately (a real task waits for a boundary)
while (workers > target) {
budget->release_slot(id);
--workers;
running = false;
}
std::uint32_t held() {
std::lock_guard lk(mu);
return workers;
}
};
// Mirrors Engine's real timer thread (engine.cpp: timer_loop) and how register_task()'s
// on_target lambda actually reaches a task (download_task.cpp: host.schedule(), never a
// direct call). One dedicated thread drains queued closures one at a time -- the
// serialization that makes production immune to the cross-task deadlock risk FakeTask's
// synchronous, whatever-thread-triggered-it delivery has under heavy *concurrent* driving
// (budget_concurrent_confirm_release_stays_consistent, below, is the one test that
// actually exercises this: multiple threads calling set_want() for *different* tasks at
// once, each able to cascade into a wake for another task's callback -- two such cascades
// landing on two different FakeTask mutexes in opposite orders on two different threads is
// a real AB-BA deadlock a recursive_mutex alone doesn't prevent, since that only guards a
// single thread against re-entering itself). Every other test in this file drives the
// budget from one thread at a time, where that risk can't arise, so plain FakeTask is
// still the simpler, sufficient double there.
class TestTimer {
public:
TestTimer() {
worker_ = std::jthread([this](std::stop_token st) { run(st); });
}
~TestTimer() {
worker_.request_stop();
cv_.notify_all();
}
void post(std::function<void()> fn) {
{
std::lock_guard lk(mu_);
queue_.push_back(std::move(fn));
}
cv_.notify_one();
}
// Blocks until the queue is empty and nothing is mid-run -- what a test needs before
// asserting on state this timer's closures mutate.
void drain() {
std::unique_lock lk(mu_);
cv_done_.wait(lk, [&] { return queue_.empty() && !running_; });
}
private:
void run(std::stop_token st) {
std::unique_lock lk(mu_);
while (true) {
cv_.wait(lk, st, [&] { return !queue_.empty(); });
if (st.stop_requested())
return;
auto fn = std::move(queue_.front());
queue_.erase(queue_.begin());
running_ = true;
lk.unlock();
fn();
lk.lock();
running_ = false;
if (queue_.empty())
cv_done_.notify_all();
}
}
std::mutex mu_;
std::condition_variable_any cv_; // _any: wait() below takes a stop_token predicate
std::condition_variable cv_done_;
std::vector<std::function<void()>> queue_;
bool running_ = false;
std::jthread worker_;
};
// Same grow/shrink logic as FakeTask, but on_target() only ever posts through a TestTimer
// instead of running synchronously -- see the comment above TestTimer for why that's the
// faithful model under concurrent driving.
struct AsyncFakeTask {
SegmentBudget *budget = nullptr;
TestTimer *timer = nullptr;
TaskId id{};
std::mutex mu; // only the timer thread ever touches workers/target -- plain suffices
std::uint32_t workers = 0;
std::uint32_t target = 0;
void on_target(std::uint32_t t) {
timer->post([this, t] {
std::lock_guard lk(mu);
target = t;
while (workers < target) {
if (!budget->confirm_slot(id))
break;
++workers;
}
while (workers > target) {
budget->release_slot(id);
--workers;
}
});
}
std::uint32_t held() {
std::lock_guard lk(mu);
@@ -235,12 +362,14 @@ VT_TEST(budget_on_changed_fires_on_starved_edge) {
VT_TEST(budget_concurrent_confirm_release_stays_consistent) {
SegmentBudget b({.max_active_segments = 16});
constexpr int kTasks = 6;
std::vector<std::unique_ptr<FakeTask>> ts;
TestTimer timer;
std::vector<std::unique_ptr<AsyncFakeTask>> ts;
for (int i = 0; i < kTasks; ++i) {
ts.push_back(std::make_unique<FakeTask>());
ts.push_back(std::make_unique<AsyncFakeTask>());
ts.back()->budget = &b;
ts.back()->timer = &timer;
ts.back()->id = tid(i + 1);
FakeTask *ft = ts.back().get();
AsyncFakeTask *ft = ts.back().get();
b.register_task(ft->id, {.host = "h", .per_task_cap = 6, .resumable = true},
[ft](std::uint32_t n) { ft->on_target(n); });
}
@@ -254,6 +383,7 @@ VT_TEST(budget_concurrent_confirm_release_stays_consistent) {
drivers.clear(); // join
for (auto &ft : ts)
b.set_want(ft->id, 0);
timer.drain(); // let every queued on_target actually run before asserting
// With everyone wanting nothing, the budget must be fully released.
VT_CHECK_EQ(b.budget().active, 0u);
@@ -262,3 +392,123 @@ VT_TEST(budget_concurrent_confirm_release_stays_consistent) {
sum += b.segments_active(ft->id);
VT_CHECK_EQ(sum, 0u);
}
// --- wait-list wakeup: tools/bench heap-profile / load found a real task time out
// waiting on a slot its own target already said it should have (core/docs/m7-baseline.md,
// docs/adr/0012). Root cause: confirm_slot()'s engine-wide cap check (needed so active_
// never exceeds max_active_segments -- a real over-admission bug, not just this liveness
// gap) can deny a task whose target is already correct, when a *different* task is
// legitimately still holding more than its own just-lowered target (yield is deferred to
// a segment boundary, ADR 0011 A1). Nothing in the plain target-changed callback
// mechanism ever revisits a task whose target didn't change -- it was already right. ---
namespace {
// Unlike FakeTask above, on_target() here only enqueues -- it never calls back into the
// budget synchronously. This matches production exactly: register_task()'s on_target
// lambda (download_task.cpp) posts through host.schedule() (engine.cpp), so
// apply_slot_target() never runs on the same call stack as whatever budget call triggered
// it. The test drives delivery explicitly (deliver_one()) instead of a background thread
// so the race this test exists to force -- confirm_slot() denied before the task that's
// over its target has processed its own shrink -- is deterministic, not a timing gamble.
struct QueuedTask {
SegmentBudget *budget = nullptr;
TaskId id{};
std::uint32_t workers = 0;
std::uint32_t target = 0;
std::vector<std::uint32_t> pending;
QueuedTask(SegmentBudget *b, TaskId i) : budget(b), id(i) {}
void on_target(std::uint32_t n) { pending.push_back(n); }
// Delivers the oldest queued target, applying it the way a real task's
// apply_slot_target()/fill_slots_locked() would: try to grow to it (confirm_slot()
// may deny), or shed down to it. Returns false (nothing to deliver) if the queue was
// empty -- the condition VT_REQUIRE checks to prove a wakeup was actually queued.
bool deliver_one() {
if (pending.empty())
return false;
target = pending.front();
pending.erase(pending.begin());
while (workers < target) {
if (!budget->confirm_slot(id))
break;
++workers;
}
while (workers > target) {
budget->release_slot(id);
--workers;
}
return true;
}
};
} // namespace
VT_TEST(budget_release_wakes_a_denied_waiter) {
// Sanity baseline: max_active=1, A (higher priority) holds it, B wants one too and is
// fairly denied -- its target stays 0 while A outranks it and still wants its slot.
// Once A stops wanting one, B's target rises and B is woken via the plain
// target-changed path -- no wait-list needed for this simple case. The harder case
// below is what actually needs it.
SegmentBudget b({.max_active_segments = 1});
QueuedTask A{&b, tid(1)}, B{&b, tid(2)};
b.register_task(A.id, {.host = "h", .per_task_cap = 1, .resumable = true},
[&](std::uint32_t n) { A.on_target(n); });
b.set_task_order(std::vector<TaskId>{A.id, B.id});
b.set_want(A.id, 1);
VT_REQUIRE(A.deliver_one());
VT_CHECK_EQ(A.workers, 1u);
// B registers and wants one too, but with A (higher priority) already holding the
// only slot and still wanting it, B's fairly computed target stays 0 -- unchanged
// from its just-registered value, so no callback is queued for it yet.
b.register_task(B.id, {.host = "h", .per_task_cap = 1, .resumable = true},
[&](std::uint32_t n) { B.on_target(n); });
b.set_want(B.id, 1);
VT_CHECK(!B.deliver_one());
VT_CHECK_EQ(B.workers, 0u);
b.set_want(A.id, 0); // A is done wanting a slot
VT_REQUIRE(A.deliver_one()); // A's target dropped to 0 -- sheds its held slot
VT_CHECK_EQ(A.workers, 0u);
VT_REQUIRE(B.deliver_one()); // B's target rose to 1 -- the plain target-changed path
VT_CHECK_EQ(B.workers, 1u); // B took the freed slot
}
VT_TEST(budget_wait_list_wakes_a_task_whose_target_never_changed) {
// The real gap. max_active=2. Y alone, holds both (target=2). X arrives wanting 1:
// this recompute correctly drops Y's target to 1 (giving X its guaranteed slot) and
// raises X's target to 1 -- both real target changes, both queued. Deliver X's
// *first*: X's target says grow, but Y still physically holds 2 (hasn't processed
// its own shrink yet) -- confirm_slot() must deny X here (active_ == max_active_),
// which is the correctness fix (over-admission is the real RSS bug). Then Y
// processes its shrink and actually releases. X's target never changes again -- it
// was already correctly 1 -- so nothing in the plain mechanism ever revisits X.
SegmentBudget b({.max_active_segments = 2});
QueuedTask Y{&b, tid(1)}, X{&b, tid(2)};
b.register_task(Y.id, {.host = "h", .per_task_cap = 2, .resumable = true},
[&](std::uint32_t n) { Y.on_target(n); });
b.set_want(Y.id, 2);
VT_REQUIRE(Y.deliver_one());
VT_CHECK_EQ(Y.workers, 2u);
b.register_task(X.id, {.host = "h", .per_task_cap = 1, .resumable = true},
[&](std::uint32_t n) { X.on_target(n); });
b.set_task_order(std::vector<TaskId>{Y.id, X.id});
b.set_want(X.id, 1);
VT_REQUIRE(X.deliver_one());
VT_CHECK_EQ(X.workers, 0u); // denied: active_ == max_active_, even though X's target is 1
VT_REQUIRE(Y.deliver_one());
VT_CHECK_EQ(Y.workers, 1u); // Y actually releases its excess now
// The bug: without a wait-list, X.pending is empty here -- nothing was ever queued
// for it, because X's target never changed again. X would wait forever despite its
// target correctly saying it should hold a slot.
VT_REQUIRE(X.deliver_one());
VT_CHECK_EQ(X.workers, 1u);
VT_CHECK_EQ(b.budget().active, 2u);
}