Skip to content

test(resources): pin that the wizard never offers more than the machine can give (backend#2221) - #571

Merged
shujaatTracebloc merged 2 commits into
developfrom
test/2221-rung-offer-bounded
Aug 25, 2026
Merged

test(resources): pin that the wizard never offers more than the machine can give (backend#2221)#571
shujaatTracebloc merged 2 commits into
developfrom
test/2221-rung-offer-bounded

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

The problem, in plain terms

When you run tracebloc resources set, a wizard asks how much of the machine a training run may use. It must never offer you an amount the machine can't actually deliver — otherwise you pick it, it applies, and training later fails to schedule with no hint that the number was impossible from the start.

Today the wizard gets this right. Two tests prove it for two specific machines: one that shrank under its configured budget, and one too small to fit anything at all.

The property backend#2221 asks for is universal, though — "never offer a ladder rung the VM cannot honour" — and there's a change already planned that would satisfy both existing tests and break it. The epic records a size ladder as a decision (XS 4 · S 16 · M 32 · L 64 · XL 128 GiB). Offer a fixed rung and, on a small VM, it's unhonourable by construction. Both existing tests would still pass.

What we did

No production change. One swept test asserting the invariant across six machine shapes, including #2221's own measured case: a 6 CPU / 11.67 GiB Docker Desktop VM where two uncapped k3d node containers each report the whole thing.

For every shape it checks:

Why bounding on the node is correct here, given #2221 is about nodes lying

Worth stating, since it looks like a contradiction. The wizard bounds on node allocatable, and #2221 is precisely the ticket about node containers double-counting the VM.

It's honest for one run: a node's allocatable can never exceed the VM's, so a per-run bound taken from the node is at worst conservative. The double-count misleads about concurrency — two runs admitted where one fits — and that's backend#2419's admission gate, which now queues rather than over-committing. Not this prompt's business.

My first version of this test was worthless, and the mutation run is what said so

Reporting it because the failure is instructive.

I answered the wizard's prompt with "Use as much as possible". The real option string is "Use as much as possible (recommended if this machine is just for tracebloc)". A prefix matches no option, so the fake prompter returned the default, the wizard left the budget untouched, and my assertion happily read the current value instead of an offered one.

It passed on all six machines. Then I mutated the wizard to offer a fixed 8-core / 32-GiB rung — the exact scenario this test exists to catch — and it still passed. A test that never reaches the code under test cannot fail for it.

Fixed by answering the full option string, and by choosing a current budget (cpu=1,memory=3Gi) that sits below the floor, so a silent no-op can't masquerade as a pass either.

Three mutations now redden it:

mutationresult
the wizard offers a fixed 8-core / 32-GiB rung5 of 6 shapes fail
the CPU offer forgets the platform overhead7 failures
the memory offer ignores the machine entirely7 failures

Testing

go test ./internal/... all packages pass
go vet ./... clean
gofmt -l internal/ clean

This closes #2221's fourth box

boxstate
Cap the k3d node containersdone — client-runtime#363, cli#541, client's honest-topology twins
Surface the chain in doctordone — cli#541
Installer states a minimum and offers to fix itclient#832, awaiting re-approve
Never offer a rung the VM can't honourthe behaviour was already right; this makes it enforced rather than incidental

🤖 Generated with Claude Code


Note

Low Risk
Only adds CLI wizard tests and helpers; production paths are unchanged.

Overview
Test-only change — no wizard or resources set behavior is modified.

Adds TestWizard_NeverOffersMoreThanTheMachineCanHonour, a table-driven sweep over six node CPU/memory shapes (including the #2221 Docker Desktop / k3d case). Each case drives applyResourcesSet in dry-run with the wizard answering the full “Use as much as possible (recommended…)” string, then checks the printed cpu=N,memory=MGi plan via new helpers appliedCPUAndGiB and mustQty.

The assertions encode backend#2221’s universal property: applied limits must stay within resources.MaxRunCores / MaxRunGiB, respect the 1-core / 2-GiB floor, apply exactly the machine max for max-out (so silent no-ops or fixed ladder rungs fail), and require an honest error when the node cannot seat the floor.

Extends resources_set_test.go imports (regexp, strconv, resource.Quantity, internal/resources) to support that parsing and sizing math.

Reviewed by Cursor Bugbot for commit cd7e643. Bugbot is set up for automated code reviews on this repo. Configure here.

…ne can give (backend#2221)
#2221's fourth scope item -- "never offer a ladder rung the VM cannot honour" --
was covered by two examples (a shrunk machine, a too-small machine). The property
it actually asks for is universal, and a future rung ladder (the epic's XS 4 / S 16
/ M 32 / L 64 / XL 128 GiB decision of record) is exactly the change that satisfies
both examples and breaks the property: offer a fixed rung and it is unhonourable by
construction on a small VM.
Swept over six machine shapes including #2221's own measurement (a 6 CPU /
11.67 GiB Docker Desktop VM). A machine that cannot seat the floor must fail
honestly rather than offer something unhonourable, which folds in the Bugbot #241
rule as part of the same invariant.
My first version was VACUOUS: it answered the prompt with a prefix that matched no
option, so the wizard left the budget alone and the assertion read the CURRENT
value. It passed on every machine and survived a mutation making the wizard offer
a fixed 8-core / 32-GiB rung -- the precise case it exists to catch. Fixed by
answering the full option string and choosing a current budget a no-op cannot hide
behind; three mutations now redden it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTraceblocshujaatTracebloc self-assigned this Aug 25, 2026
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 25, 2026 13:45
Comment threadinternal/cli/resources_set_test.go
…backend#2221)
Cursor Bugbot Medium on #571. I claimed a current budget of cpu=1,memory=3Gi made
a silent no-op fail the floor check. It does not -- 1 core / 3 GiB sits at and
above the 1-core / 2-GiB floor, and every shape in the table can honour it, so a
wizard that never offered anything passed every assertion.
The real backstop is an equality check: "use as much as possible" has an exact
contract, so a no-op (which applies the CURRENT budget), a fixed rung, and a prompt
string that matches no option all land somewhere other than the machine maximum
and fail. A bound alone is satisfied by offering nothing.
Proof it now works: re-introducing the prefix-only prompt answer -- the vacuous
version this test shipped with -- reddens 5 of 6 shapes, where before it passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test, and the two vacuity bugs recorded in the file are the best part — a prefix answer that matched no option so the fake returned the default and the assertion landed on the current budget, surviving a mutation that made the wizard offer a fixed 8-core/32-GiB rung; and the claim that 1 core / 3 GiB was the no-op backstop when it sits above the 1-core/2-GiB floor, so a silent no-op passed. Both are exactly what this test exists to catch, both caught in itself, both left written down rather than quietly fixed.

The equality check is what carries it. A bound alone is satisfied by offering nothing, and gotCores != maxCores || gotGiB != maxGiB is the assertion a no-op, a prefix mismatch and a fixed rung all fail — which is the property #2221 actually asks for, rather than the two examples that covered it.

Two things I checked rather than assumed:

Parsing the output instead of a return value is right, and the reason in the comment is the right reason: a bound the code respects internally and misreports is still a bound that misleads. Worth noting appliedCPUAndGiB's regex only matches memory=(\d+)Gi, so a wizard that ever emitted Mi would t.Fatalf with "no cpu=N,memory=MGi in the output" rather than silently matching nothing. That fails closed, which is the direction you want — just flagging that it is load-bearing and not incidental.

The expectation is derived from MaxRunCores/MaxRunGiB, the same helpers production uses, so this asserts the wizard applies the bound and not that the bound is right. That is the correct trade — restating the formula here would be the second copy — and it holds because those two have their own coverage in internal/resources/contract_test.go and set_test.go. Worth being explicit that the invariant leans on that, so nobody thins those out later thinking this test has them covered.

Nothing to approve against yet: 18 checks are pending on ff21f21e after the latest push. The Bugbot thread that was open when I started reading has been resolved since.

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit cd7e643. Configure here.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — a genuinely well-built invariant test. Verified it clears the house bar:

  • Non-vacuous: answers the full option string ("Use as much as possible (recommended…)") that a real option matches, so the wizard actually runs the code under test — and the comment credits the earlier prefix-mismatch bug that made it vacuous, now fixed.
  • Mutation-proof: the exact-equality assertion (gotCores != maxCores || gotGiB != maxGiB) fails a no-op (current budget 1c/3Gi ≠ max on every shape) and a fixed 8c/32Gi rung alike — the two failure modes #2221 warns about.
  • Right altitude: it bounds against resources.MaxRunCores/MaxRunGiB of the (single) node rather than restating numbers, and the multi-node largest-not-sum reduction (#399's double-count) is already pinned by TestNodeLarger, so a single node here is correct rather than a gap.
  • Floor-fail path asserts an honest error, not an unhonourable offer.

CI green, no open threads.

@shujaatTracebloc
shujaatTracebloc merged commit b5f4593 into developAug 25, 2026
27 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the test/2221-rung-offer-bounded branch August 25, 2026 14:29
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shujaatTracebloc@saadqbal@saqlainsyed007@LukasWodka