From 9a2f76af61ab6b1383a26fb619fd5c03547e0750 Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Wed, 26 Aug 2026 14:11:04 +0200 Subject: [PATCH] fix(resources): report an interrupted `resources set` as interrupted, not failed (backend#2255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit helm.Upgrade wrapped every Runner error as "helm upgrade failed", so a Ctrl-C / cancelled context mid-`helm upgrade` — after helm may have already applied the values — was reported as a flat failure with no progress, telling the user nothing changed when the change could be live. - helm.Upgrade now distinguishes an interrupt (ctx cancelled, or the helm child exiting 130 before NotifyContext flips ctx.Err()) from a genuine failure, returning the new helm.ErrInterrupted sentinel plus the resolved Plan so the caller can surface what was in flight. Mirrors the cli package's installerRunInterrupted (Bugbot #394/#397). - resources set shows a live wait line during the apply (progress), and on an interrupt mid-upgrade reports "may already have applied — re-run to confirm" (via launcher()) and exits 130, never "helm upgrade failed". A Ctrl-C before the upgrade runs (probe / repo add-update) also exits a quiet 130 via installerRunInterrupted, but without the "may have applied" note. - Tests for both halves at the helm and cli layers: cancelled-context and exit-130 interrupts, a pre-upgrade interrupt, a genuine failure, and the progress wait-line on a real apply. Co-Authored-By: Claude Opus 4.8 --- internal/cli/resources_set.go | 36 +++++- internal/cli/resources_set_test.go | 119 ++++++++++++++++++ .../cli/testdata/golden/zz-all-strings.golden | 4 + internal/helm/upgrade.go | 39 ++++++ internal/helm/upgrade_test.go | 84 +++++++++++++ 5 files changed, 281 insertions(+), 1 deletion(-) diff --git a/internal/cli/resources_set.go b/internal/cli/resources_set.go index 072097a0..47cadfde 100644 --- a/internal/cli/resources_set.go +++ b/internal/cli/resources_set.go @@ -598,8 +598,42 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o Env: env, DryRun: dryRun, } - plan, err := helm.Upgrade(ctx, params) + // Apply. Under --dry-run helm.Upgrade runs nothing and returns immediately, so + // no spinner. A real apply shells out to `helm upgrade --wait`, which can take + // several seconds during which nothing prints — show a live wait line so the run + // doesn't look frozen and a Ctrl-C mid-flight isn't invisible (backend#2255). + var ( + plan helm.Plan + err error + ) + if dryRun { + plan, err = helm.Upgrade(ctx, params) + } else { + sp := p.Spinner("Applying the resource change…", "Ctrl-C to cancel") + plan, err = helm.Upgrade(ctx, params) + sp.Stop() + } if err != nil { + // A Ctrl-C / cancelled context mid-upgrade is NOT a failure: helm may have + // already written the values before it was killed, so the change can be + // live on the cluster. Report that honestly and exit quietly (130) like + // seal/upgrade/prepare-host — never "helm upgrade failed", which reads as + // "nothing changed" and sends the user off to re-run blind (backend#2255). + if errors.Is(err, helm.ErrInterrupted) { + p.Newline() + p.Warnf("Interrupted before the change could be confirmed.") + p.Hintf("It may already have applied — re-run `%s resources set` to check the current per-run ceiling.", launcher()) + p.Detailf("in-flight command: %s", plan.Command) // verbose-only + return &exitError{code: exitInterrupted} + } + // A Ctrl-C that landed BEFORE the mutating upgrade (during the reuse-flag + // probe or the repo add/update) — nothing was applied, so no "may have + // applied" note, but still a quiet 130 like the rest of the CLI rather than + // a scary exit-1 "context canceled". Reuses the shared helper, which also + // catches the exit-130 race where helm dies before ctx.Err() flips. + if installerRunInterrupted(ctx, err) { + return &exitError{code: exitInterrupted} + } return &exitError{code: exitFailure, err: err} } diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index dd2adbb9..ed18fd7a 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -564,6 +564,125 @@ func TestSet_DryRunAppliesNothing(t *testing.T) { } } +// failingHelm installs a helm.Runner whose mutating `helm upgrade` returns err — +// the reuse-flag probe and repo calls still succeed — so the apply path can be +// driven into its failure / interrupt handling without a real cluster. +func failingHelm(t *testing.T, err error) { + t.Helper() + orig := helm.Runner + helm.Runner = func(_ context.Context, name string, args ...string) (string, error) { + if len(args) >= 2 && args[0] == "upgrade" && args[1] == "--help" { + return " --reset-then-reuse-values", nil // reuse-flag probe + } + if len(args) >= 2 && args[0] == "upgrade" { // the mutating upgrade (release != "--help") + return "boom", err + } + return "", nil // repo list / add / update + } + t.Cleanup(func() { helm.Runner = orig }) +} + +// TestSet_InterruptReportedNotFailed: a Ctrl-C / cancelled context mid-`helm +// upgrade` must exit 130 with an honest "may have applied" note — NOT exit 1 +// "helm upgrade failed", which reads as "nothing changed" when the change may +// actually be live (backend#2255). +func TestSet_InterruptReportedNotFailed(t *testing.T) { + failingHelm(t, errors.New("signal: killed")) + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // the SIGINT handler already propagated the cancel + + var buf bytes.Buffer + err := applyResourcesSet(ctx, ui.New(&buf, ui.WithColor(false)), nil, + setTarget(cs), cluster.KubeconfigOptions{Path: "/tmp/kc"}, + setReq{cores: "4", coresSet: true, yes: true}) + + if got := exitCode(t, err); got != exitInterrupted { + t.Fatalf("an interrupted apply must exit %d, got %d (%v)\n%s", exitInterrupted, got, err, buf.String()) + } + out := buf.String() + if !strings.Contains(out, "Interrupted") || !strings.Contains(out, "may already have applied") { + t.Errorf("interrupt output should say the change may already have applied:\n%s", out) + } + if strings.Contains(out, "helm upgrade failed") { + t.Errorf("must not frame an interrupt as a helm failure:\n%s", out) + } +} + +// TestSet_GenuineHelmFailureExitsFailure: a real helm failure (live context, a +// plain non-130 error) still exits with the failure code and "helm upgrade +// failed" — the interrupt handling must not swallow real failures (backend#2255). +func TestSet_GenuineHelmFailureExitsFailure(t *testing.T) { + failingHelm(t, errors.New("boom")) + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + + var buf bytes.Buffer + err := applyResourcesSet(context.Background(), ui.New(&buf, ui.WithColor(false)), nil, + setTarget(cs), cluster.KubeconfigOptions{Path: "/tmp/kc"}, + setReq{cores: "4", coresSet: true, yes: true}) + + if got := exitCode(t, err); got != exitFailure { + t.Fatalf("a genuine helm failure must exit %d, got %d (%v)", exitFailure, got, err) + } + if !strings.Contains(err.Error(), "helm upgrade failed") { + t.Errorf("a genuine failure should read as a helm failure: %v", err) + } +} + +// TestSet_PreUpgradeInterruptIsQuiet: a Ctrl-C during the repo add/update phase — +// BEFORE the mutating `helm upgrade` — exits quietly (130) like the rest of the +// CLI, not a scary exit-1 "context canceled". But since nothing was applied yet, +// it must NOT claim the change may have applied (backend#2255). +func TestSet_PreUpgradeInterruptIsQuiet(t *testing.T) { + orig := helm.Runner + helm.Runner = func(_ context.Context, name string, args ...string) (string, error) { + if len(args) >= 2 && args[0] == "upgrade" && args[1] == "--help" { + return " --reset-then-reuse-values", nil // reuse-flag probe + } + if len(args) >= 2 && args[0] == "repo" && args[1] == "update" { + return "boom", errors.New("signal: killed") // Ctrl-C mid repo update + } + return "", nil + } + t.Cleanup(func() { helm.Runner = orig }) + + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var buf bytes.Buffer + err := applyResourcesSet(ctx, ui.New(&buf, ui.WithColor(false)), nil, + setTarget(cs), cluster.KubeconfigOptions{Path: "/tmp/kc"}, + setReq{cores: "4", coresSet: true, yes: true}) + + if got := exitCode(t, err); got != exitInterrupted { + t.Fatalf("a pre-upgrade interrupt must exit %d, got %d (%v)\n%s", exitInterrupted, got, err, buf.String()) + } + if strings.Contains(buf.String(), "may already have applied") { + t.Errorf("nothing was applied before the upgrade ran — must not claim it may have:\n%s", buf.String()) + } +} + +// TestSet_ShowsProgressOnRealApply: a real (non-dry-run) apply prints a live wait +// line, so the run isn't invisible while `helm upgrade --wait` runs and a Ctrl-C +// mid-flight isn't a mystery (backend#2255, requirement #2). +func TestSet_ShowsProgressOnRealApply(t *testing.T) { + fakeHelm(t) // every call succeeds + cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"}) + + var buf bytes.Buffer + err := applyResourcesSet(context.Background(), ui.New(&buf, ui.WithColor(false)), nil, + setTarget(cs), cluster.KubeconfigOptions{Path: "/tmp/kc"}, + setReq{cores: "4", coresSet: true, yes: true}) + if err != nil { + t.Fatalf("real apply: %v", err) + } + if !strings.Contains(buf.String(), "Applying the resource change") { + t.Errorf("a real apply should surface a progress line:\n%s", buf.String()) + } +} + // --- wizard ----------------------------------------------------------------- // TestWizard_PreselectedMax: on a terminal with no flags, accepting the default diff --git a/internal/cli/testdata/golden/zz-all-strings.golden b/internal/cli/testdata/golden/zz-all-strings.golden index dc1fe99a..bc709360 100644 --- a/internal/cli/testdata/golden/zz-all-strings.golden +++ b/internal/cli/testdata/golden/zz-all-strings.golden @@ -125,6 +125,7 @@ screen. %s/%d are runtime placeholders. "Add/resize a node to meet the job's requests, or lower RESOURCE_REQUESTS on jobs-manager." "Already signed out." "Applies to your next training run; a run already going keeps its size." +"Applying the resource change…" "Ask one of these admins (or ask them to grant you access)" "Bookkeeping cleanup incomplete — the old table is gone, but its run-journal/salt rows may remain: %s" "Bookkeeping cleanup incomplete — the table is gone, but its run-journal/salt rows may remain: %s" @@ -235,6 +236,8 @@ screen. %s/%d are runtime placeholders. "Ingestion summary" "Ingestor SA token" "Ingests a local dataset into your secure environment's storage,\nsubmits the ingestion run, and follows it to completion (streaming\nprogress + the final summary). Your data never leaves your own\ninfrastructure. Supports %[1]d tasks across the image, text, and\ntabular / time-series families; pick one with --task.\n\n is the data itself. What it looks like depends on the task:\n\n tabular / time-series — the dataset is a single CSV. Pass the .csv\n file directly, or a folder holding exactly one .csv:\n\n churn.csv (the .csv file itself)\n or\n churn/\n data.csv (the one .csv in the folder)\n\n image (classification, object/keypoint detection) — a folder with\n labels.csv + an images/ subfolder:\n\n cats_dogs/\n labels.csv (required)\n images/ (required)\n 001.jpg\n ...\n\n text (classification, masked language modeling) — a folder with\n labels.csv + a %[2]s/ subfolder (masked language modeling uses %[3]s/):\n\n reviews/\n labels.csv (required)\n %[2]s/ (required — %[3]s/ for masked language modeling)\n 001.txt\n ...\n\nA bare .csv file is accepted only for the tabular / time-series family;\nimage and text datasets must be a folder.\n\nAccepted image extensions: .jpg, .jpeg, or .png (case-insensitive).\nAll images in one dataset must share a single type — the cluster\nvalidates the type it was told to expect.\n\nv0.1 caps the dataset at 1 GiB total + 500 MiB per file. Larger\ndatasets need the v0.2 cloud-source story (S3/GCS/HTTPS sources) —\nsee tracebloc/client#147 non-goals.\n\nExit codes:\n 0 files staged + ingested successfully (or --detach: just staged + submitted)\n 2 schema validation failed (synthesized spec rejected) or\n v0.1-unsupported task passed\n 3 local-layout or kubeconfig error\n 4 cluster reachable but no tracebloc client / shared storage missing\n 5 ingestor SA token couldn't be obtained, or jobs-manager\n rejected the token (401/403)\n 6 destination table already exists (re-run with --overwrite to\n replace it, or pick a different --name)\n 7 pre-flight succeeded but staging the files failed\n (Pod creation, image pull, exec stream, or remote tar error) —\n or, with --overwrite, removing the old table failed\n 8 jobs-manager rejected the submit (4xx/5xx other than auth)\n 9 ingestion Job exited non-zero, or completed with row-level\n failures the summary panel reports" +"Interrupted before the change could be confirmed." +"It may already have applied — re-run `%s resources set` to check the current per-run ceiling." "It reports more memory than the machine really has, so two trainings that each look like they fit can together run it out of memory and take the environment down. Run one training at a time; to fix it for good, recreate the environment as a single-node one. `%s doctor --verbose` shows the numbers and the exact flags." "Kept local data and config (~/.tracebloc); cleared the active-client pointer — --keep-data." "Kept on tracebloc" @@ -531,6 +534,7 @@ screen. %s/%d are runtime placeholders. "images" "images/ and annotations/ don't pair up: %s. Every image needs a same-named .xml annotation (and vice versa) — the cluster rejects mismatches after the upload." "images/ and masks/ don't pair up: %s. Every image needs a same-named \"_mask.png\" in masks/ (and vice versa) — the cluster rejects mismatches after the upload." +"in-flight command: %s" "infer from CSV" "inferring schema from CSV: %w" "ingested %s of %s records (%.1f%%)" diff --git a/internal/helm/upgrade.go b/internal/helm/upgrade.go index aa0f8429..dc90c3f2 100644 --- a/internal/helm/upgrade.go +++ b/internal/helm/upgrade.go @@ -22,6 +22,7 @@ package helm import ( "context" + "errors" "fmt" "os" "os/exec" @@ -29,6 +30,18 @@ import ( "strings" ) +// ErrInterrupted marks an upgrade that was aborted by the operator (Ctrl-C / +// SIGINT) or a cancelled context, rather than one helm rejected on its own. It +// matters because helm may have ALREADY applied the values before it was killed +// — the change can be live on the cluster — so callers MUST report it as +// "interrupted; the change may have applied", never as "helm upgrade failed" +// (backend#2255). Test for it with errors.Is(err, helm.ErrInterrupted). +var ErrInterrupted = errors.New("helm upgrade interrupted") + +// sigintExit is a child's exit code after SIGINT (128 + SIGINT): helm exits this +// on a terminal Ctrl-C. Mirrors exitInterrupted in the cli package. +const sigintExit = 130 + const ( // repoName / repoURL / chartName mirror install-client-helm.sh's // TRACEBLOC_HELM_REPO_NAME / _URL / TRACEBLOC_CHART_NAME so a CLI-driven @@ -155,11 +168,37 @@ func Upgrade(ctx context.Context, p UpgradeParams) (Plan, error) { args := buildArgs(p, chartRef, reuseFlag, f.Name(), timeout) if out, uerr := Runner(ctx, "helm", args...); uerr != nil { + if runInterrupted(ctx, uerr) { + // Aborted (Ctrl-C / cancelled ctx) mid-upgrade — NOT a helm failure. + // helm may have already written the values before it was killed, so + // the change can be live. Hand back the resolved Plan (not Plan{}) so + // the caller can show what was in flight, and flag it as an interrupt + // (the ErrInterrupted sentinel) so it's reported honestly instead of + // "helm upgrade failed" (backend#2255). + return plan(chartRef, args, valuesYAML), ErrInterrupted + } return Plan{}, fmt.Errorf("helm upgrade failed: %w\n%s", uerr, strings.TrimSpace(out)) } return plan(chartRef, args, valuesYAML), nil } +// runInterrupted reports whether a helm shell-out ended because the operator +// aborted it (Ctrl-C) rather than helm failing on its own. ctx.Err() catches a +// cancel the signal handler already propagated; but on a terminal Ctrl-C the helm +// child can die and CombinedOutput return BEFORE NotifyContext flips ctx.Err() (a +// race), so also treat helm's 130 (128+SIGINT) exit as an interrupt. Mirrors the +// cli package's installerRunInterrupted (Bugbot #394/#397). +func runInterrupted(ctx context.Context, runErr error) bool { + if ctx.Err() != nil { + return true + } + var ee *exec.ExitError + if errors.As(runErr, &ee) { + return ee.ExitCode() == sigintExit + } + return false +} + // buildArgs assembles the helm upgrade argv. Order mirrors the installer/cronjob: // release, chart, --namespace, [--kube-context], [--kubeconfig], --version, reuse, // -f values, --wait --timeout. Context/kubeconfig/version are appended only when diff --git a/internal/helm/upgrade_test.go b/internal/helm/upgrade_test.go index 8b5399f0..16a256c2 100644 --- a/internal/helm/upgrade_test.go +++ b/internal/helm/upgrade_test.go @@ -2,6 +2,8 @@ package helm import ( "context" + "errors" + "os/exec" "strings" "testing" @@ -16,6 +18,9 @@ type fakeRunner struct { help string // fail, when set, makes the matching call return an error. failOn string + // failErr overrides the error returned on a failOn match (default errBoom) — + // e.g. a real *exec.ExitError so the interrupt classifier can be exercised. + failErr error } func (f *fakeRunner) run(_ context.Context, name string, args ...string) (string, error) { @@ -23,6 +28,9 @@ func (f *fakeRunner) run(_ context.Context, name string, args ...string) (string f.calls = append(f.calls, call) joined := strings.Join(call, " ") if f.failOn != "" && strings.Contains(joined, f.failOn) { + if f.failErr != nil { + return "boom", f.failErr + } return "boom", errBoom } if len(args) >= 2 && args[0] == "upgrade" && args[1] == "--help" { @@ -191,6 +199,82 @@ func TestUpgrade_HelmFailureSurfaces(t *testing.T) { } } +// TestUpgrade_GenuineFailureIsNotInterrupt pins the "real failure" half of the +// backend#2255 contrast: a plain helm failure on a live context (no cancel, a +// non-130 error) stays a "helm upgrade failed" error and is NEVER misclassified +// as an interrupt — the interrupt handling must not swallow real failures. +func TestUpgrade_GenuineFailureIsNotInterrupt(t *testing.T) { + f := &fakeRunner{help: " --reset-then-reuse-values", failOn: "upgrade client-123"} + install(t, f) + _, err := Upgrade(context.Background(), baseParams()) + if err == nil { + t.Fatal("expected an error when helm upgrade fails") + } + if errors.Is(err, ErrInterrupted) { + t.Errorf("a genuine failure must NOT be classified as an interrupt: %v", err) + } + if !strings.Contains(err.Error(), "helm upgrade failed") { + t.Errorf("a genuine failure should read as a helm failure: %v", err) + } +} + +// TestUpgrade_CancelledContextIsInterrupt is the interrupt half of the contrast: +// a Ctrl-C / cancelled context while the mutating `helm upgrade` is in flight must +// surface as ErrInterrupted — not "helm upgrade failed" — because helm may have +// ALREADY applied the values before it was killed. The resolved Plan comes back +// too (not Plan{}) so the caller can show what was in flight (backend#2255). +func TestUpgrade_CancelledContextIsInterrupt(t *testing.T) { + f := &fakeRunner{help: " --reset-then-reuse-values", failOn: "upgrade client-123"} + install(t, f) + ctx, cancel := context.WithCancel(context.Background()) + cancel() // the signal handler already propagated the cancel + + plan, err := Upgrade(ctx, baseParams()) + if !errors.Is(err, ErrInterrupted) { + t.Fatalf("a cancelled context must yield ErrInterrupted, got %v", err) + } + if strings.Contains(err.Error(), "helm upgrade failed") { + t.Errorf("an interrupt must not be framed as a helm failure: %v", err) + } + if _, ok := f.upgradeCall(); !ok { + t.Errorf("the upgrade should still have been attempted; calls=%v", f.calls) + } + // Plan is populated so the caller can surface what was in flight. + if !strings.Contains(plan.Command, "upgrade client-123") { + t.Errorf("interrupt must return the resolved Plan for progress display, got %+v", plan) + } +} + +// TestUpgrade_HelmExit130IsInterrupt covers the race the classifier guards: a +// terminal Ctrl-C can kill the helm child (exit 130 = 128+SIGINT) and +// CombinedOutput can return BEFORE NotifyContext flips ctx.Err(). On a LIVE +// context, only that exit code marks the interrupt. +func TestUpgrade_HelmExit130IsInterrupt(t *testing.T) { + if _, err := exec.LookPath("sh"); err != nil { + t.Skip("sh not available to synthesize an exit-130 ExitError") + } + // A real *exec.ExitError with code 130 — exactly what helm.Runner surfaces + // when the helm child is killed by SIGINT. + exit130 := exec.Command("sh", "-c", "exit 130").Run() + if exit130 == nil { + t.Fatal("expected a non-nil error from exit 130") + } + f := &fakeRunner{help: " --reset-then-reuse-values", failOn: "upgrade client-123", failErr: exit130} + install(t, f) + + plan, err := Upgrade(context.Background(), baseParams()) // live ctx: only the exit code says interrupt + if !errors.Is(err, ErrInterrupted) { + t.Fatalf("helm exit 130 on a live context must be an interrupt, got %v", err) + } + if strings.Contains(err.Error(), "helm upgrade failed") { + t.Errorf("an interrupt must not be framed as a helm failure: %v", err) + } + // Plan comes back populated so the caller can surface what was in flight. + if !strings.Contains(plan.Command, "upgrade client-123") { + t.Errorf("interrupt must return the resolved Plan, got %+v", plan) + } +} + // TestUpgrade_RefusesUnpinnedRemoteChart: a remote chart ref with no // ChartVersion must be REFUSED before any shell-out — an unpinned upgrade would // pull the latest chart and silently change the release (FIX 1).