fix(chart): stop the single-replica worker readiness probe from 502ing all plugin traffic (BLO-31945) - #1673
fix(chart): stop the single-replica worker readiness probe from 502ing all plugin traffic (BLO-31945)#1673allyblockcast[bot] wants to merge 3 commits into
Conversation
…g all plugin traffic (BLO-31945) The worker StatefulSet runs replicas: 1 behind a readinessProbe with timeoutSeconds: 5. A readiness probe sheds traffic to healthy peers; at one replica there are none, so three slow /healthz responses delete the only Service endpoint and every API-to-worker call 502s with "Worker tier unreachable" from worker-tier-proxy.ts. Alertmanager webhook delivery fails with it, then retries 4x into the worker that just failed its probe. Measured on paperclip-0 over 24h to 2026-09-05T20:30Z (kubelet prober_probe_*), same endpoint and window: Readiness (timeout 5s): 100.2 failed / 8559 = 1.171% Liveness (timeout 10s): 13.1 failed / 2838 = 0.462% Readiness fails at 2.53x the liveness rate on the identical /healthz, and the per-probe timeout is the only material difference. p99.9 of successful liveness probes is 8.59s, so the 5s readiness timeout sat below the endpoint's own tail latency. Raise readiness timeoutSeconds 5 -> 10 (matching liveness, above the measured tail) and periodSeconds 10 -> 15 so a probe cannot outlast its period. Liveness and startup are unchanged. Worst-case endpoint removal goes 30s -> 45s, which costs nothing at replicas: 1 where removal has no upside. Mitigation, not a cure: the durable fix is a second replica (BLO-29307 / BLO-29004) or a /healthz that cannot be blocked by the event loop. The decision to keep rather than retire the probe is recorded in values.yaml. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
…ss at replicas 1 (BLO-31945) Asserts the invariant whose violation caused the outage: on a single-replica worker, readiness.timeoutSeconds must be >= liveness.timeoutSeconds, because readiness has no peer to shed to and dropping the only endpoint 502s all plugin traffic. Also asserts timeoutSeconds <= periodSeconds. Scoped to replicas === 1 so it does not block tightening readiness again once the worker runs two or more replicas (BLO-29307 / BLO-29004). Verified as a real guard, not a tautology: against master's values (timeout 5) the new test fails with the intended message; with the timeout raised to 10 all three tests in the file pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: aebca91
The core change is correct, well-evidenced, and correctly scoped. I verified the premise rather than taking it from the description:
templates/statefulset.yaml:9really isreplicas: 1(hardcoded, not a value), so the "no peer to shed to" argument holds.templates/statefulset.yaml:952renderstoYaml .Values.workerProbes.readinessverbatim, so the new keys land as written.values.blockcast.yamldoes not overrideworkerProbes, so this is not a no-op in the deployed environment. Itsreplicas: 2(line 375) is underapi:, a different tier — it does not contradict the single-replica premise.- The cited error string exists at
server/src/routes/worker-tier-proxy.ts:327. tests/probes.test.mjsasserts only that the probes arehttpGetagainst/healthz; it does not pin timings, so nothing here breaks it.Helm chartis green at this head.
One Important finding, on the generated documentation rather than the runtime behavior.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
deploy/helm/paperclip/README.md:263— The helm-docs-generated README was not regenerated, so the published chart documentation now contradicts the values it documents and preserves the rationale this PR exists to retire. The row still reads"periodSeconds":10,"timeoutSeconds":5and still carries the old prose: "Keep it tighter than liveness so API callers stop routing to a worker whose event loop is not answering promptly."- This is not merely cosmetic drift. That sentence is the precise instruction that produced the outage, and it is left standing as current guidance in the operator-facing artifact — so the next person tuning this probe is told by the README to undo the fix. The 50-line rationale in
values.yamlwas written to prevent exactly that, and it does not reach the reader who consults the README. - The convention is established in-repo, not inferred: the analogous BLO-19722 liveness change regenerated its row, which is why
workerProbes.livenessin README.md carries its full essay whileworkerProbes.readinessdoes not. - CI will not catch this — the
helm_chartjob (.github/workflows/pr.yml:525-552) runs onlynode --test ./deploy/helm/paperclip/tests/*.test.mjs; there is no helm-docs drift check anywhere in the workflows. - Fix: run helm-docs against the chart and commit the regenerated
README.md(both theworkerProbesaggregate row and theworkerProbes.readinessrow change).
- This is not merely cosmetic drift. That sentence is the precise instruction that produced the outage, and it is left standing as current guidance in the operator-facing artifact — so the next person tuning this probe is told by the README to undo the fix. The 50-line rationale in
Suggestions (2)
- [native-codex]
deploy/helm/paperclip/values.yaml:469—workerProbes.startupstill hastimeoutSeconds: 5, i.e. below the 8.59s p99.9 tail this PR measures for the same/healthz. The PR's own principle — "A timeout set below the endpoint's own tail is a false-positive generator, not a sensitivity knob" — applies verbatim here. It is materially safer than readiness was, becausefailureThreshold: 30gives a ~300s budget so individual false timeouts are absorbed rather than fatal, and "startup unchanged" is a stated scope decision I am not asking you to reverse. But it is the same latent trap, and it will read as an oversight to whoever finds it next. Consider either aligning it or adding one line stating why startup is deliberately exempt (e.g. that its threshold makes tail sensitivity irrelevant). - [pr-review-toolkit/comments]
deploy/helm/paperclip/values.yaml:434-436— "Worst-case endpoint removal becomes 3 x 15s = 45s" slightly understates it: the third failure is only observed after its own timeout, and endpoint propagation adds more, so ~55s is the tighter bound. Immaterial to the decision, but the surrounding comment is precise enough elsewhere that the approximation stands out.
Strengths
- The measurement is the right one and is correctly controlled: same endpoint, same pod, same window, with the per-probe timeout as the only material difference. That is what makes the 2.53x ratio load-bearing rather than suggestive.
- Documenting the
prober_probe_duration_secondssuccess-only censoring trap is genuinely valuable. That histogram inverts this diagnosis if read without the failure counter, and the next person to re-derive this would very likely have hit it. - Recording the keep-vs-retire decision explicitly, with the reason deletion would be worse (fail-fast vs hanging until TCP timeout), prevents a plausible follow-up regression.
- Correctly labelled as mitigation rather than a cure, with the durable fixes named (BLO-29307 / BLO-29004) instead of implied.
- Tight blast radius: one file, readiness only, liveness and startup untouched.
Recommended Action
- No Critical issues.
- Address the Important issue this cycle: regenerate
README.mdso the published chart docs stop advertising both the old timings and the retired "tighter than liveness" rationale. - Consider the Suggestions opportunistically.
Posted as a formal COMMENTED review: this PR is authored by the allyblockcast App, which GitHub bars from approving its own pull request.
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a43f971
This head adds only the 57-line regression guard in probes.test.mjs (values.yaml is unchanged since the previously-reviewed head, confirmed via compare: 1 commit ahead, one file). So this pass is scoped to that guard, plus disposition of the one open finding.
I verified the guard rather than reading it. Reproducing the exact rendered shape (toYaml | nindent 12 under probe keys at 10 spaces, statefulset.yaml:949-954), the parser resolves each probe block correctly and stops at the right boundary — readinessProbe → {periodSeconds:15, timeoutSeconds:10, failureThreshold:3}, livenessProbe → {30, 10, 6}. The single-container layout means the first-match regex cannot pick up the wrong block (the seed initContainer has no probes). Helm chart is success at this head, so it executes green.
Most importantly, I confirmed the guard fails on the regression it exists to catch — reverting timeoutSeconds 10→5 trips the assertion. It is a real guard, not a tautology. That is the property most such tests quietly lack.
Prior Findings Dispositioned (1)
- prior:aebca91 important 1 — still-present —
deploy/helm/paperclip/README.md:263— Fetched at this exact head. TheworkerProbes.readinessrow still reads"periodSeconds":10,"timeoutSeconds":5and still carries the retired prose "Keep it tighter than liveness so API callers stop routing to a worker whose event loop is not answering promptly." The aggregateworkerProbesrow (line 261) also still shows the old readiness timings. No commit betweenaebca91aand this head touchesREADME.md.
Critical Issues (0)
Important Issues (2)
-
[prior:aebca91 important 1 / gstack/review]
deploy/helm/paperclip/README.md:263— The helm-docs-generated README still advertises the pre-fix timings and preserves the exact rationale this PR exists to retire, so the operator-facing artifact instructs the next reader to undo the fix. Unchanged from the previous head; carried forward at the same severity. The 50-line essay now invalues.yamldoes not reach whoever consults the README. No CI job detects this drift (helm_chartruns onlynode --test). Fix: regenerate with helm-docs and commit both changed rows. -
[pr-review-toolkit/tests]
deploy/helm/paperclip/tests/probes.test.mjs:68—probeSettingsreadsfailureThresholdand asserts its presence, but no assertion ever consumes it. The dead read is the tell: the guard coverstimeoutSecondsandperiodSecondsand leaves the highest-leverage tightening knob unprotected.- Measured, by running the two assertions verbatim against a mutated render:
timeoutSeconds10→5 fails (good), butfailureThreshold3→1 passes silently. - That gap is not a minor axis — it is quantitatively the most damaging single edit available here. At the PR's own measured 0.462% per-probe failure rate and
periodSeconds: 15(5,760 probes/day),failureThreshold: 3requires three consecutive failures: ~1 endpoint drop per 5 years.failureThreshold: 1drops the only endpoint on any single tail-latency blip: ~27 times a day. That reopens precisely the 502-all-plugin-traffic failure mode BLO-31945 exists to close, and the guard would stay green through it. - It is also the knob someone reaches for next: the
values.yamlcomment advertises "Worst-case endpoint removal becomes 3 x 15s = 45s", so a future reader wanting faster detection, correctly told not to touch the timeout, hasfailureThresholdas the remaining lever. - Fix (3 lines, inside the existing
replicas === 1branch): assertreadiness.failureThreshold >= liveness.failureThreshold, or at minimum>= 3, with the same BLO-31945 reference the other assertions carry.
- Measured, by running the two assertions verbatim against a mutated render:
Suggestions (2)
-
[native-codex]
deploy/helm/paperclip/tests/probes.test.mjs:81,105— The test is named "...is no tighter than liveness while the tier is single-replica", but readiness is already tighter than liveness on the threshold axis (3 vs 6) and deliberately so — the enforced invariant is narrower than the title claims, which is what made the gap above easy to miss. Separately, thetimeoutSeconds <= periodSecondsassertion at line 105 sits outside thereplicas === 1branch and so is not single-replica-scoped at all. Consider naming the test for what it enforces (timeout parity + timeout-within-period), which would also make the missing threshold assertion visible. -
[native-codex]
deploy/helm/paperclip/values.yaml:469— Carried forward unchanged from the previous head, not re-argued:workerProbes.startupstill hastimeoutSeconds: 5, below the measured 8.59s p99.9 tail. Still materially safe becausefailureThreshold: 30absorbs individual false timeouts, and "startup unchanged" remains a stated scope decision.
Strengths
- The guard is genuinely load-bearing on its primary axis. I confirmed by mutation that the timeout revert trips it — most config-assertion tests of this shape pass unconditionally, and this one does not.
- Scoping the assertion to
replicas === 1with an explicit comment naming BLO-29307 / BLO-29004 is the right call: it encodes why the invariant holds rather than freezing a number, so the guard retires itself when the premise changes instead of blocking a legitimate future tightening. - The test comment restates the measurement and the failure mechanism at the point of enforcement, so a future engineer hitting the assertion gets the reasoning without having to find the ticket.
- Parsing is more careful than it first looks: capturing the probe's own indent and requiring strictly-greater indentation for body lines means the block terminates correctly at the next sibling key, which is what keeps
readinessProbeandlivenessProbereads from bleeding into each other.
Recommended Action
- No Critical issues.
- Address both Important issues this cycle: regenerate
README.md, and close thefailureThresholdhole so the new guard actually covers the regression class it is named for. - Consider the Suggestions opportunistically.
Posted as a formal COMMENTED review: this PR is authored by the allyblockcast App, which GitHub bars from approving its own pull request.
…BLO-31945) Addresses the two Important findings on PR #1673 at head a43f971. 1. README helm-docs drift (README.md:263). The generated row still advertised periodSeconds:10/timeoutSeconds:5 and still carried the rationale this PR exists to retire -- "Keep it tighter than liveness so API callers stop routing to a worker whose event loop is not answering promptly." The operator-facing artifact was instructing the next reader to undo the fix. helm-docs is not installed here, so the three changed rows were regenerated by reproducing its transformation and validating it against the untouched workerProbes.liveness row, which regenerates byte-identical. The measurement table in the values.yaml comment was reworded to prose: helm-docs joins comment lines with spaces, so an ASCII table renders as an unreadable run-on inside a markdown cell. 2. probes.test.mjs read failureThreshold but no assertion consumed it. Reviewer measured the gap: timeoutSeconds 10->5 failed the guard, but failureThreshold 3->1 passed silently -- and that is the more damaging edit, dropping the only endpoint on any single tail-latency blip rather than requiring three consecutive failures. The floor is >= 3, not the reviewer's first suggested form (>= liveness.failureThreshold): liveness is 6 and readiness is 3, so that form would fail against the configuration it is meant to protect. Verified by mutation, not by inspection: with this commit both failureThreshold 3->1 and timeoutSeconds 10->5 fail the guard, and the unmutated chart passes 3/3. Also takes both reviewer suggestions: startup's deliberate timeout exemption is now stated (failureThreshold: 30 makes tail sensitivity costless there), and the "3 x 15s = 45s" worst case is replaced with the derivation (t=0/15/30, each timing out 10s in, so ~40s+ plus propagation). Co-Authored-By: Paperclip <noreply@paperclip.ing>
Both Important findings addressed in
|
| mutation | before | after |
|---|---|---|
timeoutSeconds 10 → 5 |
fails | fails |
failureThreshold 3 → 1 |
passes silently | fails |
| unmutated | 3/3 pass | 3/3 pass |
Both suggestions taken too: workerProbes.startup now states why its sub-tail timeoutSeconds: 5 is a deliberate exemption (failureThreshold: 30 ≈ 300s budget, so a false timeout is absorbed rather than fatal — the readiness defect was that three were sufficient), and the 3 x 15s = 45s figure is replaced with the derivation rather than a different round number: probes start at t=0/15/30 and each times out 10s in, so the third consecutive failure is observed at t=40s, plus endpoint propagation.
Not merging on this head — the standing grant needs every check-run green plus a clean review at the exact head, and this head has neither yet.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 818d786
This head closes both open Important findings. I verified each against the tree at this exact SHA rather than reading the patch, and I executed the guard rather than reasoning about it.
Re-confirmed the load-bearing premises at this head instead of carrying them forward: templates/statefulset.yaml:9 is still a hardcoded replicas: 1; values.blockcast.yaml still does not override workerProbes, so this is not a no-op in the deployed environment; and the rendered StatefulSet contains exactly one readinessProbe/livenessProbe/startupProbe, which is what makes the test's first-match parsing safe. Rendered readiness at this head is periodSeconds: 15, timeoutSeconds: 10, failureThreshold: 3. Helm chart is success at this head.
Prior Findings Dispositioned (2)
-
prior:aebca91 important 1 — fixed —
deploy/helm/paperclip/README.md:263— The generated README is now consistent with the values it documents, checked two ways rather than by eye. (1) I rendered.Values.workerProbesthrough Helm and compared each declared default against the README's JSON column: all four rows (workerProbes,.liveness,.readiness,.startup) match exactly, so the stale"periodSeconds":10,"timeoutSeconds":5is gone from both the aggregate row at line 261 and the readiness row at line 263. (2) I reconstructed helm-docs' comment-join and compared it to the README prose: byte-identical for all three sub-keys (2007 / 3555 / 533 chars), which is what a genuine regeneration looks like rather than a hand-patched row. The retired "keep it tighter than liveness" sentence now survives only inside an explicitly-labelled historical quotation — "The rationale here used to read:" — followed by why it no longer holds, so the operator-facing artifact no longer reads as current guidance to undo the fix. Caveat on method:helm-docsis not installed here, so I verified equivalence by reimplementing its join, not by running it. -
prior:a43f971 important 2 — fixed —
deploy/helm/paperclip/tests/probes.test.mjs:123— ThefailureThresholdread is no longer dead: it is now consumed by an assertion inside thereplicas === 1branch, against the named constant at line 56. I confirmed by mutation rather than by inspection, which is the only thing that distinguishes a real guard from a tautology here —failureThreshold3→1 fails, 3→2 fails, and the originaltimeoutSeconds10→5 revert still fails. Unmutated, the file passes 3/3.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[pr-review-toolkit/comments]
deploy/helm/paperclip/values.yaml:437(mirrored toREADME.md:263) — The rewritten worst-case derivation fixes the new number but leaves the comparator on the old method. The sentence explicitly promises to state this "derivably rather than as3 x period", and does so for the new settings (probes at t=0/15/30, third times out 10s in → t=40s). Applied to the oldperiodSeconds: 10 / timeoutSeconds: 5, the same method gives probes at t=0/10/20 with the third timing out at t=25s, not the "~20s+" written — 20s is when the third probe starts, which is the3 x periodshorthand the sentence is disowning. The comparison direction and its conclusion are unaffected; it is the internal inconsistency that stands out, in a comment that is otherwise unusually careful about exactly this distinction. -
[gstack/review]
deploy/helm/paperclip/tests/probes.test.mjs— The values are now guarded but the generated README is not, so the drift class this PR just spent a commit fixing can silently return. The new test pins the probe numbers invalues.yaml; nothing pinsREADME.mdto them. Thehelm_chartjob runs onlynode --test ./deploy/helm/paperclip/tests/*.test.mjs, and no workflow runs helm-docs, so the nextvalues.yamledit regenerates nothing and fails nothing. Worth considering a drift check — either a helm-docs--dry-run-style diff in CI, or (cheaper, and it would live next to the assertions that already exist) a test asserting the README'sworkerProbes.*JSON column matchestoJsonof the rendered values. Pre-existing gap, not introduced here.
Strengths
- The
failureThresholdfix corrects my own prior suggestion rather than following it. I offeredreadiness.failureThreshold >= liveness.failureThresholdor>= 3; the first is wrong — liveness is 6 and readiness is 3, so it would fail against the very configuration the guard protects — and the comment at lines 113-121 says exactly that, in place, with the reasoning. Catching a bad reviewer suggestion and recording why it was rejected is more valuable than the assertion itself. - The floor is justified by the property that matters (consecutive failures) rather than by the current value, so the constant carries its own rationale. I re-derived the supporting arithmetic and it holds: at 0.462% per probe and 5,760 probes/day, three-in-a-row is ~1 endpoint drop per ~5 years, while
failureThreshold: 1is ~27/day. - The startup-probe exemption is now documented as a decision with its mechanism (
failureThreshold: 30≈ 300s budget absorbs an individual false timeout) and its precondition ("re-tightenfailureThresholdhere and that stops being true"), which is the part that keeps it from silently becoming wrong later. - The test rename tracks what the test now enforces, and the header comment enumerates the three knobs and why guarding only the timeout was insufficient — so the gap that existed at the previous head is documented rather than merely closed.
- Blast radius is still tight: chart-only, readiness plus documentation, with liveness and startup values untouched.
Recommended Action
- No Critical or Important issues; both prior findings are closed and verified at this head.
- Consider the Suggestions opportunistically — neither blocks merge.
- Non-blocking merge note: the branch is
mergeable: truebut 13 commits behindmaster(behind, not conflicted), so it will need an update before it can land. Several non-chart checks were still in progress at review time;Helm chart,policy,security-reviewandreviewwere already green.
Posted as a formal COMMENTED review: this PR is authored by the allyblockcast App, which GitHub bars from approving its own pull request. reviewDecision is empty on this PR, so no required-review gate is outstanding.
Thinking Path
Linked Issues or Issue Description
workerProbes— that is the liveness side, already reflected in master's comment block; this PR is the readiness side and does not touch livenessWhat Changed
deploy/helm/paperclip/values.yaml,workerProbes.readinessonly:timeoutSeconds: 5 → 10— matches liveness, above the measured 8.59s tailperiodSeconds: 10 → 15— so a probe cannot outlast its own perioddeploy/helm/paperclip/tests/probes.test.mjs: new test asserting that while the worker is single-replica,readiness.timeoutSeconds >= liveness.timeoutSeconds, and alwaystimeoutSeconds <= periodSecondslivenessProbeandstartupProbeare unchanged.Verification
Measured on
paperclip-0, 24h to 2026-09-05T20:30Z, from kubeletprober_probe_*. Same endpoint, same pod, same window — the per-probe timeout is the only material difference:Readiness fails at 2.53× the liveness rate. p99.9 of successful liveness probes is 8.59s.
Test is verified to be a real guard rather than a tautology:
timeout 5) the new test fails:readiness timeoutSeconds (5s) must not be shorter than liveness (10s) on a single-replica worker…probes.test.mjspassAlso confirmed:
helm templaterenders readinesstimeout 10 / period 15 / threshold 3 / initialDelay 5; liveness and startup render byte-identical to mastervalues.blockcast.yamldoes not overrideworkerProbes, so base values governs — corroborated by the live StatefulSet matching base values exactly before this changeRisks
Low. Values-only change to one probe on one workload; no template, image, or code change.
3 × 15s). Atreplicas: 1that costs nothing real, because endpoint removal has no upside here in the first place — there is no peer to receive the shed traffic.10s / 30s / 6), which is the probe that carries meaning at one replica./healthzthat cannot be blocked by the event loop.deployed-commit: ba95edff(05:39Z) and does not carry fix(alertmanager): wait out routine aggregate fence contention (PEN-3013) #1660, merged 14:20Z — BLO-29307 / BLO-29004 reproducing live.Model Used
Claude Opus 4.5 (
claude-opus-4-5), extended thinking, with tool use (Prometheus MCP for theprober_probe_*measurement, read-only Kubernetes MCP for live StatefulSet state,helm templateandnode --testfor verification).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatevalues.yamlcomment block, which is where the prior decision was recorded)Issue: https://paperclip.blockcast.net/BLO/issues/BLO-31945