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

internal/cli/doctor.go

@@ -492,6 +492,25 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		ready = healthLine{doctor.StatusFail,
 			"Not ready — this machine is big enough, but the platform's own services have already claimed the room.",
 			fmt.Sprintf("Ask for less per training run, or give the machine more memory/CPU. Do NOT size runs to the machine here — that measures the machine's total, not what is free, so it would ask for MORE and leave the training stuck. `%s doctor --verbose` shows the exact numbers and the knob to turn.", launcher())}
+	// MEASURED training-blockers that can co-occur with a Pending pod held by a
+	// running job, so they are read BEFORE the Wait-Warn below (backend#3248,
+	// Bugbot Medium on #641). checkImagePull and checkPVC are
+	// INDEPENDENT of Pod health and Node capacity, so either can be Fail while
+	// `stuckPending && heldByJob` is also true — a pod Pending on an image it
+	// cannot pull, or beside a dataset volume that never bound, is a measured
+	// failure, not a wait. Below the Wait-Warn these Fails were shadowed: a node
+	// held by a job with any pod Pending past grace made `doctor` exit 0 (Warn)
+	// over an exit-2 failure. They sit above the stuck-Pending arm too — the same
+	// measured-beats-inferred rule that lets the Wait-Warn refute the Pending
+	// inference puts a measured cause ahead of it.
+	case by["Image pull secret"].Status == doctor.StatusFail:
+		ready = healthLine{doctor.StatusFail,
+			"Not ready — the training images can't be pulled.",
+			fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())}
+	case by["Dataset volume (PVC)"].Status == doctor.StatusFail:
+		ready = healthLine{doctor.StatusFail,
+			"Not ready — dataset storage isn't available.",
+			fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())}
 	case stuckPending && heldByJob:
 		// A PENDING POD WHOSE CAUSE HAS BEEN MEASURED (Bugbot High on #639,
 		// backend#2870). This is the transient shortage's own symptom: a job is
@@ -509,11 +528,17 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		// cause -- the envelope fits this machine and a running job holds it --
 		// which refutes the inference, and a Fail here would exit 2 on healthy
 		// training (the Bugbot High on #628) while recommending a resize that
-		// changes nothing. The two arms that still outrank this one are measured:
-		// a Pod-health FAIL (crash-loop) and the OverCommitted Fail.
+		// changes nothing. The arms that still outrank this one are all MEASURED
+		// failures — a Pod-health FAIL (crash-loop), the OverCommitted Fail, and
+		// the image-pull-secret and dataset-volume Fails just above — because a
+		// measured training-blocker must not be hidden behind a wait (backend#3248:
+		// those two Fails used to sit BELOW this arm, so a Pending pod beside a
+		// running job made `doctor` exit 0 over a real, exit-2 failure).
 		//
 		// checkPods does not know WHY a pod is Pending, so a pod stuck on an
-		// image pull beside a running job would land here too; the remedy names
+		// image pull beside a running job could still reach this arm when the
+		// image-pull-secret probe itself is healthy (the secret exists, but the
+		// pull is slow or the registry is briefly unreachable); the remedy names
 		// what to do if the wait outlives the job rather than pretending the
 		// attribution is certain.
 		ready = healthLine{doctor.StatusWarn,
@@ -536,9 +561,11 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		// grace window is left to the stuck-Pending arm below, whose "usually not
 		// enough free compute, or an image that can't be pulled" wording is an
 		// honest age-based inference (a large first pull on a cold node CAN exceed
-		// the grace). Only the measured capacity Fails (OverCommitted) and a hard
-		// Pod-health crash-loop Fail outrank it. Its remedy is the OPPOSITE of the
-		// transient Warn's (inspect the pod, do NOT wait); the pod it names is one
+		// the grace). The arms that outrank it are all measured: a Pod-health
+		// crash-loop Fail, the OverCommitted Fail, and the image-pull-secret and
+		// dataset-volume Fails above (backend#3248 — those two sit above the
+		// wait-for-capacity Warn, hence above this arm too). Its remedy is the
+		// OPPOSITE of the transient Warn's (inspect the pod, do NOT wait); the pod it names is one
 		// `--verbose` away -- and PLAIN TERMS, no Kubernetes vocabulary, like its
 		// neighbours (the granular checkNodeFit remedy carries the `kubectl` form).
 		ready = healthLine{doctor.StatusFail,
@@ -555,14 +582,6 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		ready = healthLine{doctor.StatusFail,
 			"Not ready — part of your secure environment can't start yet.",
 			fmt.Sprintf("Some pods are stuck starting — usually not enough free compute, or a training image that can't be pulled. %s Then re-run `%s doctor`; if it persists, email support@tracebloc.io with `%s doctor --diagnose`.", computeRemedy(runtime.GOOS), launcher(), launcher())}
-	case by["Image pull secret"].Status == doctor.StatusFail:
-		ready = healthLine{doctor.StatusFail,
-			"Not ready — the training images can't be pulled.",
-			fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())}
-	case by["Dataset volume (PVC)"].Status == doctor.StatusFail:
-		ready = healthLine{doctor.StatusFail,
-			"Not ready — dataset storage isn't available.",
-			fmt.Sprintf("Email support@tracebloc.io with the output of `%s doctor --diagnose`.", launcher())}
 	case by["Node capacity"].Status == doctor.StatusFail:
 		ready = healthLine{doctor.StatusFail,
 			"Not ready — not enough free compute to start a training.",
@@ -633,6 +652,28 @@ func summarizeDoctor(results []doctor.Result, tok tokenState) (connected, ready
 		// the OK default): training still runs via the jobs-manager's CPU fallback.
 		ready = healthLine{doctor.StatusUnknown,
 			"Ready to run training — couldn't check free compute (run with --verbose)", ""}
+	case by["Image pull secret"].Status == doctor.StatusWarn &&
+		strings.HasPrefix(by["Image pull secret"].Detail, doctor.CantReadImagePullSecret):
+		// checkImagePull can't-check: the secret (or the jobs-manager that names it)
+		// could not be READ, not read-and-found-missing. It carries no signal about
+		// whether images can be pulled, so it lands here in the Unknown tier — never
+		// the measured "images can't be pulled" Fail above, which is now promoted
+		// over the wait-for-capacity Warn and would flip a healthy environment to
+		// exit 2 on an RBAC blip (backend#3248, LukasWodka on #643).
+		//
+		// PLAIN TERMS, no Kubernetes vocabulary — "image pull secret" is jargon that
+		// belongs one --verbose away in renderDoctorDetails, so this line mirrors the
+		// Fail arm's "training images can't be pulled" wording (Bugbot on #643).
+		ready = healthLine{doctor.StatusUnknown,
+			"Ready to run training — couldn't check whether training images can be pulled (run with --verbose)", ""}
+	case by["Dataset volume (PVC)"].Status == doctor.StatusWarn &&
+		strings.HasPrefix(by["Dataset volume (PVC)"].Detail, cluster.PVCReadErrPrefix):
+		// checkPVC can't-check: the PVC could not be READ (Forbidden / network),
+		// not read-and-found-unbound. Same reasoning as the image-pull arm — a
+		// can't-read is no signal, so it stays in the Unknown tier rather than the
+		// measured "dataset storage isn't available" Fail above (backend#3248).
+		ready = healthLine{doctor.StatusUnknown,
+			"Ready to run training — couldn't check dataset storage (run with --verbose)", ""}
 	default:
 		ready = healthLine{doctor.StatusOK, "Ready to run training", ""}
 	}

internal/cli/doctor_test.go

@@ -572,6 +572,61 @@ func TestSummarizeDoctor(t *testing.T) {
 		}
 	})
 
+	// backend#3248 (Bugbot Medium on #641). The Wait-Warn arm above is an
+	// INFERENCE that a Pending pod beside a running job is only waiting for the
+	// room. checkImagePull and checkPVC are MEASURED and independent of
+	// Pod health and Node capacity, so either can be Fail in that exact state — a
+	// missing pull secret, or a dataset volume that never bound, beside a running
+	// job. The Wait-Warn used to sit ABOVE those Fail arms, so the measured failure
+	// was shadowed and `doctor` exited 0 (Warn) instead of 2 (Fail). Pin that a
+	// measured Fail wins the precedence, while the Wait-Warn still applies when
+	// there is no measured Fail.
+	t.Run("a measured Fail beside a running job outranks the wait-for-capacity Warn", func(t *testing.T) {
+		// The full waiting_for_capacity state: a running job holds the room AND a
+		// pod is Pending past grace — together the arm that returns the Wait-Warn.
+		// Detail built from the producer's constant, same discipline as the arm.
+		waiting := withDetail(allOK, "Node capacity", doctor.StatusWarn,
+			doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish")
+		waiting = withDetail(waiting, "Pod health", doctor.StatusWarn,
+			"Pending > 5m0s: [train-second]")
+		// allOK omits "Image pull secret" (a fixture shortcut; Run() does emit it),
+		// and `with` only mutates an entry that already exists — so add it OK here,
+		// or the image-pull case below would silently stay unset and never flip.
+		waiting = append(waiting, res("Image pull secret", doctor.StatusOK))
+
+		// Precondition: with no measured Fail, that state is the exit-0 Wait-Warn.
+		// Assert the Wait-Warn's OWN top line, not merely "a Warn" (LukasWodka on
+		// #643): the bare heldByJob arm below is also a Warn, so a status-only check
+		// would still pass if the reorder were undone and the state fell through to
+		// it — leaving the exit-0 → exit-2 claim this test exists for unpinned.
+		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)
+		}
+
+		// Each measured Fail, dropped into that same state, must win — top line and
+		// exit code both. Per-case subtests: map order is randomized, so a shared
+		// loop with t.Fatalf would report one nondeterministic case and hide the other.
+		measured := map[string]struct{ name, wantText string }{
+			"image pull secret Fail": {"Image pull secret", "images can't be pulled"},
+			"dataset volume Fail":    {"Dataset volume (PVC)", "dataset storage isn't available"},
+		}
+		for label, m := range measured {
+			t.Run(label, func(t *testing.T) {
+				c, r := summarizeDoctor(with(waiting, m.name, doctor.StatusFail), tokenOK)
+				if r.status != doctor.StatusFail {
+					t.Fatalf("a measured Fail beside a running job must win — the Wait-Warn shadowed it and doctor exited 0 over a real failure, got %v (%q)", r.status, r.text)
+				}
+				if !strings.Contains(r.text, m.wantText) {
+					t.Errorf("want the measured Fail's own top line %q, got %q", m.wantText, r.text)
+				}
+				if v := doctorVerdict(c.status, r.status); v != doctor.StatusFail {
+					t.Errorf("the verdict must own the Fail (exit 2), not the wait Warn (exit 0), got %v", v)
+				}
+			})
+		}
+	})
+
 	t.Run("a Pending pod with NO running job is still the stuck-Pending Fail", func(t *testing.T) {
 		// The other side: the arm above is scoped to the co-occurrence. A pod
 		// Pending on a machine where nothing holds the room is the generic,
@@ -583,6 +638,28 @@ func TestSummarizeDoctor(t *testing.T) {
 		}
 	})
 
+	t.Run("a measured Fail with a Pending pod but NO running job still outranks the stuck-Pending inference", func(t *testing.T) {
+		// backend#3248, the no-heldByJob half of the reorder. Moving the measured
+		// Fails above the Wait-Warn necessarily moves them above the plain
+		// stuck-Pending Fail too (the Wait-Warn sits above stuck-Pending,
+		// backend#2870). Both are exit-2, so the observable change is which top
+		// line and remedy the operator sees — the measured image-pull cause, not
+		// the generic "usually not enough free compute" guess. Pin it, matching
+		// this file's discipline of nailing every ordering a reshuffle could undo.
+		results := withDetail(allOK, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [trainer-x]")
+		results = append(results, res("Image pull secret", doctor.StatusFail))
+		_, r := summarizeDoctor(results, tokenOK)
+		if r.status != doctor.StatusFail {
+			t.Fatalf("a measured image-pull Fail must stay a Fail, got %v (%q)", r.status, r.text)
+		}
+		if !strings.Contains(r.text, "images can't be pulled") {
+			t.Errorf("the measured image-pull cause must win over the generic stuck-Pending guess, got %q", r.text)
+		}
+		if strings.Contains(r.remedy, "resources set max") {
+			t.Errorf("a measured image-pull Fail must not send the operator to resize compute: %q", r.remedy)
+		}
+	})
+
 	// backend#3247: the defect combination. A training pod is scheduled but
 	// wedged on an image pull (Node capacity Fail, StuckJobPod) and Pod health
 	// also sees it Pending past grace (stuckPending). This used to roll up through
@@ -639,6 +716,67 @@ func TestSummarizeDoctor(t *testing.T) {
 		}
 	})
 
+	// backend#3248 (LukasWodka on #643): checkImagePull / checkPVC now return a
+	// can't-check Warn (with a distinct prefix) when the secret / PVC could not be
+	// READ, distinct from a measured missing / unbound Fail. A can't-read carries
+	// no signal, so it must roll up to the Unknown tier — never the promoted
+	// measured Fail, which would flip a healthy environment to exit 2 on an RBAC blip.
+	t.Run("a can't-READ image-pull or PVC is an honest can't-check, not the promoted Fail", func(t *testing.T) {
+		imgCantRead := append(append([]doctor.Result{}, allOK...),
+			doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
+		if _, r := summarizeDoctor(imgCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "training images can be pulled") {
+			t.Errorf("a can't-read image-pull must roll up to a plain-terms can't-check, got %v (%q)", r.status, r.text)
+		}
+		pvcCantRead := withDetail(allOK, "Dataset volume (PVC)", doctor.StatusWarn,
+			cluster.PVCReadErrPrefix+"ns/client-pvc: is forbidden")
+		if _, r := summarizeDoctor(pvcCantRead, tokenOK); r.status != doctor.StatusUnknown || !strings.Contains(r.text, "dataset storage") {
+			t.Errorf("a can't-read PVC must roll up to a can't-check, got %v (%q)", r.status, r.text)
+		}
+	})
+
+	// The exact regression from thread 1: a running job holds the room, the next
+	// pod is Pending (the wait-for-capacity state), AND the pull secret could not
+	// be read. Because that read failure is now a can't-check (not a Fail), it no
+	// longer promotes over the Wait-Warn — the operator is told to wait, not handed
+	// a false "images can't be pulled" exit 2 on a healthy environment.
+	t.Run("a can't-READ secret beside a running job stays the wait Warn, not a false exit-2", func(t *testing.T) {
+		results := withDetail(allOK, "Node capacity", doctor.StatusWarn,
+			doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish")
+		results = withDetail(results, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [train-second]")
+		results = append(results, doctor.Result{Name: "Image pull secret", Status: doctor.StatusWarn, Detail: doctor.CantReadImagePullSecret + ` "reg": secrets is forbidden`})
+		c, r := summarizeDoctor(results, tokenOK)
+		if r.status != doctor.StatusWarn || !strings.Contains(r.text, "waiting for it") {
+			t.Fatalf("a can't-read secret must not flip the wait-for-capacity Warn to a Fail, got %v (%q)", r.status, r.text)
+		}
+		if v := doctorVerdict(c.status, r.status); v == doctor.StatusFail {
+			t.Errorf("a read blip must not make doctor exit 2 on a healthy environment, got verdict %v", v)
+		}
+	})
+
+	// backend#3248 thread 3 (LukasWodka on #643). When the machine over-commits AND
+	// a running job holds the room AND the next pod is Pending, both findings are
+	// Warns (exit 0). The plain-heldByJob case puts the over-commit Warn first
+	// ("a machine that lies about its size outranks a running job"), but the
+	// Pending variant cannot: the Wait-Warn sits above the stuck-Pending Fail
+	// (backend#2870) and the over-commit Warn must stay below that Fail (a Warn may
+	// not shadow a Fail), so by transitivity the Wait-Warn wins here. It is
+	// message-only (both exit 0) and pre-existing; lifting it would need a dedicated
+	// arm, not a reorder. Pin the current behavior so the gap is recorded, not implied.
+	t.Run("over-commit Warn is shadowed by the wait-for-capacity Warn in the Pending variant (known, message-only)", func(t *testing.T) {
+		results := withDetail(allOK, "Machine capacity", doctor.StatusWarn,
+			"Docker VM 7.75 GiB → 2 nodes claiming 15.50 GiB — Kubernetes believes 2.00× the memory this machine has")
+		results = withDetail(results, "Node capacity", doctor.StatusWarn,
+			doctor.HeldByRunningJob+": a Ready node fits a training job (cpu=1, memory=4864Mi) beside the platform's own pods, but running job(s) on n1 hold cpu=1, memory=4864Mi right now, so the next run waits Pending until they finish")
+		results = withDetail(results, "Pod health", doctor.StatusWarn, "Pending > 5m0s: [train-second]")
+		_, r := summarizeDoctor(results, tokenOK)
+		if r.status != doctor.StatusWarn {
+			t.Fatalf("both findings are Warns → exit 0, got %v (%q)", r.status, r.text)
+		}
+		if !strings.Contains(r.text, "waiting for it") {
+			t.Errorf("known message-only gap: the Pending variant shows the wait-for-capacity Warn, not the over-commit Warn — if a dedicated arm is added, update this pin, got %q", r.text)
+		}
+	})
+
 	t.Run("a crash-looping pod still outranks the running-job explanation", func(t *testing.T) {
 		// The exception is scoped to the stuck-Pending WARN; a Pod-health FAIL is
 		// a measured failure with a different fix, and must keep winning.