fix(e2e): fail on a failed release of cilium and csi as on the parent, and report addon remediation - #3594

LGTM. Verified statically and hermetically (no cluster): the fix premise checks out against the code — the aggregated apiserver unconditionally stamps RetryOnFailure on every Application HR (rest.go:1601-1611), cilium/csi set it explicitly, and addons without a strategy fall back to the default, which matches the guard's teardown-vs-retry logic. The guard aborts the run for real (set -eu in the chainsaw step, not decorative), there's no local-assignment status masking, and the empty-selection path is fail-closed, not fail-open. 30/30 unit tests pass; non-vacuity confirmed by two mutations.

Non-blocking notes:

• The guard's three kubectl reads are not wrapped in the existing kubectl_wait_retry helper. In the etcd-noisy sandbox a single transient "leader changed" would redden the whole run. Fail-closed is the right default here; wrapping the reads in a bounded retry (distinguishing a transient error from a missing field) would remove a new false-red source.
• Addon-guard coverage is bounded by DisableWait (an addon torn down after the check point won't be seen) — best-effort, already acknowledged.
• grep "^${_prefix}" treats the prefix as a regex; input is controlled so no practical risk, but grep -F would be stricter.

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 740-751: Source remediation-guard.sh in the runtime path before
the cozy_guard_all_helmreleases helper is invoked. Update
hack/e2e-chainsaw/_lib/run-kubernetes.sh so helmrelease_has_teardown and
helmrelease_has_remediation_cycle are defined before this guard logic executes.

---

Nitpick comments:
In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Line 1659: Update the parent HelmRelease readiness wait around kubectl wait
for kubernetes-${test_name} to handle timeout failure explicitly, print scoped
diagnostics for that HelmRelease with kubectl describe hr in the relevant
namespace, then exit with failure while preserving the existing 5-minute wait
and success path.

Actionable comments posted: 4

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/agents/e2e-testing.md`:
- Line 65: Revise the guidance around .status.history to state that
helm-controller/api is only a pinned dependency and does not contain this
repository’s controller behavior for reading or truncating history. Keep claims
about retention and strategy-specific truncation explicitly attributed to the
upstream helm-controller source, while preserving the existing e2e timing and
shared remediation-guard helper guidance.
- Line 66: Update the paragraph describing the helmrelease_has_teardown rule to
match cozy_guard_helmrelease’s implemented behavior: document that an
uninstalled Snapshot is treated as unexplained only when
.spec.install.strategy.name is RetryOnFailure. Remove claims about
remediation.retries or upgrade remediation strategy unless the guard is expanded
to read and handle those fields.

In `@hack/e2e-chainsaw/_lib/run-kubernetes.sh`:
- Around line 1001-1010: Bound every kubectl read in the guard helpers: wrap the
strategy and history reads near lines 1001-1010, the Ready read near lines
1025-1029, and the HelmRelease list read near lines 1103-1107 in timeout -k 5
30, adding --request-timeout=30s to each kubectl invocation.
- Around line 1995-2005: Wrap the parent HelmRelease wait for
“kubernetes-${test_name}” in a failure branch that, when kubectl wait fails,
prints the HelmRelease YAML/status conditions and relevant namespace events,
then exits immediately with failure. Keep the existing 5-minute readiness wait
and ensure the remediation guard does not run after a timeout.

---

Outside diff comments:
In `@docs/agents/e2e-testing.md`:
- Around line 46-47: Update the convention in this document to prohibit all EXIT
and RETURN traps, removing the exceptions for single script steps and explicit
BATS subshells. Revise the related guidance and examples, including the
references to hack/e2e-test-openapi.bats and sourced shell libraries, so cleanup
is performed explicitly without either trap type.

This PR splits the old single helmrelease_has_remediation_cycle reading into a narrow helmrelease_has_teardown (only an uninstalled Snapshot) and the existing broad reading, and uses .spec.install.strategy.name to decide which one is fatal. It also extends the guard from the parent HelmRelease to the tenant addon HelmReleases via cozy_guard_addon_helmreleases/cozy_guard_all_helmreleases in hack/e2e-chainsaw/_lib/run-kubernetes.sh. I verified the wiring, the ordering guarantee, and the test coverage directly against the PR head. I agree with the design. LGTM with a couple of non-blocking notes.

What I verified

• cozy_guard_all_helmreleases tenant-test "kubernetes-${test_name}" is called at hack/e2e-chainsaw/_lib/run-kubernetes.sh:2029, and the tenant-snapshot trap - EXIT is at line 2048 — the guard genuinely runs before the trap is disarmed, matching the ordering claim in the new hack/run-kubernetes-remediation_test.bats test.
• helmrelease_has_remediation_cycle in hack/e2e-chainsaw/_lib/remediation-guard.sh gains a missing local statuses declaration alongside the new helmrelease_has_teardown, closing a real global-variable leak — a genuine correctness fix, not just a comment change.
• The new hack/run-kubernetes-remediation_test.bats fixture-driven kubectl stub distinguishes "read failed" (*.FAILS) from "field absent" (missing fixture) and from "kubectl warns on stderr but exits 0" (*.WARNS), and there are dedicated tests proving a stray stderr warning doesn't silently downgrade a real teardown or mask an empty history. That's the right level of paranoia for this kind of guard.
• The ordering-pin test in hack/run-kubernetes-serial-console_test.bats was re-anchored from grep -n 'helmrelease_has_remediation_cycle "${history_statuses}"' to grep -n '^ *cozy_guard_all_helmreleases ', i.e. on the call site rather than on the callee's now-renamed local variables — this correctly avoids brittleness from the refactor.

Non-blocking / notes

1. One of the open CodeRabbit threads (hack/e2e-chainsaw/_lib/run-kubernetes.sh, originally line 751) claims remediation-guard.sh "is not sourced from the current repository and is not referenced by run-kubernetes.sh." That's incorrect — line 3 of run-kubernetes.sh at the very commit CodeRabbit reviewed and at current head both read . hack/e2e-chainsaw/_lib/remediation-guard.sh. No action needed on the code, but worth resolving/dismissing the thread explicitly so it doesn't linger as a false "unaddressed Major finding."
2. The new guard's kubectl reads (cozy_guard_helmrelease's strategy/history/Ready reads, cozy_guard_addon_helmreleases's listing) have no --request-timeout/timeout wrapper, unlike other reads added elsewhere in this file. Both a prior human review and a later CodeRabbit pass flagged this independently: a wedged apiserver at guard time (which runs late, near the tenant-snapshot trap) could stall and, per the guard's own doc comments, cost part of the diagnostic budget. Worth a follow-up.
3. docs/agents/e2e-testing.md's new bullet describes two configurations that can produce an uninstalled Snapshot (default-strategy install remediation, and an upgrade remediation set to strategy: uninstall), but cozy_guard_helmrelease only reads .spec.install.strategy.name — it doesn't check .spec.install.remediation.retries or .spec.upgrade.remediation.strategy. This gap is honestly self-documented in the code's own comment block ("the rule implemented is narrower than the rule stated..."), and no current addon is in the affected shape (retries: -1 everywhere), so it's a real but currently-inert limitation rather than a bug.
4. The plain kubectl wait hr -n tenant-test "kubernetes-${test_name}" --timeout=5m --for=condition=ready call (unchanged by this diff, only the surrounding comment was rewritten) still prints nothing but kubectl's own timeout message on failure. CodeRabbit raised this twice; it predates this PR and is arguably out of scope for a remediation-guard fix, but it's a reasonable follow-up since the guard right below it depends on this wait having succeeded.
5. Per the metadata, this PR is CONFLICTING against main and needs a rebase before it can merge — noted as a mergeability item, not a logic concern; nothing in the diff suggests the conflict hides a real semantic clash.

Overall: the core reasoning (RetryOnFailure is stamped on every Application-served HelmRelease, so a failed Snapshot there is not evidence of a teardown) is correctly implemented and heavily unit-tested. The open notes above are worth acting on before or shortly after merge, but none of them constitute a regression or a broken assertion.

The classification is right and the tests are the best part of this PR. Requesting changes on where the guard is wired in, and on the field that arms it.

On the tests first, because they deserve saying. I mutated 23 ways, gate removed, gate inverted, teardown predicate widened to failed, || true on each of the two reads, stderr folded into each capture, the addon loop swallowing verdicts, the composite dropping either verdict, read order swapped, index() traded for an anchored grep, the call deleted, the call suffixed with || true. Every one turns a test red, each with a case that exists for it. Both suites behave identically under bats and under hack/cozytest.sh, 24 of 24 and 12 of 12. Every repo fact the change rests on holds too: the parent gets RetryOnFailure on both install and upgrade, cilium and csi set it, all 19 addons set retries: -1, nothing under packages/ sets an uninstalling upgrade remediation, and both truncation variants behave as the comments say.

The guard cannot run in the case it was written for. Its own comment says a child remediation surfaces only as whatever downstream deadline it eventually breaks, a node that never turns Ready, a PVC that never binds, with nothing in the run naming the teardown that preceded it. The call is at run-kubernetes.sh:2520 and every one of those deadlines is upstream of it, the node-Ready wait at :1942 with exit 1 at :1954, the csi HelmRelease wait at :2156, then the LB and NFS checks. Under set -eu the script ends there.

This PR's own CI is the demonstration. Tenant cilium never went Ready, coredns, csi, ingress-nginx, metrics-server and ouroboros all sat on dependency '...-cilium' is not ready, the run died at the node-join deadline, and cozy_guard_all_helmreleases was never reached. Had cilium been torn down instead of retried the output would have been identical and the teardown still unnamed. cozy_report_node_join_failure prints each HelmRelease's Ready message and not .status.history, so the failing path is exactly as blind as before this PR. Calling a print-only classification from cozy_report_node_join_failure, or from _tenant_snapshot_on_fail at :1915, closes it. If the happy-path-only placement is deliberate then say so in the comment and in the docs bullet, because both currently claim more.

The field that arms the guard fails soft and nothing pins it. The fatal verdict is a string equality against a value read by kubectl -o jsonpath, and with --allow-missing-template-keys at its default a path that stops resolving prints nothing and exits 0, so empty takes the soft branch. That is the same class of silent failure the empty-history check exists to catch, applied to .status.history and not to the new read. Bump helm-controller/api past a rename of spec.install.strategy and every teardown, including a real removal of the tenant CNI, becomes a NOTE about its own install remediation, green run, all 24 unit tests still passing because they feed the strategy from a fixture rather than through the jsonpath. The printed evidence does not help either, (install strategy <unset>) is the expected line for 17 of the 19 addons. Worth knowing the existing pin does not cover this shape, remediation-guard.bats:143 pins .status.history[].status against a hand-written YAML fixture rather than against the module in go.mod, so the docs claim that the shape is pinned against helm-controller/api v2 is stronger than what that test does. Cheapest fix is a canary, the parent is RetryOnFailure unconditionally so an empty strategy on the parent should be fatal.

Smaller things, none blocking alone. The retries: 0 gap you document at :1596-1605 is one more field in the same jsonpath, appending @{.spec.install.remediation.retries} and treating either condition as fatal closes it and deletes ten lines explaining why it is open. The enumerate-from-the-cluster design was chosen precisely so a release added later is covered without editing this file, and that is the shape where it does not hold. The read-order argument at :1543-1546 says no truncation variant can empty a non-empty history, which is true, but the controller also calls Status.ClearHistory() outright on an unmanaged release, so if that lands between the two reads the guard sees Ready true and empty history and fails the run with the wrong explanation. Tiny window, but the claim should be the narrower one. And it is worth naming in the docs that three releases out of twenty can actually fail the run on history content, since for the other seventeen every history shape is a note. That is a defensible design, it just is not what covering the tenant addon HelmReleases reads as.

On the two failure directions. A real failure read as a fine teardown is reachable and you relaxed it deliberately, failed is now a note everywhere, and the justification is correct against the controller since an active retry returns an upgrade rather than uninstall remediation. But this run shows the limit of recovered by design, cilium logged UpgradeFailed ... stalled resources: [Deployment/cozy-cilium/cilium-operator status: 'Failed'] eleven times over ten minutes and never recovered, and in that state the guard prints its failed-Snapshot note and returns 0. Right call for this guard, the run is already red, but retried and wedged in a retry loop are the same output and the wording is the only difference. The opposite direction is effectively unreachable, a release pairing RetryOnFailure with an uninstalling upgrade remediation would trip the fatal branch on a removal it is configured to perform, and nothing under packages/ does that.

The red E2E is not yours. Both suites fail at the node-join deadline behind tenant cilium stuck on cilium-operator, #3780 which only moves documentation fails identically, #3790 shows the same cilium signature, and nightlies have been red seven days running. Everything this PR touches in the run lives below the line where those runs die, which is also why this code has never executed on a cluster, and why the placement point above matters more than it otherwise would.

Verdict

LGTM

The teardown-vs-retry split now fails the run on a failed or uninstalled Snapshot of a RetryOnFailure release (the parent, cilium, csi) and only notes it on the default-strategy addons, which is the opposite of the fail-soft-on-failed shape the earlier objection was raised against. I reproduced the central guarantee by mutation: flipping the fatal branch to return 0 reddens 8 tests in hack/run-kubernetes-remediation_test.bats. The fatal-path tests assert failure in the positive form, so they are not among the negated assertions cozytest.sh swallows. Reviewed hermetically at head 2294fafa1, no cluster. One correctness limitation below is worth fixing but does not hold the review (a test-harness false-negative, not a product or data risk), and the code already improves on main.

Two things gate the merge and neither is in the diff. First, myasnikovdaniil's CHANGES_REQUESTED is still open on the PR; the rework answers its substance (a failed Snapshot on a RetryOnFailure release is now fatal, not soft), but the review has not been dismissed or re-submitted, so a human block stands until that reviewer looks again. Second, the Run unit and controller tests check (job 110989675196) is red on this head and it is not this PR: internal/controller/workloadmonitor_controller_test.go fails to build against sigs.k8s.io/controller-runtime v0.25.1, whose controllertest.FakeInformer has Synced as a method and no SyncedLock field, while the test assigns fi.Synced = true and locks fi.SyncedLock; that file, the fake and the go.mod pin are byte-identical at the merge-base and untouched here, and the PR's own Go test (internal/fluxcontract) builds and passes. The breakage is on main itself and it is what keeps E2E from running at this head.

Findings

• [MINOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5191, A single failed install that recovers on one retry leaves no failed Snapshot, so the guard passes on exactly the race it targets

• [NIT] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5290, Ready-condition jsonpath rename silently retires the empty-history-on-Ready canary

• [MINOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5243, Guard reads are fail-closed and unretried: one transient apiserver error on any release reds the run

Still open from my earlier rounds

The Ready-condition half of the empty-history canary is still soft. The parent now fails the run on an empty .spec.install.strategy.name, which closes the strategy-rename half; a rename of the Ready-condition jsonpath is not covered, because _ready would read empty everywhere, a Ready release with an empty history would take the "not Ready ... not inspected" branch and pass, and the canary would be off with nothing to notice. Latent and second-order (a release with a real teardown carries a non-empty history), so it does not hold the review.

Checked and correct

The guard runs before the EXIT trap is disarmed, so a fatal verdict on the passing path still triggers the tenant snapshot, an ordering pinned by the re-anchored serial-console test. _COZY_GUARD_REPORT_DONE dedups the classification across all three call sites (guard, node-join collector, exit handler). The trap is armed at the CR apply and the snapshot half no-ops until CURRENT_TENANT_KC is set, so a wait that times out before the tenant is reachable still gets its history printed. Reads are bounded (timeout -k + --request-timeout), a cut-off read keeps its non-zero status rather than being folded into "no history", and stderr is kept out of the captures. The stale denominators from the earlier comments are gone and the runner-canary pin is re-anchored on CURRENT_TENANT_KC=.

Verdict

LGTM

The rework since my last pass is a claim correction, not a behaviour change, plus one hardening branch, and it lands well. Reviewed hermetically at head b42805bb, no cluster.

Last round I left this at LGTM with three non-blocking notes and flagged two gates that were not in the diff: a red Unit & controller tests and an open block from another reviewer. The CI gate is gone. #4729 merged on 2026-10-02 and the rebase onto current main picked it up, so Unit & controller tests is green and E2E (in-tree) actually ran and passed on this head rather than being skipped behind a red main.

What changed, and how my earlier notes landed:

• The recovered-install gap I raised is now stated as a limitation rather than denied. cozy_guard_helmrelease, remediation-guard.sh, docs/agents/e2e-testing.md and both commit messages drop the "a recovered retry still fails" overstatement; the RetryOnFailure fixture moves to the history a real double-failure-then-recover leaves (deployed, superseded, failed), and a new test pins the one-failure-then-recover blind spot as a deliberate pass. Not widening detection here is the right call: "more than one history entry" would false-red the OIDC suite, which upgrades the parent on purpose before the guard runs. Tracked in #4736.
• The strategy canary is tighter. On the parent, any .spec.install.strategy.name other than RetryOnFailure now fails the run, not only an empty one, so a future rename of spec.install.strategy can no longer route every footprint to the note branch on a green run. I mutated the branch to return 0 and the new parent-strategy test goes red, so the assertion is live and not one of the negated shapes the runner swallows.

I confirmed the tests are real by mutation: neutralising the new parent-strategy branch reds the parent guard fails when its install strategy is anything but RetryOnFailure, and dropping the node-join pointer from the cilium message reds a failed cilium install is not blamed on the chart without naming a late node join. All 44 cases in hack/run-kubernetes-remediation_test.bats pass at head.

The Ready-condition canary note from before does not hold up as a risk: the unconditional kubectl wait hr --for=condition=ready on the parent at run-kubernetes.sh:6592, under set -eu, reds the run before the guard is reached if that condition is renamed, so both the probe jsonpath and the wait condition would have to move at once and the first of them fails first. Leaving it unchanged is fine.

One merge gate is still outside the diff: myasnikovdaniil's CHANGES_REQUESTED is unresolved on the PR. The substance of it is addressed (a failed Snapshot on a RetryOnFailure release is now fatal), but the review has not been dismissed or re-submitted, so a human block stands until that reviewer looks again.

Findings

• [MINOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5190, Single-release guard cannot see a failed install the next retry recovers

• [MINOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5251, Guard reads are fail-closed and unretried

• [MINOR] hack/e2e-chainsaw/_lib/run-kubernetes.sh:5449, Guard does not read the KubernetesNodes pool release

Approving, both blockers from my previous review are fixed. I checked wiring by mutation: removing trap at CR apply, report call in node-join collector, CURRENT_TENANT_PARENT_HR assignment, or running snapshot before classification makes run-kubernetes-remediation_test.bats red, same for empty and non-RetryOnFailure strategy branches on parent. Out of 30 mutants on code added since my last review 26 are caught.

Guard also ran on real cluster now, E2E on this head read parent and all addon releases in both suites and found nothing.

Not blocking: phase budget gates in cozy_report_helmrelease_remediation are not tested. No test sets _COZY_DIAG_PHASE_DEADLINE, so cozy_diag_phase_has_time always says yes, and deleting per-release gate at run-kubernetes.sh:5550 keeps everything green. Without it walk on node-join path can go past spent phase for its own 90s plus already started reads, and that time is taken from tenant snapshot. One case with deadline in the past that checks decline note and no further reads would cover all three gates. Smaller ones of same shape: required on parent call at :5542 and _cozy_guard_read_bounds at :5237 can be dropped and no test notices.