Uh oh!
There was an error while loading. Please reload this page.
fix(telemetry): the Class A lists could silently shrink (backend#2341) - #790
Merged
Merged
Conversation
Bugbot High on the client#789 promotion, holding `client` out of the 22 Aug
staging hop. The concern is real and now fixed. The stated MECHANISM is not what
happens, and the real one is worse — so both are recorded here rather than the
ticket's version being implemented as written.
WHAT THE FINDING SAID: a partial `telemetryCollector` map under `--reuse-values`
leaves the lists absent, `include` renders empty, and the DaemonSet stays Ready
collecting nothing.
WHAT ACTUALLY HAPPENS, constructed rather than reasoned about:
helm template … -f <partial map> -> all 4 globs
helm template … --set-json '{"enabled":true}' -> all 4 globs
Helm coalesces chart defaults at render time, so an ABSENT key comes back from
values.yaml. The `--reuse-values` shape does not reproduce this, and the sibling
`| default` guards the ticket points at are for scalars, which have no default to
coalesce in the same way.
THE REACHABLE ROUTE IS AN EXPLICIT NULL, and it is a genuine defect:
--set telemetryCollector.classAContainers=null -> include drops to 1 of 4
helm DELETES a key set to null, and a deleted key does NOT coalesce back. The
schema's `minItems: 1` rejects an EMPTY list and says nothing about an absent one.
AND THE QUIET CASE IS ONE LIST, NOT BOTH — which inverts the severity argument.
Nulling BOTH empties the include, and filelog refuses to start on that: loud.
Nulling ONE leaves a perfectly valid config with a shorter list, so the Collector
starts, reports Ready, and silently omits three of the four Class A containers.
That is the healthy-but-blind failure this feature exists to eliminate, and it is
the case the ticket's framing does not reach.
FIXED IN TWO LAYERS, each driven through the path that reaches it:
* values.schema.json marks both lists `required`, so a null — which deletes —
fails validation. Verified it does NOT break the three documented paths: a null
whole map (the `--reuse-values`-from-an-older-release case the schema's own
description calls out), an absent map, and an explicitly disabled Collector all
still validate and render.
* the template fails closed on each list separately. Not belt-and-braces: helm has
`--skip-schema-validation`, so this is the layer that cannot be bypassed, and the
guard drives exactly that flag.
MY FIRST VERSION OF THE TEMPLATE GUARD MISSED THE CASE THAT MATTERS. It checked
only that the combined include list was non-empty, which fires on total blindness
— the loud one — and passed the one-list case that renders short and silent. Caught
by testing it rather than by reading it; the guard now checks each list.
Tests: the shrink cases live in collector-class-a-agreement.sh, which already owns
this concern, rather than in a new file. Both schema refusals, both template
refusals under `--skip-schema-validation`, and a CONTROL asserting a partial map
still coalesces all four globs — the behaviour the finding assumed was broken, now
pinned so a change that really did break coalescing is caught.
Mutation-proven, anchors asserted applied:
schema `required` removed KILLED (refused, but for the wrong reason)
per-list template fails removed KILLED (the render succeeded)
include reads a non-existent key KILLED (1 glob, want 4)
No survivors; green on restore.
538/538 chart tests, 13/13 drift guards, make check green — all under
KUBECONFIG=/nonexistent, so none of it depends on ambient kube state.
Closestracebloc/backend#2341
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>LukasWodka
commented
Aug 22, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 020b270. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka added a commit
that referenced
this pull request
Aug 22, 2026
…#792) #790 merged with its four CI failures unfixed — I had diagnosed and fixed them locally but was holding the push for a full bats run, and the merge landed first. So `develop` is red for everyone until this goes in. Entirely my sequencing error: the fix should have been pushed as soon as it was verified, with bats as confirmation rather than a gate on sharing it. Four failures, two causes. 1. THE GUARD ASSERTED HELM 4'S ERROR TEXT, AND CI PINS 3.15.4. `standard-checks.yml` installs helm v3.15.4; this was written against v4.1.1. Two version-specific things, both mine: * the schema refusal reads `missing property 'x'` on helm 4 and `x is required` on helm 3, so all four checks saw the render refused and judged it "refused, but not for the expected reason"; * `--skip-schema-validation` did not exist before helm 3.16, so the two template-layer cases failed on an unknown flag rather than on the chart. Now matched on the PREAMBLE — "don't meet the specifications" — which is identical in both versions and still tells a schema refusal apart from the template one. The `--skip-schema-validation` half is FEATURE-DETECTED and skips loudly where the flag is absent, rather than passing silently: on CI the schema layer is what gets exercised, and the output says so. 2. A PIPE INTO AN EARLY-CLOSING READER, in the guard's own error path. `printf '%s' "$out" | head -2` SIGPIPEs its producer under `set -euo pipefail` and returns 141 — the backend#1778 class this repo has a dedicated scanner for, which is what `quality / pipefail early-close` and bats test 803 were both reporting. Replaced with the documented here-string idiom. The scanner then caught my REPLACEMENT for cause 1 as well: `helm template --help | grep -q --skip-schema-validation` closes the pipe on its first hit. Now captured into a variable and matched with `case`, so there is no pipe at all. Two instances of one class, in a fix for a different class — the gate earned its keep twice in one change. Verified on the post-merge develop: pipefail gate exit 0, its 42 bats tests green, shellcheck -S warning clean, the guard's five cases green, 13/13 drift guards. The other five guards added this session were also written against helm 4, and CI's drift log shows all of them passing on 3.15.4 — only this one carried version-specific assertions. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
ContributorAuthor
/fr-pass |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes tracebloc/backend#2341 — Bugbot High on the #789 promotion, holding
clientout of the 22 Aug staging hop. Fixed ondevelopas the ticket asks, so #789 can be re-cut without re-rolling its review.The concern is real and is fixed. The stated mechanism is not what happens, and the real one is worse — so both are recorded rather than the ticket being implemented as written.
What the finding said vs what happens
Constructed rather than reasoned about:
Helm coalesces chart defaults at render time, so an absent key comes back from
values.yaml. The--reuse-valuesshape does not reproduce this.The reachable route is an explicit null
helm deletes a key set to null, and a deleted key does not coalesce back. The schema's
minItems: 1rejects an empty list and says nothing about an absent one.And the quiet case is one list, not both — which inverts the severity
filelogrefuses to start on that. Loud.Fixed in two layers, each driven through the path that reaches it
values.schema.jsonmarks both listsrequired, so a null fails validation. Verified this does not break the three documented paths — a null whole map (the--reuse-values-from-an-older-release case the schema's own description calls out), an absent map, and an explicitly disabled Collector all still validate and render.The template fails closed on each list separately. Not belt-and-braces: helm has
--skip-schema-validation, so this is the layer that can't be bypassed, and the guard drives exactly that flag.My first version of the template guard missed the case that matters
It checked only that the combined include list was non-empty — which fires on total blindness (the loud one) and passed the one-list case that renders short and silent. Caught by testing it rather than reading it. The guard now checks each list.
Tests
The shrink cases go in
collector-class-a-agreement.sh, which already owns this concern, rather than a new file:That last one is a control, pinning the behaviour the finding assumed was broken — so a change that really did break coalescing is caught.
requiredremovedTest plan
helm unittest .— 538 passedmake drift— 13/13 guards greenmake check— parse, shellcheck-S warning, helm-lint, helm-vocabKUBECONFIG=/nonexistent— no dependence on ambient kube statestage=prepare recut=true recut_repos=client🤖 Generated with Claude Code
Note
Medium Risk
Changes Helm schema and fail-closed template logic for telemetry log collection. Wrong guards could block upgrades or still allow a Ready Collector that silently drops Class A logs.
Overview
Stops the edge Collector from rendering a healthy-but-blind config when Helm deletes a Class A list set to
null(backend#2341). Nulling one list used to leave a valid shorterfileloginclude: Ready, and missing most Class A containers.values.schema.jsonnow requiresclassAContainersandclassANodeAgentContainers. The ConfigMap template fails closed on each list (not only the combined include), including under--skip-schema-validation. Chart bump to 1.9.61.Tests pin both layers plus the control that a partial
telemetryCollectormap still coalesces all four globs.Reviewed by Cursor Bugbot for commit 020b270. Bugbot is set up for automated code reviews on this repo. Configure here.