Skip to content

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

Merged
saadqbal merged 2 commits into
developfrom
fix/351-doctor-egress-probe
Jul 21, 2026
Merged

fix(doctor): stop requests-proxy readiness false-green — honest neutral until the egress probe (cli#351)#369
saadqbal merged 2 commits into
developfrom
fix/351-doctor-egress-probe

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What

Part 2 of #351. checkRequestsProxy (the "Service Bus egress (requests-proxy)" check) treated the requests-proxy Deployment's ReadyReplicas > 0 as proof that "Service Bus egress works" — a false green. Readiness is not the same as the proxy being able to relay egress to Service Bus. This PR removes that false green.

When the relay is present and Ready, the check now returns a neutral, honest StatusUnknown — rendered as a dim · line, not a green check — with the 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

The genuine, locally-detectable failure signals are unchanged and still fail:

  • requests-proxy Deployment missing
  • 0 ready replicas (relay down → result egress WILL stall mid-run)

Why the honest downgrade, not an active probe

An active in-cluster egress probe is not cleanly feasible today, so per the ticket's fallback this PR does the honest downgrade instead:

  • The requests-proxy exposes no Service-Bus-reachability health endpoint yet. That is the paired, not-yet-shipped client-chart ticket (backend#1143), which adds readiness/liveness probes to "enable the doctor egress check to mean something."
  • doctor runs off-cluster and is a read-only sweep. Reaching the in-cluster relay would need a port-forward/exec, i.e. new pods/portforward permissions the doctor does not hold — out of scope for this fix. And with no health endpoint to hit, it would only re-confirm "container listening" = readiness again.
  • Under a corporate proxy the relay's own Service Bus client can fail silently (backend#1142 Theme C), making readiness an even weaker signal there.

So the correct move now is to stop asserting a green we cannot back up. StatusUnknown carries no signal (Worst() ignores it), so a healthy environment's overall verdict stays "All checks passed" while this one line honestly reads neutral. The real green check arrives once backend#1143 ships the health probe and doctor consumes it (tracked in epic backend#1142).

This meets the acceptance bar: doctor no longer reports a green "egress works" purely from readiness.

Files

  • internal/doctor/doctor.gocheckRequestsProxy: Ready branch now returns StatusUnknown + honest detail; doc comment rewritten to explain the readiness-vs-egress gap.
  • internal/cli/doctor.godoctor --help blurb: "...is Ready" → "...is running (egress not directly verified)".
  • internal/doctor/doctor_test.go — tests updated to lock the downgrade (table case, wording test, nil-release + bare-name "found" cases), plus a healthy-cluster assertion that the egress line reads neutral end-to-end.

Test plan

make ci green end-to-end:

  • go build ./..., go vet ./... — clean
  • gofmt -s -l internal/ / goimports -local — no drift
  • go test -race -cover ./... — all 17 packages pass (doctor 93.0%)
  • make lint (errcheck, ineffassign, misspell, staticcheck) — clean
  • schema-check, vulncheck (no vulnerabilities), file-budget, deadcode — all pass

Refs #351 — fixes the readiness false-green here; the active egress probe (a real ✓) stays open, blocked on backend#1143.

🤖 Generated with Claude Code


Note

Low Risk
Behavior change is limited to doctor messaging and one check’s status when the proxy is healthy; failures for missing/down proxy are unchanged and unknown statuses do not affect exit codes.

Overview
Service Bus egress (requests-proxy) no longer shows a green check when the deployment is merely Ready. That path now returns StatusUnknown with copy that the relay is running but egress to Service Bus is not actively verified—readiness only means the container started, not that relay works (cli#351; real probe blocked on backend#1143).

StatusUnknown is documented for a second case: checks that ran but refuse to assert OK when they cannot back it up, not only when prerequisites (e.g. unreachable API) blocked the check. Verdict rollup still ignores unknown, so missing or not-ready requests-proxy stays fail and a healthy cluster can still roll up to all passed.

Tests lock the ready → unknown behavior, updated detail wording (not actively verified), and nil-release / bare-name “found” cases.

Reviewed by Cursor Bugbot for commit 002841a. Bugbot is set up for automated code reviews on this repo. Configure here.

@LukasWodka LukasWodka self-assigned this Jul 21, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 57b4632. Configure here.

Comment thread internal/doctor/doctor_test.go
@LukasWodka LukasWodka changed the title fix(doctor): real egress verification for requests-proxy, no readiness false-green (cli#351 Part 2) fix(doctor): stop requests-proxy readiness false-green — honest neutral until the egress probe (cli#351) Jul 21, 2026
@LukasWodka
LukasWodka force-pushed the fix/351-doctor-egress-probe branch from 57b4632 to be3b41d Compare July 21, 2026 10:45
Comment thread internal/doctor/doctor.go
@saadqbal

Copy link
Copy Markdown
Collaborator

One design call worth surfacing: the honest downgrade only shows up under --verbose. On a healthy cluster with a merely-Ready proxy, egress=StatusUnknown folds into connected=OK/ready=OK, so doctorVerdict still prints the fully-green "Everything looks good — you're ready to run training." The default headline a normal user sees is byte-identical to before — the neutral "· egress not actively verified" line lives only in the verbose details.

So we've killed the false green on the per-check line (that's the acceptance bar), but the default UX still reads as full confidence. Intentional per the PR description (StatusUnknown carries no signal by design), just flagging it as a conscious call — if the intent is that users stop seeing an unbacked green, the rolled-up view would need the note too.

LukasWodka added a commit that referenced this pull request Jul 21, 2026
…es-to-assert)

Asad's review on #369: the StatusUnknown doc only covered the 'prerequisite
unavailable' case, but the requests-proxy egress check now uses it for a
different reason — the check ran fine and just declines to assert an egress
green it can't back. Documents both meanings on the sentinel.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka and others added 2 commits July 21, 2026 13:21
Rebased onto develop after the doctor redesign (#365), which kept the
readiness-based StatusOK. checkRequestsProxy no longer greens off ReadyReplicas
alone — a present + Ready relay returns a neutral StatusUnknown ("running,
egress not actively verified"), never a checkmark; down / missing still fail.
worstStatus ignores StatusUnknown, so a healthy environment's overall verdict is
unchanged. The real green arrives once the requests-proxy health probe
(backend#1143) ships and doctor consumes it.

Refs #351

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…es-to-assert)

Asad's review on #369: the StatusUnknown doc only covered the 'prerequisite
unavailable' case, but the requests-proxy egress check now uses it for a
different reason — the check ran fine and just declines to assert an egress
green it can't back. Documents both meanings on the sentinel.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodka force-pushed the fix/351-doctor-egress-probe branch from 0d9e554 to 002841a Compare July 21, 2026 11:22
@LukasWodka

Copy link
Copy Markdown
Contributor Author

Agreed it's a conscious call — and for now I'm keeping the healthy-cluster headline green. Reasoning:

StatusUnknown is deliberately no-signal: an un-probed egress path isn't evidence egress is broken (WhoAmI already confirmed this machine reaches tracebloc, and the relay is Ready), so degrading the default "ready to run training" headline on every healthy run would cry wolf. The genuine green arrives when backend#1143 ships the requests-proxy egress probe and doctor consumes it — at that point the check flips from neutral to a real ✔ with signal behind it. Until then, the per-check neutral (the acceptance bar) + the now-extended StatusUnknown doc is the honest surface.

If we want the rolled-up view to carry an "egress not actively verified" note before #1143 lands, that's a small verdict-rollup tweak — I'd rather scope it with the doctor-redesign owner than bolt it onto this PR. Leaving that sub-call to Lukas.

@saadqbal
saadqbal merged commit 62b46bf into develop Jul 21, 2026
20 checks passed
@saadqbal
saadqbal deleted the fix/351-doctor-egress-probe branch July 21, 2026 11:30
@LukasWodka

Copy link
Copy Markdown
Contributor Author

/fr-pass — verified live on dev: with requests-proxy Ready, doctor --verbose renders the Service Bus egress line as a neutral dim · ("running, but egress to Service Bus is not actively verified") — no false green; overall verdict still healthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants