fix(resources): report an interrupted resources set as interrupted, not failed (backend#2255)

[aptracebloc]

Closes tracebloc/backend#2255 — https://github.com/tracebloc/backend/issues/2255

Problem

internal/helm/upgrade.go wrapped everyhelm shell-out error as
helm upgrade failed: %w, with no distinction for an interrupt. So when
tb resources set is interrupted (SIGINT / cancelled context) after helm had
already applied the values, it reported "helm upgrade failed" and showed no
progress — telling the user nothing changed when the change was actually live.

Fix

1. Distinguish interrupt from failure.helm.Upgrade now classifies the
   mutating helm upgrade error: a cancelled context, or the helm child exiting
   130 (128+SIGINT) before NotifyContext flips ctx.Err() (the same race
   installerRunInterrupted guards, Bugbot feat(cli): tracebloc prepare-host wrapper (#1178, cli) #394/Release develop → main #397), is returned as the new
   helm.ErrInterrupted sentinel — plus the resolved Plan (not Plan{})
   so the caller can show what was in flight. Genuine failures are unchanged.

2. Report it honestly + surface progress.resources set now shows a live
   wait line (Applying the resource change… Ctrl-C to cancel) during the real
   apply, so the run isn't invisible. On an interrupt mid-upgrade it prints
   "Interrupted before the change could be confirmed. It may already have
   applied — re-run <launcher> resources set to check…" and exits 130,
   never "helm upgrade failed". A Ctrl-C before the upgrade runs (reuse-flag
   probe / repo add-update) also exits a quiet 130 via installerRunInterrupted,
   but without the "may have applied" note — nothing was applied yet.

3. Tests for both halves at each layer: cancelled-context and exit-130
   interrupts vs a genuine failure (helm package); interrupt-reported-not-failed,
   pre-upgrade quiet interrupt, genuine-failure-still-fails, and the progress
   wait-line on a real apply (cli package).

Notes for the reviewer

• sigintExit = 130 is duplicated from the cli package's exitInterrupted on
  purpose — internal/helm can't import internal/cli (cycle). The classifier
  mirrors installerRunInterrupted, which the cli layer reuses directly.
• Out of scope (kept to one self-contained change): a helm upgrade --wait
  readiness timeout applies the values but still reports "helm upgrade failed".
  Same misleading class, different (non-SIGINT) trigger — worth a follow-up.

Evidence

$ go test ./internal/helm/... ./internal/cli/...
ok github.com/tracebloc/cli/internal/helm
ok github.com/tracebloc/cli/internal/cli
$ golangci-lint run ./internal/helm/... ./internal/cli/...
0 issues.

/code-review (xhigh) run before push: 6 findings surfaced, 4 fixed in this
branch (launcher name, pre-upgrade quiet interrupt, dead interruptedErr
output-append, missing progress-line test), 2 skipped with rationale (verbose
in-flight temp-path is diagnostic-only; the --wait-timeout case above).

🤖 Generated with Claude Code

---

Note

Low Risk
CLI/helm UX and error classification only; no change to what gets applied on success; genuine failures still surface as failures.

Overview
Fixes backend#2255: when resources set is interrupted during helm upgrade --wait, the CLI no longer reports "helm upgrade failed" (which implies nothing changed) and no longer sits silent while helm runs.

helm.Upgrade now classifies aborts via helm.ErrInterrupted (cancelled context or child exit 130, covering the race where helm dies before ctx.Err() flips). On interrupt it still returns the resolved Plan so the caller can show what was in flight; real helm errors stay helm upgrade failed.

resources set shows a spinner (Applying the resource change…) on a real apply. Mid-upgrade interrupt exits 130 with an honest "may already have applied" hint and a re-run suggestion; interrupts before the mutating upgrade (repo/probe) also exit 130 quietly via installerRunInterrupted, without that hint. Tests cover both layers plus the progress line; golden strings pick up the new messages.

^{Reviewed by Cursor Bugbot for commit 9a2f76a. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

Read this closely and ran it locally. The classification is correct and the coverage is genuine on both halves — I chased one possible misclassification and it turned out to be unreachable, so I'm not raising it as a finding. Not approving only because 14 checks are still pending.

The distinction the fix rests on is the one that matters to a user. "helm upgrade failed" after helm has already written the values is worse than an unhelpful message — it's an actively false statement about cluster state, and it points the user away from checking. Returning the resolved Plan rather than Plan{} on the interrupt path is the detail that makes the honest message possible; without it the caller would have nothing to show and would fall back to something vague.

The exit-130 fallback is not belt-and-braces, it's the common case. On a terminal Ctrl-C the SIGINT reaches helm and the CLI at once, so the child can die and CombinedOutput return beforeNotifyContext flips ctx.Err(). Checking only ctx.Err() would have left the plain interactive Ctrl-C — the exact scenario in the ticket — still reported as a failure, intermittently, which is the worst kind of bug to chase. Good that this mirrors the reasoning already settled in installerRunInterrupted under #394/#397 rather than re-deriving it.

What I went looking for and did not find.runInterrupted keys on ctx.Err() != nil, which is also true for context.DeadlineExceeded — so a timeout would be reported as an operator interrupt and exit 130. I grepped the whole path: there is no WithTimeout or WithDeadline anywhere between the command entry and Upgrade, and the only cancellation source is NotifyContext. So ctx.Err() can only ever be Canceled here and the case is unreachable. Flagging it would have cost you a round-trip for nothing — noting it only so it's on record that if a deadline is ever introduced upstream, errors.Is(ctx.Err(), context.Canceled) becomes the correct predicate.

Mutation-checked at both layers. Removing the interrupt classification from Upgrade:

--- FAIL: TestUpgrade_CancelledContextIsInterrupt
--- FAIL: TestUpgrade_HelmExit130IsInterrupt
--- FAIL: TestSet_InterruptReportedNotFailed

Three tests, spanning the helm package and the cli package, so the behaviour is pinned at the boundary and at the user-visible report — not just the unit. And the negative cases (TestUpgrade_GenuineFailureIsNotInterrupt, TestSet_GenuineHelmFailureExitsFailure) correctly stayed green under the mutation, which is what proves they're testing the failure direction rather than passing by accident. A suite where every test reddens under one mutation usually means the tests only assert one thing; this one splits properly.

And TestSet_PreUpgradeInterruptIsQuiet is the case I'd expect to be missed. A Ctrl-C during the reuse-flag probe or repo add-update must exit 130without the "may already have applied" note, because nothing was applied — telling the user to go check a change that was never attempted is its own small lie. Covering that separately rather than lumping all interrupts together is the right call.

One cosmetic note, non-blocking.sigintExit = 130 in helm and exitInterrupted in cli are two names for the same constant, and runInterrupted is a near-identical twin of installerRunInterrupted. Fine as-is — 128+SIGINT is fixed by POSIX, so the value can't drift — but the predicate could: if someone later teaches one of them to treat, say, SIGTERM as an interrupt too, the two paths silently diverge. Not worth a shared package for nine lines; worth knowing they're a pair.

Ping me when checks land and I'll pick it up.

[LukasWodka]

Approving 9a2f76af — 28 checks pass, 3 skipping, no threads. Head is unchanged from the commit I reviewed, so the verification stands:

• the exit-130 fallback is the common case, not belt-and-braces — on a terminal Ctrl-C the helm child can die before NotifyContext flips ctx.Err(), so a ctx.Err()-only check would leave the plain interactive interrupt intermittently misreported, which is the worst version of this bug to chase;
• returning the resolved Plan rather than Plan{} is what makes the honest message possible at all;
• the DeadlineExceeded misclassification I went looking for is unreachable — no WithTimeout or WithDeadline anywhere between command entry and Upgrade, so ctx.Err() can only ever be Canceled here;
• mutation-proved across both packages: removing the classification reddens TestUpgrade_CancelledContextIsInterrupt, TestUpgrade_HelmExit130IsInterrupt and TestSet_InterruptReportedNotFailed, while both genuine-failure tests stay green — which is what shows they test the failure direction rather than passing by accident.

The sigintExit / exitInterrupted twin-constant note stays a non-blocking observation; nothing needed for this to land.

[aptracebloc]

Thanks for the close read — and for running it locally and mutation-checking both layers.

Checks have landed (28 green, 3 skipped, Bugbot clean), so it's ready whenever you want to pick it back up — re-requested your review.

On your two non-blocking notes, both intentional and on record:

• DeadlineExceeded vs Canceled. Same conclusion I reached: the only cancellation source in the path is NotifyContext (no WithTimeout/WithDeadline between command entry and Upgrade), so ctx.Err() can only be Canceled and the misclassification is unreachable today. Leaving runInterrupted keyed on ctx.Err() != nil rather than churning the branch; if a deadline is ever introduced upstream, errors.Is(ctx.Err(), context.Canceled) is the correct predicate to switch to.
• sigintExit / runInterrupted as a pair. Deliberate — internal/helm can't import internal/cli (cycle), so the constant and the classifier are mirrored rather than shared. The value is POSIX-fixed (128+SIGINT), but you're right that the predicate could drift; if either side ever learns a new interrupt signal (e.g. SIGTERM), the two need to move in lockstep.