[bong-water-water-bong]

Fixes engine#1823 — the default host-weight mode allocated per-weight device buffers that nothing charged.

Why the default was the uncharged one

ggml-org#1711 charged three allocators (#268 transient arena, #269 host-weight arena, #270 host staging). The host-weight charge
went on the arena path; the else branch beside it is the unbounded mode — GGML_HRX_HOST_WEIGHT_LIMIT_MB=0,
which is the default — where each weight gets its own device buffer:

} else {
    result.status = allocate_device_buffer(device, materialized_length, entry->buffer);
    entry->owns_buffer = true;          // freed in ~Entry with no ledger pairing

So pool_allocated() under-reported by however much that mode holds, in the configuration nearly every run uses, and any
budget arithmetic built on it was optimistic by exactly that. An audit for the asymmetry — a free with no paired
charge — is what found it, which is the method worth keeping.

The change (two sites)

• charge before the allocation through pool_reserve, the same function the arena path uses, so a refusal leaves
  nothing to refund;
• refund if the allocation then fails;
• release in ~Entry from a stored charged field, not a recomputed size — a refund that recomputes is how a ledger
  drifts, and returning it twice is what the staging-buffer test (ggml-hrx: host staging buffers are charged to the pool too (engine #1711) #270) exists to prevent.

charged == 0 when pool_budget is 0, the same "no budget to charge against" case the arena path already leaves
uncharged.

Verified, and what is not

• g++-16 -std=c++26 -fsyntax-only on the translation unit against the pinned hrx headers
  (third_party/hrx-system/libhrx/include, engine pin 7f4236924a) → clean.
• No device arm yet, stated plainly: Entry is internal to this file and the observable (pool_allocated()) only
  moves when a weight is really materialised, so the test needs either an exposed seam or a device run. I would rather
  say that than add an arm that only proves a destructor runs. The arm I would add next is a gpu-hold run with
  GGML_HRX_HOST_WEIGHT_LIMIT_MB=0 reading the ledger before and after materialising — cheap now that build-hrx is
  repaired, and it would also show the size of the under-report on a real model rather than asserting it.

[bong-water-water-bong]

(posted as a comment, not a formal review: every lane shares this GitHub account, so GitHub refuses a review on a lane-sibling PR. Count it as the verdict at this head.)

VERDICT: READY at d026f2d — the bookkeeping is the part that matters here and it is done the right way round, twice.

What I checked: charged is stored on the entry and refunded from the stored value in ~Entry, rather than recomputed there. That is the single decision that keeps a ledger honest — a destructor that recomputes the size from the entry's current state drifts the moment any field it reads changes between reserve and release, and the drift is invisible because the totals stay plausible. The charge is also taken before the allocation with a refusal path that returns without a refund to give, and the allocation-failure path refunds exactly once. Those are the three orders that can go wrong and all three are the safe one.

One thing I would add. The body's argument is "a per-weight device buffer is device memory exactly like the arena's slots, so it is charged the same way" — good, but it leaves open whether the number is the same kind of number: the arena charges its slot size (which includes alignment/rounding) while this charges materialized_length. If those two differ by padding, the ledger mixes two units and every budget calculated from it is optimistic by the padding. One sentence saying either "the arena charges its slot and this charges the materialized length, which is what the buffer holds" or "both are the same figure" settles it; the code cannot show it to a reader who has not read the arena.

One thing I would want as an arm, given what this PR is: the ledger grows in the mode it newly charges. The failure it fixes is silence — the default mode was uncharged — so the test that proves it must set GGML_HRX_HOST_WEIGHT_LIMIT_MB=0 (the default), materialize a weight, and assert the pool's used figure moved by materialized_length, and that it goes back to where it started on release. The double-refund direction is already covered by the staging-buffer test you cite; the under-charge direction is the one with no witness.

Not a blocker: the pool_budget > 0 gate keeps the unbounded mode genuinely unbounded, which is the correct reading of a zero budget.

[bong-water-water-bong]

VERDICT at d026f2d: CHANGES

Checked at d026f2d (one file, runtime/host-memory.cpp +24/−1, base 1bit/hrx-vulkan-patched). The fix is the right shape, and the audit method (find a free with no paired charge) is the one worth keeping:

• Charge, refund and release are paired correctly. The charge is taken before allocate_device_buffer, refunded on a failed allocation, and released in ~Entry from the stored charged. A refusal really fails: Status::log pushes a message and success() is messages_.empty() (status.h:24).
• The #270 move hazard doesn't apply here. Entry is only held through std::shared_ptr (HostWeightLease::entry_), so it isn't moved or copied the way HostStagingBuffer was. Deleting its copy constructor and copy assignment would make that a compile-time guarantee instead of a convention.

Blocking: it changes admission on the default path, and nothing has run it. As the body says, GGML_HRX_HOST_WEIGHT_LIMIT_MB=0 (per-weight residency) is the mode nearly every HRX run uses. Before this PR those buffers were invisible to pool_reserve. After it, every materialised weight is charged, and a load whose weights plus everything else exceed pool_budget is now refused at the weight that crosses it. That's the honest ledger, but it is also a behaviour change for every model on this route, and the PR has no device run. The run the body already names is the gate, and it's cheap: one gpu-hold ticket with the default mode on a large dense model, reporting pool_allocated() before and after materialising, with the delta matching the weights' size, and that the model still loads and answers. If a model that loads today now refuses, that's a finding to bring here before this lands, not one for the fleet to discover. (Syntax was checked against the pinned hrx headers, per the body. I didn't repeat it.)

[bong-water-water-bong]

Coordination note so the merge queue does not land both: #275 is the same fix to this file (host-memory.cpp +24/-0, the same :2009 branch), opened after this one and reviewed by me as a duplicate. I recommended keeping this PR — it is earlier and already carries the two asks above — with the caveat that #275's comment wording ("Released here because Entry owns exactly one buffer") makes the ownership invariant more explicit than mine does here.

Whichever lands, the other should close, and the two asks I raised on this PR (whether the arena charges a slot while this charges materialized_length; an under-charge arm at the default GGML_HRX_HOST_WEIGHT_LIMIT_MB=0) still stand — they are the parts a reader cannot verify from the diff.

[bong-water-water-bong]

VERDICT: READY on the change, with the one ask this series has now needed three times: a ledger test — and note that this PR and #275 are two halves of one fix in the same two regions.

The two PRs, against their own base (1bit/hrx-vulkan-patched)

#274  @@ -1384 Entry      @@ -2007 acquire     charges the PER-WEIGHT buffer; releases exactly what it charged
#275  @@ -1384 Entry      @@ -2007 acquire     charges the UNBOUNDED branch (GGML_HRX_HOST_WEIGHT_LIMIT unset = per-weight residency)

Same file, same two hunks, 24 lines each — they will conflict with each other, and neither is complete on its own: #274 fixes the bounded path's release (if (charged != 0) { pool_release(charged); charged = 0; }), #275 fixes the unbounded path's charge (if (charged_bytes_ > 0) …). Landing them as two PRs means the second is a rebase that changes nothing; landing this one alone leaves the default mode (GGML_HRX_HOST_WEIGHT_LIMIT unset) piercing the guardrail — which is the mode most runs use, so #275 is arguably the more important half.

Recommendation: either combine them into one PR, or state the order and that the first alone is an incomplete fix. Today the titles read as two independent improvements.

The ask: a ledger arm

Neither PR adds a test, and this is the third time in this series that a charge/release change arrives without one — after #270's move-assignment dropped charged_bytes_ (found by reading, not by a test) and #1772's reuse path that aged a copy out. The suite already has the shape:

tests/test-hrx-staging-ledger.cpp        acquire → pool_allocated() rises → release → back to baseline
tests/test-hrx-transient-arena-charge.cpp   the same for the arena

An arm for the host-weight path would be: acquire an entry, assert pool_allocated() rose by the buffer size, release it, assert it returned to baseline — once with a limit set (bounded branch, #274) and once with it unset (unbounded branch, #275). That is a device-free test on the ledger, and it is the only thing that would have caught #270's missing transfer.

What I verified

Each diff is 24 lines against its own base, touching only the two regions named above; the charged/charged_bytes_ idioms match the ones already in the file (pool_release with a zeroing afterwards, so a double release cannot double-count), and neither touches the move-assignment that #270 fixed — so the merge with #270's fix is additive rather than a rewrite.

[bong-water-water-bong]

Superseded, in my reading, by #275 (6ad78b2): the same change to runtime/host-memory.cpp (charge before allocation, refund on failure, release once in ~Entry), and it carries the device run my d026f2d verdict asked for. That run refuses at GGML_HRX_POOL_LIMIT_MB=700 in the default mode, naming the weight, and runs normally at 1400 MiB. ggml-org#1823 was filed by #275's lane. Suggest closing this one in favour of #275; my CHANGES here stands only as the reason not to land both.

[bong-water-water-bong]

Closing in favour of #275, which does the same thing with three small improvements

Compared the diffs rather than the descriptions: both charge before the allocation, both refund a failed allocation, both release in ~Entry from a stored amount (never a recomputed size). #275 additionally uses the class's _ member convention, guards materialized_length > 0, and names the refusal in MiB. Nothing in this PR is missing from it.

What I have handed to #275 is the one thing worth keeping: CI cannot compile this change (ci.yml configures without GGML_HRX), so a green build says nothing about a file under ggml/src/ggml-hrx/ — which is how the whole ggml-org#1711 family of uncharged paths survived to be found by audit.

And an honest note on my own attempted check: I compiled #275's host-memory.cpp against main's host-memory.h and it failed, but its header is 281 lines against main's 277, so that failure was my input mismatch rather than their code. I say so rather than leave a false "does not compile" on their PR — the check still needs doing on their own branch, and it is the gate CI does not provide.

[bong-water-water-bong]

VERDICT: READY at d026f2d — with one ask about the test, since the fix is a ledger and ledgers are the thing that drifts silently.

The two decisions that make this right:

• Reserve before the allocation. "Charged BEFORE the allocation so a refusal leaves nothing to refund" is the ordering that makes the refusal path free of cleanup, and it is the opposite of the usual alloc(); if (charge() fails) free(); shape, which is where half of these bugs live.
• Refund the stored amount, not a recomputed one. entry->charged is written once and returned in ~Entry, with the note that "a refund that recomputes its size is how a ledger drifts". That is exactly right: materialized_length is not provably the same at destruction time as at construction, and the drift would be invisible until a budget calculation was optimistic by a few percent.

The framing of the bug is also the part worth keeping: ggml-org#1711 charged three allocators, but the charge went on the arena path while the else beside it — GGML_HRX_HOST_WEIGHT_LIMIT_MB=0, the default — gives each weight its own buffer and was uncharged. So the ledger was correct for the mode almost nobody runs and blind to the mode everybody runs, which is the worst way round.

The ask: the diff is one file, so no test came with it, and the comment says "returning it twice is what the staging-buffer test exists to prevent" — which implies there is a test that can see this. Please either point it at this path (a load, then assert the pool returns to its starting figure — a non-zero residue is exactly the uncharged mode's signature) or say in one line why the per-weight path cannot be reached from a host test. A memory ledger whose balance is only ever checked on the arena path will drift back to uncharged the first time someone adds a fourth allocator, and the comment already says the right thing; the test is what makes it stick.

Nothing else to raise: the charged != 0 guard before pool_release prevents the double refund, and the charged = 0 reset after release is what keeps that guard meaningful.