docs(chart): record that Recreate + --atomic rolls back a degraded jobs-manager (backend#1922)

[saqlainsyed007]

What

Comment-only change to client/templates/jobs-manager-deployment.yaml. Records the Option B decision from backend#1922: the jobs-manager readiness design accepts that a degraded pod is rolled back during a rollout, rather than changing runtime behaviour to keep it serving.

Why

backend#1922 (Bugbot finding, staging hop round 4) traced a real cross-repo mechanism: when the ingestion HTTP server fails to bind 8080 at startup, jobs-manager deliberately keeps running (SB polling + training continue), but readinessProbe: tcpSocket: 8080 then never passes. With strategy: Recreate + the auto-upgrade CronJob's helm upgrade --atomic, the old pod is already gone and the never-Ready new pod makes --wait time out → --atomic rolls back to the last good release.

The existing comment argued the opposite — "Nothing kills it" — and also justified preferring tcpSocket over /healthz on the grounds that "/healthz is an unconditional 200". Both statements are now misleading:

• Recreate + --atomicdoes effectively end the degraded pod during a rollout (revert to prior revision, outage for the --timeout window).
• client-runtime#327 made /healthz a real readiness report (authz_mount + schema + metadata_db, 503 when any is broken), so the "unconditional 200" rationale no longer holds.

This PR corrects both and adds a ROLLBACK ON THE DEGRADED PATH note recording that backend#1922 weighed making the degraded state serve (bind 8080 unconditionally + 503, or wire readiness to /healthz) and chose to accept the rollback as the safer default for an unattended edge. No probe or strategy change.

Scope / decision

This is the documentation half of the decision. If we ever revisit the trade, it becomes a jobs_manager.py bind-behaviour change (client-runtime) or an httpGet /healthz switch — deliberately not a chart tweak, which is why nothing operational changes here. Related: the deferred second half of backend#1779.

Verification

• helm template ... --show-only templates/jobs-manager-deployment.yaml renders unchanged (comments only; probe/strategy identical).

Draft pending the decision being confirmed on backend#1922.

---

Note

Low Risk
Comments and a chart version bump only; rendered probes and strategy are identical.

Overview
Comment-only update on the jobs-manager readiness probe, plus a chart bump to 1.9.59. Probe settings and rollout strategy are unchanged.

Corrects two stale claims: /healthz is no longer an unconditional 200 (client-runtime#327), and a failed readiness check does kill the new pod during a rollout because Recreate plus auto-upgrade helm upgrade --atomic times out and rolls back.

Adds a ROLLBACK ON THE DEGRADED PATH note (backend#1922): keep tcpSocket on 8080 and accept rollback rather than serving a half-broken image. Revisiting that is a runtime bind change or an httpGet /healthz switch, not a chart tweak.

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

[LukasWodka]

Approving. This is the dependency for client-runtime#359, and it adds exactly the anchor that PR quotes — I checked the pairing rather than assuming it: the heading ROLLBACK ON THE DEGRADED PATH appears once, in client/templates/jobs-manager-deployment.yaml, which is the precise file and heading #359's comment names. So merging this first makes that cross-reference resolve on arrival.

The part I think is the most valuable and would be easy to skip past: this corrects a comment that had decayed into teaching the wrong thing.

/healthz is NOT the unconditional 200 an earlier version of this comment described — client-runtime#327 made it a real readiness report

A comment asserting /healthz carries no information beyond a TCP connect is exactly the kind of stale claim that gets acted on later — someone reads it and rules out an httpGet probe on false grounds. Replacing it with what /healthz actually reports (authz_mount + schema + metadata_db, 503 when broken) and naming the switch as a deferred, deliberate call under backend#1779 turns a wrong statement into a decision record. Worth more than the rollback note itself.

The rollback note lands the distinction cleanly too: readiness-only keeps a degraded pod alive outside a rollout, and the rollout case is different because Recreate deletes the old pod before the new one starts. I verified both halves against this file on develop — strategy: Recreate at :27-28 and readinessProbe: tcpSocket: 8080 at :220-222 — so the mechanism as described is the mechanism as configured. And the note is honest that accepting the rollback costs an outage for the --timeout window rather than presenting it as free.

Also good: the initialDelaySeconds comment previously said "Nothing kills it. That is the whole point" — true outside a rollout, false during one. That's now scoped to OUTSIDE a rollout with the rollout consequence called out. Same class of fix as the /healthz one.

On the Chart.yaml bump to 1.9.52: I checked before reading anything into it, since hand-bumping a version file is normally the release train's job and not an author's. chart-version-guard.yml — "chart content ⇒ Chart.yaml version bump" — requires it whenever chart content changes, and it passes here. So the bump is the gate being satisfied, not a hand-cut version. Correct as-is.

Gate: 21 checks pass (Bugbot included), 3 skipping, MERGEABLE, zero unresolved threads.

Merge-order note: this should land before client-runtime#359, so #359's quoted heading resolves. I've left #359 unapproved pending that; it needs no further change of its own.

[LukasWodka]

Re-approving — my earlier approval was auto-dismissed by the new commits, not withdrawn.

Re-read the full diff rather than assuming the push was inert: all three hunks are byte-identical to what I approved (the /healthz correction, the initialDelaySeconds rollout caveat, and the ROLLBACK ON THE DEGRADED PATH note). The only difference is Chart.yaml now reads 1.9.58 → 1.9.59 instead of 1.9.51 → 1.9.52, i.e. develop moved and the bump was re-cut on top — still chart-version-guard being satisfied, not a hand-cut version.

Everything from my first review stands, including the merge-order note: this should land before client-runtime#359, whose comment quotes the ROLLBACK ON THE DEGRADED PATH heading this PR adds.

[saadqbal]

The best thing in here is that it corrects a false claim in the comment it's extending, rather than only adding to it. Verified: /healthz is no longer the unconditional 200 the old comment described — submit_ingestion_run.py:_healthz_report() checks authz_mount, schema and metadata_db and returns 503 when any is broken. The docstring there makes the same argument from the other side ("a mechanism whose answer is not connected to the thing it names… making it mean something is therefore the prerequisite for wiring it, not a follow-up to it"), so the two now agree instead of one silently contradicting the other.

The initialDelaySeconds correction is the one that would actually have misled someone. "Nothing kills it. That is the whole point" was true outside a rollout and false during one, and the distinction is the entire subject of backend#1922. Splitting it into outside-a-rollout vs during-one is right, and recording the accepted trade — --atomic self-heals to a revision known to work, better than shipping a half-broken image to an unattended edge — with the explicit note that revisiting it is a jobs_manager.py change and not a chart tweak, is the part that will still be useful in six months.

Verified the mechanism it describes end to end: strategy: Recreate at :27, readinessProbe tcpSocket 8080 at :220, --atomic at auto-upgrade-cronjob.yaml:121 (with its own note that it implies --wait). All three are where the comment says they are.

One path into this failure mode that isn't written down anywhere

While checking "the ingestion server failed to bind 8080", I went looking for the other ways 8080 ends up unbound. There are two, and you've handled one of them deliberately:

Handled:INGESTION_HTTP_DISABLED gates the whole probe block at :199 with truthiness matched to the runtime's not os.getenv(...), and jobs_manager_test.yaml:782–795 tests both "1" and the empty string. That's exactly right and I'm not asking for anything there.

Not handled — INGESTION_HTTP_PORT:

• the runtime binds int(os.getenv("INGESTION_HTTP_PORT", "8080")) (jobs_manager.py, in main())
• jobs-manager-deployment.yaml:526 is a generic {{- range $key, $value := .Values.env }} passthrough, so any key an operator puts in .Values.env reaches the container
• git grep INGESTION_HTTP_PORT across the chart and values.yaml returns nothing — both containerPort (:197) and the probe (:222) hardcode 8080

So --set env.INGESTION_HTTP_PORT=9090 is a supported values path that binds 9090, leaves the probe polling 8080, and produces a pod that can never become Ready — which under Recreate + --atomic is precisely the rollback this PR is documenting, on an input where the server started perfectly. Worse than the exception path, because there's no error in the log to point at: the server logs a successful start and the release rolls back anyway.

The fix is the pattern already in the file — derive rather than restate:

{{- $ingestPort := (default dict .Values.env).INGESTION_HTTP_PORT | default 8080 }}

then use $ingestPort for both containerPort and tcpSocket.port, with a unit test alongside the two existing INGESTION_HTTP_DISABLED cases.

Not blocking this PR — everything it says is true, and it's comment-only. But it's the same class as the gate you already wrote, and I couldn't find a ticket for it (#1779 and #1922 are adjacent; neither covers the port). Worth one in backend before it's found the hard way on a customer edge.

Approving.

[saadqbal]

Heads-up on the version, no action needed on the review: develop is now at 1.9.59 (client#784 landed it) and client#787 is taking 1.9.60, so this PR's 1.9.58 → 1.9.59 bump is now a certain conflict on Chart.yaml. It'll want 1.9.61 after a git merge origin/develop.

My approval stands — this is purely the chart-version race I flagged on #784, not anything about the change.