fix(seaweedfs): make naming audit fail closed on kubectl and payload errors

hack/seaweedfs-naming-audit.sh was fail-open: any kubectl failure produced an empty table indistinguishable from an honestly clean fleet (namespace LIST failure = zero namespaces walked, secret/pvc/sts LIST failures = zero findings, all under 2>/dev/null with no error handling). The runbook uses this output as the gate before PVC deletion, as post-deletion verification, and as an upgrade precondition, so a transient API error could green-light destroying data (#3431). Route every kubectl call through a run_kubectl helper: non-zero exit prints a FATAL line naming the failed query and propagates the code up the whole chain (enumerations restructured to capture-then-check, since an exit inside $(...) dies with the subshell). By-name GETs distinguish legitimate absence from real errors via --ignore-not-found on a separate existence check; a Secret that exists but has no decodable release payload, or one that decodes without a chart name, is corrupt state and fails loudly instead of silently dropping the tenant. Incomplete PV-age evidence now marks the generation incomplete and falls to the safe no-direction branch, so a failed GET can no longer flip OVERLAP into a wrong Step-3 deletion candidate. A successful run with zero findings still prints the same bytes as before. Tests: 14 new cozytest cases — failure injection for every LIST and by-name GET site, corrupt-payload variants, and full-output golden diffs for the success paths (verified byte-identical to the pre-change script). 25/25 green. Fixes #3431 Assisted-By: Claude <noreply@anthropic.com> Assisted-By: GPT-5 <noreply@openai.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> (cherry picked from commit 3dc6cbd4f4)
2026-11-09 22:13:18 +09:00 · 2026-08-17 10:14:34 +00:00
1 parent 22414e604f
commit 11ffa32419
@@ -138,3 +138,372 @@ EOF
  done < "$chart/rendered"
  rm -rf "$chart"
 }
+
+# -----------------------------------------------------------------------------
+# Fail-closed tests (issue #3431).
+#
+# The classification tests above mock nothing below the classifier. These drive
+# the KUBECTL layer, where the fail-open bug lived: a kubectl call that failed
+# used to return empty stdout, byte-identical to a genuinely clean fleet, so the
+# audit printed an empty table and exited 0 -- the exact false "nothing to do" the
+# operator is told to trust before deleting PVCs. They shim `kubectl` on PATH with
+# a fake and assert the audit fails LOUDLY (non-zero exit + a FATAL naming the
+# query) on any real error, while still treating a genuinely-absent object as
+# clean. Two families:
+#
+#   * enumerating LISTs (get ns / secret / pvc / sts) -- a failure is always fatal;
+#   * by-NAME GETs (a release secret in system_releases, a PVC/PV in pv_epoch) --
+#     `--ignore-not-found` splits a real error (fatal) from a legitimate absence:
+#     an absent release revision is a clean skip; an absent PV degrades to the
+#     safe "cannot establish direction" note and must NOT become a deletion
+#     candidate (the range-narrowing flip Codex reproduced).
+#
+# The two golden tests guard the reverse: on a healthy cluster the output stays
+# byte-for-byte what it was on origin/main, including the MIXED path that exercises
+# every pv_epoch GET.
+#
+# The fake is a real executable on PATH, so it exercises the actual exit-status
+# handling in run_kubectl and the propagation up through every caller -- a for
+# loop over `$(...)` swallows the status, so this is where a regression would hide.
+
+# _release_blob [chart-name] -- the value kubectl returns for a Helm release
+# secret's `.data.release`: base64(base64(gzip(json))). release_json base64-decodes
+# twice and gunzips it, and system_releases reads the chart name from
+# chart.metadata.name (as real Helm payloads carry it). Defaults to a cozy-seaweedfs
+# release so the audit confirms the tenant; pass another chart name for a non-
+# SeaweedFS release, or the literal EMPTY for a chartless {} payload.
+_release_blob() {
+  _rb_name=${1:-cozy-seaweedfs}
+  if [ "$_rb_name" = EMPTY ]; then
+    _rb_json='{}'
+  else
+    _rb_json='{"name":"seaweedfs-system","chart":{"metadata":{"name":"'"$_rb_name"'"}}}'
+  fi
+  printf '%s' "$_rb_json" | gzip | base64 | base64 | tr -d '\n'
+}
+
+# _write_fake_kubectl <dir> <fail> <absent-pv> <blob> <pvcs> -- drop a fake kubectl
+# into <dir>.
+#   <fail>       one of ns | secretlist | secretget | secretget_absent |
+#                secret_missing_field | secret_bad_payload | pvclist | pvcget |
+#                sts | pvget | "" -- the single call to make behave badly. Real-
+#                error targets exit non-zero with a stderr message; secretget_absent
+#                mirrors an absent Secret (--ignore-not-found: exit 0, empty -o
+#                name); secret_missing_field / secret_bad_payload keep the Secret
+#                present but return an empty / undecodable .data.release payload.
+#   <absent-pv>  a PV name (pv-legacy|pv-legacy2|pv-renamed) to report as absent
+#                (exit 0, empty), exercising the "bound claim, PV gone" path.
+#   <pvcs>       newline-separated `get pvc -o name` LIST output.
+# It otherwise walks one confirmed seaweedfs-system tenant. The by-name PVC->PV and
+# PV->timestamp maps are fixed here; timestamps are chosen so the two legacy PVs
+# straddle the single renamed PV (true answer OVERLAP), which is what makes the
+# range-narrowing flip observable. Kept POSIX and free of a column-0 `}` so
+# cozytest.sh's awk converter passes the heredoc through untouched.
+_write_fake_kubectl() {
+  # Grouped redirect (one open of the file). The closing brace is indented, so
+  # cozytest.sh's awk -- which only rewrites a `}` in column 0 -- leaves it and the
+  # heredoc alone.
+  {
+    printf '#!/bin/sh\n'
+    printf "FAIL='%s'\n" "$2"
+    printf "ABSENT_PV='%s'\n" "$3"
+    printf "BLOB='%s'\n" "$4"
+    printf "PVCS='%s'\n" "$5"
+    cat <<'FAKE'
+verb=${1:-}; res=${2:-}; args="$*"
+fail() { echo "fake kubectl: $1 (real error)" >&2; exit 1; }
+if [ "$verb $res" = "get ns" ]; then
+  [ "$FAIL" = ns ] && fail "get ns"
+  printf 'namespace/tenant-test\n'; exit 0
+fi
+if [ "$verb $res" = "get secret" ]; then
+  case "$args" in
+    *sh.helm.release.v1*)
+      # release_json now asks two questions: existence (-o name) then payload
+      # (jsonpath .data.release). Mirror that split so absence, real error, and
+      # corrupt-payload are all reachable independently.
+      case "$args" in
+        *"-o name"*)
+          [ "$FAIL" = secretget ] && fail "release secret existence GET"
+          [ "$FAIL" = secretget_absent ] && exit 0
+          printf 'secret/sh.helm.release.v1.seaweedfs-system.v1\n'; exit 0 ;;
+        *)
+          [ "$FAIL" = secret_missing_field ] && exit 0
+          if [ "$FAIL" = secret_bad_payload ]; then printf '@@@not-base64@@@\n'; exit 0; fi
+          printf '%s\n' "$BLOB"; exit 0 ;;
+      esac ;;
+    *owner=helm*) printf '1\n'; exit 0 ;;
+    *) [ "$FAIL" = secretlist ] && fail "namespace secret LIST"
+       printf 'seaweedfs-system\n'; exit 0 ;;
+  esac
+fi
+if [ "$verb $res" = "get pvc" ]; then
+  case "$args" in
+    *"-o name"*)
+      [ "$FAIL" = pvclist ] && fail "pvc LIST"
+      [ -n "$PVCS" ] && printf '%s\n' "$PVCS"
+      exit 0 ;;
+    *data1-seaweedfs-system-volume-0*) [ "$FAIL" = pvcget ] && fail "pvc GET"; printf 'pv-renamed\n'; exit 0 ;;
+    *data1-seaweedfs-volume-1*)        [ "$FAIL" = pvcget ] && fail "pvc GET"; printf 'pv-legacy2\n'; exit 0 ;;
+    *data1-seaweedfs-volume-0*)        [ "$FAIL" = pvcget ] && fail "pvc GET"; printf 'pv-legacy\n'; exit 0 ;;
+  esac
+  exit 0
+fi
+if [ "$verb $res" = "get sts" ]; then
+  [ "$FAIL" = sts ] && fail "sts LIST"
+  exit 0
+fi
+if [ "$verb $res" = "get pv" ]; then
+  [ "$FAIL" = pvget ] && fail "get pv"
+  case "$args" in
+    *pv-legacy2*) [ "$ABSENT_PV" = pv-legacy2 ] && exit 0; printf '2099-01-01T00:00:00Z\n'; exit 0 ;;
+    *pv-legacy*)  [ "$ABSENT_PV" = pv-legacy ]  && exit 0; printf '2020-01-01T00:00:00Z\n'; exit 0 ;;
+    *pv-renamed*) [ "$ABSENT_PV" = pv-renamed ] && exit 0; printf '2020-06-01T00:00:00Z\n'; exit 0 ;;
+  esac
+  exit 0
+fi
+exit 0
+FAKE
+  } > "$1/kubectl"
+  chmod +x "$1/kubectl"
+}
+
+# _pvcs_mixed -- the `get pvc -o name` LIST for a MIXED tenant: two legacy claims
+# and one release-named claim, whose PV vintages truly overlap.
+_pvcs_mixed() {
+  printf '%s\n' \
+    persistentvolumeclaim/data1-seaweedfs-volume-0 \
+    persistentvolumeclaim/data1-seaweedfs-volume-1 \
+    persistentvolumeclaim/data1-seaweedfs-system-volume-0
+}
+
+# _expected_L / _expected_mixed_overlap -- golden output built with the SAME printf
+# contract the script uses, so a drift in row count, text, or padding fails the diff.
+_expected_L() {
+  printf '%-24s %-14s %-8s %s\n' NAMESPACE RELEASE CLASS NOTE
+  printf '%-24s %-14s %-8s %s\n' --------- ------- ----- ----
+  printf '%-24s %-14s %-8s %s\n' tenant-test seaweedfs-system L 'chart-named only; the upgrade adopts it, nothing to do'
+}
+_expected_mixed_overlap() {
+  printf '%-24s %-14s %-8s %s\n' NAMESPACE RELEASE CLASS NOTE
+  printf '%-24s %-14s %-8s %s\n' --------- ------- ----- ----
+  printf '%-24s %-14s %-8s %s\n' tenant-test seaweedfs-system MIXED 'both generations; the chart REFUSES until one is removed'
+  printf '%-24s %-14s %-8s   %s\n' '' '' '' 'PV vintages OVERLAP => no candidate. An interrupted Step 2 re-bind looks exactly like this (both generations on original PVs). Finish Step 2 if one is in progress; otherwise escalate. Do NOT run Step 2a.'
+  printf '%-24s %-14s %-8s   %s\n' '' '' '' 'CANDIDATE ONLY: "original" does not mean the other set is EMPTY. A duplicate that'
+  printf '%-24s %-14s %-8s   %s\n' '' '' '' 'served writes and later crashed looks identical here. Verify emptiness before deleting.'
+}
+
+@test "fails closed when 'kubectl get ns' fails (whole-cluster mode)" {
+  # No namespace args -> main enumerates namespaces itself. If that LIST fails,
+  # the OLD code audited zero namespaces and still printed an empty clean table.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" ns "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'list namespaces'
+}
+
+@test "fails closed when the namespace-wide secret LIST fails" {
+  # system_releases enumerates Helm releases with a namespace-wide secret LIST --
+  # the exact call that timed out in the field and reported a false clean.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" secretlist "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'list Helm release secrets'
+}
+
+@test "fails closed when the by-name release-secret existence GET hits a real error" {
+  # Codex Finding 1: system_releases uses release_json to decide whether a release
+  # IS SeaweedFS. Both LISTs succeed, then a transient/forbidden by-name Secret GET
+  # used to leave `chart` empty -> the tenant was silently skipped -> exit 0, empty
+  # table. A real error here must now be fatal, not a false clean.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" secretget "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'Helm release secret'
+}
+
+@test "an absent release-secret revision is a clean skip, not a failure" {
+  # The one legitimately-empty case for release_json: the revision secret does not
+  # exist (pruned by Helm history limit). The existence check (--ignore-not-found -o
+  # name) returns empty + exit 0, so the release is simply not confirmed as
+  # SeaweedFS. No error, no crash -- the audit completes and reports nothing here.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" secretget_absent "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -eq 0 ]
+  [ "$(printf '%s\n' "$out" | grep -c 'FATAL')" -eq 0 ]
+}
+
+@test "fails closed when a present release secret has no .data.release payload" {
+  # Codex re-review round 2: the existence check passes (Secret EXISTS), but its
+  # .data.release field is empty/missing. That is CORRUPT state, not an absence --
+  # and because the earlier fix used --ignore-not-found -o jsonpath, which returns
+  # empty+0 for BOTH, it used to read as a clean skip. It must be a loud stop.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" secret_missing_field "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'no .data.release payload'
+}
+
+@test "fails closed when a present release secret has an undecodable payload" {
+  # Existence passes, .data.release is non-empty but is not base64(base64(gzip(...))),
+  # so every decode fails. The old `|| return 0` converted that decode failure into
+  # a clean 0 return -> silent skip. A corrupt payload must be fatal.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" secret_bad_payload "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'not decodable'
+}
+
+@test "fails closed when a present release secret decodes to a chartless payload" {
+  # Codex round 3: the payload fetches and DECODES cleanly (valid JSON {}), so the
+  # decode guards all pass -- but it carries no chart name. system_releases then
+  # extracted chart='' and silently skipped the tenant -> exit 0, clean table. A
+  # decoded helm.sh/release.v1 release without a chart name is corrupt: fatal.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" "" "" "$(_release_blob EMPTY)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'no chart name'
+}
+
+@test "a present release secret for a non-SeaweedFS chart is a silent legitimate skip" {
+  # The counterpart the guard must NOT break: a real, well-formed release whose
+  # chart is simply not cozy-seaweedfs. It has a chart name (so it is not corrupt),
+  # but the wrong one, so it is filtered out exactly as before -- no row, no error.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" "" "" "$(_release_blob cozy-postgres)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -eq 0 ]
+  [ "$(printf '%s\n' "$out" | grep -c 'FATAL')" -eq 0 ]
+  # Not confirmed as SeaweedFS -> no classification row for the release.
+  [ "$(printf '%s\n' "$out" | grep -c 'seaweedfs-system')" -eq 0 ]
+}
+
+@test "fails closed when 'kubectl get pvc' LIST fails" {
+  # The secret enumeration succeeds and confirms a seaweedfs-system tenant, so the
+  # audit reaches the per-namespace PVC LIST; a failure there must abort, not read
+  # as "this tenant has no claims".
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" pvclist "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'list PVCs'
+}
+
+@test "fails closed when 'kubectl get sts' LIST fails" {
+  # PVC LIST succeeds (empty), so the audit reaches the StatefulSet LIST; a
+  # failure there must abort rather than read as "no StatefulSets".
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" sts "" "$(_release_blob)" ""
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q 'StatefulSets'
+}
+
+@test "fails closed when the by-name PV GET hits a real error" {
+  # Codex Finding 2, error half: pv_epoch reads each bound PV's age by name. A real
+  # error (RBAC/timeout) must abort, not silently drop that PV from the range.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" pvget "" "$(_release_blob)" "$(_pvcs_mixed)"
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -ne 0 ]
+  printf '%s\n' "$out" | grep -q 'FATAL'
+  printf '%s\n' "$out" | grep -q "get PV '"
+}
+
+@test "an absent PV degrades to the safe fallback, never a wrong candidate" {
+  # Codex Finding 2, absence half. True vintages: legacy PVs 2020 + 2099 straddle
+  # the single renamed PV 2020-06 => OVERLAP. Report the newest legacy PV (2099) as
+  # ABSENT: the observed legacy range collapses to {2020} and, unguarded, "every
+  # release-named PV is strictly newer" would fire -- naming the release-named set a
+  # deletion candidate on incomplete evidence. The generation must instead read as
+  # incomplete and fall to "cannot establish direction".
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" "" pv-legacy2 "$(_release_blob)" "$(_pvcs_mixed)"
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  rm -rf "$d"
+  echo "rc=$rc"; echo "$out"
+  [ "$rc" -eq 0 ]
+  [ "$(printf '%s\n' "$out" | grep -c 'FATAL')" -eq 0 ]
+  printf '%s\n' "$out" | grep -q 'direction cannot be established from PV ages'
+  # The whole point: incomplete evidence must NOT be reported as a deletion candidate.
+  [ "$(printf '%s\n' "$out" | grep -c 'candidate duplicate')" -eq 0 ]
+}
+
+@test "success path (L) is byte-identical to the expected fixture" {
+  # A single chart-named claim, no release-named one, no StatefulSets => L, adopt in
+  # place. Full-output compare, so an extra row / warning / duplicate line fails.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" "" "" "$(_release_blob)" "persistentvolumeclaim/data1-seaweedfs-volume-0"
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  printf '%s\n' "$out" > "$d/got"
+  _expected_L > "$d/want"
+  echo "rc=$rc"; echo "--- got ---"; cat "$d/got"; echo "--- want ---"; cat "$d/want"
+  [ "$rc" -eq 0 ]
+  diff "$d/want" "$d/got"
+  rm -rf "$d"
+}
+
+@test "success path (MIXED/overlap) is byte-identical and exercises pv_epoch" {
+  # Both generations present with all PVs readable => the MIXED path runs every
+  # pv_epoch GET and lands on OVERLAP. Full-output compare against the golden.
+  d=$(mktemp -d)
+  _write_fake_kubectl "$d" "" "" "$(_release_blob)" "$(_pvcs_mixed)"
+  rc=0
+  out=$(PATH="$d:$PATH" SEAWEEDFS_AUDIT_LIB=0 sh "$PWD/hack/seaweedfs-naming-audit.sh" tenant-test 2>&1) || rc=$?
+  printf '%s\n' "$out" > "$d/got"
+  _expected_mixed_overlap > "$d/want"
+  echo "rc=$rc"; echo "--- got ---"; cat "$d/got"; echo "--- want ---"; cat "$d/want"
+  [ "$rc" -eq 0 ]
+  diff "$d/want" "$d/got"
+  rm -rf "$d"
+}
@@ -44,7 +44,15 @@
 #
 # Usage:  hack/seaweedfs-naming-audit.sh [namespace...]
 #         KUBECONFIG=<file> hack/seaweedfs-naming-audit.sh
-# Exit:   0 always (audit only). Nothing is mutated.
+# Exit:   0        the audit RAN TO COMPLETION; the table is the whole answer
+#                  (an empty table then means a genuinely clean fleet).
+#         non-zero a kubectl query FAILED, so the audit is INCOMPLETE and the
+#                  table must NOT be trusted -- a message naming the failed query
+#                  is on stderr. This is fail-CLOSED by design (see run_kubectl):
+#                  the output gates a destructive runbook step, so an unreachable
+#                  API aborts loudly rather than printing an empty table that
+#                  reads identically to "nothing to do".
+# Nothing is ever mutated on either path -- every kubectl call is read-only.
 #
 # POSIX sh, deliberately. hack/seaweedfs-naming-audit.bats sources this file, and
 # cozytest.sh sources the converted test into its own /bin/sh — which is dash on
@@ -55,6 +63,48 @@
 # particular is not POSIX and dash 0.5.12 (Ubuntu) rejects it outright.
 set -u

+# run_kubectl <what> <kubectl-args...> -- run ONE read-only kubectl query
+# fail-CLOSED and print its stdout. This audit is the documented gate in front of
+# a destructive runbook step (deleting a tenant's PVCs), so a query that FAILS
+# must never be mistaken for a query that found nothing: an enumerating LIST
+# returns empty stdout on a timeout or an RBAC denial, byte-identical to the same
+# LIST succeeding on a genuinely clean fleet, and the old `2>/dev/null` on each
+# call turned an unreachable API into a silent false "nothing to do". Every
+# enumeration is routed through here so any failure is loud and terminal instead.
+#
+# On success prints kubectl's stdout verbatim -- which may legitimately be empty,
+# because a LIST that matched nothing is honestly clean; keeping that case
+# distinct from failure is the whole point. On a non-zero exit prints the failed
+# query to stderr and returns that exit code; kubectl's own stderr stays on fd 2
+# and reaches the operator unfiltered. Callers MUST propagate the non-zero status
+# (`|| return`/`|| exit`) -- a bare `for x in $(run_kubectl ...)` would swallow
+# it, because a for-loop ignores the exit status of its word-list command, and an
+# `exit` inside the `$(...)` subshell would only leave that subshell.
+run_kubectl() {
+  _rk_what="$1"; shift
+  if _rk_out=$(kubectl "$@"); then
+    printf '%s' "$_rk_out"
+    return 0
+  else
+    _rk_rc=$?
+    printf 'seaweedfs-naming-audit: FATAL: %s failed (kubectl %s -> exit %s). Refusing to report a clean fleet on an unreachable API.\n' \
+      "$_rk_what" "$*" "$_rk_rc" >&2
+    return "$_rk_rc"
+  fi
+}
+
+# audit_fatal <message> -- the counterpart to run_kubectl for corruption the API
+# call itself did NOT report: the query succeeded, but what it returned is
+# impossible for a healthy object (a helm.sh/release.v1 Secret with no decodable
+# release payload). Same fail-CLOSED contract -- print a FATAL line naming the
+# object to stderr and return non-zero; callers MUST propagate it. A silent skip
+# here would be the identical false-clean this whole script guards against, just
+# one field deeper than an unreachable API.
+audit_fatal() {
+  printf 'seaweedfs-naming-audit: FATAL: %s Refusing to report a clean fleet on corrupt state.\n' "$1" >&2
+  return 1
+}
+
 # renamed_volume_prefix <release> -- reconstruct the name 4.31 gives the volume
 # component of <release>. Mirrors seaweedfs.fullname + seaweedfs.componentName
 # (see packages/system/seaweedfs/templates/_naming.tpl, which the chart's guard
@@ -71,18 +121,51 @@ renamed_volume_prefix() {
  printf '%s-volume' "$(printf '%s' "$full" | cut -c1-56 | sed 's/-$//')"
 }

-# release_json <ns> <release> [rev] -- decode a Helm release secret.
-# Helm stores base64(gzip(json)) in Secret.data.release, and Kubernetes base64s
-# the data value again, hence the two decodes.
+# release_json <ns> <release> [rev] -- decode a Helm release secret's payload, or
+# "" when that revision's secret does not exist. Helm stores base64(gzip(json)) in
+# Secret.data.release, and Kubernetes base64s the data value again, hence the two
+# decodes.
+#
+# This is a by-NAME GET, but it is NOT best-effort: system_releases decides whether
+# a release IS SeaweedFS from the result, so a swallowed failure silently drops a
+# real tenant -- the false-clean this script exists to prevent. EXISTENCE and
+# EXTRACTION are therefore two separate questions, because `--ignore-not-found
+# -o jsonpath` cannot tell them apart (it returns empty + exit 0 both for an absent
+# Secret AND for a present Secret whose .data.release is missing):
+#
+#   1. Existence, via `--ignore-not-found -o name`. Empty + exit 0 = the revision
+#      was pruned by Helm's history limit -- a legitimate absence; return "" and
+#      let rev1_scheme / first_deployed treat it as "pruned". A real error (RBAC,
+#      timeout, apiserver down) stays non-zero and run_kubectl makes it fatal.
+#   2. The Secret EXISTS, so its payload MUST decode. A helm.sh/release.v1 Secret
+#      with no .data.release, a payload that is not base64(base64(gzip(...))), or an
+#      empty JSON after decoding is CORRUPT state, not an absence -- and here that
+#      is safety-critical, so every such anomaly is audit_fatal, never a silent skip
+#      (contrast pv_epoch, where an empty field is a benign "no evidence").
+#
+# The extraction GET drops --ignore-not-found deliberately: the Secret existed a
+# moment ago, so a NotFound now is a mid-audit deletion race, and failing closed on
+# it is correct.
 release_json() {
-  kubectl get secret -n "$1" "sh.helm.release.v1.$2.v${3:-1}" -o jsonpath='{.data.release}' 2>/dev/null \
-    | base64 -d 2>/dev/null | base64 -d 2>/dev/null | gunzip 2>/dev/null
+  _rj_secret="sh.helm.release.v1.$2.v${3:-1}"
+  _rj_exists=$(run_kubectl "check Helm release secret '$_rj_secret' in namespace '$1'" \
+    get secret -n "$1" "$_rj_secret" --ignore-not-found -o name) || return $?
+  [ -n "$_rj_exists" ] || return 0
+  _rj_field=$(run_kubectl "read .data.release of secret '$_rj_secret' in namespace '$1'" \
+    get secret -n "$1" "$_rj_secret" -o jsonpath='{.data.release}') || return $?
+  [ -n "$_rj_field" ] || { audit_fatal "secret '$_rj_secret' in namespace '$1' exists but has no .data.release payload (corrupt Helm release)."; return 1; }
+  _rj_json=$(printf '%s' "$_rj_field" | base64 -d 2>/dev/null | base64 -d 2>/dev/null | gunzip 2>/dev/null) \
+    || { audit_fatal "secret '$_rj_secret' in namespace '$1': .data.release is not decodable base64(base64(gzip(json))) (corrupt Helm release)."; return 1; }
+  [ -n "$_rj_json" ] || { audit_fatal "secret '$_rj_secret' in namespace '$1': release payload decoded to nothing (corrupt Helm release)."; return 1; }
+  printf '%s' "$_rj_json"
 }

 # revisions <ns> <release> -- retained revision numbers, oldest first.
 revisions() {
-  kubectl get secret -n "$1" -l "name=$2,owner=helm" \
-    -o jsonpath='{range .items[*]}{.metadata.labels.version}{"\n"}{end}' 2>/dev/null | sort -n
+  _rev_out=$(run_kubectl "list Helm revision secrets for '$2' in namespace '$1'" \
+    get secret -n "$1" -l "name=$2,owner=helm" \
+    -o jsonpath='{range .items[*]}{.metadata.labels.version}{"\n"}{end}') || return $?
+  printf '%s\n' "$_rev_out" | sort -n
 }

 # system_releases <ns> -- the SeaweedFS <name>-system Helm releases in a namespace.
@@ -92,12 +175,29 @@ revisions() {
 # unrelated release in a namespace that happens to run SeaweedFS would be reported
 # as a SeaweedFS tenant. Confirm the chart.
 system_releases() {
-  for rel in $(kubectl get secret -n "$1" \
-                 -o jsonpath='{range .items[?(@.type=="helm.sh/release.v1")]}{.metadata.labels.name}{"\n"}{end}' 2>/dev/null \
-               | grep -E -- '-system$' | sort -u); do
-    for rev in $(revisions "$1" "$rel"); do
-      chart=$(release_json "$1" "$rel" "$rev" | sed -n 's/.*"name":"\(cozy-seaweedfs\)".*/\1/p' | head -1)
-      if [ -n "$chart" ]; then printf '%s\n' "$rel"; fi
+  _sr_secrets=$(run_kubectl "list Helm release secrets in namespace '$1'" \
+    get secret -n "$1" \
+    -o jsonpath='{range .items[?(@.type=="helm.sh/release.v1")]}{.metadata.labels.name}{"\n"}{end}') || return $?
+  for rel in $(printf '%s\n' "$_sr_secrets" | grep -E -- '-system$' | sort -u); do
+    _sr_revs=$(revisions "$1" "$rel") || return $?
+    for rev in $_sr_revs; do
+      _sr_json=$(release_json "$1" "$rel" "$rev") || return $?
+      # Empty "" here means the revision secret was pruned (release_json's absence
+      # path) -- a legitimate skip. A NON-empty payload, however, was fetched and
+      # decoded successfully, so it MUST name its chart: a helm.sh/release.v1 release
+      # always carries chart.metadata.name. A payload without one ({} , or any valid
+      # JSON lacking it) is corrupt/unexpected, and since "no chart name" is
+      # indistinguishable downstream from "not SeaweedFS", silently skipping it is
+      # the very false-clean this guard exists to stop -- so it is FATAL. A present
+      # but different chart name is a real, non-SeaweedFS release and stays a
+      # legitimate skip, filtered exactly as before by the cozy-seaweedfs test.
+      # ("chart" is the release struct's only such key -- vendored subcharts nest
+      # under "dependencies" -- so this match is unambiguous.)
+      if [ -n "$_sr_json" ]; then
+        _sr_chart=$(printf '%s' "$_sr_json" | sed -n 's/.*"chart":{"metadata":{"name":"\([^"]*\)".*/\1/p' | head -1)
+        [ -n "$_sr_chart" ] || { audit_fatal "secret 'sh.helm.release.v1.$rel.v$rev' in namespace '$1' decoded to a payload with no chart name (corrupt Helm release)."; return 1; }
+        if [ "$_sr_chart" = cozy-seaweedfs ]; then printf '%s\n' "$rel"; fi
+      fi
      break
    done
  done
@@ -106,28 +206,52 @@ system_releases() {
 # first_deployed <ns> <release> -- epoch seconds of the release's first install,
 # from any retained revision (the field is identical on all of them).
 first_deployed() {
-  for rev in $(revisions "$1" "$2"); do
-    ts=$(release_json "$1" "$2" "$rev" | sed -n 's/.*"first_deployed":"\([^"]*\)".*/\1/p' | head -1)
+  _fd_revs=$(revisions "$1" "$2") || return $?
+  for rev in $_fd_revs; do
+    _fd_json=$(release_json "$1" "$2" "$rev") || return $?
+    ts=$(printf '%s' "$_fd_json" | sed -n 's/.*"first_deployed":"\([^"]*\)".*/\1/p' | head -1)
    if [ -n "$ts" ]; then date -u -d "$(printf '%s' "$ts" | cut -c1-19)" +%s 2>/dev/null; return; fi
  done
 }

 # rev1_scheme <ns> <release> -- "legacy" | "renamed" | "" (revision 1 pruned).
 rev1_scheme() {
-  m=$(release_json "$1" "$2" 1)
+  m=$(release_json "$1" "$2" 1) || return $?
  [ -n "$m" ] || return 0
  if printf '%s' "$m" | grep -q 'name: seaweedfs-master'; then printf 'legacy'
  elif printf '%s' "$m" | grep -qE "name: $2(-seaweedfs)?-master"; then printf 'renamed'
  fi
 }

-# pv_epoch <ns> <pvc> -- creation epoch of the PV the claim is BOUND to.
+# pv_epoch <ns> <pvc> -- creation epoch of the PV the claim is BOUND to, or "" when
+# there is no usable PV age. Two by-NAME GETs.
+#
+# A real API error must abort (an epoch silently missing from a range can invert
+# the strict-newer comparison and name the WRONG deletion candidate), so both GETs
+# go through run_kubectl. But UNLIKE release_json, an EMPTY field here is NOT
+# corruption and is NOT fatal -- because the consequence differs. When release_json
+# returns empty for a present Secret the tenant is dropped from the report: a
+# false-clean. When pv_epoch returns "" the claim merely contributes no epoch,
+# which marks its whole generation INCOMPLETE (see audit_ns) and forces the safe
+# "direction cannot be established -> classify by hand" branch. That degradation is
+# conservative by construction: it never yields a false-clean and never names a
+# candidate. So the two empty-field cases are deliberately kept benign:
+#   * PVC .spec.volumeName empty -- a Pending / unbound claim, a routine state;
+#   * PV .metadata.creationTimestamp empty -- shouldn't happen (the apiserver always
+#     stamps it), but if it ever did the only effect is one missing epoch, i.e. the
+#     same safe incomplete fallback, so a hard stop is not warranted here.
+# `--ignore-not-found` keeps a genuinely-absent PVC/PV (NotFound) on that same
+# benign path rather than turning it into a run_kubectl failure, and the final
+# `|| return 0` keeps an unparseable timestamp there too; only a real run_kubectl
+# error propagates.
 pv_epoch() {
-  pv=$(kubectl get pvc -n "$1" "$2" -o jsonpath='{.spec.volumeName}' 2>/dev/null)
+  pv=$(run_kubectl "get PVC '$2' in namespace '$1'" \
+    get pvc -n "$1" "$2" --ignore-not-found -o jsonpath='{.spec.volumeName}') || return $?
  [ -n "$pv" ] || return 0
-  t=$(kubectl get pv "$pv" -o jsonpath='{.metadata.creationTimestamp}' 2>/dev/null)
+  t=$(run_kubectl "get PV '$pv' (bound by PVC '$2' in namespace '$1')" \
+    get pv "$pv" --ignore-not-found -o jsonpath='{.metadata.creationTimestamp}') || return $?
  [ -n "$t" ] || return 0
-  date -u -d "$(printf '%s' "$t" | cut -c1-19)" +%s 2>/dev/null
+  date -u -d "$(printf '%s' "$t" | cut -c1-19)" +%s 2>/dev/null || return 0
 }

 # classify_mixed_direction <lmin> <lmax> <rmin> <rmax> -- direction of a MIXED
@@ -153,17 +277,21 @@ classify_mixed_direction() {

 audit_ns() {
  ns="$1"
-  for rel in $(system_releases "$ns"); do
+  _rels=$(system_releases "$ns") || return $?
+  for rel in $_rels; do
    prefix=$(renamed_volume_prefix "$rel")
    legacy_pvcs=""; renamed_pvcs=""
-    for pvc in $(kubectl get pvc -n "$ns" -o name 2>/dev/null | sed 's|persistentvolumeclaim/||'); do
+    _pvcs=$(run_kubectl "list PVCs in namespace '$ns'" get pvc -n "$ns" -o name) || return $?
+    for pvc in $(printf '%s\n' "$_pvcs" | sed 's|persistentvolumeclaim/||'); do
      case "$pvc" in
        "data1-${prefix}"*)      renamed_pvcs="$renamed_pvcs $pvc" ;;
        data1-seaweedfs-volume*) legacy_pvcs="$legacy_pvcs $pvc" ;;
      esac
    done
    legacy_sts=""; renamed_sts=""
-    for sts in $(kubectl get sts -n "$ns" -l app.kubernetes.io/name=seaweedfs -o name 2>/dev/null | sed 's|statefulset.apps/||'); do
+    _sts_list=$(run_kubectl "list SeaweedFS StatefulSets in namespace '$ns'" \
+      get sts -n "$ns" -l app.kubernetes.io/name=seaweedfs -o name) || return $?
+    for sts in $(printf '%s\n' "$_sts_list" | sed 's|statefulset.apps/||'); do
      case "$sts" in
        "${prefix}"*)      renamed_sts="$renamed_sts $sts" ;;
        seaweedfs-volume*) legacy_sts="$legacy_sts $sts" ;;
@@ -184,21 +312,30 @@ audit_ns() {
    fi

    # Both generations. Report which is ORIGINAL from durable evidence.
-    fd=$(first_deployed "$ns" "$rel")
-    scheme=$(rev1_scheme "$ns" "$rel")
+    fd=$(first_deployed "$ns" "$rel") || return $?
+    scheme=$(rev1_scheme "$ns" "$rel") || return $?
    printf '%-24s %-14s %-8s %s\n' "$ns" "$rel" "MIXED" "both generations; the chart REFUSES until one is removed"
    if [ -n "$scheme" ]; then
      printf '%-24s %-14s %-8s   revision 1 was installed with the %s names => the %s generation is ORIGINAL\n' \
        "" "" "" "$scheme" "$scheme"
    fi
-    lmin=""; lmax=""; rmin=""; rmax=""
+    # The direction rule is "EVERY PV of one generation strictly newer than every
+    # PV of the other". That holds only if we saw EVERY bound PV: a claim whose PV
+    # age we could not read (unbound, or PV gone) leaves the range unbounded on one
+    # side, and a silently-narrowed range can flip OVERLAP into a confident (wrong)
+    # candidate. So a generation with any unreadable claim is INCOMPLETE
+    # and forces the safe "cannot establish" branch -- distinct from a real API
+    # error, which pv_epoch has already turned into a hard failure above.
+    lmin=""; lmax=""; rmin=""; rmax=""; l_incomplete=0; r_incomplete=0
    for p in $legacy_pvcs; do
-      e=$(pv_epoch "$ns" "$p"); [ -n "$e" ] || continue
+      e=$(pv_epoch "$ns" "$p") || return $?
+      if [ -z "$e" ]; then l_incomplete=1; continue; fi
      { [ -z "$lmin" ] || [ "$e" -lt "$lmin" ]; } && lmin=$e
      { [ -z "$lmax" ] || [ "$e" -gt "$lmax" ]; } && lmax=$e
    done
    for p in $renamed_pvcs; do
-      e=$(pv_epoch "$ns" "$p"); [ -n "$e" ] || continue
+      e=$(pv_epoch "$ns" "$p") || return $?
+      if [ -z "$e" ]; then r_incomplete=1; continue; fi
      { [ -z "$rmin" ] || [ "$e" -lt "$rmin" ]; } && rmin=$e
      { [ -z "$rmax" ] || [ "$e" -gt "$rmax" ]; } && rmax=$e
    done
@@ -206,7 +343,7 @@ audit_ns() {
      printf '%-24s %-14s %-8s   oldest PV vs first_deployed: chart-named +%ss, release-named +%ss (context only, not the rule)\n' \
        "" "" "" "$((lmin - fd))" "$((rmin - fd))"
    fi
-    if [ -n "$lmin" ] && [ -n "$rmin" ]; then
+    if [ -n "$lmin" ] && [ -n "$rmin" ] && [ "$l_incomplete" = 0 ] && [ "$r_incomplete" = 0 ]; then
      case $(classify_mixed_direction "$lmin" "$lmax" "$rmin" "$rmax") in
        legacy-original)
          printf '%-24s %-14s %-8s   every release-named PV is strictly newer => chart-named is ORIGINAL, the release-named set is the candidate duplicate (Step 3)\n' "" "" "" ;;
@@ -216,7 +353,7 @@ audit_ns() {
          printf '%-24s %-14s %-8s   PV vintages OVERLAP => no candidate. An interrupted Step 2 re-bind looks exactly like this (both generations on original PVs). Finish Step 2 if one is in progress; otherwise escalate. Do NOT run Step 2a.\n' "" "" "" ;;
      esac
    else
-      printf '%-24s %-14s %-8s   a generation has no bound PVs (Pending claims, or StatefulSets only) => direction cannot be established from PV ages. Resolve the Pending claims or classify by hand.\n' "" "" ""
+      printf '%-24s %-14s %-8s   direction cannot be established from PV ages: a generation has no bound PVs (Pending/unbound claims, or StatefulSets only) or a bound PV age could not be read. Resolve those claims or classify by hand. Do NOT run Step 2a.\n' "" "" ""
    fi
    printf '%-24s %-14s %-8s   CANDIDATE ONLY: "original" does not mean the other set is EMPTY. A duplicate that\n' "" "" ""
    printf '%-24s %-14s %-8s   served writes and later crashed looks identical here. Verify emptiness before deleting.\n' "" "" ""
@@ -227,9 +364,10 @@ main() {
  printf '%-24s %-14s %-8s %s\n' NAMESPACE RELEASE CLASS NOTE
  printf '%-24s %-14s %-8s %s\n' --------- ------- ----- ----
  if [ "$#" -gt 0 ]; then
-    for ns in "$@"; do audit_ns "$ns"; done
+    for ns in "$@"; do audit_ns "$ns" || exit $?; done
  else
-    for ns in $(kubectl get ns -o name 2>/dev/null | sed 's|namespace/||'); do audit_ns "$ns"; done
+    _ns_list=$(run_kubectl 'list namespaces (whole-cluster mode)' get ns -o name) || exit $?
+    for ns in $(printf '%s\n' "$_ns_list" | sed 's|namespace/||'); do audit_ns "$ns" || exit $?; done
  fi
 }