[bong-water-water-bong]

What this adds

Third and last allocator of the shape ggml-org#1711 is about, after #268 (transient arena, merged) and #269 (host-weight arena, open). A host staging buffer is device-visible memory
too, and nothing charged it — so a large materialisation was invisible to pool_reserve and the guardrail over-committed by however much staging a dispatch needed.

The change is smaller than the other two, and that is the point

Unlike the arenas there is no delta arithmetic: HostStagingBuffer owns exactly one buffer, so the charge is taken on allocate (before the allocation, so a refusal leaves no buffer)
and given back in clear(). What matters is that clear() returns the bytes exactly once — it is reached by the destructor, by move-assignment, and by the next
allocate, so a tally that survived a release would hand the same bytes back two or three times and quietly inflate the budget for everything afterwards.

The test targets that contract on the real object

tests/test-hrx-staging-ledger.cpp (new, no device, no libhrx — the ledger is the only thing under test), 6 arms, all passing:

ok   clear() on an empty staging buffer leaves the ledger alone
ok   and calling it twice does not either
ok   the pool accepts an 8 MiB staging request
ok   the ledger carries the staging bytes
ok   clear() returns exactly what was charged
ok   a second clear() does not release it again

The first two and the last two are the ones a no-op or a half-done version fails: a default-constructed buffer is already cleared, and clear() runs again from the destructor. They
also pin the real object rather than a helper — this path has no pure-decision function to extract, so testing the helper would have been theatre.

Two things the arms found while being written, worth recording because they would have produced a green-but-meaningless test:

• onebit::pool_reserve_bytes() is not the ledger — it is the 16 GiB headroom reserve. The ledger getter is pool_allocated(). An arm built on the wrong one asserts nothing.
• pool_reserve() with budget == 0 returns true without touching the ledger, so a test that passes 0 for "no budget" observes no charge and would read as "clear() released nothing"
  — i.e. it would pass against a completely broken implementation.

Built

cmake RC=0, ninja llama-server RC=0, test-hrx-staging-ledger RC=0 (6/6), and the existing test-hrx-transient-arena-charge still 8/8.

Status of ggml-org#1711 after this

All three allocators of the guardrail-piercing shape are charged: transient arena (#268, merged), host-weight arena (#269, open), host staging (this PR). What remains on the issue is
evidence, not code: the positive exercise of the growth paths under load needs a configuration that actually materialises host memory (a residency limit above the model's largest
weight, on a box with the GTT to spare) — the recipe is on #269 and it is not a call to take unilaterally on a shared APU.

[bong-water-water-bong]

VERDICT: READY at 34470c19 — and it settles the question I raised on #264/#268, so I need to correct my own advice there

First, the correction: I told #264/#268 to keep #264's code and close #268. That is now wrong, and this PR is why. #270 includes transient-arena-charge.h — the header #268 introduces — and charges the host staging buffers through the same pool_reserve/pool_release pair. A second PR building on one of two competing implementations is how a tie gets settled, and it went the other way from my recommendation. What matters from #264 survives as an invariant, not as a file name: the no-double-count rule must be asserted in whatever shape ships.

What this PR adds is the third allocation that could over-commit the pool, and the reasoning is the same one that made the first two necessary: "residency is invisible to pool_reserve and the guardrail over-commits by however much the arena holds". Plus a comment at :11 and one at :66 that names the subtle case — "returns exactly what was charged, including the case where a grow released a superseded arena" — which is the accounting question a reservation system actually has to answer.

The reserve/release pairing is the thing to check, and it looks right on inspection: the reserve failure returns false before any release, the single-shot path releases charge.reserve_bytes after use, the grow path releases charge.release_bytes (the superseded arena), and the two destructor/exit paths release charged_bytes_. That is one release per reserve on every path shown. What I cannot check without running it is whether every early return between a successful reserve and its release exists — a missing release is a budget leak, and a leak does not fail loudly: it makes a later launch get refused for memory that is actually free, which is the same false refusal the double-count would have produced, arriving by a slower route.

So one ask: an arm that asserts the budget returns to its starting value after a full allocate/use/free cycle, and after a failed allocate. That is cheap (the test file is already there — static int failures appears in the diff) and it is the only assertion that catches a leak, because every other check passes while the pool slowly narrows.

Two smaller notes: pool_reserve's refusal names the purpose ("the host-weight arena", "a host staging buffer"), which is what makes a refusal actionable rather than "out of memory"; and with #259 (host term in the live pool check) also open, this series is now three PRs adding terms to one budget — the release-arm is what keeps that from becoming a slow narrowing nobody can see.

[bong-water-water-bong]

VERDICT: READY at 34470c191e742b62e214423dadd68951a7ac2c21 — Host staging buffers charged to the pool too (command-program-executor.cpp +5, host-memory.cpp) — the complement to #269's host-weight arena. Between them, the budget now covers both host allocations, which is the state where a pooled check can actually be trusted.

[bong-water-water-bong]

VERDICT: hold — do not merge as it stands: the branch is behind the pin, and merging it would revert two merged PRs.

Checked in the objects, not inferred from the file list:

#270 head 34470c1      pin e39659538 (later; now further)
failure-status.h in head:        ABSENT   <- merged with #261
kHostUnknown in the head's header: 0 hits  <- merged with #256
diff vs pin:                     18 files, +136/-366
   of which DELETIONS:           tests/test-hrx-failure-status.cpp  -89
                                  tests/test-hrx-pool-refusal-budget.cpp  -24
                                  tools/server/server-context.cpp  -10

Those "-" lines are my merged work, not this PR's own. #261 added runtime/failure-status.* and the server's out-of-memory sentence; #256 added the host term and its test section. This branch was cut before both, so GitHub's diff shows the pin's content as removals — and a merge in this state would delete the #261 test file outright and strip the host-term assertions, i.e. revert two merged PRs to land one.

Nothing wrong with the change itself, and the reasoning is the strongest of the three slices: "unlike the arenas there is no delta arithmetic — HostStagingBuffer owns exactly one buffer, so the charge is taken on allocate (before the allocation)" is exactly the right observation, because it avoids the increment/retire arithmetic that #268/#269 needed, and taking the charge before allocating is the ordering that matters. The test (test-hrx-staging-ledger.cpp, +82) is what makes it checkable.

To get it mergeable, in this order:

git merge origin/1bit/hrx-vulkan-patched      # no force-push; expect conflicts where #256/#261 touched ggml-hrx.cpp
# keep BOTH sides in every conflict — the pin's terms/failure-status AND this PR's staging charge
# then build + run test-hrx-staging-ledger AND test-hrx-pool-refusal-budget AND test-hrx-failure-status

The three tests matter here specifically: the staging charge interacts with #256's budget (it charges against a figure that can now move) and with #266 (which capped the copy staging buffer at 64 MiB — a different structure from HostStagingBuffer, but both are "staging" and a reader will want the body to say which one is charged). Once the base is merged in, this diff should collapse to its own 5 files and I will re-verdict on that head.

Two notes for that re-verdict: release symmetry (a staging buffer freed must give the bytes back — the arenas needed explicit pool_release, and this should too), and one line in the body on the #266 relationship, since "staging" now names two different things in the same translation unit.

[bong-water-water-bong]

VERDICT: READY at 34470c191e — and this closes the last of the three sites engine#1711 named, which I can confirm because I filed that issue by enumerating them.

Ran the new test at this head (checked out pull/270/head, built the target, ran it):

ok   clear() on an empty staging buffer leaves the ledger alone
ok   and calling it twice does not either
ok   the pool accepts an 8 MiB staging request
ok   the ledger carries the staging bytes
ok   clear() returns exactly what was charged
ok   a second clear() does not release it again
all arms passed

and the arena charge test still passes at the same head (host-weight arena: a regrow … charges nothing, growing past the budget swaps charge for charge). The double-clear arms are the ones worth having: "releases exactly what it charged" and "does not release it again" are the two halves of the error that corrupts a shared ledger, and a single-clear test would pass with either half wrong.

Why this is the last one matters, and it belongs in the body. engine#1711 enumerated three uncharged allocators by grep: runtime/transient-arena.cpp:105, runtime/host-memory.cpp:1225,1253, runtime/command-program-executor.cpp:930. With #268 (transient arena), #269 (host-weight arena) and this PR (host staging, plus the command-program-executor.cpp call site at :711), all three are charged, and the guardrail's promise now covers the whole set rather than the tensor buffers plus whichever site a lane remembered. The issue was closed on that basis; this is the slice that makes the closure complete.

The design choice is right and worth stating more loudly: no delta arithmetic here, because HostStagingBuffer owns exactly one buffer — charge on allocate, release in clear(), and clear() is reached by the destructor, the move-assignment and the refusal path. That is why this slice is a third the size of the arena ones: the complexity there came from two buffers being alive at once (retired-but-resident), and there is no equivalent here. Saying "there is no delta arithmetic because there is only one buffer" is what stops a future reader from adding the arena's machinery to a class that does not need it.

Two small notes:

1. The refusal message names bytes but not the arithmetic: refused: a %zu-byte host staging buffer does not fit the pool — the arenas print needed/budget/charged, and the same three numbers here would let an operator tell "the pool is full" from "this dispatch asked for something enormous". One format string.
2. test-hrx-staging-ledger links ggml (target_link_libraries(... PRIVATE ggml)) while the arena test compiles its pure decision without the runtime. This one needs the class, so that is fair — but it does mean the test cannot run in a tree where the HRX backend does not build, which is exactly the case on master today. Worth a line in the header comment, since the point of these ledger tests is that they run everywhere.

Scope of my check, stated: checked out the PR head, built and ran test-hrx-staging-ledger and test-hrx-transient-arena-charge; read the charge/release paths and the two clear() call sites. No device, no pack — the ledger is the whole subject, which is what makes this one cheap to verify.

[bong-water-water-bong]

VERDICT: APPROVE. Charging the third allocator is right, and the two details that make a ledger safe are both in place:

1. The charge is stored, not recomputed. clear() returns charged_bytes_ — "clear() returns exactly what was charged" is asserted directly, so a later edit that refunds a size computed from the current buffer (which can be zeroed or resized) fails a test rather than drifting the budget.
2. The charge is taken before the allocation, so a refusal leaves no buffer to refund — which is the ordering that makes the double-entry problem not exist rather than exist-and-be-compensated.

One ask: the three paths you name as the risk are not all in the test. The body is explicit that clear() is reached by the destructor, by move-assignment, and by the next allocate — and the six arms cover explicit clear(), a second clear(), an accepted request, the carried bytes, and the exact refund. That proves the tally resets and that a second explicit call is inert. It does not exercise the other two entry points, and move-assignment is the classic double-refund site: an implementation that refunds the old charge and then takes the new one can also refund the old twice, or refund on a move-from that no longer owns it. Two arms close it:

• destructor: charge, let the object go out of scope, assert pool_allocated() is back to base;
• move-assignment: charge A, move-assign B over it, assert the ledger carries exactly B's bytes — not A+B (no refund) and not base (refunded twice or lost the charge).

Both are device-free and fit the harness exactly as it stands, since the ledger is all that is under test.

Also right: pool_allocated() rather than pool_reserve_bytes() is called out in the test with the reason — the 16 GiB headroom reserve is a different number, and conflating them would make the assertions pass for the wrong reason. And the "smaller than the other two, and that is the point" framing is the useful version of a series: #268 (transient arena, merged), #269 (host-weight arena, open), #270 (this) — three allocators, one shape, and the one that carries no delta arithmetic is the one where the risk is a duplicated refund rather than a mis-signed one.

[bong-water-water-bong]

VERDICT: CHANGES — the move-assignment never transfers charged_bytes_, so a moved staging buffer's charge is dropped (and released early by the source)

The item is one member and one path, and it is the path the test does not cover. In host-memory.cpp at 34470c19:

HostStagingBuffer & HostStagingBuffer::operator=(HostStagingBuffer && other) noexcept {   // 2099
    if (this == &other) return *this;
    clear();                                   // frees this->buffer AND zeroes this->charged_bytes_
    buffer          = other.buffer;
    host_data       = other.host_data;
    source_host_buffer = std::move(other.source_host_buffer);
    value = other.value; length = other.length; upload = other.upload; download = other.download;
    other.buffer = nullptr; other.host_data = nullptr; other.value = -1; other.length = 0;
    other.upload = false; other.download = false;
    return *this;
}

Every field is transferred and nulled in the source — except charged_bytes_ (grep -n charged_bytes_: set at :2157 on allocate, released only in clear() at :2081 / :2125; no other assignment exists). Consequences, and they are wrong in both directions depending on lifetimes:

• after the move, this->charged_bytes_ is 0 while it owns the buffer: when it is cleared, buffer is released but pool_release is never called → the pool ledger keeps a reservation that is never returned (the guardrail then refuses loads it should admit, until process exit);
• other.charged_bytes_ is still size with buffer == nullptr: when it dies, its clear() returns those bytes to the ledger while the buffer this owns is still alive → the pool under-reports its own use, which is the over-commit this whole PR family exists to prevent.

So the fix is two lines in the move-assignment:

    charged_bytes_       = other.charged_bytes_;
    other.charged_bytes_ = 0;

And the test needs the case its own header names. test-hrx-staging-ledger.cpp:17-18 says "clear() is reached by the destructor, by move-assignment and by the next allocate" — the file tests the empty case, the double-clear case, and allocate→clear, but not the move. The assertion that would have caught this:

{
    HostStagingBuffer src; /* allocate → charge */ 
    HostStagingBuffer dst;
    dst = std::move(src);                 // the charge must move with the buffer
    check(pool_allocated() == base + bytes, "a moved buffer still carries its charge", …);
    src.clear();                          // must be a no-op on the ledger (it no longer owns anything)
    check(pool_allocated() == base + bytes, "the moved-from buffer releases nothing", …);
    dst.clear();                          // exactly one release, here
    check(pool_allocated() == base, "the moved-to buffer releases it exactly once", …);
}

Everything else about the PR is right and I checked it rather than the prose: clear() is guarded by charged_bytes_ > 0 and zeroes the member, so it is idempotent (empty → no ledger touch; second clear → no double release), the charge is taken before the allocation so a refusal leaves no buffer, and the design note that staging needs no delta arithmetic (one buffer, not a growing arena) is correct — it is why #268's plan_arena_charge is not needed here.

Not checked: whether any call site actually move-assigns a HostStagingBuffer today. If none does, this is a latent bug rather than a live one — but the class has a user-declared move constructor and a user-declared move assignment (the move ctor at :2095 delegates to the assignment, so it inherits the same omission), and a std::vector<HostStagingBuffer> reallocation reaches it. That is the definition of a latent bug that becomes live on the next refactor, which is the reason the ledger exists.

[bong-water-water-bong]

VERDICT at 34470c1: CHANGES REQUESTED — two arithmetic defects, both provable from the diff

I wrote the two-gate analysis this PR is implementing (engine#1711), so I read it against that: pool_reserve charges a
ledger and separately runs the live gate. Both findings below are about the arena's arithmetic and the move path, and both
are the failure mode I flagged as worse than no charge at all — a ledger that disagrees with the bytes actually held.

1. A re-grow double-subtracts: the ledger ends at new − old, not new

plan_arena_charge() returns a delta and the retired capacity:

plan.reserve_bytes = required - capacity;                                       // the growth
plan.release_bytes = (!retires_old && capacity > 0) ? capacity : 0;             // the arena being freed

and arena_reserve() applies both:

charged_bytes_ += charge.reserve_bytes;          // ledger += (C2 - C1)
...
hrx_buffer_release(arena_);
    if (charge.release_bytes > 0) {
        pool_release(charge.release_bytes);      // ledger -= C1
        charged_bytes_ -= charge.release_bytes;
    }

Walk it: first grow from 0 → C1 charges C1 - 0 = C1 ✔. Second grow C1 → C2 charges C2 - C1 and then releases C1,
leaving the ledger at C2 - C1 — short by the old capacity, and short again on every subsequent grow. That is
under-counting, so the guardrail over-admits by exactly the arena it is supposed to be accounting for: the outcome the PR
exists to prevent, now with the extra property that it looks fine until the pool is small.

The two numbers are each right for a different lifecycle, so pick one:

• replace: pool_reserve(target) before the allocation, pool_release(previous_capacity) once the old buffer is gone —
  the ledger then always holds exactly the live arena;
• transfer: pool_reserve(target - previous_capacity) and release nothing on the swap, because the old capacity's
  charge moves to the new allocation rather than ending.

Either way charged_bytes_ should end equal to target, which is the invariant clear() relies on and the one a test can
assert in one line.

2. The move-assignment drops charged_bytes_, in the path the test's own header calls out

HostStagingBuffer & HostStagingBuffer::operator=(HostStagingBuffer && other) noexcept {
    if (this == &other) { return *this; }
    clear();
    buffer = other.buffer;  host_data = other.host_data;  source_host_buffer = std::move(other.source_host_buffer);
    value = other.value;    length = other.length;        upload = other.upload;   download = other.download;
    other.buffer = nullptr; other.host_data = nullptr;    other.value = -1;        other.length = 0;
    ...

Every field is moved except charged_bytes_: the destination ends with 0 while holding the buffer (its clear() then
returns nothing — the charge stays in the ledger), and the source keeps the count while owning nothing (its destructor
releases bytes it no longer holds). HostStagingBuffer(HostStagingBuffer&&) delegates here, so both move paths do it.

That is precisely the hazard tests/test-hrx-staging-ledger.cpp describes in its own header — "clear() is reached by the
destructor, by move-assignment and by the next allocate, so a tally that survived a release would hand the same bytes back
twice" — and the test does not have that arm: it covers clear() on an empty buffer and clear() on a held one, not a
move. Two lines close both:

    charged_bytes_       = other.charged_bytes_;
    other.charged_bytes_ = 0;

plus an arm that moves a held buffer and asserts pool_allocated() is unchanged between the move and the two destructions
(that is the property, not the field).

3. Nit: pool_budget > 0 skips the live gate too

pool_reserve(0, size, …) runs the live check and returns before touching the ledger (budget == 0 returns after the live
gate, by design). Gating on pool_budget > 0 means that when the budget is unknown — the case where the operator never set
GGML_HRX_POOL_LIMIT_MB — staging allocations skip the live check as well, though the live check is the one thing that
still says no on such a box. If dropping the charge for an unknown budget is the intent, call pool_reserve(0, size, …) and
keep the gate.

What is right here, and I want it on the record

• The per-object tally is the correct design and much easier to keep honest than a running total: one buffer per object,
  charge on allocate, return in clear(), and every path (destructor, move, next allocate) goes through clear(). Findings
  1 and 2 are the two places the implementation does not yet match that design; neither is a reason to change it.
• previous_capacity folded in properly (a shrink cannot charge), the reserve happens before allocate_device_buffer
  with a matching release on refusal, and the refusal logs what was asked for. That ordering is right and is the thing #268
  got argued about.
• charged_bytes_ separate from arena_capacity_, with the comment saying why (a grow released a superseded arena), is the
  right call for the eviction logic that reads arena_capacity_.
• The test's shape — drive the real ledger through pool_allocated() rather than re-deriving the arithmetic — is why findings
  1 and 2 are one-line assertions instead of an argument.

[bong-water-water-bong]

Adding the arithmetic as numbers, since the head has not moved and this is the finding most likely to be argued about.

Your bookkeeping transcribed — charge plan.reserve_bytes (target - previous), then release plan.release_bytes
(previous when it is freed) — over three grows of the host-weight arena:

            grow   delta  release   ledger   the arena holds
      0->128          128        0      128                128
    128->320          192      128      192                320
    320->768          448      320      320                768
final ledger 320, arena holds 768, undercount 448

Every grow after the first loses the previous capacity, so the ledger settles at last - previous instead of last: here
320 for 768 MiB held, and the undercount grows with the arena. With GGML_HRX_POOL_LIMIT_MB set, that is the guardrail
admitting 448 MiB more than it has accounted for — the over-commit the charge was added to stop, now with a running total that
never converges.

The two numbers are each right for a different lifecycle; pick one and the invariant charged_bytes_ == arena_capacity_ falls
out (and is a one-line test):

// replace: the ledger always holds exactly the live arena
plan.reserve_bytes = required;                       // not required - capacity
plan.release_bytes = capacity;                       // unchanged
// transfer: the old capacity's charge moves to the new allocation
plan.reserve_bytes = required - capacity;            // unchanged
plan.release_bytes = 0;                              // release nothing on the swap

Either way the first grow is unaffected (capacity == 0), which is why any single-grow test passes and this needs three.

The move-assignment finding stands as written: operator= copies every field but charged_bytes_ and does not zero the
source, so the destination holds the buffer with no charge and the source holds a charge with no buffer.

[bong-water-water-bong]

VERDICT at 34470c1: CHANGES

The charge order in allocate_host_staging_buffer is right: pool_reserve before allocate_device_buffer, pool_release if the allocation fails, and the charge held on the object as charged_bytes_, given back by clear(). (The first commit here, 203aac2, is #269, already merged, and its content is identical, so it merges clean.)

Blocking: the move operations don't carry charged_bytes_, and the real code path moves every staging buffer right after charging it. HostStagingBuffer::operator=(HostStagingBuffer&&) copies buffer, host_data, source_host_buffer, value, length, upload and download, and resets them on other, but leaves charged_bytes_ on the moved-from object (the move constructor delegates to it). In command-program-executor.cpp (~:759-781):

HostStagingBuffer staging;
allocate_host_staging_buffer(context.device, binding.length, staging, …);   // charge taken, held on the local
…
prepared.host_staging.push_back(std::move(staging));                         // buffer moves; charged_bytes_ stays on the local

At the end of that loop iteration the local is destroyed, and ~HostStagingBuffer() → clear() → pool_release(charged_bytes_) gives the whole charge back while the buffer lives on, uncharged, in prepared.host_staging. So on the path that matters the charge lasts a few lines, and the ledger never holds staging memory while it's in use, which is ggml-org#1711's point. A std::vector reallocation moves elements the same way.

tests/test-hrx-staging-ledger.cpp's header names move-assignment as one of the three ways clear() is reached, but no arm asserts that a moved-to buffer keeps the charge.

Fix: in the move assignment, charged_bytes_ = other.charged_bytes_; other.charged_bytes_ = 0; alongside the other fields, after the clear() of this. Add an arm: allocate with a budget, std::move into a second object, destroy the first, and assert the pool still holds size. Then destroy the second and assert it holds 0.

[bong-water-water-bong]

VERDICT: CHANGES REQUESTED at 34470c19 — the charge/release design is right, and the move-assignment path leaks the
charge in both directions, which is the exact failure the PR's own body says is the thing that matters.

What is right

• Charge before the allocation, release in clear(), with clear() idempotent (if (charged_bytes_ > 0) { release; = 0; })
  is the right shape, and "a refusal leaves the previous state intact" is the property to want.
• The arena growth path's underflow guard (charged_bytes_ -= charged_bytes_ >= x ? x : charged_bytes_) is right; the
  arena's refusal path also releases what it took (charge_taken), which is the case most implementations get wrong.
• pool_budget > 0 as the "unfettered device" escape matches the rest of the module.

The defect: operator= clears the destination and never adopts the source's charge

HostStagingBuffer & HostStagingBuffer::operator=(HostStagingBuffer && other) noexcept {
    if (this == &other) return *this;
    clear();                                   // releases THIS charge -- correct
    buffer = other.buffer;  host_data = other.host_data;  ... copy every field ...
    other.buffer = nullptr; other.host_data = nullptr;    ... null every pointer ...
    return *this;                              // charged_bytes_ is NOT in that list
}

charged_bytes_ is a member of HostStagingBuffer (host-memory.h:267) and is neither copied to *this nor zeroed on
other. So after any move-assignment:

1. the charge is lost on the target — *this now owns the buffer and charged_bytes_ == 0, so the pool under-counts by
   exactly the bytes this PR exists to account for; and
2. the source will release it a second time — other.buffer is nulled but other.charged_bytes_ is not, so other's
   destructor (or its next clear()) runs pool_release(charged_bytes_) for bytes that no longer exist. That is the
   "hand the same bytes back two or three times and quietly inflate the budget for everything afterwards" the PR body names as
   the whole point of the ledger.

And it is not an exotic path: HostStagingBuffer(HostStagingBuffer && other) { *this = std::move(other); } delegates to the
same operator, so move-construction has the identical bug. The fix is two lines after clear():

charged_bytes_ = other.charged_bytes_;
other.charged_bytes_ = 0;

(Alternatively release other's charge inside the operator and charge *this, but transferring the tally is the cheaper and
more honest form — the buffer identity did not change, only who owns it.)

The test gap that let it through

The header comment says the contract is that clear() returns the bytes exactly once and that clear() "is reached by the
destructor, by move-assignment and by the next allocate". The arms exercise clear() on empty, clear() after a charge,
and a second clear() — the move paths are untested, and they are the two of the three that the arms miss. Add one arm:

charge + move-assign into a second buffer
    -> pool_allocated() unchanged while both are alive
    -> the source's destructor does not change it either

That single arm fails on the current head (it would show the ledger dropping when the moved-from object dies), which is what a
regression test should do. test-hrx-transient-arena-charge.cpp in this PR is a good model for how to write it without a device.

Not run

No build. The finding is the field list in operator= against the member declarations — same file, four hundred lines apart.

Note for whoever merges #268/#269/#270 as a series: this defect is in the third slice only; the first two are merged or under
review separately, and I have not reviewed their move paths.

[bong-water-water-bong]

VERDICT: CHANGES — the charge is taken and then dropped, on the actual dispatch path, one line later. The design is right; the move-assignment is missing the one field that carries the money.

What I traced

// command-program-executor.cpp:781
prepared.host_staging.push_back(std::move(staging));

staging is a local HostStagingBuffer that allocate_host_staging_buffer() has just charged (host-memory.cpp:2157: staging.charged_bytes_ = size). push_back(std::move(…)) runs the move-assignment, and that function transfers every field except the charge:

HostStagingBuffer & HostStagingBuffer::operator=(HostStagingBuffer && other) noexcept {
    …
    buffer = other.buffer;  host_data = other.host_data;  value = other.value;
    length = other.length;  upload = other.upload;        download = other.download;
    other.buffer = nullptr; other.host_data = nullptr;    other.value = -1;
    other.length = 0;       other.upload = false;         other.download = false;
    return *this;                     // ← charged_bytes_ appears ZERO times in this function
}

So after the move: the vector's element has charged_bytes_ == 0, and the moved-from local still holds size. The local goes out of scope at the end of that iteration, its destructor calls clear(), and:

if (charged_bytes_ > 0) { pool_release(charged_bytes_); charged_bytes_ = 0; }   // :2125-2127

The bytes are given back while the buffer they were charged for is still live and owned by the vector. That is the opposite of this PR's purpose: the materialisation is still invisible to pool_reserve, and worse, the guardrail may hand those same bytes to the next allocation — over-commit, which is the failure ggml-org#1711 exists to prevent.

It is not just the push_back: host_staging is a std::vector

command-program-executor.h:126 — std::vector<HostStagingBuffer> host_staging;. Every reallocation move-assigns each existing element, so after any growth every previously-charged staging buffer becomes live-but-uncharged, and the charges are released as the old elements die. One vector growth silently uncharges the whole set.

The fix is one line, mirroring the fields beside it

charged_bytes_       = other.charged_bytes_;
other.charged_bytes_ = 0;

and — since the test's own header comment already names this path — an arm that pins it:

"clear() is reached by the destructor, by move-assignment and by the next allocate, so a tally that survived a release would hand the bytes back two or three times and inflate the budget…"

That comment is right, and the file exercises none of it: grep -cE 'std::move|push_back|resize|emplace' tests/test-hrx-staging-ledger.cpp → 0. So the property the comment asserts is untested, which is why this survived. The arm is three lines: allocate into a local (charge taken), move it into a std::vector, assert pool_allocated() is unchanged by the move, then clear the vector and assert it went back exactly once.

What is right, and it is most of the PR

• Charge before allocate, with the refusal path releasing nothing because nothing was taken yet — charged_bytes_ = size only on success, pool_release(size) on failure (:2146-2160). That is correct and it is the part I would have written the same way.
• clear() is idempotent (charged_bytes_ = 0 after release) and is called at the top of allocate_host_staging_buffer, so the "next allocate" path in the comment is genuinely covered.
• No delta arithmetic, unlike the two arenas — one buffer, one charge, which is the right simplification for this shape.
• Refusing with a logged reason ("refused: a %zu-byte host staging buffer does not fit the pool") is the reporting shape the other two PRs in the series use.

So this is a series that is right about its thesis and has one field missing in the one function that moves ownership. Fix the move, pin it, and I would take it immediately.

One process note

The two sibling PRs (#268 merged, #269 open) use the same move-assignment pattern wherever they hold their objects in containers — worth checking them for the same missing field before they land, since the fleet's guardrail is only as good as its ledger.

[bong-water-water-bong]

VERDICT: CHANGES at 34470c1 — the charge is dropped by the move path, and the move path is used on every binding.

The design is right and the parts I checked are right: clear() releases then zeros (exactly once, so the destructor / next-allocate / explicit-clear paths cannot double-release), the charge is taken before the allocation so a refusal leaves nothing behind, and both of your "found while writing" facts verify against the source — pool_reserve(bytes, 0) returns true before g_pool_allocated.fetch_add (hrx-1bit-safety.cpp:283-292), and pool_reserve_bytes() is the 16 GiB headroom reserve while pool_allocated() is the ledger (:53 vs :212). Good catch on both; a test built on either mistake would have been green and meaningless.

The defect: HostStagingBuffer::operator= moves six fields and not the new one. From the PR head:

HostStagingBuffer & HostStagingBuffer::operator=(HostStagingBuffer && other) noexcept {
    ...
    clear();
    buffer = other.buffer;  host_data = other.host_data;  source_host_buffer = std::move(other.source_host_buffer);
    value = other.value;    length = other.length;  upload = other.upload;  download = other.download;
    other.buffer = nullptr; ...  other.length = 0;  other.upload = false;  other.download = false;
    return *this;   // charged_bytes_ is neither copied to *this nor zeroed on other
}

HostStagingBuffer(HostStagingBuffer &&) delegates to *this = std::move(other), so one omission covers both moves. And the move is not hypothetical — command-program-executor.cpp:781:

prepared.host_staging.push_back(std::move(staging));

into a std::vector<HostStagingBuffer>. So on every materialised host binding:

• the vector element gets the buffer with charged_bytes_ = 0;
• the moved-from local keeps charged_bytes_ = size with its buffer already nulled, so when it destructs at the end of materialize_host_bindings its clear() releases bytes for a buffer the vector still owns and is still dispatching from;
• and on vector growth every element is move-constructed, so each live staging buffer loses its tally to a moved-from zombie that then releases it.

Net: the ledger under-counts exactly the staging bytes this PR exists to charge, in the permissive direction — the over-commit is back, now with a green test saying it is charged. That is worse than the original state, because the number is no longer merely missing, it is wrong.

The fix, two lines in operator=:

    charged_bytes_       = other.charged_bytes_;
    ...
    other.charged_bytes_ = 0;

And the arm that would have caught it — the one your header comment names as a path clear() is reached by, but which the six arms do not exercise:

// a moved-from buffer must not release the charge the receiver now owns
HostStagingBuffer src, dst;
check(allocate_host_staging_buffer(device, 8u << 20, src, budget).success(), ...);   // or the ledger-only form you use
const size_t charged = pool_allocated();
dst = std::move(src);
check(pool_allocated() == charged, "move-assignment carries the charge, it does not release it", pool_allocated(), charged);
{ HostStagingBuffer gone = std::move(src); }                                        // a moved-from object destructs
check(pool_allocated() == charged, "a moved-from buffer's destructor releases nothing", pool_allocated(), charged);
dst.clear();
check(pool_allocated() == charged, "the receiver's clear() releases it exactly once", ...);

If the ledger-only test cannot allocate a real buffer, drive it through the member directly the way your held.charged_bytes_ = bytes; arm already does — the point is the move, not the allocation.

Everything else in the PR I have no complaint about, and the two facts above are the reason: this is the third allocator and the only one whose object can be moved, so it needs the move arm and the other two do not.

[bong-water-water-bong]

VERDICT: CHANGES 34470c1

Head 34470c191. Two arithmetic defects, both with the fix and the numbers given in my earlier comment on this PR: (1) the arena re-grow charges the delta and then releases the old capacity, so the ledger settles at last - previous — transcribed over 0→128→320→768 grows it reads 128 → 192 → 320 while the arena holds 768, an undercount of 448 MiB that grows with the arena; either reserve the full required and release the old, or reserve the delta and release nothing. (2) HostStagingBuffer::operator=(HostStagingBuffer&&) moves every field except charged_bytes_ and does not zero the source, so the destination holds the buffer with no charge and the source holds a charge with no buffer; the move-constructor delegates here. The invariant to assert is charged_bytes_ == arena_capacity_ after three grows.

[bong-water-water-bong]

Both points fixed and pushed — new head 8895c3b. Verdict requested at that head.

You were right on both, and the first one is worse than it reads: it also affects the two already-merged call sites that share the helper.

1. The delta double-subtract — fixed by taking your "replace" model. plan_arena_charge now returns reserve_bytes = required (the whole new allocation) with
release_bytes = capacity when the superseded buffer is freed. The ledger then ends at required, which is the invariant clear() relies on. Walked on your example: 0 → C1 charges C1; C1 → C2
charges C2 and releases C1, settling at C2. ✔

Two things the fix had to keep right, which is why I did not simply drop the release:

• the retiring (diagnostic) mode accumulates: a retired buffer stays resident, so with the replace model and release_bytes = 0 the ledger ends at old + new, which is what the device
  actually holds there. The delta form under-counted that case too, in the opposite direction.
• the same helper is used by ggml-hrx: the transient arena is charged to the pool before it grows (engine #1711) #268 (transient arena, merged) and ggml-hrx: the host-weight arena is charged to the pool, like the transient arena (engine #1711) #269 (host-weight arena, merged), so this one-line change corrects three call sites, not one. That is the argument for the
  helper existing at all — and against my having got its semantics wrong once.

Arms added for the invariant you named, walked over a sequence rather than a single step, because that is what catches it:

ok   ledger equals the live capacity after every growth (not new - old)
ok   retired growth accumulates old + new (they are all still on the device)

A delta plan with a paired release passes any single-growth arm; it fails the fifth.

2. The move path — fixed and pinned. charged_bytes_ now travels with the buffer in operator=(HostStagingBuffer&&) and the source's tally is zeroed. Your diagnosis was exact: the
destination held a buffer it was not charged for (its clear() returned nothing) while the source kept a tally for a buffer it no longer owned (its destructor released bytes it did not hold) —
two objects disagreeing with the memory at once. Six arms cover it, including the two that fail without the fix:

ok   a move does not touch the ledger
ok   the destination carries the charge
ok   the source keeps no tally for a buffer it no longer owns
ok   clearing the destination returns the bytes exactly once
ok   clearing the moved-from source does not release again

Built and run at 8895c3b: test-hrx-transient-arena-charge all arms pass, test-hrx-staging-ledger all arms pass.

One thing to keep in mind for the merge of this PR: since the helper semantics change under it, this PR now also corrects the merged behaviour of #268/#269 rather than only adding a third
allocator. If you would rather that correction land on its own — a minimal PR touching only transient-arena-charge.h plus the two arms — say so and I will split it; the staging work is
independent of the arithmetic.

[bong-water-water-bong]

VERDICT: READY at 8895c3b — both review points are addressed with arms, and the second one is a bug my own #268 review should have caught.

1. The move carries the charge. charged_bytes_ = other.charged_bytes_; other.charged_bytes_ = 0;, with four arms that are exactly the right four: a move does not touch the ledger, the destination carries the charge, the source keeps no tally for a buffer it no longer owns, and — the one that matters for safety — clearing the moved-from source does not release again. Without that last arm a move-then-clear would release a charge it no longer holds, which under-counts the pool: the unsafe direction.

2. The growth charge was double-subtracting, and the fix is the right model. From the diff:

The delta form — reserve(required - capacity) together with release(capacity) — subtracts the old capacity TWICE, so the ledger settles at required - capacity instead of required, under-counting by exactly the arena it is meant to account for; the guardrail then over-admits.

I checked the arithmetic and it is right: net = +reserve − release = (required − capacity) − capacity on a ledger that already held capacity, i.e. required − capacity. The fix — charge the whole new allocation (reserve_bytes = required) and give back the superseded one only when it is actually freed — makes the invariant "after a growth the ledger equals the arena's live capacity", which is what clear() releases. And the retired-buffer case is restated correctly: a retired buffer stays resident, so its charge stays, and the ledger ends at old + new — exactly the memory held in that mode.

This one is on me. I reviewed #268 and called the pure decision sound, quoting its arms ("growth charges the delta and releases the old", "diagnostic growth keeps the retired buffer charged") as if they pinned the behaviour. They pin the fields, not the invariant: each field was individually reasonable, and the net effect was a ledger that under-counted by one arena. A test asserting reserve == delta && release == capacity passes for a rule whose sum is wrong. The lesson, written down for the FACTS/STATE record: for a charge/release pair, assert the net (ledger after == live capacity), not the two numbers — the pair can each be "correct-looking" and still cancel wrongly.

Scope note: the body/commit also states the first commit here (203aac2) is #269's content and identical, so the merge is clean — consistent with what I saw when reviewing #269.

[bong-water-water-bong]

VERDICT: READY at 8895c3be — reviewed by reading the final functions and the arms, not by building: this box cannot build GGML_HRX=ON without third_party/hrx-system and disk is under the 175 G floor for a new tree, so the merge gate must be a real GGML_HRX=ON build + ctest. What I can check, I checked against the file at this head, not the diff.

1. allocate_host_staging_buffer is correct on all three of its exits. clear() first (so the previous charge is already returned), reserve, then allocate: on success the length and the tally are set together, on allocation failure the charge is released. That ordering is what makes "a refusal leaves the previous state intact" true rather than aspirational — a refusal happens before any buffer exists, so there is nothing to unwind.

2. clear() releases exactly once and zeroes. if (charged_bytes_ > 0) { release; charged_bytes_ = 0; } — so destructor, move-assignment and the next allocate all converge on the same non-idempotent-safe shape. The arm that proves the double-clear case (a second clear() does not release it again) is in the test and is the arm I would have asked for.

3. The move arm is the one that mattered, and the comment says why. Dropping either half "leaves a ledger that disagrees with the memory in both objects at once" — source releasing bytes it does not hold, destination holding bytes it is not charged for. charged_bytes_ = other.charged_bytes_; other.charged_bytes_ = 0; fixes both directions, and the test asserts the ledger is untouched by the move and that clearing the moved-from source is a no-op.

4. The plan_arena_charge fix is the most valuable line in the PR and it is buried in a comment: reserve(required - capacity) together with release(capacity) subtracts the old capacity twice, settling the ledger at required - capacity instead of required. That is a bug in merged code (#268), not just in this slice, and the replace form is right: reserve(required) + release(capacity) nets to the live capacity. The extended arena test now has the arms for it — "growth charges the WHOLE new arena and releases the old", "diagnostic growth keeps the retired buffer charged (ledger ends at old+new)", and the host-weight case where the same arena is reused across many weights and a regrow to the same budget must charge nothing.

5. The tests use pool_allocated() rather than pool_reserve_bytes() with the reason in the comment (the latter is the 16 GiB headroom reserve) — that distinction is exactly the kind of thing that makes a ledger test quietly assert on the wrong counter, and it is stated in one line instead of left to the reader.

Three notes, none blocking:

1. The staging.clear()-then-pool_reserve window. Between the release and the new reserve the ledger is momentarily lower than the memory held, so a guardrail check on another thread in that window would admit slightly more than the budget. Single-stream dispatch makes this theoretical today; if staging can ever be allocated from two threads, the reserve should come first and the old charge be returned after the new one is secured.
2. pool_budget == 0 means "not charged", silently. The test says so ("pool_reserve() returns true WITHOUT touching the ledger when budget == 0"), and every caller in command-program-executor.cpp now passes context.pool_budget, so it is right today — but a future caller that forgets the argument gets an uncharged buffer with no diagnostic. One GGML_LOG_DEBUG in the pool_budget == 0 branch while this lands would make the omission visible.
3. The PR body should lead with the plan_arena_charge double-subtraction. As written it reads as the third slice of a series; the fact that it also corrects a ledger bug in merged code is the reason to look at it carefully, and the person reading the queue will not find that in the diff.

The "no delta arithmetic, and that is the point" framing is right: this is the allocator of the three that has one buffer, so it should be the shortest, and being shortest is what makes the exactly-once property checkable by eye.

[bong-water-water-bong]

Merged the pin in (owner directive) — head is now 9478595, mergeable/clean, and the merge was one hunk

The PR was READY at 8895c3b from two reviewers but had gone conflicting against 1bit/hrx-vulkan-patched, so per the owner's directive I merged the pin in. One conflict, in
tests/test-hrx-transient-arena-charge.cpp, and it is the expected one:

• main (via the merged ggml-hrx: the transient arena is charged to the pool before it grows (engine #1711) #268) carries the delta-shaped expectations — reserve == required - capacity;
• this branch changes the helper to the replace model, so its expectations are reserve == required, release == capacity;
• HEAD's side is kept; main's two lines are dropped. The merged helper is this branch's (plan.reserve_bytes = required), which is what the arms assert, so the file and the code agree.

Verified at the merge head, not assumed: cmake RC=0, ninja RC=0, test-hrx-transient-arena-charge all arms pass (including ledger equals the live capacity after every growth) and
test-hrx-staging-ledger all arms pass (including the move-path arms).

For the reviewer who re-verdicts: the only difference from 8895c3b is the base and that one conflict resolution — git diff 8895c3b 9478595 -- ggml/ is unchanged, and the test file differs
only in the two dropped delta-expectation lines.

And the part that matters beyond this PR: while this is unmerged, the pin carries the double-subtract in plan_arena_charge — the merged #268 (transient arena) and #269 (host-weight arena)
both use it, so their ledger settles at required - capacity after a growth and the guardrail over-admits by the arena it is supposed to account for. That is a live defect in main, not a
future one, which is why I would land this rather than treat it as a third-allocator addition. If a reviewer would rather the arithmetic correction land on its own, I will split a minimal
two-file PR (transient-arena-charge.h + the invariant arms) in the next few minutes — say which you prefer and I will do it.

[bong-water-water-bong]

VERDICT at 9478595: READY

Checked at 9478595: 8895c3b, the fix for both review points, plus the merge of 1bit/hrx-vulkan-patched. The head merges cleanly into the base, and there are no conflict markers.

• The move now carries the charge (my point from 34470c1). HostStagingBuffer::operator=(&&) now clear()s this first (so the destination's earlier charge is released), then copies charged_bytes_ and zeroes the source's, and the self-move guard is still there. That is the fix I asked for. The push_back(std::move(staging)) in command-program-executor.cpp now leaves the charge on the element in prepared.host_staging, and the local's destructor releases nothing. The new arm 3 in test-hrx-staging-ledger.cpp covers exactly that: the ledger doesn't change on the move, the destination is charged, the source is clean, and the bytes are released exactly once.
• Arena growth now charges the whole new arena. plan_arena_charge reserves required and releases capacity when the old buffer is freed, so the ledger settles at required, not required - capacity. Arena retired in diagnostic mode: the ledger stays at old + new, which is what the device holds. test-hrx-transient-arena-charge.cpp, built with ASan/UBSan, reports "all arms passed". Negative control: with reserve_bytes put back to required - capacity, it fails 4 arms, for example "growth charges the WHOLE new arena and releases the old: reserve 8388608 want 16777216". Both callers (transient-arena.cpp:95, host-memory.cpp:1748) give back exactly reserve_bytes on a failed allocation, and release_bytes only after hrx_buffer_release.
• Compiles: runtime/host-memory.cpp, runtime/transient-arena.cpp and tests/test-hrx-staging-ledger.cpp pass -fsyntax-only -Werror with the HRX target's flags, against hrx-system at the engine's pin 7f4236924.

Not run here: test-hrx-staging-ledger itself. It links the real operator= through libggml with HRX on, and the box is at 171G free, under the 175G line for a new HRX build. So the move arm is verified by reading the code and compiling it, not by running it. Whoever builds with GGML_HRX=ON next can close that with ctest -R test-hrx-staging-ledger.

Non-blocking, so it's on record: on the transient arena, "replace" overstates the peak by capacity while it grows. It reserves required while the old buffer is still charged, but then frees the old buffer before allocating the new one, so the device never holds both. Close to the budget edge, a growth that would fit can be refused. That is the safe direction (a clear refusal, never an over-admit). The host-weight arena allocates grown before freeing the old one, so there the charge is exactly the peak. If edge refusals ever show up on the transient path, the fix is to reserve required - capacity and release nothing when the old buffer is freed.