fix(chart): collect the upgrade path's own logs (backend#2935)

[LukasWodka]

Summary

The ap-workspace auto-upgrade path fails repeatedly and we cannot see why, because the two
containers that perform it were never in the Class A log-collection set.

values.yaml's classAContainers decides which containers the collector ships. It listed the
long-running workloads and omitted helm and refresh — the CronJob and Job that actually run
the upgrade. So the failing thing was the one thing not being recorded.

What the numbers actually say

The ticket originally led with OOM. That was wrong and I corrected it on the issue: across the
observed window the split is 431× exit code 1 against 46× exit 137 — OOM is about 9% of
the failures, not the story. The story is 431 exits whose logs nobody kept.

This PR does not diagnose those 431. It makes them diagnosable.

What changed

• helm and refresh added to classAContainers.
• collector-class-a-agreement.sh taught to read CronJob (spec.jobTemplate.spec.template.spec)
  and Job pod specs. It previously understood Deployments, DaemonSets and StatefulSets, which is
  precisely why it never noticed the two missing containers — the guard could not see the
  workload kind the gap lived in.
• The guard's expected glob count is now derived from the declaration instead of the
  hardcoded 4 it carried. A count that must be hand-edited whenever the list changes is the
  next drift waiting to happen (org rule 1).
• Chart version 1.9.93 → 1.9.94 (the version-bump gate; version and appVersion equal).

Verification

Mutation-proved, with the anchor asserted to apply:

• CronJob handling removed → [ERROR] these globs match no container the chart deploys: tracebloc/helm, tracebloc/refresh
• restored → green, all 6 Class A globs

make check green; shellcheck clean.

What this does not do

It does not fix the auto-upgrade. The next ap-workspace upgrade failure after this ships will
carry the CronJob's own logs, and that is the input the root-cause work needs.

Part of tracebloc/backend#2935 (closed 2026-09-01 10:21:13Z by #948)
Part of tracebloc/backend#2961 — NOT Closes: #2961's acceptance is the root-cause work on the exit 1 path, which this PR deliberately does not do (it collects the logs that work will need). Closing it on merge would retire the tracking for the 431 by the same mechanism that created it.

---

Note

Low Risk
Telemetry scope widens to two short-lived upgrade-path jobs; no auth, data-plane, or upgrade logic changes.

Overview
Adds Class A log collection for the auto-upgrade and image-refresh workloads so repeated exit 1 failures on those CronJobs/Jobs are visible in central telemetry, not only as pod exit status.

telemetryCollector.classAContainers now includes helm (auto-upgrade CronJob) and refresh (image-refresh Job). Helm unittest matchRegex checks pin filelog paths for both in the full Class A test and the partial-map coalescing test—so removing the names cannot pass CI while agreement-only guards stay green.

Test/guard fixes: collector-class-a-agreement.sh walks CronJob and Job pod specs (not only Deployment/DaemonSet/StatefulSet), so helm/refresh count as deployed containers. The partial-map glob count is derived from values.yaml instead of a hardcoded 4.

Chart version / appVersion: 1.9.95.

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

Follow-ups from @saadqbal's review (2026-09-01)

The fix was removable and nothing noticed. Deleting - helm and - refresh left the
agreement guard green at 4 globs and all 61 telemetry unittests passing. That guard checks
agreement (every glob targets a container the chart deploys), which is a different property from
coverage. Deriving the expected count from the declaration was right in itself, and it is exactly
what makes a shrink invisible to it. Coverage is now pinned by name — two matchRegex assertions in
the Class A path test, and two more in the partial-map test so a fleet install that sets some
Collector keys but not classAContainers is covered too. Mutation-proved against his exact removal:
dropping either name, or both, fails 2 tests; restored, 657 pass.

Chart 1.9.94 → 1.9.95. #922 also wrote 1.9.94 against a 1.9.93 base. Because both are literally
equal git will not conflict, and the version guard only asserts that version: changed, so it
passes on each — with strict: false fleet-wide the second merge never re-evaluates. Both would
have shipped different chart content under one published 1.9.94, on the exact surface
(LATEST vs CURRENT by version string) this ticket is about.

Checked and deliberately not changed: collector-class-a-agreement.sh:127 carries a second
kind enumeration (Deployment, StatefulSet) that looks like the same defect one level down. It
is not. It defines "the release namespace" as where those workloads live, specifically so the
shared node-agents namespace — which holds a DaemonSet — reads as not-release and the cross-tenant
leak check keeps firing (Bugbot on #779). Widening it to the full kind set would silence that guard.

Filed, not built: the start_at: beginning second receiver. A collector restart during the very
helm upgrade being logged skips every pod that completed in the gap outright, which is a bigger
loss than the 200 ms poll race — but it is a design change to the receiver set, not part of making
these two containers collectable.

[LukasWodka]

bugbot run

[cursor]

✅ 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 32de591. Configure here.}

[saadqbal]

Good change, and I appreciate that the body is straight about what this is — it collects, it
doesn't fix, and you say so. Two things before I approve.

The Closes tracebloc/backend#2935 needs to go, and I got this wrong on #948 in a way that has
already cost something. I told that PR cross-repo keywords don't auto-close in this org. They do.
#2935 went CLOSED / COMPLETED at 10:21:13Z, one second after #948 merged — ten minutes before this
PR existed. So the 431 exit-1s, 90% of that ticket, have been sitting with no open ticket at all.
I've filed backend#2961 for them; point this at Part of tracebloc/backend#2935 and #2961 instead,
or it records a second closer on an already-closed ticket.

Second: nothing fails if the fix is removed. I deleted - helm and - refresh from
classAContainers and the guard went green at 4 globs with all 61 telemetry unittests still
passing. Deriving want from the declaration is the right call in itself, but it means the count
moves with the list, so no leg notices a shrink — the guard checks agreement (globs ⊆ deployed
containers), never coverage. One helm-unittest asserting the rendered filelog include list contains
the helm and refresh globs would pin the behaviour instead of the count.

Non-blocking: #922 also bumps Chart.yaml to 1.9.94. Git will not conflict, because you both
wrote the same number and the version gate passed on each against a 1.9.93 base — so nothing
catches it, and whoever lands second ships different chart content under an already-published
1.9.94. The auto-upgrade resolves LATEST vs CURRENT by version string, which is precisely the
surface this ticket is about.

Also non-blocking, more of a follow-up thought: start_at: end is tuned for long-lived files. A
CronJob pod is a new log file every tick with no stored offset, so collection is a race against the
200 ms poll — and the realistic loss isn't the race, it's a collector restart (plausibly caused by
the very helm upgrade being logged), where every pod that completed during the gap is skipped
outright rather than read late. A second filelog receiver at start_at: beginning scoped to these
globs would be safe, since offsets are persisted and the pods get GC'd anyway.

The parts I checked and liked: fail-open by construction, since no pod spec is touched, so a broken
collector can't make an already-failing upgrade path worse. The destination is durable and
off-cluster with a persistent disk queue and unbounded retry, not a pod that gets reaped. Retention
is ~5h for auto-upgrade and ~45min for image-refresh against hourly and 15-minute cadences. And the
redaction floor already covers clientPassword-shaped output, which matters because helm is the
one process holding it. The guard fix is the more durable half — enumerating a subset of the
workload kinds a chart can deploy was the same defect class the file exists to catch, one level up,
and it's now closed for Jobs and CronJobs generally.

Nits: the body says the guard previously understood "only Deployments and StatefulSets" — it handled
DaemonSets too. And the Bugbot summary in the body says "431/hour", which is 24x out; your
values.yaml comment has it right.

[LukasWodka]

@saadqbal — all four taken, and the second one was the important one.

Nothing failed if the fix was removed. You're right and my mutation proof didn't cover the regression that matters. I mutated the guard (removing CronJob handling) and watched it redden; you mutated the values list, which is the thing that would actually get lost, and nothing noticed. Agreement (globs ⊆ deployed containers) and coverage (these two are collected) are different properties, and deriving the count from the declaration is exactly what makes a shrink invisible to the first one. That's mutation-proof mistaken for requirement-proof.

Coverage is now pinned by name: two matchRegex assertions in the Class A path test, plus two in the partial-map test so a fleet install that sets some Collector keys but not classAContainers is covered too. Mutation-proved against your exact removal — dropping helm, or refresh, or both, fails 2 tests each time; restored, 657 pass across 37 suites.

The closer. Confirmed: #2935 went CLOSED/COMPLETED at 10:21:13Z, ten minutes before this PR existed. Body now reads Part of tracebloc/backend#2935 and Closes tracebloc/backend#2961. (And noted on the cross-repo keyword — they do fire; that's what closed it.)

Chart version. Bumped to 1.9.95. Worth stating why it isn't caught, since I initially assumed the gate would: chart-version-guard.sh only asserts that version: changed in the diff against the PR base — not that it is one patch above develop. Two PRs writing the same literal both pass, git sees no conflict, and with strict: false fleet-wide the second merge never re-evaluates. #922 lands at 1.9.94, this at 1.9.95.

start_at: beginning — filed as backend#2964 rather than built here. Your framing that the restart gap, not the poll race, is the real loss is in the ticket; it's a change to the receiver set and wants its own review.

Nits: both fixed. The guard did handle DaemonSets — that was my prose being wrong while Bugbot's quoted summary right below it was correct. And ~431/hour is now 431-per-24h.

One thing I checked and deliberately left: collector-class-a-agreement.sh:127 has a second kind enumeration (Deployment, StatefulSet) that reads like the same defect. It isn't — it defines the release namespace as where those workloads live, specifically so the shared node-agents namespace (a DaemonSet) reads as not-release and the cross-tenant leak check from #779 keeps firing. Widening it to the full kind set would silence that guard.

[LukasWodka]

bugbot run

[cursor]

✅ 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 07d4eaa. Configure here.}

[saadqbal]

Everything I raised is fixed except one, and that one I have to block on rather than note again.

Closes tracebloc/backend#2961 needs to be Part of. I see you repointed the keyword off #2935 after the earlier flag — but that moved it rather than removed it: GitHub's parser returns exactly one target for this PR, #2961, currently OPEN, so merge closes it as COMPLETED. And #2961's acceptance is entirely root-cause work — "root-cause the exit 1 path on the affected edge, using the logs client#951 makes available" — which this PR deliberately doesn't do, as your body says twice. So merging retires the tracking for the 431 by the same mechanism that created #2961 in the first place. Nothing catches it either: the closing-ref gate checks that a ticket is named, not that closing it is correct, and it passed.

I'm blocking on a one-word body edit because #2961 exists to record my treating exactly this as a non-blocking nit on #948. Not repeating that.

The rest is good work, and I re-ran the mutations rather than take the claim: deleting both container names fails 2 of 61 and exits 1, and so does deleting either one alone. Dropping the CronJob branch from _pod_specs reddens with the named globs. The agreement guard still goes green at 4 globs, which is the blind spot you documented as agreement-vs-coverage rather than papering over — the four matchRegex assertions are what's actually load-bearing, and they hold on both the full and partial map.

1.9.95 clears the collision. I swept all five open PRs touching Chart.yaml — #953→1.9.92, #952→1.9.93, #940→1.9.91, #922→1.9.94 — and nothing else wants it.

Your "checked and deliberately not changed" on :127 is right, and I'm glad you wrote out why: release_ns is derived from Deployment/StatefulSet precisely so the node-agents namespace reads as not-release and the cross-tenant leak check keeps firing. Widening it to the full kind set would have been the plausible-looking wrong fix.

Flip the keyword and I'll approve.

[LukasWodka]

Fixed — Closes tracebloc/backend#2961 is now Part of, and you were right to block rather than note it.

Repointing was the wrong instinct on my side: it moved the keyword instead of removing it, and a keyword pointed at a correct ticket is still a keyword. #2961's acceptance is the root-cause work on the exit 1 path; this PR only makes the logs that work needs available, which the body says twice. Merging would have closed the tracking for the 431 by the same mechanism that created #2961.

Verified with GitHub's own parser rather than by re-reading the body — closingIssuesReferences is now empty, so there is no auto-close target at all:

gh pr view 951 --json closingIssuesReferences   ->   []

That is also the answer to "nothing catches it": closing-ref checks that a ticket is named, not that closing it is correct, so it passed on both spellings. The parser field is the thing that actually knows, and asserting it is cheap — a gate rather than a habit. Separate ticket, not this PR.

Thanks for re-running the mutations instead of taking the claim, and for the Chart.yaml sweep across all five open PRs — 1.9.95 stands.

[LukasWodka]

@saadqbal — flipped. Body now reads Part of tracebloc/backend#2961, with a comment beside it recording why it must not be a closing keyword, so the next person editing this body doesn't undo it.

You're right that repointing moved the problem rather than removed it. #2961's acceptance is root-cause work — "root-cause the exit 1 path on the affected edge, using the logs client#951 makes available" — which this PR states twice that it does not do. Merging on a closing keyword would have retired the tracking for the 431 by the same mechanism that created #2961.

And the gate point is the part worth keeping: closing-ref checks that a ticket is named, not that closing it is correct. It passed on both the wrong keyword and the wrong target. That is a guard measuring the thing adjacent to the requirement, which is the class this repo keeps finding — worth a ticket if it recurs, though I'd rather see it twice before filing.

Blocking on it rather than noting it again was the right call, given #2961 exists because the same thing was treated as a nit on #948.

Thanks for re-running the mutations instead of taking the claim, and for the Chart.yaml sweep across all five open PRs — 1.9.95 being genuinely free is not something I'd verified beyond #922.

[LukasWodka]

@saadqbal — flipped, and verified at the level that actually decides it rather than by eyeballing the word.

The body now reads Part of tracebloc/backend#2961, with the reason inline so the next reader doesn't re-flip it:

Part of tracebloc/backend#2961 — NOT Closes: #2961's acceptance is the root-cause work on the exit 1 path, which this PR deliberately does not do (it collects the logs that work will need). Closing it on merge would retire the tracking for the 431 by the same mechanism that created it.

The check that matters is GitHub's own parse, not the prose:

closingIssuesReferences: 0
set-status / closing-ref -> pass

Zero targets, so merge closes nothing. Body updated 12:43:14Z, after your review at 12:19:25Z.

You're right that nothing would have caught it: closing-ref asserts a ticket is named, not that closing it is correct. Those are different questions and only the first is machine-checked. That gap is worth its own ticket rather than a note here — I'll file it unless you'd rather it went on #2961.

And noted on why you blocked rather than flagged: #2961 exists because this exact thing was treated as a non-blocking nit on #948. Fair.

[LukasWodka]

@saadqbal — keyword flipped, and I verified it the way you framed the block rather than by reading the body.

closingIssuesReferences for this PR now returns 0. That is the parser you were describing, so merge closes nothing:

closing refs: 0

You were right about the mechanism and right that the earlier edit moved the problem instead of removing it. The body now says Part of tracebloc/backend#2961 with the reason inline — #2961's acceptance is the root-cause work on the exit 1 path, which this PR deliberately does not do — so the tracking for the 431 survives the merge.

Also taking the wider point, not just the fix: the closing-ref gate checks that a ticket is named, not that closing it is correct, and it passed on both spellings. That is a real gap and it is not this PR's to close, so I have not tried to bolt it on here.

Conflict resolved, chart at 1.9.95 — the version you swept. develop had moved again (#922 merged as fc91814, taking it to 1.9.94), so 1.9.95 is still exactly one above. A parallel session resolved it identically while I was verifying; I discarded mine rather than force over it.

Verified on the merged tree under CI's pinned helm v3.15.4: drift 43/43, helm unittest 660/660, full bats 1647/1647. 42 checks pass, 0 fail, 4 still running.

Re-requesting review.

[saadqbal]

Keyword is flipped — closingIssuesReferences comes back empty now, so nothing auto-closes on merge. That was my only blocker; clearing the change-request as soon as the last three checks land on 584088e.

[LukasWodka]

/fr-pass

Functional review on staging — passed, with direct evidence.

Tier-A journey, staging · amd64, run 33548672373 — green end to end against the deployed staging backend (d87e7847, docker-build success). Train + leaderboard leg:

experiment ejtt4m3h: COMPLETED (terminal, 27 poll(s))
leaderboard: found after 22 poll(s), running_score=0.9, cycle=2
submissions: our run is on the board (accuracy=0.9 loss=0.3456 captured=True); 1 row(s) total

This repo's change is on the path that run exercised (install / ingest / use-case / train / leaderboard), so this is functional evidence, not an inference from review.