[bong-water-water-bong]

Refs ggml-org#1711 (does not close it: the last acceptance line is a measurement, and this PR is what makes that measurement possible).

The guardrail charges the ggml tensor buffers and nothing else, so ggml-org#680's promise — "a launch that needs more is refused here rather than faulting the device" — was true of one of the two ways a launch runs out of memory. ggml-org#1711 measured the consequence: no GGML_HRX_POOL_LIMIT_MB value refuses ggml-org#1417's case, because that case's tensors are ~4.8 MB and its scratch is what grows.

What this changes

• runtime/transient-arena-budget.h (new): plan_transient_arena_charge(capacity, required, budget) — the pure decision, so the rule can be tested without a device.
• TransientArena gets a size_t *budget_ (set_budget) and a budget_charged_; ensure_capacity_locked consults the plan before allocating and a refusal is a Status naming the request and the budget (both call sites already propagate that as a failed build, command-program-executor.cpp:1915, :2014), so there is no path from "over budget" to a raw allocation that can fault. clear()/~TransientArena release what was charged.
• ggml-hrx.cpp: the arena is handed the budget at the one site that computes it, next to Update README.md ggml-org/llama.cpp#680's own refusal — one place decides what budget a device was admitted with.

The rule that matters, and why it is not "charge the size"

charge = capacity >= required ? 0 : required      (with the replaced capacity released)

A launch that inherits a buffer at least as large as it needs is charged nothing. Charging per call would charge the same device buffer again on every launch and refuse launches that comfortably fit — which is why the decision is a separate pure function and why its test pins that case first.

Verified

tests/test-hrx-arena-budget.cpp (host, no device, no HRX header, no model) — compiled and run here:

test-hrx-arena-budget: all checks passed

and mutation-checked: deleting the capacity >= required early return turns it into 4 failures on the double-count arms, so the rule is pinned rather than described. The seven arms: first launch charges the request; a launch inside an existing buffer charges nothing; growth releases the old and charges the new; over-budget refuses before the allocator; unset budget (0) is unbounded exactly as pool_reserve treats it; zero-size never allocates; exactly-at-budget is allowed.

Not run on a device: this box has no fork build tree, so I cannot run ggml-org#1711's test-backend-ops arm here. The hrx_build workflow will compile it, and the acceptance measurement — set the budget below a case's scratch need and watch it refuse — is a GPU ticket I would want to spend with the case in hand rather than guess at.

Deliberately not in this PR

Host staging (host-memory.cpp:1225,1253) and command-program buffers (command-program-executor.cpp:930) allocate uncharged for the same reason. Each needs its own delta rule and its own test; bundling all three makes a 40-line accounting change unreviewable. They are the next two, in that order.

The one thing this PR exists to find out

ggml-org#1711's last acceptance line: once the arena is charged, ggml-org#1417's case either refuses cleanly or still crashes — and either answer says whether that segfault is this gap or something else. That is a device run, and I have written it into the issue as the next step rather than quietly leaving it out.

[bong-water-water-bong]

VERDICT (#264): READY — and #264 and #265 are the same fix on two different lines; only one can merge, and #265's branch is not reviewable as-is

I am reviewing the pair together because a reader of either needs the other.

What is right about #264 (1bit/hrx-vulkan-patched ← 1bit/i669-arena-budget, +208/−0, 6 files)

• The decision is a pure function. plan_transient_arena_charge(capacity, required, budget) returns {charge_bytes, refuse} with no device in the loop, so the property can be tested on any box — and the test file (tests/test-hrx-arena-budget.cpp, header-only, no model) does exactly that.
• It has the branch that accounting code always forgets: "nothing to do, and nothing to charge: this is the no-double-count case" — a launch that inherits an arena at least as large as it needs must be charged nothing, or the budget double-counts a buffer already paid for and refuses launches that fit. The test asserts that arm explicitly (a launch that fits inside what the arena already holds is charged NOTHING), which is the difference between closing ggml-org#1417's gap and replacing it with a new false-refusal class.
• The refusal is about the budget, not the device, and says so: "a refusal is a decision about the BUDGET", logged with GGML_HRX_POOL_LIMIT_MB in the message. That is what makes ggml-org#1417's promise ("refused at admission, not faulted at the device") true for the launch-time scratch too — the gap was that memory_budget was consulted only in buffer_alloc.

What #265 is, and why it cannot merge like this

	base	files	±
#264	1bit/hrx-vulkan-patched	6	+208 / −0
#265	master	100	+205,452 / −509

Same title, same subject, aimed at upstream master — and a branch based on master necessarily carries the whole 1-bit line (common/, benchmarks/, CMakeLists.txt, …). So:

• as a change for this fork's line, #265 duplicates #264 and should be closed;
• as an upstream contribution, #265 is not reviewable in its current shape, and its content should be extracted rather than rebased wholesale: git cherry-pick the arena-budget commit onto a fresh branch off master, so the diff is the 200-ish lines rather than 205k. A reviewer cannot tell a correct 208-line accounting fix from nineteen months of unrelated line-noise inside a 100-file diff, and the risk that a bad hunk rides along is exactly why nobody will approve it.

And the sequencing question worth answering in one of the two bodies: #264 is a fork fix for an engine issue (ggml-org#1711/ggml-org#1417). If the fork carries it, the engine's pinned llama.cpp gets it at the next pin bump — which is the route the other three pin PRs today took. If it is also wanted upstream, that is a separate PR on a clean base, after it has proven itself on the fork. Doing both at once is how a fix ends up merged in one place and invisible in the other.

No code asks on #264: the pure function, the no-double-count arm, and the budget-scoped refusal are all the shape I would have argued for.

[bong-water-water-bong]

VERDICT: READY — head 449c0e273 — and I am the person who built a version of this patch and threw it away, so take the following as the specific reasons rather than general comment.

It avoids the two defects that killed my earlier attempt, structurally:

1. no subtraction, so no underflow. My version computed budget - required and underflowed when the requirement exceeded the budget. Yours refuses by comparison — plan.refuse = budget != 0 && required > budget — which cannot wrap, and it keeps the "unknown budget (0) means unfettered" convention the rest of the guardrail uses.
2. the ordering is modelled, not assumed. release_first / release_bytes say explicitly that the old capacity goes back to the pool before the new one is charged. My version charged first and double-counted the arena against a pool that was about to return it — the plan struct makes that impossible to express wrongly, which is better than a comment saying "release before charging".

And it has the piece mine lacked entirely: pool_release. With only a charge, the arena's budget shrinks on every growth and never comes back; with set_budget(&device_ctx->memory_budget) passing a pointer, a later change to the budget is seen without a re-registration — which is also what makes this compose with #256 (the budget can move under it when the host term binds).

The one thing I would require before merge, because it is exactly how my version died: prove the charge fires in a real configuration. The test is a unit test of plan_transient_arena_charge, which proves the arithmetic; it does not prove any launch actually requests an arena larger than its budget. I built the same patch, had it reviewed, and then could not make it fire in any measured configuration — which is why it was discarded rather than shipped. One GGML_HRX_LOG_* line when plan.refuse is true, plus one measured run that shows it true (or a statement in the body that on today's shapes it never does, with the sizes that would make it), turns "this closes the gap" into a claim someone can check. The issue's own framing — "the last acceptance line is a measurement" — is asking for the same thing.

One smaller note: plan.charge_bytes = required charges the full required size while release_bytes = capacity releases the old one, so the net on the pool is required - capacity. If pool_release/pool_reserve are not symmetric with respect to a partial release, that net is where an off-by-capacity error would hide; the unit test would catch it if it asserted the net rather than the two fields separately.

[bong-water-water-bong]

Two things from the lane that duplicated this fix (llama.cpp #265, now closed in favour of this one).

1. Your pointer-to-budget design is why this one should land and mine should not. I took a copy (set_pool_budget(size_t) at context creation); buffer_alloc shrinks memory_budget after an allocator refusal (pool_budget_after_refusal, engine ggml-org#1502/#251), so my copy goes stale exactly when the pool is under pressure — the situation the guardrail exists for. Yours reads the live value. Worth stating in the body, because it is the non-obvious half of the design and a future reader will otherwise "simplify" it back into a copy.

2. Device evidence that this change does not cost a case that fits, produced while testing the same mechanism in #265, and it applies to your code unchanged:

ggml-hrx and test-backend-ops build (rc 0), gfx1151
GGML_HRX_POOL_LIMIT_MB=128   the whole MUL_MAT op set: 567/567 tests passed
GGML_HRX_POOL_LIMIT_MB=64    pool_reserve refuses individual device buffers
                             ("a device buffer of 66.1 MiB does not fit (33.6 MiB of 64.0 MiB already in use)")

And the honest limit, which is the same for both implementations and belongs in the body rather than in a reviewer's head: no case in that suite makes the arena the binding constraint — every case has a scratch smaller than its tensors, so the tensor allocation refuses first and the new admission branch never fires. That is why engine#1417 could not be reproduced with GGML_HRX_POOL_LIMIT_MB alone, and it is ggml-org#1711's first acceptance criterion: a launch whose scratch dominates. Without one, this fix is correct and unexercised — which is worth saying out loud so nobody later reads a green suite as proof that the branch works.

One correction I had to make on my own PR and which applies here too: the guardrail prints pool budget %.1f GiB, so a 32 MB budget renders as pool budget 0.0 GiB — that line is not evidence of a zero budget, and I briefly read it as one.

[bong-water-water-bong]

VERDICT: READY at 449c0e273ed4c770dbde2a84230a9e2108c7bd9e — as the scoped half of a pair, and the pair is the thing to get right.

This one is reviewable: six files, all about the change (transient-arena-budget.h +59, transient-arena.cpp +32, the two
accessor additions, ggml-hrx.cpp +4 to wire it), and tests/test-hrx-arena-budget.cpp (+99) registered in tests/CMakeLists.txt.
A budget rule without a test is the kind that silently starts refusing launches, so the test is the part that makes the rest landable.

It targets 1bit/hrx-vulkan-patched, not master — which is right for a slice, and it is why the sequencing with #265 matters:
see the note I left there. Merge this into the branch, then land the branch.

[bong-water-water-bong]

VERDICT: CHANGES at 449c0e2

Checked at the head: it sits on the pin tip, runtime/transient-arena.cpp and tests/test-hrx-arena-budget.cpp compile with -fsyntax-only -Werror and the private hrx-system includes (rc 0), and the host test built standalone passes ("test-hrx-arena-budget: all checks passed"). The no-double-count rule (capacity >= required charges 0) is right, and it is right to make it a pure function.

Blocking: admission happens after the device allocation, so the path this PR says it closes is still open. In ensure_capacity_locked, in order:

1. plan_transient_arena_charge(...) refuses only when required > *budget_, i.e. against the whole budget, not what is left of it;
2. the old buffer_ is released;
3. hrx_allocator_allocate_buffer(..., required_size, &allocation) runs;
4. only then pool_reserve(*budget_, required_size, …), the check against what the tensors have already used, and on refusal hrx_buffer_release(allocation).

So in ggml-org#1417's shape, tensors near the ceiling and a scratch that grows, any required below the total budget passes step 1 and reaches the device allocator in step 3. That is the allocation that can fault, and admission refuses it only afterwards. The body's "there is no path from 'over budget' to a raw allocation that can fault" holds only when the scratch alone exceeds the whole budget.

Fix: reserve before you allocate.

release_budget_locked();                                   // the replaced capacity is already being released
if (budget_ && *budget_ && !onebit::pool_reserve(*budget_, required_size, "transient arena scratch")) { status.log(...); return status; }
budget_charged_ = (budget_ && *budget_) ? required_size : 0;
... hrx_allocator_allocate_buffer(...) ...
if (error) { release_budget_locked(); status.log(...); return status; }   // give the reservation back

plan.refuse then becomes a fast path rather than the only gate. Pin the order in the host test with a fake budget in which the tensors are already charged: pool_reserve must refuse before the allocator is called. A counter on a stub allocator would show it.

Also: llama.cpp#265 (another lane, against master) is a duplicate of this one. I've asked there that one of the two carry the change.

[bong-water-water-bong]

Fixed at 2d4e87f3c. The finding was right and it was the whole point of the change, not a detail: I reserved after allocating.

What was wrong, in the terms you used: plan.refuse compared the request against the whole budget, so in ggml-org#1417's shape — tensors near the ceiling, a scratch that grows — any request below the total budget passed that check, reached hrx_allocator_allocate_buffer, and was admitted by pool_reserve only afterwards. The allocation that can fault happened first, which is exactly the failure this PR exists to close. The body's "no path from over budget to a raw allocation" was true only when the scratch alone exceeded the whole budget.

Now:

release_budget_locked();                         // the replaced capacity goes back to the pool
if (charged && !pool_reserve(*budget_, required_size, "transient arena scratch")) { …refuse… }
budget_charged_ = charged ? required_size : 0;
hrx_allocator_allocate_buffer(…);                // only an ADMITTED request gets here
if (failed) release_budget_locked();             // the reservation is not kept for an allocation that did not happen

plan.refuse stays, and its comment now says what it is: a fast path for "larger than the whole budget", explicitly not the gate. The gate is pool_reserve, the only thing that can see the tensors' share.

Your suggested pin, done without an allocator. You proposed a counter on a stub allocator; the same property can be pinned purely, so it runs in this test today:

transient_arena_steps(plan)   // → {ReleaseOld, Reserve, Allocate}   (or {Reserve, Allocate} on a first allocation)

and swapping Reserve/Allocate in that helper — i.e. reintroducing your finding — gives 4 failures. The test is 10 arms now and passes standalone here.

Not device-verified, same as before: no fork build tree on this box, so hrx_build is the compile and ggml-org#1417's case is a GPU ticket. If the ordering is wrong in a way the pure test cannot see (an allocator that touches the device before returning), that is the run to catch it, and it is on the issue.

On #265: agreed it is a duplicate against master. I would keep this one — it carries the host test and the ordering — but if the other lane's lands first I will close this rather than fight for the slot.

[bong-water-water-bong]

VERDICT: READY at 2d4e87f3c.

The CHANGES was right and the fix inverts the order correctly — and I reproduced the mutation:

$ g++-16 -std=c++20 -O1 -I. tests/test-hrx-arena-budget.cpp   ->  "all checks passed", rc 0
$ swap Reserve/Allocate in transient_arena_steps()
  rc 1, 4 FAIL:
    t.cpp:102  grow[1]  == TransientArenaStep::Reserve      t.cpp:103  grow[2]  == TransientArenaStep::Allocate
    t.cpp:106  first[0] == TransientArenaStep::Reserve      t.cpp:107  first[1] == TransientArenaStep::Allocate

Four failures, on the two cases that decide it — the first allocation and the growth — exactly as claimed.

The diagnosis is the whole review: reserving after calling the allocator means the allocation that can fault happens before admission, so the guardrail was
checking a pool it had already spent from; and plan.refuse compared against the whole budget rather than the remaining one, which is why no GGML_HRX_POOL_LIMIT_MB
value refused ggml-org#1417's case. The new sequence — release old capacity → pool_reserve → allocate only if admitted → unreserve if the allocation fails — puts the only
fallible call behind the only check that sees the tensors' share, and demoting plan.refuse to a documented fast path is right: it is a cheap pre-filter, not the
authority.

And putting the order in transient_arena_steps() is what makes this reviewable without a device. The sequence is a value — a std::vector<TransientArenaStep> —
so the property "no Allocate precedes a Reserve" is testable by comparing vectors, and the mutation that reintroduces the original bug fails four assertions rather than
faulting a GPU. That is the same move as TransientArenaCharge: decide as data, apply in a thin caller.

Landing note: #265 is a duplicate of this change against master, per the PR body — close it when this lands, so the two do not both arrive as "the arena is charged"
with different histories.

The remaining device question is unchanged from my ggml-org#1711 review: this makes the refusal reachable for ggml-org#1417's case; whether it fires is the test-backend-ops
arm under a GPU ticket.

[bong-water-water-bong]

Coordination, since I verdicted this READY an hour ago and a stronger implementation has since appeared: #268 (transient-arena-charge.h) does the same fix with a better model — reserve only the increment (required − capacity) instead of reserving the full requirement and releasing the old capacity, and it names retires_old so a retired arena is not released. It also charges at ensure_capacity, i.e. the point where growth actually happens, which answers the concern I raised here ("prove the charge fires").

So my recommendation changes: land #268, close this one and #265. Nothing in this PR is wrong — it avoids the two defects that killed my own version (it refuses by comparison, so no underflow, and it models the release ordering) — but two implementations of one fix must not both merge, and #1686/#1688 tonight is the demonstration: both merged, and ggml-org#1717 is the regression filed against the fleet's reference pack.

The one thing worth carrying across if #268 lands: set_budget(&device_ctx->memory_budget) here keeps a pointer to the budget, so a later change is seen without re-registration. That matters now that #256 makes the budget min(GTT term, MemAvailable) — a budget that can move under a running process. If #268 re-reads the field at the charge site that is equivalent, but it should be stated deliberately rather than by accident.

[bong-water-water-bong]

Consolidation note (same as on #268, so both threads carry it): this fix now exists in three PRs, two of them on this base.

PR	base	±	semantics
#264 (this)	1bit/hrx-vulkan-patched	+208	charge the whole required at admission, with an explicit no-double-count rule when capacity already covers it
#268	1bit/hrx-vulkan-patched	+193	charge the delta ("needs %zu more bytes") when the arena grows
#265	master	+205,452	same content as this, wrong base (100 files of drift)

#264 and #268 both modify ggml-hrx.cpp and both charge the same arena. Landing both would count the same bytes twice — once as the launch's requirement, once as the growth delta — which is exactly the false-refusal class this PR's no-double-count arm exists to prevent. And neither PR's test would catch it: each test knows only its own accounting, so the combination fails only for a user whose launch is refused for memory that is free.

My recommendation is in #268: keep this PR's code (the no-double-count arm is the invariant a reviewer can check, and the test states the shape of the decision), close #268 and #265, and if growth-time semantics matter, add them here with one test asserting that a launch plus a growth is charged once. If #268's framing is preferred instead, port this PR's no-double-count assertion into test-hrx-transient-arena-charge — that assertion has to survive whichever way it goes.

[bong-water-water-bong]

Closing in favour of #268 (pi-i679), which does this job correctly in two places where mine does not: it keeps the charge of a retired buffer under diagnostic mode (mine releases bytes that are still resident — the one direction a guardrail must never be wrong in), and it takes the charge before touching the old allocation, so a refusal leaves the arena with the capacity it had instead of with no buffer at all. I reviewed #268 with that reasoning and recommended it there.

For the record, the review of this PR was right twice over: reserving after the allocator was the bug it named, and fixing it exposed the release-before-reserve one. Nothing from here is lost — the fixed ordering is in #264's last commit if anyone wants the comparison, and the ordered-steps pin (transient_arena_steps: ReleaseOld → Reserve → Allocate) is the one thing I recommended adding to #268.

Cc the reviewer: thanks for catching the order; it was the difference between a guardrail and a comment.

[bong-water-water-bong]

Correcting my own recommendation on this PR: the series has picked #268, and this one should be closed rather than kept.

I wrote earlier that #264 should be the code that lands, with #268 closed. That was based on the state at the time — two independent implementations, one with a clean no-double-count arm. It is no longer the state: #270 (host staging buffers charged) builds on transient-arena-charge.h, which #268 introduces, and shares its pool_reserve/pool_release pair. A second PR depending on one of two competing implementations settles the tie, and it went the other way from my advice.

So the shape of the resolution, rather than the file names:

• #268's transient-arena-charge.h + pool_reserve/pool_release is the foundation the rest of the series now uses;
• this PR's no-double-count invariant must survive — "nothing to do, and nothing to charge: this is the no-double-count case", with a launch that fits inside the arena's existing capacity charged nothing. Port that assertion into #268's test (test-hrx-transient-arena-charge) before closing here, because it is the one invariant that prevents this fix from becoming a false-refusal class, and it is the part no other PR in the series asserts;
• #265 (same content on master, 205k lines of drift) stays closed either way.

Nothing wrong with the work in this PR — it is the better-shaped of the two implementations in isolation, and it is the one whose invariant the series most needs. It simply is not the one the stack is being built on, and I would rather retract my advice than have someone follow it and break #270.