fix(doctor): measured image-pull/dataset Fail outranks the wait-for-capacity Warn (backend#3248)

[aptracebloc]

Closes tracebloc/backend#3248

Problem

The stuckPending && heldByJob arm in summarizeDoctor (internal/cli/doctor.go) returned StatusWarn above the Image-pull-secret and Dataset-volume StatusFail arms. checkImagePull and checkPVC are independent of Pod health and Node capacity, so a genuine failure (missing pull secret, unbound PVC) can co-occur with a pod Pending past grace beside a running job — the exact case the Wait-Warn matches. Sitting below the Warn, that measured failure was shadowed: doctor exited 0 (Warn) instead of 2 (Fail), hiding a real training-blocker behind a "wait for the job" warning.

Fix

Reorder the switch so the two measured Fail arms are read before the Wait-Warn. The Wait-Warn must stay above the plain stuck-Pending Fail (backend#2870), so the measured Fails necessarily move above stuckPending too — consistent with the switch's measured-beats-inferred rule.

Class check: the only Fail arm still below the Wait-Warn is the generic Node-capacity Fail, which is mutually exclusive with heldByJob (one Result can't be both Warn and Fail), so it can't co-occur and can't be shadowed.

Composition with backend#3247 (cli#642, now merged)

#3247 landed first (it was the promotion blocker) and also edits this switch — it adds a StuckJobPod Fail arm and reworks the checkNodeFit producer. This branch was merged onto it; the composed order places the measured image-pull/dataset Fails above both the Wait-Warn and #3247's StuckJobPod arm, and updates #3247's now-stale arm comment.

Review follow-up (LukasWodka on this PR)

LukasWodka's review found that checkImagePull/checkPVC returned StatusFail for any read error (Forbidden/timeout), not only a measured absence — so promoting those arms could flip a healthy environment to exit 2 on an RBAC/timeout blip. Fixed at the producer:

• checkImagePull: a non-IsNotFound Secrets.Get error is now a can't-check StatusWarn (CantReadImagePullSecret prefix); StatusFail only for a genuine not-found / wrong-type / malformed secret.
• checkPVC: a DiscoverSharedPVC read failure (cluster.PVCReadErrPrefix, now a shared constant) is a can't-check StatusWarn; StatusFail only for a read-and-found missing / unbound PVC.
• summarizeDoctor gets two matching can't-check Unknown arms, so a read failure drops to the honest "couldn't check …" tier instead of the promoted Fail.
• Thread 3 (over-commit Warn shadowed by the Wait-Warn in the Pending variant) is pre-existing and message-only; a clean reorder is blocked by transitivity (the stuck-Pending Fail sits between the two Warns), so the current behavior is pinned by a test with a comment recording the gap.

Tests

• The measured Fail beside a running job now wins (exit 2); the Wait-Warn still applies with no measured Fail; the no-heldByJob half is pinned.
• A can't-read pull-secret/PVC rolls up to an honest can't-check (never the promoted Fail), including the exact "read blip beside a running job stays the wait Warn" regression.
• Producer tests for the checkImagePull/checkPVC read-error → can't-check split.

Verification

go build ./..., go vet, gofmt -s -l, golangci-lint v2.12.2 (0 issues), and the full go test ./... suite (incl. the zz-all-strings golden, regenerated + reviewed) — all green.

Version-bump gate

No VERSION bump: develop carries the pending, unreleased 0.10.24 (latest tag v0.10.23), so the gate passes as-is; hand-bumping would violate the org's "never hand-bump a version file" rule.

— drafted with Claude Code

---

Note

Medium Risk
Changes doctor exit codes and user-facing rollup ordering for common “job running + pod pending” states; logic is heavily tested but misclassification would mislead operators on training readiness.

Overview
tracebloc doctor no longer reports exit 0 (“wait for the running job”) when image-pull or dataset-storage checks have a real Fail alongside a pending pod and a job holding capacity. summarizeDoctor now evaluates those measured Fail arms before the wait-for-capacity Warn, so exit 2 and the correct headline win over the inference.

Read errors vs real failures: checkImagePull and checkPVC treat RBAC/timeout “couldn’t read” as Warn with stable prefixes (CantReadImagePullSecret, PVCReadErrPrefix on DiscoverSharedPVC errors), not as missing-secret or unbound-PVC Fail. The rollup maps those to Unknown “couldn’t check …” lines so a transient read blip does not flip a healthy cluster to exit 2 after the reorder.

Tests pin precedence, can’t-read behavior, and the known message-only case where over-commit Warn is still shadowed by the wait Warn when a pod is pending.

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

[LukasWodka]

Reviewed at 89b9880ab3d33c661d3e168871e08fa28bbeaaec. Verified in a detached worktree rather than from the description: read the whole summarizeDoctor switch and both producers, ran go test ./... (green, 18 packages), and ran two hand mutations.

The reorder is correct. I enumerated the behaviour delta by hand and it is exactly two cases: stuckPending && heldByJob plus a measured image-pull/PVC Fail goes Warn(0) → Fail(2) — the bug — and stuckPending alone plus a measured Fail stays exit 2 but swaps the generic "not enough free compute" headline (which sends the operator to resources set max, wrong for this state) for the measured cause. Every arm the two Fails now jump over is either the same exit code or the inference the measured-beats-inferred rule already subordinates. Nothing that produced a Fail now produces a Warn or an Unknown.

Both new tests are non-vacuous. Restoring internal/cli/doctor.go from the merge-base against the new tests reddens both, with the mutation demonstrably applied — got warn ("…the next one is waiting for it to finish.") and got "Not ready — part of your secure environment can't start yet.". computeRemedy really does contain the resources set max needle, so that assertion can fail too. The waiting fixture builds its Detail from doctor.HeldByRunningJob rather than a retyped string, and adding Image pull secret to the fixture (with the comment explaining that allOK omits it and with only mutates existing entries) is a genuine correction, not padding.

One ask, then I'm happy to approve.

The precondition doesn't pin which Warn — and I have the mutation

internal/cli/doctor_test.go:598

if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn {

This is the only assertion that the waiting fixture reaches the Wait-Warn arm at doctor.go:514. But there is a second Warn for the same signal — the plain heldByJob arm at doctor.go:589 — and the image-pull/PVC Fails were already above that one before this PR. So if the fixture ever stops satisfying stuckPending (it hinges on the Pod health Detail not being prefixed could not list pods, doctor.go:452-453), the state falls through to :589, the precondition still sees a Warn, and the whole test passes against the unfixed code.

That is not hypothetical. Changing only the fixture's Pod-health detail to "could not list pods: forbidden" and running against the pre-fix doctor.go:

--- PASS: .../wait-for-capacity_Warn/image_pull_secret_Fail
--- PASS: .../wait-for-capacity_Warn/dataset_volume_Fail

Green, on exactly the code the test exists to refuse.

Two lines close it, and I verified the tightened version reddens under the same drift with a message that says why:

if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn ||
	!strings.Contains(r.text, "the next one is waiting for it to finish") {
	t.Fatalf("precondition: the wait-for-capacity state should be the Wait-Warn, got %v (%q)", r.status, r.text)
}

precondition: the wait-for-capacity state should be the Wait-Warn, got warn
  ("Ready to run training — but a job is already using this machine's free compute, so the next run waits for it.")

Notes, not asks

Three inline comments on the producer side. In short: the arms are promoted on a "MEASURED" premise that checkImagePull (internal/doctor/doctor.go:1055) and checkPVC (internal/cluster/pvc.go:100) don't quite honour — both map any read error (Forbidden, timeout) to StatusFail, and checkImagePull hardcodes the Detail as not found while doing it. This switch is otherwise careful to separate can't-check from measured (stuckPending excludes could not list pods; there's a whole Unknown tier built on that). Pre-existing, and correctly out of scope here — but this PR is what makes it reachable in the healthy-running-training state, so it's worth a producer-side follow-up in the shape of backend#3247.

The class check itself holds; I'd only reword it, since the stuckPending Fail at :547 is also below the Wait-Warn (it's the inference the Warn refutes, so excluding it is right — the sentence just wants "measured Fail arms"). The Machine capacity Warn at :563 is likewise below and not mutually exclusive with the Wait-Warn, which inverts the rule its twin at :589 documents for itself — but both are exit 0 and it predates this PR.

[LukasWodka]

Heads-up, no action needed from me: this now conflicts. #642 merged at 09:12Z (66b73d0) and touches the same four files — internal/cli/doctor.go, internal/doctor/doctor.go and both test files — so GitHub reports CONFLICTING. It needs a rebase onto develop before it can go anywhere.

The good news is the two compose cleanly on substance. After a rebase the switch reads:

Pod health Fail
Node capacity Fail + OverCommitted
image pull secret Fail      ← this PR
dataset volume (PVC) Fail   ← this PR
stuckPending && heldByJob   (the wait-for-capacity Warn)
Node capacity Fail + StuckJobPod   ← #642
stuckPending
heldByJob

which is coherent in the direction you'd want: #642's arm says the pod is wedged "usually a training image that can't be pulled", and your measured pull-secret Fail sits above it as the root cause of which that is the symptom. No state is orphaned and nothing new is unhandled.

My earlier comment still stands unchanged — the two-line precondition tightening at doctor_test.go:598. Worth folding into the rebase rather than a separate push.

One thing to watch while you rebase, because it bears on your file: #642 added a continue in checkNodeFit that drops an assigned-but-Pending job pod from both the job sum and the request sum. The scheduler reserves a pod's requests as soon as NodeName is set, so that capacity is now counted by nobody, and a node with 4Gi genuinely free reports as able to schedule an 8Gi run. I've raised it separately — flagging it here only because it changes what heldByJob and the Node-capacity verdict mean underneath your reordering, and you'll be reading that code anyway.

[aptracebloc]

bugbot run

[cursor]

Stale Bugbot comment from a previous run.

[LukasWodka]

Re-reviewed at f687edf58d36e65d8e9dba3afdde4ab0ca2c2b02 (unchanged across my read), in a detached worktree. go build ./..., gofmt -s -l, go vet ./... clean; go test ./... green, 18 packages.

All three producer notes are fixed, and one of them better than I proposed. checkImagePull now splits on !apierrors.IsNotFound(err) and returns a can't-check StatusWarn carrying CantReadImagePullSecret, with the false "not found" detail gone from the read path — and the producer test asserts that absence explicitly. On the PVC side you were right to reject my cheap fix: excluding the detail in the CLI arm alone would have turned a read failure into a false green in the non-co-occurrence case, and the producer split plus cluster.PVCReadErrPrefix is the correct shape. On thread 3, the transitivity argument holds — the Wait-Warn must sit above the stuck-Pending Fail and the over-commit Warn must stay below it, so the lift really is unsound without a dedicated arm. The recorded-gap pin is the right call and I'm not asking for the arm.

The reclassification does not weaken the fix, which was my main worry going in, so I measured it rather than reasoning about it. Against a real fake clientset at this head:

image-pull: status=fail detail="image pull secret \"reg\" not found"
pvc:        status=fail detail="no PersistentVolumeClaim named \"client-pvc\" found in namespace \"tracebloc\"…"

A genuine absence produces a real NotFound, so the StatusFail path still runs; a genuinely missing or unbound PVC never carries PVCReadErrPrefix. Both still reach the promoted arms and exit 2 above the Wait-Warn. It is a partition, not a widening.

Both new branches are mutation-proof, anchors confirmed applied:

mutation	result
if !apierrors.IsNotFound(err) → if true	--- FAIL: TestCheckImagePull/missing_secret_->_fail: => warn ("could not read image pull secret \"reg\": …"), want fail
if strings.HasPrefix(err.Error(), cluster.PVCReadErrPrefix) → if true	--- FAIL: TestCheckPVC: missing PVC => warn, want fail

Composed arm order after the #642 merge is what we sketched, with nothing orphaned and arms 5/6 mutually exclusive by construction (heldByJob needs Node capacity == Warn, StuckJobPod needs == Fail). The measured pull-secret Fail sitting above #642's "stuck starting (usually an image that can't be pulled)" reads as root cause above symptom.

CI is fully green — 30 pass, nothing pending, and both Bugbot contexts came back clean after your bugbot run.

Still open: the precondition, unchanged

internal/cli/doctor_test.go:598 is byte-identical to what I flagged, and there's no reply on it — so I want to check it wasn't simply lost in the rebase rather than deliberately skipped.

I re-ran the mutation at this head to confirm it still bites. Undoing just the reorder reddens both flagship subtests properly (got warn ("…the next one is waiting for it to finish.")). But adding the fixture drift on top — Pod health detail → "could not list pods: forbidden" — gives:

--- PASS: TestSummarizeDoctor/a_measured_Fail_beside_a_running_job_outranks_the_wait-for-capacity_Warn

Green, on the unfixed code, because the state falls through to the bare heldByJob arm and the precondition still sees a Warn.

One thing I got more precise this round, and it does soften the severity I first gave this. Under that combined mutation the package is still red — but via a measured Fail with a Pending pod but NO running job…, which pins the weaker claim (the headline swap above stuckPending, both exit 2). So the reorder is not unpinned. What is unpinned is the exit-0 → exit-2 claim above the Wait-Warn, i.e. the thing this PR is. A maintainer chasing that other failure could re-green the suite with the actual fix undone.

Same two lines as before:

if _, r := summarizeDoctor(waiting, tokenOK); r.status != doctor.StatusWarn ||
	!strings.Contains(r.text, "the next one is waiting for it to finish") {
	t.Fatalf("precondition: the wait-for-capacity state should be the Wait-Warn, got %v (%q)", r.status, r.text)
}

Push that and I'll approve immediately — nothing else is holding this.

Nits, no action needed

• checkImagePull's other can't-check Warn — "couldn't read jobs-manager to resolve image pull secrets — skipping" — carries no prefix, so it matches neither the Fail arm nor the new Unknown arm and falls to default → a full green ✔. Pre-existing and not a regression, but it is the same producer and the same can't-check-vs-measured class this commit is auditing, so worth a follow-up rather than leaving the split half-applied.
• zz-all-strings.golden drops "reading PVC %s/%s: %w" for the opaque "%s%s/%s: %w". literalString won't fold a const *ast.Ident (deliberately — "Absent is honest; half is not"), so that wording is now inventoried nowhere, and the producer test matches the const against itself. Cosmetic; the golden is still internally consistent.

[saqlainsyed007]

Reviewed at f687edf. The precedence reorder is sound — status zero-value is StatusOK so missing map keys can't false-fire, the new can't-read Warn arms are reachable, and the PVCReadErrPrefix / CantReadImagePullSecret classification matches the producers. The golden update is complete, and the only other DiscoverSharedPVC caller (clustertarget.go) surfaces the error as-is, so the format-string change is behavior-neutral.

Three inline findings to triage before merge:

• doctor.go checkImagePull: the secret-Get read error is now a can't-check, but the pre-existing dep==nil path ("couldn't read jobs-manager … skipping", also an RBAC/List failure via findDeployment) still has no matching summarizeDoctor arm and rolls up to default green — the same false-green class, half-closed.
• cli/doctor.go: lifting the image-pull/PVC Fails also moves them above the #3247 StuckJobPod Fail and generic Node-capacity Fail; in a co-occurring missing-secret → ImagePullBackOff case the operator now sees the generic "Email support" line instead of the pod-naming #3247 remedy (message-only, both exit 2). (You already acknowledged this on an earlier thread.)
• Fourth/fifth near-identical "Warn-with-prefix → Unknown tier" arm; a single CantCheck marker on doctor.Result would collapse N prefix-matched arms into one rule.

Note: no Go toolchain in the review environment, so I did not execute go test — the PR's own CI (Test, golangci-lint, govulncheck) is green.

[cursor]

Stale Bugbot comment from a previous run.

[aptracebloc]

Pushed f7f12c7 addressing both reviews on this head:

• @LukasWodka — the precondition tightening you flagged at doctor_test.go:598 is in: the co-occurrence test now asserts the Wait-Warn's own top line ("…the next one is waiting for it to finish"), not merely StatusWarn, so a fall-through to the bare heldByJob arm can't green it if the reorder were undone. Also confirmed the two nits: the golden's opaque "%s%s/%s: %w" is cosmetic (the const-ident fold is deliberately not harvested), and the jobs-manager can't-read nit is now fixed (below).
• @saqlainsyed007 — the jobs-manager-unreadable false green is closed (that dep == nil Warn now carries CantReadImagePullSecret, so the rollup drops it to the Unknown tier). The altitude point (a structural CantCheck marker vs. N prefix-matched arms) is tracked as a separate behavior-preserving follow-up.

All threads resolved; go build / vet / gofmt / golangci-lint clean, full go test ./... green, golden regenerated + reviewed.

[aptracebloc]

bugbot run

[LukasWodka]

Re-reviewed at f7f12c7d577c7063b36b9b7d455802a2b5c6fccb. Both of my asks are fixed, and the second one is fixed better than I asked for.

The precondition now pins the arm, not just the status. doctor_test.go:598 asserts the Wait-Warn's own top line alongside StatusWarn, which defeats the mutation I demonstrated: under fixture drift the state falls to the bare heldByJob arm, whose text is "…so the next run waits for it" and does not contain "the next one is waiting for it to finish" — so the precondition now fails loudly instead of passing on the unfixed code. The comment recording why status-only was insufficient is the part that keeps it from being re-loosened by someone who doesn't know about the second Warn arm.

The jobs-manager can't-check is closed as a class, not an instance. I raised that one as a "no action needed" nit — the findDeployment == nil Warn carried no prefix, so it matched neither the Fail arm nor the new Unknown arm and fell through to default → a full green ✔. Giving it the same CantReadImagePullSecret prefix routes it to the Unknown tier with its sibling. That was the right call and I'd have accepted a deferral, so thanks for taking it now.

Not approving yet, on two gate items — neither of them a criticism of the change:

1. Cursor Bugbot is still pending at this sha.
2. One open Bugbot thread, and I checked it rather than just relaying it — it's correct. internal/cli/doctor.go:664 reads "couldn't check the image pull secret", while its PVC twin at :672 says "couldn't check dataset storage" and the measured Fail at :508 says "the training images can't be pulled". So the new line is the only one of the three in Kubernetes vocabulary, in the one function whose whole job is plain terms. Something like "couldn't check the training image credentials" keeps the trio consistent.

Fix that and let Bugbot go green and I'll approve — there is nothing else outstanding from me.

[cursor]

Stale Bugbot comment from a previous run.

[saqlainsyed007]

Re-review at f7f12c7. All three of my prior findings are addressed or soundly deferred: the jobs-manager-unreadable false-green is now closed via the shared CantReadImagePullSecret prefix + matching Unknown-tier arm (with a producer test), the measured-Fail-over-StuckJobPod ordering is consistent with the measured-beats-inferred rule, and the CantCheck-marker refactor is tracked as its own follow-up. Thanks for the thorough rework.

Not approving yet on one open item: the Cursor Bugbot thread on the image-pull can't-check line is correct — "image pull secret" is Kubernetes vocabulary in a rolled-up line this file itself documents as jargon-free (the twin PVC arm rightly says "dataset storage"). Reword to plain terms (e.g. "…couldn't check the training images…") or reply declining, and I'll approve. (closing-ref is the known org-wide infra defect, neutral.)

[aptracebloc]

bugbot run

[cursor]

✅ 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 e4a446d. Configure here.}

[saqlainsyed007]

Approve at e4a446d. The Bugbot copy point is fixed: the image-pull can't-check rollup line now reads "…couldn't check whether training images can be pulled (run with --verbose)" — the "image pull secret" jargon is gone, matched to the Fail arm's "training images can't be pulled" wording, and the plain-terms invariant holds across all the rolled-up arms (the twin dataset-storage arm stays consistent). Verified against the code, not just the thread state.

All three of my earlier findings are settled too: the jobs-manager-unreadable false-green is closed via the shared CantReadImagePullSecret prefix + Unknown-tier arm (with a producer test), the measured-Fail-over-StuckJobPod ordering is sound under the measured-beats-inferred rule, and the CantCheck-marker dedup is tracked as a follow-up. CI fully green (closing-ref included), MERGEABLE, no open threads. LGTM.

[LukasWodka]

Approving at e4a446db9e0b4f9da54075b23f3eeaf1f6743b01.

The wording is right, and it's the right kind of right — "couldn't check whether training images can be pulled" now mirrors the Fail arm's "the training images can't be pulled", so the two arms describe the same thing at two confidence levels instead of in two vocabularies. Jargon stays one --verbose away in renderDoctorDetails, which is the invariant summarizeDoctor documents three times.

Everything I asked for across the rounds is closed, and I verified each against the code rather than the commit messages:

• The precondition pins the arm, not just the status. doctor_test.go:598 asserts the Wait-Warn's own top line, which defeats the mutation I demonstrated — under fixture drift the state falls to the bare heldByJob arm, whose text differs, so it now fails loudly instead of passing on the unfixed code. The comment recording why status-only was insufficient is what stops it being re-loosened by someone who doesn't know there are two Warn arms.
• The producers separate can't-read from measured. checkImagePull splits on !apierrors.IsNotFound(err); checkPVC classifies on cluster.PVCReadErrPrefix. You were right to reject my cheaper fix on the PVC side — excluding the detail in the CLI arm alone would have produced a false green off the non-co-occurrence path.
• The reclassification doesn't weaken the fix. I measured that rather than reasoned about it: against a fake clientset, a genuinely absent secret still yields NotFound → StatusFail, and a genuinely missing PVC never carries the read prefix. Both still reach the promoted arms and exit 2. A partition, not a widening.
• The jobs-manager can't-check — which I raised as a no-action nit — got the same prefix so it routes to the Unknown tier instead of falling through to a green ✔. Class, not instance.

Composed arm order after the #642 merge is coherent: the measured pull-secret Fail sits above #642's "stuck starting (usually an image that can't be pulled)", which reads as root cause above symptom, and arms 5/6 are mutually exclusive by construction.

Good PR. The follow-up commit was the better half of it — catching that promoting an arm above a Warn only works if the arm is really a measurement, and fixing that at the producer instead of papering over it in the rollup.