Skip to content

fix(chart): give jobs-manager the readiness probe that would have caught the crash-loop (backend#1779) - #699

Merged
LukasWodka merged 2 commits into
developfrom
fix/1779-jobs-manager-probes
Aug 13, 2026
Merged

fix(chart): give jobs-manager the readiness probe that would have caught the crash-loop (backend#1779)#699
LukasWodka merged 2 commits into
developfrom
fix/1779-jobs-manager-probes

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Part of tracebloc/backend#1779.

The finding that changes the ticket

#1779 says the chart half must not ship first. Its argument:

the obvious fix — "add a readiness probe on /healthz" — would have been green
throughout the incident … Doing (2) before (1) is the trap and should be called
out in review if it shows up as a standalone PR.

I checked that, and for the incident it describes, it does not hold. Startup
order in jobs_manager.py::main() on origin/develop:

linewhat happens
3392auth = authenticate_user(backend_address, username, password)
3414 / 3425auth failed → logger.error(...); sys.exit(1)
~100 further lines, incl. another sys.exit(1) at 3511
3531run_server_in_thread(port=INGESTION_HTTP_PORT default 8080)the port opens here

The 2026-08-11 outage was a bad backend auth. The process therefore exited at
3414, ~140 lines before anything listened on 8080. Nothing was bound to the
port, so a probe against it — tcpSocketorGET /healthz — would have got
connection-refused and been red, not green. The ticket's own evidence says so:

14:26:03 ingest: connection refused to that pod's 8080, x16 tasks

Connection refused is a failing probe. So the false-green came entirely from
finding #1 (no probe at all — a container with no readiness probe is Ready as
soon as it is running), not from finding #2.

What remains true from #1779, and what I am demoting:

  • Finding Develop #1 is real and is what this PR fixes. Verified: jobs-manager-deployment.yaml contains zero probe keys, while mysql-deployment.yaml and requests-proxy-deployment.yaml each have all three. egress-proxy and resource-monitor-daemonset also have none.
  • /healthz really is an unconditional 200 (submit_ingestion_run.py:1697-1699 on origin/develop — the ticket cites :1681, the file has drifted 16 lines, the code is exactly as quoted).
  • ⚠️Demoted: "probing /healthz would create a new false-green." Not for this incident class. It is true for a narrower class — process listening, but MySQL / Service Bus / the backend token has since died — where /healthz would answer 200 regardless. Worth fixing, but it is not a prerequisite for this PR, and it is not what made the rollout lie.
  • ⚠️Demoted: the strict work ordering. (2) before (1) is not a trap here; (2) alone fixes the observed failure. I would keep (1) open as its own ticket for the narrower class.

Also worth noting: the precedent the ticket points at, #1143's requests-proxy probes, are tcpSocketnot HTTP /healthz. So "copy #1143's shape" and "probe /healthz" were never the same instruction.

Why a port probe is the right signal, not GET /healthz

Because of that same ordering, reaching port 8080 is itself the evidence:
jobs-manager only listens after backend auth has succeeded andcreate_app()
has run the ingestion_runs migration against MySQL. A TCP connect therefore
already implies "authenticated + MySQL reachable". An unconditional
200 {"status":"ok"} adds nothing on top of that.

What this PR does

  • A readinessProbe only, tcpSocket: 8080, on the api container, with initialDelaySeconds: 10 to cover the auth-retry + MySQL-migration start.

  • Gated on INGESTION_HTTP_DISABLED, matching the runtime's truthiness exactly. The runtime uses ingestion_enabled = not os.getenv("INGESTION_HTTP_DISABLED"), so any non-empty value disables the server — and a non-empty string is likewise truthy to Helm, so the two agree on "1", on "false", and on "". Without this gate an operator who disabled ingestion would get a pod that never becomes Ready. (The chart itself never sets the var; .Values.env is a free-form passthrough, so a user can.)

  • No livenessProbe and no startupProbe, deliberately, with the reason in the template. Both kill the container when they fail. jobs_manager.py:3535-3542 catches a failed run_server_in_thread on purpose: "Startup failure here is fatal for the ingestion flow but must not take down the rest of jobs-manager — SB polling can still operate independently and training submissions keep working." A liveness probe on 8080 would restart the pod in exactly that case, reversing that decision from the chart. Readiness pulls the pod out of the Service, which is the part that needs to happen; restarting a process that is still training models is not. #1779 asks for this to be "a recorded decision rather than an omission" — it now is, in the template and in a test.

    This is where Bugbot caught me (Medium, and correct — see the thread). The first revision of this PR did add a startupProbe with a 150s budget, having reasoned only about livenessProbe. But a failing startup probe also makes the kubelet kill the container, so it reintroduced the exact restart the PR argued against, via a different key — and since the retry fails identically it would CrashLoopBackOff a pod that was still training. Fixed in 9984e87 by removing it; readiness alone never restarts anything, and is all the false-green fix needs.

  • Chart version/appVersion 1.9.37 → 1.9.38 (chart-version-guard requires a bump for any client/** content change).

Out of scope, recorded in a test rather than silently skipped: pods-monitor listens on nothing, so it has no port to probe — giving it one needs a health surface first.

Tests — real output

make check (lint + drift + helm-lint + helm-vocab):

chart-env-vocabulary: all 28 checks passed
==> check: green (bats + helm unit tests are in 'make check-all')

scripts/gen-manifest.sh --check stays clean:

scripts/manifest.sha256 is up to date.

Full helm-unittest (all 30 suites, not just the one I touched):

Charts: 1 passed, 1 total
Test Suites: 30 passed, 30 total
Tests: 396 passed, 396 total

make bats — exit 0, 970 tests (last line ok 970 print_summary image_pull_ca: ...).

make helm-template — all four platforms render (aks, bm, eks, oc).

The jobs_manager_test.yaml suite went 35 → 41 tests.

Proof the tests are not inert

Six deliberate mutations of the template, each reverted after measuring. Each
one reddens a different assertion, and these were re-run after the Bugbot fix:

#MutationResult
1Reintroduce a startupProbe (the exact bug Bugbot found)1 failedshould NOT give the api container any probe that restarts it
2Add a livenessProbe on 80801 failed — same test
3Delete the readinessProbe block3 failed — incl. should give the api container a readiness probe on the ingestion port
4Point the readiness probe at port 99992 failed — proves the port is pinned, not merely the probe's presence
5Remove the INGESTION_HTTP_DISABLED gate1 failedshould drop the port probe when the ingestion server is disabled
6initialDelaySeconds 10 → 01 failedshould give the readiness probe room for a slow auth + migration start

Mutations 1 and 2 are the regression guard for the Bugbot finding: neither
restarting probe can come back silently. Mutation 4 is why the port and
thresholds are asserted by value — isNotEmpty alone would have survived it.

I could not prove the probe goes red against a live kubelet from here — that
needs a cluster. What is proven is that the probe exists, targets the port the
ingestion server actually binds, disappears exactly when that server is disabled,
and that every one of those properties fails if the template regresses.

Docs

No CLAUDE.md / BUGBOT.md / runbook statement is made false by this change.
Deliberately untouched (open PRs elsewhere): client/values.yaml,
docs/SECURITY.md, .github/workflows/build-k3s-cuda.yaml,
scripts/check-digest-drift.sh.

…ght the crash-loop (backend#1779)
jobs-manager had no probes at all, so the kubelet called the container Ready the
moment it was running. That is why `kubectl rollout status` printed "successfully
rolled out" over a pod with 4 restarts in 72 seconds on 2026-08-11, and why the
E2E agent's client-health step went green (backend#1723, backend#1756).
Adds startupProbe + readinessProbe (tcpSocket 8080) to the api container, gated
on INGESTION_HTTP_DISABLED with the same truthiness the runtime uses, so an
operator who disables the ingestion server does not get a pod that never becomes
Ready.
Deliberately NO livenessProbe, and the reason is recorded in the template:
jobs_manager.py catches a failed run_server_in_thread on purpose ("must not take
down the rest of jobs-manager — SB polling can still operate independently"), and
a liveness probe on 8080 would reverse that decision from the chart. Readiness
removes the pod from the Service, which is the part that needs to happen.
A port probe rather than GET /healthz, on purpose: jobs-manager opens 8080 only
after backend auth succeeds and the MySQL migration has run, so accepting a
connection already implies both. #1779 argues probing /healthz would be a new
false-green; for the incident it describes that is not so — the process exited
before opening the port, so any probe on 8080 would have been red. See the PR
for the evidence. /healthz being an unconditional 200 is still true, and still
worth fixing for the narrower "listening but a dependency died" case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursorcursorBot 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 ba602aa. Configure here.

Comment threadclient/templates/jobs-manager-deployment.yaml Outdated
Bugbot, Medium, and correct: the startupProbe reversed the very decision
this PR argued for when it omitted livenessProbe.
A startupProbe is not the gentle cousin of a liveness probe. The kubelet
KILLS the container when it fails, exactly as liveness does. So on the one
failure mode that motivated backend#1779 -- run_server_in_thread raising,
which jobs_manager.py:3535-3542 catches ON PURPOSE because "Startup
failure here is fatal for the ingestion flow but must not take down the
rest of jobs-manager -- SB polling can still operate independently and
training submissions keep working" -- the 30 x 5s budget would expire and
CrashLoopBackOff a pod that was still training models.
The PR wrote several paragraphs rejecting that outcome for livenessProbe
and then shipped it via a different key.
readinessProbe alone does the part that needed doing: take the pod out of
the Service so nothing routes POST /internal/submit-ingestion-run at a
dead port, and stop `kubectl rollout status` reporting success over a
crash-loop. It never restarts anything, so the runtime's decision to
survive a failed server start stays the runtime's to make.
Tests: the startupProbe values test is replaced by one asserting its
ABSENCE, with the reasoning above, so this cannot be reintroduced quietly.
The disabled-state test no longer asserts notExists on startupProbe --
it does not exist in either state, so that assertion would have passed
without saying anything about the gate.
396/396 helm-unittest across 30 suites, make check green,
gen-manifest --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Valid and correct — fixed in 9984e87. This one is worth spelling out, because the PR argued against the outcome and then shipped it through a different key.

A startupProbe is not the gentle cousin of a liveness probe. The kubelet kills the container when it fails, exactly as liveness does. So on the one failure mode that motivated backend#1779 — run_server_in_thread raising, which jobs_manager.py:3535-3542 catches on purpose:

"Startup failure here is fatal for the ingestion flow but must not take down the rest of jobs-manager — SB polling can still operate independently and training submissions keep working."

— the 30 × 5s budget would expire and CrashLoopBackOff a pod that was still training models. This PR wrote several paragraphs rejecting precisely that for livenessProbe, then reintroduced it as startupProbe.

readinessProbe alone does the part that needed doing: take the pod out of the Service so nothing routes POST /internal/submit-ingestion-run at a dead port, and stop kubectl rollout status reporting success over a crash-loop. It never restarts anything, so the runtime's decision to survive a failed server start stays the runtime's to make.

Two test changes so this cannot come back quietly:

  • the startupProbe values test is replaced by one asserting its absence, carrying the reasoning above;
  • the disabled-state test no longer asserts notExists: startupProbe — it does not exist in either state, so that assertion would have passed without saying anything about the gate. That is the vacuous-assertion shape this epic (backend#1729) exists to catch, so it is worth not leaving behind.

396/396 helm-unittest across 30 suites, make check green, gen-manifest.sh --check clean.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice — careful PR, and the incident analysis in the description actually changes the ticket's premise rather than just quoting it. Verified locally: readinessProbe renders on all four platforms (aks/bm/eks/oc) at tcpSocket 8080 matching the api container's containerPort, drops cleanly when INGESTION_HTTP_DISABLED is set and stays when it's empty, and the gate uses the proper (default dict .Values.env) guard. Readiness-only is the right call, and the two notExists tests are a good regression fence against a liveness/startup probe sneaking back in. helm-unittest 41/41 green. 👍

@LukasWodka
LukasWodka merged commit 06ed783 into developAug 13, 2026
22 checks passed
@LukasWodka
LukasWodka deleted the fix/1779-jobs-manager-probes branch August 13, 2026 09:30
Sign up for freeto 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

@LukasWodka@saadqbal