From b716f1e51486df007f59664c6ba083952c8d686a Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Thu, 20 Aug 2026 12:00:17 +0200 Subject: [PATCH 1/3] feat(resources): stamp `resources set` as a human choice (backend#2220) Companion to client#768, and the half that makes the marker mean anything. client#768 taught the installer to write env.RESOURCE_PROVENANCE (installer | user | unknown). Without this side, an edge the installer marked `installer` and the operator then re-sized by hand would KEEP saying `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is strictly worse than no marker at all, so the two PRs are only correct together. BuildEnvSpec now writes RESOURCE_PROVENANCE=user unconditionally. Unconditional for the same reason NoGPUEnvValue is: the apply runs `helm upgrade --reset-then-reuse-values`, which re-applies the release's stored values on top of chart defaults, so an omitted key is silently RE-INHERITED rather than cleared. There is no branch on which omitting it would be safe. `tracebloc resources set` is by definition the human choice -- there is no variant of running it that isn't. Read side: Training.Provenance, normalised through NormalizeProvenance, which maps anything unrecognised -- empty, junk, a value this binary predates, wrong case -- to `unknown`, never to a guess. Returning "" would have made callers branch on empty and invent their own default, which is a second policy and exactly what this ticket removes. Callers MUST treat `unknown` as a human choice: it means we do not know, and guessing `installer` risks overruling an operator. Surfaced in `resources --verbose` as "set by", not in the default view: it answers a support question ("did someone set this, or did we?"), not one an operator needs on every run, and a line of bookkeeping that never changes the numbers should not grow the default output. `unknown` renders with its explanation rather than the bare word, which invites the wrong conclusion -- it does not mean something is broken. The env map already flows into helm.UpgradeParams.Env, so no new plumbing. The added apply-boundary test starts from an installer-marked edge specifically, because the unit test proves the map is right while this proves the map arrives -- and a marker that never lands looks identical to one that did, with the consequence surfacing much later. zz-all-strings.golden regenerated: one line, "set by", reviewed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. 6 new tests in internal/resources (including a set-then-read round trip) + 1 apply-boundary test in internal/cli. Refs: RFC-BACKEND-664 P0, client#768, client-runtime#358, cli#538 Co-Authored-By: Claude Opus 5 --- internal/cli/resources.go | 23 +++ internal/cli/resources_set_test.go | 34 ++++ .../cli/testdata/golden/zz-all-strings.golden | 1 + internal/resources/provenance_test.go | 156 ++++++++++++++++++ internal/resources/resources.go | 38 ++++- internal/resources/set.go | 18 +- 6 files changed, 268 insertions(+), 2 deletions(-) create mode 100644 internal/resources/provenance_test.go diff --git a/internal/cli/resources.go b/internal/cli/resources.go index c636297c..fe54022a 100644 --- a/internal/cli/resources.go +++ b/internal/cli/resources.go @@ -154,6 +154,11 @@ func renderResources(ctx context.Context, p *ui.Printer, target *clusterTarget) } else { p.Field("resource env", "(unset — using chart default "+resources.DefaultTraining+")") } + // backend#2220: who chose the ceiling above. Verbose-only — it answers a + // support question ("did someone set this, or did we?"), not one an + // operator needs on every run, and the default view should not grow a + // line for bookkeeping that never changes the numbers. + p.Field("set by", provenanceLine(train.Provenance)) if nodeErr == nil && len(machine.GPU) == 0 { p.Field("gpu", "none detected") } @@ -170,6 +175,24 @@ func renderResources(ctx context.Context, p *ui.Printer, target *clusterTarget) return nil } +// provenanceLine renders RESOURCE_PROVENANCE for humans (backend#2220). +// +// "unknown" gets an explanation rather than the bare word, because the bare word +// invites the wrong conclusion. It does not mean something is broken: it means +// the value predates the marker, and an installer-written size and a deliberate +// `resources set` are indistinguishable once the value differs from the historic +// default. That is why it is treated as a human choice and left alone. +func provenanceLine(provenance string) string { + switch provenance { + case resources.ProvenanceUser: + return "explicitly set (tracebloc resources set)" + case resources.ProvenanceInstaller: + return "sized to this machine at install time" + default: + return "unknown — predates provenance tracking, treated as an explicit choice" + } +} + // machineLine renders the machine-capacity value: "8 CPU · 32 GiB" (+ " · 1 GPU" // when a device is present). func machineLine(m resources.Machine) string { diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 366b6d30..236860e5 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -170,6 +170,40 @@ func TestSet_ApplyBuildsHelmArgsAndValues(t *testing.T) { } } +// TestSet_StampsUserProvenance: the apply carries RESOURCE_PROVENANCE=user all +// the way into the values helm is given (backend#2220). +// +// The unit test on BuildEnvSpec proves the map is right; this proves the map +// actually reaches helm. Worth having separately because the failure mode is +// silent: a marker that never lands looks identical to one that landed, and the +// consequence only shows up much later, when a ladder re-derives a size the +// operator had chosen on purpose. +func TestSet_StampsUserProvenance(t *testing.T) { + // Start from an edge the INSTALLER marked — the dangerous case. After a + // human `resources set`, the label must flip to user; leaving it as + // "installer" would advertise a deliberate choice as ours to overwrite. + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=2,memory=8Gi", + "RESOURCE_PROVENANCE": "installer", + }) + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, dryRun: true, yes: true, + }) + if err != nil { + t.Fatalf("dry-run: %v\n%s", err, out) + } + if !strings.Contains(out, "RESOURCE_PROVENANCE") { + t.Errorf("the plan does not mention RESOURCE_PROVENANCE at all:\n%s", out) + } + if !strings.Contains(out, "user") { + t.Errorf("the plan does not stamp the set as a human choice:\n%s", out) + } + // And the envelope still reflects what was asked for (Decision A). + if !strings.Contains(out, "cpu=4,memory=16Gi") { + t.Errorf("plan lost the requested ceiling:\n%s", out) + } +} + // TestSet_KeepsUnsetDimension: `set --cores 4` changes CPU only and KEEPS the // current 8Gi memory (proven via the dry-run plan's resulting values). func TestSet_KeepsUnsetDimension(t *testing.T) { diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index c5feb3b7..54a4f607 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -673,6 +673,7 @@ screen. %s/%d are runtime placeholders. "service %s/%s has no selector — can't resolve to a Pod for port-forwarding" "session: %s" "set -e\nrm -rf %q\nmkdir -p %q\n/bin/tar -xf - -C %q\nif [ -e %q ]; then mv %q %q; fi\nif ! mv %q %q; then\n if [ -e %q ]; then mv %q %q; fi\n exit 1\nfi\nrm -rf %q\nfind %q -maxdepth 1 \\( -name %q -o -name %q \\) -mmin +60 -exec rm -rf {} + 2>/dev/null || true" +"set by" "setting up jobs-manager port-forward: %w" "sha256[:8]" "shared PVC" diff --git a/internal/resources/provenance_test.go b/internal/resources/provenance_test.go new file mode 100644 index 00000000..0e0ce497 --- /dev/null +++ b/internal/resources/provenance_test.go @@ -0,0 +1,156 @@ +package resources + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// backend#2220 scope bullet 4. The marker only works if BOTH writers maintain +// it: the installer stamps installer/unknown, and `resources set` stamps user. +// If this side is missing, an edge the installer marked `installer` and the +// operator then re-sized by hand keeps saying `installer` — a human choice +// wearing the one label that invites a future ladder to overwrite it. That is +// strictly worse than having no marker at all, which is why these are here. + +func TestBuildEnvSpecStampsUserProvenance(t *testing.T) { + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + + env := BuildEnvSpec(cpu, mem, "", resource.Quantity{}, false) + + if got := env["RESOURCE_PROVENANCE"]; got != ProvenanceUser { + t.Errorf("RESOURCE_PROVENANCE = %q, want %q — `resources set` IS the human choice", + got, ProvenanceUser) + } + // The envelope itself must be untouched by the marker (cli#143 Decision A). + if env["RESOURCE_LIMITS"] != "cpu=6,memory=24Gi" { + t.Errorf("RESOURCE_LIMITS = %q, want cpu=6,memory=24Gi", env["RESOURCE_LIMITS"]) + } + if env["RESOURCE_REQUESTS"] != env["RESOURCE_LIMITS"] { + t.Error("requests and limits diverged — Guaranteed QoS is the chart contract") + } +} + +func TestBuildEnvSpecStampsProvenanceOnTheGPUPathToo(t *testing.T) { + // Every dimension is always written; the marker must not be an exception on + // one branch, or a GPU-enabled `resources set` would leave a stale label. + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + gpu := *resource.NewQuantity(1, resource.DecimalSI) + + env := BuildEnvSpec(cpu, mem, corev1.ResourceName("nvidia.com/gpu"), gpu, true) + + if got := env["RESOURCE_PROVENANCE"]; got != ProvenanceUser { + t.Errorf("RESOURCE_PROVENANCE = %q on the GPU path, want %q", got, ProvenanceUser) + } +} + +func TestBuildEnvSpecIsUnconditional(t *testing.T) { + // BuildEnvSpec takes no prior state on purpose, so there is no branch on + // which it could omit the key. Omitting it would NOT clear it anyway: the + // apply runs `helm upgrade --reset-then-reuse-values`, which re-applies the + // release's stored values on top of chart defaults, so an omitted key is + // silently re-inherited rather than removed. Same reasoning the GPU + // NoGPUEnvValue carries. + cpu := *resource.NewQuantity(2, resource.DecimalSI) + mem := *resource.NewQuantity(8*gib, resource.BinarySI) + for _, wantGPU := range []bool{true, false} { + env := BuildEnvSpec(cpu, mem, corev1.ResourceName("nvidia.com/gpu"), + *resource.NewQuantity(1, resource.DecimalSI), wantGPU) + if _, ok := env["RESOURCE_PROVENANCE"]; !ok { + t.Errorf("wantGPU=%v: RESOURCE_PROVENANCE missing entirely", wantGPU) + } + } +} + +func TestNormalizeProvenance(t *testing.T) { + cases := map[string]string{ + "installer": ProvenanceInstaller, + "user": ProvenanceUser, + // Everything else is unknown, NEVER a guess. "unknown" is the honest + // answer for a release predating the key, and callers treat it as a human + // choice — guessing "installer" would risk overruling an operator. + "unknown": ProvenanceUnknown, + "": ProvenanceUnknown, + "banana": ProvenanceUnknown, + "Installer": ProvenanceUnknown, // case-sensitive by design: the chart enum is lowercase + "user ": ProvenanceUnknown, // no trimming — a stray space is not a verdict + "future": ProvenanceUnknown, // a value this binary predates + } + for raw, want := range cases { + if got := NormalizeProvenance(raw); got != want { + t.Errorf("NormalizeProvenance(%q) = %q, want %q", raw, got, want) + } + } +} + +func TestParseTrainingReadsProvenance(t *testing.T) { + base := map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"} + + t.Run("explicit user", func(t *testing.T) { + env := map[string]string{} + for k, v := range base { + env[k] = v + } + env["RESOURCE_PROVENANCE"] = "user" + if got := ParseTraining(env).Provenance; got != ProvenanceUser { + t.Errorf("Provenance = %q, want %q", got, ProvenanceUser) + } + }) + + t.Run("explicit installer", func(t *testing.T) { + env := map[string]string{} + for k, v := range base { + env[k] = v + } + env["RESOURCE_PROVENANCE"] = "installer" + if got := ParseTraining(env).Provenance; got != ProvenanceInstaller { + t.Errorf("Provenance = %q, want %q", got, ProvenanceInstaller) + } + }) + + t.Run("a pre-marker release reports unknown, not empty", func(t *testing.T) { + // The shape every edge in the field has today. An empty string here would + // make callers branch on "" and invent their own default — a second + // policy, which is the thing this ticket removes. + if got := ParseTraining(base).Provenance; got != ProvenanceUnknown { + t.Errorf("Provenance = %q, want %q", got, ProvenanceUnknown) + } + }) + + t.Run("provenance does not disturb the numbers", func(t *testing.T) { + env := map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + } + train := ParseTraining(env) + if !train.HasCPUMem { + t.Fatal("HasCPUMem false") + } + if train.CPU.Value() != 4 || train.Mem.Value() != 16*gib { + t.Errorf("ceiling moved: got %s / %s", train.CPU.String(), train.Mem.String()) + } + }) +} + +func TestRoundTripSetThenRead(t *testing.T) { + // The invariant that matters end to end: what `resources set` writes, the + // read path reports as a human choice. If this ever breaks, the CLI would be + // telling a future ladder it may overwrite a size the operator chose. + cpu := *resource.NewQuantity(6, resource.DecimalSI) + mem := *resource.NewQuantity(24*gib, resource.BinarySI) + + written := BuildEnvSpec(cpu, mem, "", resource.Quantity{}, false) + readBack := ParseTraining(written) + + if readBack.Provenance != ProvenanceUser { + t.Errorf("round trip lost the marker: got %q, want %q", + readBack.Provenance, ProvenanceUser) + } + if readBack.CPU.Cmp(cpu) != 0 || readBack.Mem.Cmp(mem) != 0 { + t.Errorf("round trip changed the ceiling: %s / %s", + readBack.CPU.String(), readBack.Mem.String()) + } +} diff --git a/internal/resources/resources.go b/internal/resources/resources.go index 3f4d8cb9..61d918dc 100644 --- a/internal/resources/resources.go +++ b/internal/resources/resources.go @@ -56,6 +56,37 @@ type Training struct { GPUName corev1.ResourceName GPU resource.Quantity HasGPU bool + + // Provenance is WHO chose the ceiling above (backend#2220): ProvenanceUser, + // ProvenanceInstaller, or ProvenanceUnknown. It never affects the numbers. + // + // Anything unrecognised — including a release that predates the key — + // normalises to ProvenanceUnknown, which callers MUST treat as a human + // choice. Guessing "installer" for an unattributable value would risk + // overruling an operator who had deliberately set a size, and that risk is + // the entire reason the marker exists. + Provenance string +} + +// Who chose the training envelope. Mirrors env.RESOURCE_PROVENANCE in the +// tracebloc chart (client 1.9.49+); the installer writes the first two, this +// CLI writes ProvenanceUser, and ProvenanceUnknown covers everything else. +const ( + ProvenanceInstaller = "installer" + ProvenanceUser = "user" + ProvenanceUnknown = "unknown" +) + +// NormalizeProvenance maps a raw env value onto the three known states. +// Unrecognised input — empty, junk, a future value this binary predates — is +// ProvenanceUnknown, never a guess. +func NormalizeProvenance(raw string) string { + switch raw { + case ProvenanceInstaller, ProvenanceUser: + return raw + default: + return ProvenanceUnknown + } } // MachineCapacity reports the machine headline ("equipped with …") as the @@ -87,7 +118,12 @@ func ParseTraining(env map[string]string) Training { // chart injects this exact value when the operator set no override. cpu, mem, ok = parseCPUMem(DefaultTraining) } - t := Training{CPU: cpu, Mem: mem, HasCPUMem: ok} + t := Training{ + CPU: cpu, + Mem: mem, + HasCPUMem: ok, + Provenance: NormalizeProvenance(env["RESOURCE_PROVENANCE"]), + } gpuName, gpuQty, gpuOK := parseGPU(firstNonEmpty(env["GPU_LIMITS"], env["GPU_REQUESTS"])) if gpuOK { diff --git a/internal/resources/set.go b/internal/resources/set.go index 3e9e02be..bd63545e 100644 --- a/internal/resources/set.go +++ b/internal/resources/set.go @@ -208,7 +208,23 @@ const NoGPUEnvValue = "" // always written. func BuildEnvSpec(cpu, mem resource.Quantity, gpuName corev1.ResourceName, gpu resource.Quantity, wantGPU bool) map[string]string { spec := fmt.Sprintf("cpu=%s,memory=%s", cpu.String(), mem.String()) - env := map[string]string{"RESOURCE_REQUESTS": spec, "RESOURCE_LIMITS": spec} + env := map[string]string{ + "RESOURCE_REQUESTS": spec, + "RESOURCE_LIMITS": spec, + // backend#2220: `resources set` IS the human choice, so stamp it. This + // is not cosmetic bookkeeping — it is the difference between a future + // ladder re-deriving an installer-written size and it silently + // overruling a deliberate one. + // + // It must be written unconditionally, and especially when a marker is + // already present: an edge the installer stamped `installer` and the + // operator then re-sized by hand would otherwise keep saying + // `installer`, which is the single most dangerous state the marker can + // be in — a human choice wearing a label that invites overwriting. + // Omitting the key would not clear it either, for the same + // --reset-then-reuse-values reason documented above. + "RESOURCE_PROVENANCE": ProvenanceUser, + } if wantGPU { g := fmt.Sprintf("%s=%d", gpuName, gpu.Value()) env["GPU_LIMITS"], env["GPU_REQUESTS"] = g, g From 16aa63be6b6d6f8cee3a5e7486e3608fb89c8834 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Thu, 20 Aug 2026 13:42:55 +0200 Subject: [PATCH 2/3] fix(resources set): a same-size set must still stamp `user` (Bugbot + review, #539) Bugbot's High, confirmed by @saadqbal in review, and the fix was indeed sitting three lines below the bug. BuildEnvSpec stamps RESOURCE_PROVENANCE=user unconditionally WITHIN ITSELF, but the caller could exit before ever reaching it: `sameCeiling` returned "nothing to change" whenever the desired ceiling matched the current one. So an installer-sized edge whose operator ran `resources set --max`, or passed flags restating the current ceiling, kept `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is verbatim the state BuildEnvSpec's own comment calls the most dangerous the marker can be in, and my PR body claimed this PR and client#768 "are only correct together" while this hole made that untrue: not wrong, just narrower. Fixed with the phantom-GPU branch as the template -- same condition, same reason, already written and already reviewed. An unchanged ceiling now falls through to persist when the stored marker is not already `user`. Both conditions can hold at once, so both report their reason rather than one masking the other. `unknown` counts as stale deliberately. A pre-marker edge whose operator restates the ceiling has just made that size explicit, so recording it is the honest answer; the cost is one extra apply per edge, exactly once, because the second run sees `user` and is a clean no-op again. Three existing no-op tests now carry RESOURCE_PROVENANCE=user in their fixtures (TestSet_NoOpSkipsApply, TestSet_NoOpEvenWhenCurrentNoLongerFits, TestWizard_LeaveAsIs). Their invariant is unchanged and still asserted -- a clean no-op makes no helm call -- it just needs the marker present to BE the clean case. The uncovered case became TestSet_SameCeilingStampsProvenance, a table over installer / pre-marker / junk, asserting the apply happens, that we do not claim nothing changed while correcting the marker, and that the reason is stated. zz-all-strings.golden regenerated: one line, reviewed. Also rebased onto develop now that cli#538 has merged -- `git rebase --onto origin/develop 05517ed` to drop the squashed parent's commits rather than replay them into conflicts. GitHub had already retargeted the base to develop when #538 landed, so the stacked-PR banner in the description was stale; removed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. Refs: backend#2220, client#768 Co-Authored-By: Claude Opus 5 --- internal/cli/resources_set.go | 39 ++++++++-- internal/cli/resources_set_test.go | 77 ++++++++++++++++++- .../cli/testdata/golden/zz-all-strings.golden | 1 + 3 files changed, 106 insertions(+), 11 deletions(-) diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index c9f5c27f..b76e41ee 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -238,19 +238,42 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target * // there is nothing for the fit-check to protect. Sizing an actual CHANGE // is still validated below, before anything mutates. ceilingUnchanged := sameCeiling(desired, current) - if ceilingUnchanged && !phantomGPU { + // backend#2220: an unchanged ceiling is still a HUMAN CHOICE the moment the + // operator runs this command, so it must not exit before BuildEnvSpec stamps + // RESOURCE_PROVENANCE=user. BuildEnvSpec writes the marker unconditionally + // within itself, but this early return could skip it entirely — so an + // installer-sized edge whose operator ran `resources set --max`, or passed + // flags restating the current ceiling, kept `installer`. That is the state + // BuildEnvSpec's own comment calls the most dangerous the marker can be in: + // a deliberate choice wearing the one label that invites a future ladder to + // overwrite it. Caught by Bugbot and confirmed in review on #539. + // + // `unknown` counts as stale too: a pre-marker edge whose operator restates + // the ceiling has now made that size explicit, and recording it as such is + // the honest answer. The cost is one extra apply per edge, exactly once — + // the second run sees `user` and is a clean no-op again. + staleProvenance := current.Provenance != resources.ProvenanceUser + if ceilingUnchanged && !phantomGPU && !staleProvenance { p.Newline() p.Successf("Each training run already uses up to %s — nothing to change.", perRunSize(desired)) return nil } - if ceilingUnchanged { // phantomGPU == true here - // CPU/memory budget is unchanged, but this GPU-less machine's cluster - // still requests a GPU (a stale chart default). Don't treat it as a - // clean no-op — fall through to persist so BuildEnvSpec's explicit-empty - // GPU override lands and clears it; otherwise runs stay unschedulable / - // fall back to CPU while the heartbeat keeps advertising a GPU. + if ceilingUnchanged { + // Same shape as the phantom-GPU case, and for the same reason: the budget + // is unchanged but something else still needs persisting, so this is not + // a clean no-op. Both conditions can hold at once, so both report. p.Newline() - p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.") + if phantomGPU { + // CPU/memory budget is unchanged, but this GPU-less machine's cluster + // still requests a GPU (a stale chart default). Don't treat it as a + // clean no-op — fall through to persist so BuildEnvSpec's explicit-empty + // GPU override lands and clears it; otherwise runs stay unschedulable / + // fall back to CPU while the heartbeat keeps advertising a GPU. + p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.") + } + if staleProvenance { + p.Infof("Your CPU and memory budget is unchanged — recording it as your explicit choice so it is never resized automatically.") + } } // (5) Validate + fit-check — ONLY when the ceiling actually CHANGES. An diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 236860e5..cdd7f219 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -240,7 +240,15 @@ func TestSet_MaxUsesWholeMachineMinusOverhead(t *testing.T) { // helm upgrade entirely. func TestSet_NoOpSkipsApply(t *testing.T) { calls := fakeHelm(t) - cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) + // RESOURCE_PROVENANCE=user is what makes this a CLEAN no-op (backend#2220): + // restating the ceiling on an edge whose size is already recorded as the + // operator's choice leaves nothing at all to persist. Without the marker the + // command must fall through and stamp it — covered by + // TestSet_SameCeilingStampsProvenance below. + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + }) out, err := runSet(t, cs, nil, setReq{cores: "4", memory: "16", coresSet: true, memSet: true, yes: true}) if err != nil { t.Fatalf("no-op: %v", err) @@ -255,6 +263,57 @@ func TestSet_NoOpSkipsApply(t *testing.T) { } } +// TestSet_SameCeilingStampsProvenance: the hole Bugbot found on #539 and +// saadqbal confirmed — a same-size `resources set` used to return BEFORE +// BuildEnvSpec, so an installer-sized edge kept RESOURCE_PROVENANCE=installer +// even though a human had just chosen that size. +// +// That is the state BuildEnvSpec's own comment calls the most dangerous the +// marker can be in: a deliberate choice wearing the one label that invites a +// future ladder to overwrite it. `resources set` restating the current ceiling +// IS a human choice, so it must persist. +func TestSet_SameCeilingStampsProvenance(t *testing.T) { + for _, tc := range []struct { + name string + provenance string + }{ + {"installer-sized edge", "installer"}, + {"pre-marker edge", ""}, // ParseTraining normalises to unknown + {"junk marker", "banana"}, // ...as does anything unrecognised + } { + t.Run(tc.name, func(t *testing.T) { + calls := fakeHelm(t) + env := map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"} + if tc.provenance != "" { + env["RESOURCE_PROVENANCE"] = tc.provenance + } + cs := csWith("8", "32Gi", env) + out, err := runSet(t, cs, nil, setReq{ + cores: "4", memory: "16", coresSet: true, memSet: true, yes: true, + }) + if err != nil { + t.Fatalf("same-ceiling set: %v\n%s", err, out) + } + upgraded := false + for _, c := range *calls { + if len(c) >= 3 && c[1] == "upgrade" && c[2] != "--help" { + upgraded = true + } + } + if !upgraded { + t.Errorf("a stale %q marker must NOT be a clean no-op — the apply is what stamps `user`:\n%s", + tc.provenance, out) + } + if strings.Contains(out, "nothing to change") { + t.Errorf("must not claim nothing changed while the marker is being corrected:\n%s", out) + } + if !strings.Contains(out, "explicit choice") { + t.Errorf("the reason for the apply should be stated:\n%s", out) + } + }) + } +} + // TestSet_NoOpEvenWhenCurrentNoLongerFits: restating the ceiling that's already // applied must stay a clean no-op success even when the machine has SHRUNK under // it (smaller Docker Desktop VM, lost node) — the no-op check runs before the @@ -262,7 +321,12 @@ func TestSet_NoOpSkipsApply(t *testing.T) { // change on the same shrunken machine is still fit-checked. func TestSet_NoOpEvenWhenCurrentNoLongerFits(t *testing.T) { // Node 4 CPU / 8 GiB, but the cluster already runs with cpu=8,memory=16Gi. - cur := map[string]string{"RESOURCE_LIMITS": "cpu=8,memory=16Gi"} + // Marked `user` so this stays the clean-no-op case it is testing; the + // stale-marker fall-through has its own test (backend#2220). + cur := map[string]string{ + "RESOURCE_LIMITS": "cpu=8,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + } t.Run("flags restating the current ceiling", func(t *testing.T) { calls := fakeHelm(t) @@ -460,7 +524,14 @@ func TestWizard_GPURowOmittedWhenNoGPU(t *testing.T) { func TestWizard_LeaveAsIs(t *testing.T) { calls := fakeHelm(t) pr := &fakePrompter{answers: map[string]string{"How much may one training run use?": "Leave it as it is"}} - cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"}) + // Marked `user` so "leave it as it is" is the clean no-op this test is about. + // On an edge whose size is NOT yet recorded as the operator's choice, picking + // "leave it as it is" does still persist — the marker is being corrected, and + // TestSet_SameCeilingStampsProvenance covers that (backend#2220). + cs := csWith("8", "32Gi", map[string]string{ + "RESOURCE_LIMITS": "cpu=4,memory=16Gi", + "RESOURCE_PROVENANCE": "user", + }) out, err := runSet(t, cs, pr, setReq{}) if err != nil { t.Fatalf("wizard leave: %v", err) diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index 54a4f607..69adf1b8 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -398,6 +398,7 @@ screen. %s/%d are runtime placeholders. "Wrote client id + namespace to %s (no new credential — the existing one stands)." "You don't have permission to %s in this account." "Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule." +"Your CPU and memory budget is unchanged — recording it as your explicit choice so it is never resized automatically." "Your active client is not on the cluster your kubeconfig reaches, and no client here is confirmed. Check your kubeconfig context, then run: %s doctor" "Your active client is not on the cluster your kubeconfig reaches. To point this machine at the client that IS there: %s client create" "Your data is registered as a dataset. View it at https://ai.tracebloc.io/metadata" From 5a1539f6b97a55a2f99d9759a58100214a30b076 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Thu, 20 Aug 2026 14:00:59 +0200 Subject: [PATCH 3/3] chore(release): bump VERSION to 0.10.10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit v0.10.9 is already released and this PR changes published files under internal/*, so the release train needs a version above every released tag — version-bump-gate fails otherwise (backend#1561). Co-Authored-By: Claude Opus 5 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index f314d020..ddf1d4ae 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.9 +0.10.10