fix(doctor): stop requests-proxy readiness false-green — honest neutral until the egress probe (cli#351)

internal/doctor/doctor.go

@@ -1,3 +1,3 @@
 // Package doctor implements the checks behind `tracebloc cluster doctor`:
 // a read-only, best-effort health sweep of a running tracebloc client
 // cluster. Each check reports ✔/⚠/✖ plus a one-line remedy, so a customer
@@ -42,12 +42,20 @@
 	StatusFail
 )
 
-// StatusUnknown marks a check that could not run because a prerequisite was
-// unavailable — today, the cluster API being unreachable. It carries NO signal:
-// the verdict rollup ignores it, so it never affects the overall result or exit
-// code. It exists so a single root cause (a stopped cluster) renders as one
-// honest ✖ plus neutral "couldn't check" lines, instead of every check inventing
-// a false cause.
+// StatusUnknown marks a check with no trustworthy signal either way. Two cases
+// produce it:
+//
+// 1. The check could not run because a prerequisite was unavailable — today,
+// the cluster API being unreachable. A single root cause (a stopped cluster)
+// then renders as one honest ✖ plus neutral "couldn't check" lines, instead
+// of every downstream check inventing a false cause.
+// 2. The check ran fine, but deliberately declines to assert a green it cannot
+// back — e.g. requests-proxy is present and Ready, yet readiness does not
+// prove Service Bus egress actually works and there is no probe for it yet
+// (backend#1143). A neutral line is more honest than a false ✔.
+//
+// Either way it carries NO signal: the verdict rollup ignores it, so it never
+// affects the overall result or exit code.
 const StatusUnknown Status = -1
 
 func (s Status) String() string {
@@ -519,7 +527,9 @@
 // weights egress MID-RUN; it does not block scheduling (experiments do not
 // "stay Pending" for this). Ready here is the Deployment's ReadyReplicas, which
 // (absent a readiness probe — backend#1143) only means the container started,
-// not that Service Bus egress actually works; the OK detail says so plainly.
+// not that Service Bus egress actually works. So a Ready relay returns a neutral
+// StatusUnknown ("running, egress not actively verified"), never a ✔ (cli#351);
+// the real ✔ arrives once the chart ships that probe and doctor consumes it.
 func checkRequestsProxy(ctx context.Context, cs kubernetes.Interface, ns string, release *cluster.ParentRelease) Result {
 	const name = "Service Bus egress (requests-proxy)"
 	dep := findDeployment(ctx, cs, ns, release, "requests-proxy")
@@ -539,7 +549,14 @@
  Remedy: "While requests-proxy is down, in-flight training can't relay epoch results/FLOPs to Service Bus — result egress stalls (scheduling is unaffected). kubectl describe deploy " + dep.Name + " -n " + ns,
  }
 	}
-	return Result{Name: name, Status: StatusOK, Detail: "requests-proxy Deployment Ready — relays result/FLOPs egress to Service Bus (deployment-ready only; egress not directly probed)"}
+	// Present and Ready — but readiness ≠ egress works (see the doc comment). Do
+	// not green this off readiness; report a neutral, honest "unknown" so doctor
+	// never claims Service Bus egress is verified when it hasn't been (cli#351).
+	return Result{
+ Name: name,
+ Status: StatusUnknown,
+ Detail: "requests-proxy is running, but egress to Service Bus is not actively verified — readiness only confirms the relay started, not that it can reach Service Bus",
+	}
 }
 
 // checkNodeFit verifies at least one Ready node can satisfy the resource

internal/doctor/doctor_test.go

@@ -425,7 +425,10 @@ func TestCheckRequestsProxy(t *testing.T) {
  dep *appsv1.Deployment // nil => deployment absent
  want Status
 	}{
- {"ready", requestsProxyDep("tb", 1), StatusOK},
+ // Ready is NOT a green ✔: readiness ≠ Service Bus egress works, so a running
+ // relay is an honest StatusUnknown (egress not actively verified), never OK
+ // (cli#351). A down / missing relay is still a real ✖.
+ {"ready", requestsProxyDep("tb", 1), StatusUnknown},
  {"not-ready", requestsProxyDep("tb", 0), StatusFail},
  {"missing", nil, StatusFail},
 	}
@@ -445,19 +448,23 @@ func TestCheckRequestsProxy(t *testing.T) {
 // When DiscoverParentRelease failed (release nil) but a release-prefixed
 // requests-proxy exists, the suffix fallback must still find it rather than
 // falsely report it missing (Bugbot on #89).
-// TestCheckRequestsProxy_Wording locks the cli#351 reword: requests-proxy is
-// the OUTBOUND result/FLOPs relay, so none of its lines may say experiments
-// "stay Pending" (that's the scheduling path, which it doesn't touch), and the
-// ✔ must be honest that egress is not actually probed.
+// TestCheckRequestsProxy_Wording locks the cli#351 reword + Part-2 downgrade:
+// requests-proxy is the OUTBOUND result/FLOPs relay, so none of its lines may
+// say experiments "stay Pending" (that's the scheduling path, which it doesn't
+// touch); and a Ready relay must read as a neutral StatusUnknown (egress not
+// actively verified), never a green ✔.
 func TestCheckRequestsProxy_Wording(t *testing.T) {
 	rel := &cluster.ParentRelease{ReleaseName: "tb"}
 
-ok := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 1)), ns, rel)
-	if strings.Contains(ok.Detail, "Pending") {
- t.Errorf("OK detail says %q — must not mention 'Pending' (that's scheduling, not egress)", ok.Detail)
+ready := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 1)), ns, rel)
+	if ready.Status != StatusUnknown {
+ t.Errorf("Ready relay = %v, want StatusUnknown — readiness must not green a ✔ egress claim (cli#351)", ready.Status)
 	}
-	if !strings.Contains(ok.Detail, "not directly probed") {
- t.Errorf("OK detail = %q, want it honest that egress is not directly probed", ok.Detail)
+	if strings.Contains(ready.Detail, "Pending") {
+ t.Errorf("detail says %q — must not mention 'Pending' (that's scheduling, not egress)", ready.Detail)
+	}
+	if !strings.Contains(ready.Detail, "not actively verified") {
+ t.Errorf("detail = %q, want it honest that egress is not actively verified", ready.Detail)
 	}
 
 	notReady := checkRequestsProxy(bg(), fake.NewClientset(requestsProxyDep("tb", 0)), ns, rel)
@@ -474,8 +481,10 @@ func TestCheckRequestsProxy_Wording(t *testing.T) {
 
 func TestCheckRequestsProxy_NilReleaseFindsPrefixed(t *testing.T) {
 	cs := fake.NewClientset(requestsProxyDep("tb", 1)) // "tb-requests-proxy"
-	if r := checkRequestsProxy(bg(), cs, ns, nil); r.Status != StatusOK {
- t.Fatalf("nil release with prefixed deploy => %v (%q), want ok", r.Status, r.Detail)
+	// Found (not falsely reported missing) now reads as StatusUnknown — running,
+	// egress not actively verified — rather than a green ✔ (cli#351).
+	if r := checkRequestsProxy(bg(), cs, ns, nil); r.Status != StatusUnknown {
+ t.Fatalf("nil release with prefixed deploy => %v (%q), want unknown (found, egress unverified)", r.Status, r.Detail)
 	}
 }
 
@@ -513,8 +522,9 @@ func TestCheckRequestsProxy_BareNameAcceptedWhenLabelledForRelease(t *testing.T)
 	bare.Name = "requests-proxy"
 	bare.Labels = map[string]string{"app.kubernetes.io/instance": "relA"}
 	cs := fake.NewClientset(bare)
-	if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status != StatusOK {
- t.Fatalf("bare requests-proxy labelled for relA => %v (%q), want ok", r.Status, r.Detail)
+	// Accepted (found, not missing) → StatusUnknown, not a green ✔ (cli#351).
+	if r := checkRequestsProxy(bg(), cs, ns, rel); r.Status != StatusUnknown {
+ t.Fatalf("bare requests-proxy labelled for relA => %v (%q), want unknown (accepted/found, egress unverified)", r.Status, r.Detail)
 	}
 }