feat(operator): auto-expand Ledger v3 PVCs

[flemzord]

Summary

• add a strictly opt-in autoExpansion policy to WAL and data volume specs
• add a separate VolumeExpansionReconciler that measures each replica through ledgerctl cluster disk-usage --json, with no Prometheus server dependency
• converge replica PVC requests, honor resize/cooldown state, cap growth at maximumSize, and recover from partial patches without in-memory state
• expose per-volume collection validity and freshness, and reject failed, zero-capacity, missing-timestamp, or older-than-one-minute measurements
• expose Kubernetes events and Operator metrics, regenerate CRD/RBAC/chart artifacts, and document EBS CSI prerequisites and diagnostics

Safety properties

• never shrinks a PVC or exceeds maximumSize
• never changes Cluster.spec.persistence.*.size, StatefulSet volume claim templates, or PVs
• rejects hostPath and cold-cache policies and validates expandable StorageClasses through an uncached API reader
• reads PVCs through the uncached API reader, matching the intentionally watch-free PVC RBAC contract
• preserves last successful disk bytes only for diagnostics after Statfs failure; invalid or stale values cannot trigger expansion
• evaluates WAL and data validity independently, so one failed volume does not discard a valid sample for the selected volume
• derives required health peers from the committed Raft configuration, not the gRPC connection pool
• prevents invalid, stale, missing-pool, or unreachable health samples from clearing an existing disk write gate
• does not treat an incomplete below-threshold measurement as healthy; a confirmed threshold crossing can still proceed while emitting a warning
• leaves existing installations unchanged unless autoExpansion.enabled is explicitly set

Validation

• bash scripts/agent-check — pass on the final tree
• targeted health, node, bootstrap, disk collector, gRPC, CLI, and Operator tests — pass, including race detection where applicable
• canonical PR normalization, generation, root/operator lint, build, and product packages under go test -race ./... — pass
• the canonical local gate still encounters the unchanged fixed-200ms scripts/internal/testenv/TestRunSynchronizedTimeoutTerminatesAndReportsEveryPeer load flake; the test passes 10/10 in isolation under -race
• GitHub CI at eda02a574 — all checks pass, including unit, Operator, model, scenario, Schemathesis, Antithesis workload, business/cluster E2E, coverage aggregation, and PR image build
• Codecov patch coverage — 93.71% (80% target); project coverage — 77.26% (+0.12% versus release/v3.0)
• NumaryBot — no findings; all review threads resolved
• Chainsaw volume-auto-expansion-contract scenario against Kind

The EKS staging canary remains an operational follow-up because no staging target was available in this workspace.

Jira: EN-1749
Parent: EN-867

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e7b1cd2e-d8f6-41cc-be63-6f3612c76dc1

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[NumaryBot]

✅ Approve — automated review

The active prior health-gate finding is fixed: a new leader now opens from fresh local evidence despite unavailable committed peers. No additional actionable defects were found in the current diff.

No findings.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1674 (comment)

[codecov]

Codecov Report

❌ Patch coverage is 95.67308% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.37%. Comparing base (deaa0a5) to head (3e3274a).

Files with missing lines	Patch %	Lines
internal/infra/health/healthcheck.go	94.30%	4 Missing and 3 partials ⚠️
internal/infra/monitoring/diskusage/diskusage.go	95.23%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1674      +/-   ##
================================================
+ Coverage         77.13%   77.37%   +0.23%     
================================================
  Files               478      478              
  Lines             51226    51380     +154     
================================================
+ Hits              39514    39755     +241     
+ Misses             8288     8192      -96     
- Partials           3424     3433       +9     

Flag	Coverage Δ
e2e	77.37% <95.67%> (+0.23%)	⬆️
scenario	77.37% <95.67%> (+0.23%)	⬆️
unit	77.37% <95.67%> (+0.23%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #1674 (comment)

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #1674 (comment)

[gfyrag]

Finding majeur : VolumeExpansionReconciler peut patcher des PVC pour une spec hostPath + autoExpansion

VolumeExpansionReconciler ne réutilise pas la validation qui rejette hostPath + autoExpansion. enabledVolumeExpansionDefinitions ne vérifie que AutoExpansion.Enabled, alors que le rejet hostPath vit uniquement dans validateVolumeSpec côté ClusterReconciler.

Scénario problématique : un cluster existant en PVC reçoit une update invalide ajoutant hostPath + autoExpansion. ClusterReconciler parque bien la CR en erreur, mais l’ancien StatefulSet/PVC reste actif, et le reconciler d’expansion peut encore charger, mesurer puis patcher ces PVC. Ça contredit la propriété annoncée “rejects hostPath policies”.

Suggestion : filtrer les volumes hostPath dans enabledVolumeExpansionDefinitions, ou réutiliser la même validation avant toute mesure/patch dans VolumeExpansionReconciler. Il faudrait aussi ajouter un test de régression où des PVC existants restent inchangés et où ReadDiskUsage n’est pas appelé quand la spec combine hostPath et autoExpansion.

[gfyrag]

Note d’architecture : deux reconcilers sur le même CRD

Le fait d’avoir plusieurs reconcilers qui prennent Cluster comme ressource primaire n’est pas interdit en soi, mais c’est un pattern à manier avec prudence. Il peut être acceptable ici parce que les responsabilités semblent séparées : ClusterReconciler gère le workload, les ressources déclaratives et le statut du Cluster, tandis que VolumeExpansionReconciler ne devrait piloter que l’état opérationnel des PVC live.

La contrepartie, c’est que les frontières d’ownership doivent être très nettes et que les deux reconcilers doivent partager les mêmes garde-fous de contrat. Sinon ils peuvent interpréter différemment la même spec. C’est exactement le risque visible ici : ClusterReconciler considère hostPath + autoExpansion invalide, mais VolumeExpansionReconciler peut encore agir parce qu’il ne réutilise pas cette validation.

Je ne pense donc pas que le split en deux reconcilers soit forcément un problème à lui seul, mais il impose au minimum ces invariants :

• ClusterReconciler reste le seul writer de Cluster.status et du StatefulSet/template.
• VolumeExpansionReconciler reste le seul writer des requests PVC liées à l’auto-expansion.
• toute validation liée à autoExpansion est factorisée et réutilisée par les deux chemins avant toute action externe.
• un reconciler secondaire ne doit jamais agir sur une spec que le reconciler principal parque comme invalide.

Dans ce cadre, la correction du bug hostPath + autoExpansion devrait être vue comme une correction de contrat entre reconcilers, pas seulement comme un guard local.

[shipfox-ai]

Summary

I verified both independent reviews against diff.patch and the code at HEAD (2f2c9fb2, base d387aab8). The feature is well factored — the pure decision core in volume_expansion_policy.go is genuinely side-effect free, convergence is correctly evaluated before cooldown, the above-maximumSize guard prevents an external PVC edit from propagating, PVC objects are DeepCopy-ed before mutation, AutoExpansion is correctly excluded from computeSpecHash, and the ledgerctl cluster disk-usage --json contract matches protojson output (walVolume/dataVolume, fixed64-as-string) as verified in cmd/ledgerctl/cmdutil/output.go and misc/proto/cluster.proto:186-190. However, one confirmed defect makes the feature non-functional on a real cluster: PVCs are now read through the manager's cached client while the generated RBAC still grants no watch on persistentvolumeclaims, and no test can catch it (fake client and envtest do not enforce RBAC). Together with the blast radius of policy validation failures, my recommendation is request changes. Most other candidate findings from the two reports did not survive verification and are listed as rejected at the end.

Standards

1. persistenceVolumeDefinitions duplicates the existing volume table (DRY, AGENTS.md "prefer existing repository patterns and DRY solutions over parallel abstractions")

misc/operator/internal/controller/volume_expansion_policy.go:39-63 re-declares the table that already exists in misc/operator/internal/controller/reconcile_statefulset.go:662-673 — same names, same &ledger.Spec.Persistence.* pointers, same default sizes (wal 5Gi, data 10Gi, cold-cache 10Gi). This is not cosmetic: the new validateVolumeSpec check maximumSize > initial size (cluster_controller.go:700-706) resolves the initial size from definition.DefaultSize, so if buildVolumeClaimTemplates' dflt is ever changed without the new table, admission validation will compare maximumSize against a size the StatefulSet never requests. buildVolumeClaimTemplates (and ideally pvcVolumeNames, reconcile_statefulset.go:734) should consume persistenceVolumeDefinitions so one table stays authoritative.

Minor, same axis: volume_expansion.go:317 is now the third literal copy of the StatefulSet PVC-name format (pv_protection.go:56, raft_scaledown.go:510); it belongs next to resourceName/podName.

2. Contract-unreachable branch silently returns success (AGENTS.md invariant 7)

misc/operator/internal/controller/volume_expansion.go:184-190. reconcileVolume is only reached for definitions where AutoExpansion.Enabled is true, and validateVolumeSpec (called at volume_expansion.go:169-175) already invokes resolveVolumeExpansionPolicy (cluster_controller.go:701). The second resolveVolumeExpansionPolicy error branch therefore cannot fire, yet it silently return false, nil. AGENTS.md invariant 7 requires "a branch that is unreachable by contract must surface an invariant failure rather than silently return nil". It is also a verbatim copy of the event + volumeExpansionErrorsMetric.WithLabelValues("policy", …) block at :176-181. Either have validateVolumeSpec return the resolved policy (one call, one guard) or make this branch return an invariant error.

3. ledgerctl cluster disk-usage is exec'd once per volume kind for identical data

misc/operator/internal/controller/volume_expansion.go:375-390, called per definition from :212. The RPC payload already carries both WAL and data usage (podDiskUsage at :66-69), yet collectMeasurements is invoked once for wal and once for data, so a cluster with both policies enabled opens two SPDY execs per pod per interval, spawning two ledgerctl processes inside every ledger container and doubling the exec-failure surface (each failure emits VolumeExpansionMeasurementFailed and increments the error counter) for the same bytes. Measure once per pod per reconcile and fan the result out to the enabled kinds.

Spec

1. [blocking] PVCs are read through the cached client, but RBAC grants no watch — the controller cannot work on a real cluster

• Read path: misc/operator/internal/controller/volume_expansion.go:319 (r.Get on corev1.PersistentVolumeClaim), with Client: mgr.GetClient() wired at misc/operator/cmd/operator/main.go:107-115.
• Permissions: marker misc/operator/internal/controller/volume_expansion.go:93 and generated artifacts misc/operator/config/rbac/role.yaml:29-37 / misc/operator/helm/operator/templates/clusterrole.yaml:30-38 grant delete;get;list;patch. The only RBAC addition in this PR is storage.k8s.io/storageclasses: get.

With controller-runtime v0.20.4 (misc/operator/go.mod:19), the manager client serves structured reads from the cache, so this first-ever cached PVC read lazily starts a PVC informer that requires list and watch. Without watch the reflector is denied, the cache never syncs, and the Get in loadVolumePVCs blocks on cache sync / fails on every pass — the reconciler degrades to permanent VolumeExpansionPending and no expansion ever happens. Every existing PVC access in this operator deliberately goes through the typed clientset (pv_protection.go:50, raft_scaledown.go:506), which is why the missing verb never mattered before. No test detects this: the fake client and envtest do not enforce RBAC. Fix either by adding watch to the marker and regenerating both artifacts, or — preferably, to avoid caching every PVC in the watched namespaces — by reading PVCs through r.APIReader/Clientset as the rest of the operator does.

2. An invalid opt-in expansion policy freezes the whole Cluster, and most policy rules are not enforced at admission

misc/operator/internal/controller/cluster_controller.go:596-599 now routes autoExpansion through validateSpec, whose failure sets phase: Error and returns before any workload reconciliation (cluster_controller.go:96-107; the e2e fixture misc/operator/e2e/tests/volume-auto-expansion-contract/assert-invalid.yaml encodes exactly that). The CRD only enforces "maximumSize required when enabled" and "targetPercent < thresholdPercent" (misc/operator/api/v1alpha1/cluster_types.go:1165-1166); three rules are Go-only:

• cooldown >= 6h (volume_expansion_policy.go:18,105) has no marker on the field (cluster_types.go:1197-1201, generated cooldown property is a bare string with default: 8h), even though misc/operator/README.md states "The cooldown cannot be shorter than six hours";
• maximumSize > initial size (cluster_controller.go:700-706);
• wal/data-only, because AutoExpansion sits on the shared VolumeSpec (cluster_types.go:1144) and is therefore accepted on coldCache by all three CRD blocks (cluster_controller.go:693).

Net effect: cooldown: 1h or coldCache.autoExpansion on a running cluster is admitted and then halts StatefulSet, TLS and credential reconciliation for the whole Cluster. That is a far larger blast radius than the documented ownership split ("It is separate from ClusterReconciler… volume expansion owns only live PVC storage requests", docs/technical/architecture/subsystems/operator/volume-auto-expansion.md). Recommend adding field-level CEL/kubebuilder rules for the cooldown minimum and, for this strictly opt-in subsystem, degrading a bad policy to a condition/event on the volume instead of a Cluster-wide Error phase.

3. Usage is only measured on one decision path, so the documented 5-minute measurement and its gauge go stale

misc/operator/internal/controller/volume_expansion.go:208-213: measurements are collected only when the pre-check returns volumeExpansionDecisionIncomplete. During converge, pending, cooldown and above-maximum no exec runs, so ledger_operator_volume_usage_ratio keeps its last value for up to the full 8h cooldown. misc/operator/README.md:113-117 promises "every five minutes a separate controller executes ledgerctl cluster disk-usage --json inside each Ledger pod", and the same README points operators at that gauge as a troubleshooting surface. The skip is a deliberate, well-commented optimisation — but either the README should describe it, or the gauge should be refreshed/reset on those paths so operators are not reading an 8-hour-old ratio.

4. Convergence can start a cooldown that the design doc does not describe

misc/operator/internal/controller/volume_expansion.go:239 passes decision.LastExpansionAt to patchPVCGroup, and patchPVCGroup substitutes r.now() when that value is zero (:442-444). The doc's guarantee ("cooldown does not prevent convergence") holds — converge is evaluated before the cooldown gate in volume_expansion_policy.go:196-203 — but the converse is undocumented: converging a divergence that carries no last-expansion-at annotation (external PVC edit, stripped annotations, a StatefulSet recreated with a larger template) stamps now() on the group and defers a genuinely needed expansion by up to the cooldown. Either preserve "no annotation" through convergence or document that convergence arms the cooldown.

5. The documented EKS/CSI canary has not been run (acknowledged)

docs/technical/architecture/subsystems/operator/volume-auto-expansion.md ("EKS validation": three-replica cluster, converged requests, PVC status.capacity and in-pod df, pods Ready, no second request during cooldown). Current validation stops well short: misc/operator/internal/controller/volume_expansion_integration_test.go:19-78 proves only that envtest accepts a single PVC request patch at replicas := int32(1), volume_expansion_test.go uses the fake client, and the chainsaw scenario (misc/operator/e2e/tests/volume-auto-expansion-contract/chainsaw-test.yaml) asserts only rejection and default-off. Multi-replica convergence is covered by pure-policy unit tests only. The PR body already declares the canary an operational follow-up, so this is recorded as an accepted risk rather than a defect — but it should gate enabling the policy on any production cluster, since no test in this PR exercises a real external-resizer, filesystem growth, pod readiness or a real cooldown window.

Rejected candidate findings

For the record, these were raised by one of the two reviews and did not survive verification: (a) "the group patch is documented as atomic" — the doc's "One patch" refers to one patch per PVC changing request + both annotations, which is accurate, and partial-patch recovery is explicitly covered in its failure-modes list; (b) "missing partial-patch recovery test" — convergence is covered at the policy level (volume_expansion_policy_test.go:166, :214) and no concrete defect in patchPVCGroup was demonstrated; (c) "documentation maintenance gap" — docs/technical/architecture/README.md, a new subsystems/operator/README.md, the subsystem doc and misc/operator/README.md were all updated, misc/operator/CLAUDE.md:20-25 documents which fields are hashed and is not contradicted, and there is no operator entry in any agent-context routing table to update; (d) "decision-traceability doc missing" — a process request with no correctness impact; (e) metric-label naming drift ("converged" vs "converge"), the 8-arm switch, the switch volume string dispatch (which does fail loudly, volume_expansion.go:392-397), data clumps, kubebuilder-default/Go-constant duplication, and MaxConcurrentReconciles: 4 — style or judgement calls with no demonstrated correctness, security, compatibility or test-risk impact.

Reviewed independently by Claude (claude-opus-5) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by Claude.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1674 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #1674 (comment)

[shipfox-ai]

Summary

This is a carefully built change: the pure policy module (decideVolumeExpansion) is well separated from the Kubernetes surface, the PVC read path correctly uses the uncached APIReader, proto fields 3–6 are added sequentially with no reserved (per AGENTS.md), CRD/RBAC/helm/deepcopy artifacts are consistent, and the docs (misc/operator/README.md, docs/ops/disk-space.md, docs/ops/monitoring.md, docs/ops/cli.md, new operator subsystem docs) genuinely describe the new behaviour. Unit tests are parallel, use require.Eventually, and the envtest integration test does exercise the real expand path end-to-end (volume_expansion_integration_test.go:19), so the "no e2e coverage of expansion" concern raised in review is not accurate. The material problems are all in the health write-gate half of the change: the new "an invalid/unreachable sample must not clear an existing block" rule is keyed on ConnectionPool membership rather than cluster membership, which makes it both too weak (a member missing from the pool contributes no sample at all) and too strong (a member that stays in the pool while unreachable pins the gate closed indefinitely, with no bound or override — while a leader election silently discards the same missing evidence). Recommendation: request changes, scoped to Spec findings 1–3; everything else can be addressed as follow-up comments.

Standards

1. [Medium] The sample-usability contract is duplicated inside the root module.
internal/infra/monitoring/diskusage/diskusage.go:49 (VolumeSample.Usable) and internal/infra/health/healthcheck.go:373 (remoteVolumeValidity) independently re-implement the same four gates (valid → non-zero total → observation present → ≤ 1 minute old). The operator adds a third copy at misc/operator/internal/controller/volume_expansion.go:594 with its own constant maximumDiskUsageSampleAge = time.Minute (volume_expansion.go:34) shadowing diskusage.MaximumSampleAge. AGENTS.md: "Prefer existing repository patterns and DRY solutions over parallel abstractions." This predicate now gates both write-gate recovery and PVC mutation, so silent drift between copies produces conflicting safety decisions. The operator copy is defensible (separate go.mod), the two root-module copies are not; at minimum add a cross-contract test pinning identical boundary behaviour for local vs. remote samples.

2. [Low] Contract-unreachable failure is handled with a silent return false, nil.
misc/operator/internal/controller/volume_expansion.go:190 resolves the policy a second time, after validateVolumeSpec (volume_expansion.go:178 → cluster_controller.go:696) already resolved the identical spec and returned any error, and after enabledVolumeExpansionDefinitions guaranteed AutoExpansion.Enabled. That error branch cannot be reached by contract. AGENTS.md invariant 7: "a branch that is unreachable by contract must surface an invariant failure rather than silently return nil". The same file already follows this convention (volume_expansion.go:315, :400, :663). Resolve once and return the policy, or raise an invariant: error.

3. [Low] Two authoritative volume-definition tables with duplicated default sizes.
misc/operator/internal/controller/volume_expansion_policy.go:40-63 declares wal/5Gi, data/10Gi, cold-cache/10Gi, while buildVolumeClaimTemplates (reconcile_statefulset.go:662-673) repeats the same names and defaults. This is behaviourally coupled: cluster_controller.go:700-705 compares maximumSize against the new table's default, whereas the StatefulSet provisions the request from the old one. A one-sided default change makes validation disagree with deployed storage. Drive both from one definition source.

4. [Low] Dead accessors retained as delegates.
internal/infra/monitoring/diskusage/diskusage.go:85 and :88 (UsedBytes(), TotalBytes()) now only forward to Load(), and a repo-wide grep for .UsedBytes()/.TotalBytes() returns zero call sites — every consumer moved to Load(). Beyond being dead code, they expose last-known bytes without the validity/freshness guard this PR exists to enforce, which is exactly the mistake a future caller would make. Delete both.

Not retained: _, err = parseExpansionTarget(...) (volume_expansion.go:352) — the error is handled; AGENTS.md's rule concerns ignored errors, not discarded values. misc/operator/CLAUDE.md — its "ClusterConfig Reconciliation" section states which fields are hashed, not an exhaustive exclusion list, and the AutoExpansion exclusion is documented in hash.go:29-37 plus README. Remaining reported items (data clumps in logIfAtBlock/nodeUsageReport, "wal"/"data" string kinds, 636-line file, 5-line duplicated failure branch) are design/style preferences without a demonstrated correctness impact.

Spec

1. [High] A peer that stays in the pool while unreachable pins the disk write gate closed with no bound and no override.
internal/infra/health/healthcheck.go:263 appends VolumeSample{} on every failed GetDiskUsage, and thresholds.go:40-50 requires WALValid && DataValid on every sample for allBelowResume. In strict-TLS and plaintext modes nothing ever removes a peer entry (monitorPeer/RestartConnection only run in optional mode), so a node that is down or being replaced makes recovery impossible: once diskBlocked is true it can never clear on that leader, even after space is freed or the PVC is expanded. The realistic incident is exactly the one this feature targets — disk fills, writes block, one replica is down during remediation, writes stay blocked (HTTP 429) indefinitely. Worse, the safety property is inconsistent: because the gate is node-local and followers store an empty gateState (healthcheck.go:151-158), a single leader election resets prevDiskBlocked to false and anyAtBlock then ignores the invalid sample entirely — so an election achieves precisely what the invalid sample was meant to prevent. Please bound this (e.g. treat a peer unreachable beyond N cycles as non-evidence, or require a quorum of fresh samples) and document the chosen trade-off.

2. [Medium] The stated invariant is not actually achieved: a member absent from the connection pool contributes no sample at all.
The evidence set is hc.servicePool.PeerIDs() (healthcheck.go:219), i.e. pool membership, not cluster membership. RestartConnection deletes the peer entry when the re-dial/re-probe fails (internal/infra/transport/connection_pool.go:311), and an initial pool.AddPeer failure only logs and continues (internal/infra/membership/membership.go:153-158). Subsequent cycles then omit that expected member entirely, so fresh below-resume samples from the surviving nodes satisfy allBelowResume and clear an existing block without its evidence — the exact case the change claims to prevent. Relatedly, the new conn == nil branch (healthcheck.go:225-236) is close to dead code: GetConnection and PeerIDs read the same map under the same lock, so it fires only in the narrow removal race, never for a down peer. Derive the expected node set from Raft membership (or keep failed peers in the pool) and add a regression covering remove-from-pool followed by a blocked→recovery cycle.

3. [Medium] A permanently failing local Statfs silently disables the local disk block, with no metric to notice.
thresholds.go:27-33 now requires WALValid/DataValid for anyAtBlock, and logIfAtBlock (healthcheck.go:420) gates its loud warning and antithesis assert on the same flags. Previously the last-known non-zero totals could still trip a block. Meanwhile health.disk.poll.failures is incremented only for peers (healthcheck.go:234, :255), and storage.disk.volume.bytes stops being emitted for the failing volume (diskusage.go:191-196), so a node whose collector is broken produces no gauge, no counter and no warning — only an INFO line with wal_valid=false. The fail-open half of the new contract is unobservable, which is what makes finding 1 hard to diagnose in production. Add a collection-failure counter or a per-volume validity gauge (the dashboards in misc/devenv/monitoring-dashboards/jsonnet/sections/storage_disk.libsonnet:11-12 only plot the suppressed series).

4. [Low-Medium] New runtime-only policy validation halts all reconciliation for the Cluster.
cluster_controller.go:704 (maximumSize must be strictly greater than the initial size) and volume_expansion_policy.go:105 (cooldown ≥ 6h) are enforced only in validateSpec, not as CRD CEL rules (api/v1alpha1/cluster_types.go:1165-1166 covers only "maximumSize required" and "targetPercent < thresholdPercent"). The API server therefore accepts cooldown: 1h or maximumSize == size, and Reconcile then sets ConfigValid=False, phase=Error and returns without requeue (cluster_controller.go:96-107) — freezing StatefulSet, Service, Ingress and credentials reconciliation for the whole Cluster until the spec is edited, over an opt-in storage knob. Mirror both rules as CEL so rejection happens at admission. Note also that maximumSize > size (which makes maximumSize: 100Gi with size: 100Gi a hard failure) is stricter than the documented "a hard maximumSize is required" and is not mentioned in misc/operator/README.md:145-147.

5. [Low-Medium] PVC patch has no optimistic lock, so a stale target can lose a concurrent request increase.
PVCs are read at volume_expansion.go:340, decisions are computed, then applied with client.MergeFrom(base) at volume_expansion.go:475; the current.Value() >= targetBytes guard at :461 uses the stale read. If an operator or another controller raises a request between load and patch (100Gi→180Gi while this reconciler computed 146Gi), the merge patch carries no resourceVersion precondition. In the common case the API server rejects the reduction and convergence is merely disrupted; with expansion-failure recovery enabled a reduction above current capacity can be accepted, which contradicts "never shrinks a PVC". Use client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}) and retry from a fresh APIReader read; add a test with a concurrent increase between load and patch.

6. [Low] ledger_operator_volume_usage_ratio is only refreshed on the measuring path and never cleaned up.
volume_expansion.go:215-227: collectMeasurements runs only when the first pure decision is Incomplete, so the Converge, Pending, Cooldown, AboveMax, Limit and Expand paths all return without re-measuring. During the cooldown window (default 8h) the gauge advertised as a troubleshooting surface in misc/operator/README.md:180-184 holds a stale value — precisely when operators are watching it. Series for deleted pods and Clusters are also never deleted (no Delete/DeletePartialMatch call), so label sets accumulate for the operator's lifetime.

Not retained: the Chainsaw scenario limitation (e2e/tests/volume-auto-expansion-contract/chainsaw-test.yaml) — the expand/converge/annotation path is covered against a real API server in volume_expansion_integration_test.go:19, and the PR explicitly declares the EKS/CSI canary an operational follow-up. Gauge suppression on invalid samples is documented (docs/ops/monitoring.md, docs/ops/disk-space.md:158) and a series gap is the correct semantic for "no measurement". The parsePodDiskUsage overflow coupling (volume_expansion.go:566-571) requires observedAtUs > 2^63, which is not reachable in practice.

Reviewed independently by Claude (claude-opus-5) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by Claude.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1674 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #1674 (comment)

[shipfox-ai]

Summary

This PR adds a self-contained VolumeExpansionReconciler to the operator plus a validity/freshness contract for disk-usage samples (collector → gRPC → ledgerctl → health gate → operator). The design is sound: the policy is a pure function with good unit coverage, PVC reads go through the uncached APIReader (matching the get/list/patch-only RBAC), the auto-expansion policy is correctly excluded from the pod spec hash so tuning it cannot roll the StatefulSet, MemberIDs() correctly makes Raft membership (including joint-consensus outgoing voters and learners) authoritative for peer polling, and documentation was updated in the places AGENTS.md requires. I verified every retained candidate finding against diff.patch and the code, and dropped several claims from both reports as unfounded (the docs/ops/cli.md "Storage Components" deletion is a correct stale-doc fix — displayDiskUsage has no such section; the testing.md operator test table was already stale before this PR; the uint64 threshold arithmetic only overflows above ~184 PB per filesystem; MemberIDs() is central to the stated goal, not scope creep; last-expansion-target is documented as bookkeeping, not decision state). What remains is one behaviour/documentation contradiction in the write gate, one flaky-by-construction integration test, and a set of smaller robustness/DRY issues. Recommendation: approve with comments — please address Spec #1 (fix or qualify the new guarantee) and Standards #1 (test determinism) before merge; the rest can follow up.

Standards

1. [P2] The new envtest integration test is flaky by construction and bypasses the documented helper — misc/operator/internal/controller/volume_expansion_integration_test.go:68

require.Eventually(..., 15*time.Second, 250*time.Millisecond) is used where docs/technical/contributing/testing.md:758 mandates requireEventually(t, condition, msg) (defined at suite_test.go:206). More importantly, the timing does not hold: VolumeExpansionReconciler.SetupWithManager watches only Cluster (volume_expansion.go:437-443) — deliberately no PVC informer, since RBAC grants no PVC watch — and the failure path when the PVC does not yet exist requeues after volumeExpansionRetryInterval = time.Minute (volume_expansion.go:32, volume_expansion.go:127). The test creates the Cluster first and the PVC afterwards, so the first reconcile passes necessarily fail; the assertion then only succeeds if some incidental Cluster status write from ClusterReconciler happens to arrive after the PVC is created and marked Bound. Nothing in the test guarantees that ordering, and the 15s window is shorter than the 1-minute retry. Create the PVC (bound, with status capacity) before the Cluster, or trigger a reconcile explicitly (e.g. annotate the Cluster after binding the PVC), and switch to requireEventually.

2. [P2] Two parallel authoritative volume tables with duplicated default sizes — misc/operator/internal/controller/volume_expansion_policy.go:40-63 vs misc/operator/internal/controller/reconcile_statefulset.go:662-673

The PR introduces persistenceVolumeDefinitions (wal/5Gi, data/10Gi, cold-cache/10Gi) while buildVolumeClaimTemplates keeps its own vctDef table with the same names, spec pointers, and literal defaults. validateVolumeSpec (cluster_controller.go:694-707) compares maximumSize against the default from the new table, while the PVC actually provisioned comes from the old one. A one-sided edit silently desynchronises validation from provisioning. AGENTS.md:70 ("prefer existing repository patterns and DRY solutions over parallel abstractions") applies; make buildVolumeClaimTemplates consume persistenceVolumeDefinitions.

3. [P3] The sample validity/freshness rule now exists in three hand-written copies — internal/infra/monitoring/diskusage/diskusage.go:49-55, internal/infra/health/healthcheck.go:380-401, misc/operator/internal/controller/volume_expansion.go:594-613

VolumeSample.Usable, remoteVolumeValidity, and validateMeasuredVolume each re-implement "valid ∧ total > 0 ∧ observedAt ≠ 0 ∧ age ≤ 1min", with two independent constants (diskusage.MaximumSampleAge and maximumDiskUsageSampleAge = time.Minute at volume_expansion.go:34). These predicates gate write-gate recovery and PVC growth, so drift produces different safety verdicts for local vs. remote vs. operator paths. The operator copy is unavoidable (separate Go module — please cross-reference the contract in a comment), but healthcheck.go and diskusage.go are in the same module and should share one helper (e.g. convert the gRPC response into a diskusage.VolumeSample and reuse Usable, keeping diagnostic-string generation separate).

4. [P3] Branches that are unreachable by contract silently succeed instead of failing loudly (AGENTS.md:43, invariant 7)

• misc/operator/internal/controller/volume_expansion.go:190-195: validateVolumeSpec (line 178) already calls resolveVolumeExpansionPolicy on the same immutable spec and propagates its error; the second resolution's error branch cannot fire, yet it returns false, nil.
• misc/operator/internal/controller/volume_expansion.go:362-365: validateExpandableStorageClasses falls back to the cached r.Client when APIReader == nil, but loadVolumePVCs:330-332 already fails the invariant in that case — and RBAC grants storageclasses only get (config/rbac/role.yaml), so a cached read could never work anyway. Drop the fallback.
• misc/operator/internal/controller/volume_expansion_policy.go:259-263: targetBytes <= decision.LargestRequestBytes is unreachable (at that point LargestRequestBytes < maximumBytes and targetBytes >= LargestRequestBytes + MinimumIncrement), so its TargetBytes = maximumBytes is dead.

5. [P3] Dead public API left behind by the peer-discovery switch — internal/infra/transport/connection_pool.go:403

ConnectionPool.PeerIDs() now has no production caller (only connection_pool_test.go) after healthcheck.go:216 moved to hc.node.MemberIDs(). Likewise diskusage.VolumeUsage.UsedBytes()/TotalBytes() (diskusage.go:85-88) are only referenced from diskusage_test.go now that all callers use Load(). Remove them (and the tests that exist only to exercise them) or document why they are kept.

Spec

1. [P1] The newly documented recovery guarantee does not hold across a leadership change — internal/infra/health/healthcheck.go:154-161, internal/infra/health/thresholds.go:63-69

The PR's stated property ("prevents invalid, stale, or unreachable health samples from clearing an existing disk write gate") is now written into docs/ops/cli.md:4323-4329 ("recovery requires fresh samples for all volumes on every member in the committed Raft configuration") and docs/ops/disk-space.md:146-152. But a node that loses leadership unconditionally publishes an unblocked gate (hc.gate.Store(&gateState{}), line 161), and the new leader starts with prev == false, where NextDiskBlocked returns t.anyAtBlock(samples) — invalid/unreachable member samples (VolumeSample{}) can never raise a block. Concretely: WAL fills on node C → leader A blocks writes → A fails over to B while C is unreachable/crash-looping → B's first check sees its own healthy volume plus an invalid sample for C → writes are admitted with zero fresh evidence about C, and stay admitted indefinitely. The - **Leadership change** bullet at docs/ops/disk-space.md:154 still says the state "defaults to healthy", which directly contradicts the paragraph eight lines above it. The reset itself pre-dates this PR, but this PR is what asserts the opposite guarantee, so please either carry the previous verdict into the new leader's first decision (or require a complete fresh sample set before publishing an unblocked verdict) or explicitly scope the documented property to a single leader term.

2. [P2] ledger_operator_volume_usage_ratio is stale for hours and its series are never retired — misc/operator/internal/controller/volume_expansion.go:215-227, misc/operator/internal/controller/volume_expansion.go:39-56

Measurements are collected only when the first policy decision is measurement-incomplete (line 216-219). During converge, pending, above-maximum and the default 8-hour cooldown, no pod exec happens, so the gauge keeps its last written value for the whole cooldown with no freshness signal. This contradicts misc/operator/README.md:115 ("every five minutes a separate controller executes ledgerctl cluster disk-usage --json inside each Ledger pod") and docs/technical/architecture/subsystems/operator/volume-auto-expansion.md ("On every enabled Cluster reconcile interval"). Separately, all four Vecs are labelled by namespace/cluster(/pod) and nothing ever calls Delete/DeletePartialMatch/Reset, so gauges keep publishing last-known ratios and requested bytes for deleted Clusters and scaled-down replicas for the operator's process lifetime. Either measure on every interval, or narrow the docs and expose sample freshness plus delete series on Cluster deletion / replica removal.

3. [P2] Cooldown and group validity depend on unvalidated, externally editable PVC annotations — misc/operator/internal/controller/volume_expansion_policy.go:213, misc/operator/internal/controller/volume_expansion.go:335-345

decision.LastExpansionAt is the max ledger.formance.com/last-expansion-at across the group and is never checked against now, so now.Before(LastExpansionAt.Add(policy.Cooldown)) suppresses expansion for as long as that timestamp stays in the future — a hand-edited or clock-skew-written value can disable the whole safety mechanism, and the cooldown branch (volume_expansion.go:243-247) emits only an info log and a counter, no Kubernetes event, so the suppression is invisible to operators. Symmetrically, an unparseable last-expansion-target — a write-only bookkeeping annotation that feeds no decision (_, err = parseExpansionTarget(...), line 342) — hard-fails loadVolumePVCs and blocks expansion for the entire volume group behind a generic VolumeExpansionPending warning. Clamp LastExpansionAt to <= now (and warn when clamping), and treat the target annotation as diagnostic (log and ignore a bad value) rather than fatal.

---

Reviewed independently by Claude (claude-opus-5) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by Claude.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1674 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).

Summary: #1674 (comment)

[flemzord]

Addressed the current review feedback in 5b98400 and 3e3274a:

• fenced disk-gate verdicts by leadership epoch, require fresh local evidence before reopening, and preserve quorum availability when a remote committed member is unavailable;
• made the envtest expansion trigger deterministic and added optimistic locking for PVC patches;
• reject future cooldown timestamps as decision evidence, keep the target annotation diagnostic, and expose/retire freshness-aware operator metrics;
• consolidated the volume and sample-validity contracts, removed unreachable branches and dead accessors, and added overflow-safe target arithmetic.

Validation is green on the current head: all GitHub checks pass, including unit/race, Operator, E2E Business, E2E Cluster, model, scenarios, Schemathesis, Antithesis workload, coverage, and PR image build. NumaryBot re-reviewed 3e3274a with no findings.

[shipfox-ai]

Both independent reviews (Claude and Codex) were re-verified line by line against diff.patch and the checked-out tree; several of their claims did not survive that check and were dropped (see "Rejected claims"). What remains is a well-structured, well-documented change — the operator-side expansion policy is a genuinely pure decision function with good unit/envtest coverage, PVC/StorageClass reads correctly bypass the manager cache, the policy is default-off and excluded from the pod-template hash, and the disk-evidence hardening in internal/infra/health is coherent and documented. My concern is not correctness of the happy path but the availability envelope of the new fail-closed write gate: two paths can hold WRITES_BLOCKED_DISK_FULL for an unbounded time on a cluster whose disks are fine, and one operator-side validation change can park an entire live Cluster reconcile from a PVC-only knob that the CRD happily admits. Recommendation: request changes — narrowly for Spec 1 and Spec 4 (both small, local fixes, or an explicit written acceptance); everything else can be follow-up.

Standards

1. Cross-module duplication of the sample-freshness contract (drift risk). internal/infra/monitoring/diskusage/diskusage.go:20 (MaximumSampleAge = time.Minute) + :67-71 (SampleUsable) and misc/operator/internal/controller/volume_expansion.go:35 (maximumDiskUsageSampleAge = time.Minute) + :608-630 (validateMeasuredVolume, whose comment states "This mirrors diskusage.SampleUsable in the root module") establish the same safety predicate twice for two control loops that must decide freshness identically. The nested Go module explains it but does not remove the drift risk: changing MaximumSampleAge silently leaves the operator at one minute. AGENTS.md ("prefer existing repository patterns and DRY solutions over parallel abstractions") applies. A shared/generated constant, or at minimum a test asserting the two limits are equal, would pin it. Secondary, same topic: internal/infra/health/healthcheck.go:441-470 re-checks valid, capacity and observation inline and then passes the same three booleans into diskusage.SampleUsable, so those gates have two authorities; only the age gate is genuinely shared.

2. CRD defaults duplicate the Go defaults, and the minimum cooldown has no marker. misc/operator/api/v1alpha1/cluster_types.go:1174,1181,1188,1199 (+kubebuilder:default=70/55/"10Gi"/"8h") restate defaultVolumeExpansionThresholdPercent/TargetPercent/MinimumIncrement/Cooldown in misc/operator/internal/controller/volume_expansion_policy.go:15-19. A one-sided edit diverges silently between admission-time defaulting and in-process defaulting (the latter is what in-process/unit-constructed Cluster objects get). Related: minimumVolumeExpansionCooldown = 6 * time.Hour (volume_expansion_policy.go:18) has no corresponding CRD rule — see Spec 4.

3. Documentation routing table not updated for the new subsystem. docs/technical/architecture/subsystems/operator/volume-auto-expansion.md and .../operator/README.md are new, and docs/technical/architecture/README.md correctly gained an operator row, but docs/technical/agent-context.md:23-45 still has no misc/operator/** entry. AGENTS.md "How to load context" step 3 routes agents through exactly that table, so future work in misc/operator will not be pointed at the new subsystem doc.

Spec

1. A persistent local Statfs failure blocks writes indefinitely as WRITES_BLOCKED_DISK_FULL. internal/infra/health/healthcheck.go:367-374: with no current-epoch verdict and !localWalValid || !localDataValid, check() returns without publishing anything, and CheckWritesAllowed (healthcheck.go:139-143) then rejects every write forever. The local sample goes invalid 60s after any sustained Statfs error (diskusage.go:81-86, MaximumSampleAge), so a wrong mount, an unmounted WAL path, or EIO on the data dir converts a monitoring-layer fault into a total, unbounded write outage on a cluster at 5% utilization — reported to clients with a code that says the disk is full. The added documentation (docs/ops/disk-space.md "Failure Modes") justifies only the transient post-election case, not the unbounded one. Please distinguish "verdict unknown" from "disk full" (separate error/verdict plus a metric), or bound the fail-closed state for local-evidence failures.

2. An existing disk block cannot be cleared while any committed member is unreachable — and a leader election clears it anyway. For a member with no pool connection, healthcheck.go:272-282 appends a zero-valued VolumeSample{}, and Thresholds.allBelowResume (internal/infra/health/thresholds.go:41-46) refuses recovery unless every sample is valid. Membership now comes from the committed ConfState including learners (internal/infra/node/node.go:2031-2058), so freeing disk space does not resume writes while any member — including a learner just added during scale-up — fails to report, even with a healthy quorum. That part is documented as intentional; the asymmetry is not: healthcheck.go:377-384 publishes a fresh verdict with anyAtBlock (invalid samples ignored) whenever there is no current-epoch verdict, so a leadership transfer clears the very block that "invalid evidence must not clear". The safety property is therefore escapable while the availability cost is real. Consider an explicit, observable operator escape hatch (and a metric for "recovery blocked by member N") rather than relying on a failover.

3. The post-election blocking window is bounded by a sequential peer poll, not by leadership readiness. The verdict is published only at the end of check(), after iterating members serially with GetDiskUsage and GetNodeTime, each with healthCheckCallTimeout = 5s (healthcheck.go:26, loop :260-350). Each member whose pool entry exists but hangs costs up to 10s, so with k such members the first current-epoch verdict — and therefore write availability — is delayed ~10s×k; if the OnLeaderReady fast path is missed, the fallback is the 30s --health-check-interval tick. tests/e2e/cluster/transfer_leadership_test.go:110-117,189-196 now retries post-election writes inside a 5s Eventually budget (tests/e2e/cluster/suite_test.go:27), which is smaller than the worst-case window — a real CI flake risk, and a user-visible 429 window after every failover. Publishing the local verdict independently of the peer poll, or polling peers concurrently, would remove both.

4. The CRD admits expansion policies that only Go rejects, and that rejection parks the whole Cluster. misc/operator/config/crd/bases/ledger.formance.com_clusters.yaml:2672 exposes autoExpansion under coldCache, and :2677-2679 gives cooldown a default of 8h with no minimum; the only CEL rules are maximumSize required and targetPercent < thresholdPercent (:2723-2729). Meanwhile volume_expansion_policy.go:61,138 reject cold-cache and :105 rejects cooldown < 6h. Those errors now flow through validateVolumeSpec into validateSpec, and misc/operator/internal/controller/cluster_controller.go:95-107 sets Phase = "Error" and returns before StatefulSet, TLS and credential reconciliation. So coldCache.autoExpansion: {enabled: true} or cooldown: 1h is accepted by the API server and then halts all reconciliation of a running cluster — from a knob deliberately excluded from the pod-template hash (hash.go:37-39) precisely because it must not affect workloads. Please add the missing CEL rules (drop autoExpansion from the cold-cache schema; duration(self.cooldown) >= duration('6h')) and/or degrade expansion-policy errors to a status condition + event instead of parking the reconcile.

5. Unchecked uint64 multiplication in the expansion trigger. misc/operator/internal/controller/volume_expansion_policy.go:280 evaluates measurement.UsedBytes*100 >= measurement.TotalBytes*uint64(policy.ThresholdPercent) while the neighbouring target computation deliberately uses the overflow-safe ceilMultiplyDivideUint64 (:305,315-328). The operands come straight from a pod's ledgerctl cluster disk-usage --json output via parsePodDiskUsage, which range-checks only observedAtUs and sampleAgeMs (volume_expansion.go:578-590). A bogus or corrupted report above ~184 PiB wraps and can flip the verdict in either direction (suppressing a needed expansion, or triggering one bounded by maximumSize). Realistic capacities never reach that, so this is hardening rather than a live defect, but the fix is one line plus a near-limit test.

Rejected claims

• "Deleting PeerIDs() gutted the AddPeer idempotency regression test" (connection_pool_test.go) — not supported: ConnectionPool.peers is a map[uint64]*peerEntry (connection_pool.go:186-217), so require.Len(ids, 1) was always true and never distinguished the no-op branch; Close() clears the map (:423-436), so the substituted GetConnection assertions cover the same branch, and TestConnectionPool_MultiplePeers still asserts every address individually. No regression trigger was lost. (A genuinely stronger idempotency assertion would compare GetConnection(1) pointer identity across the two AddPeer calls — worth doing, but not a standards violation.)
• "volume_expansion.go (665 lines) is a Divergent Change / should be split" — docs/technical/contributing/conventions.md:20-22 explicitly requires a struct's methods to stay in the file that defines the struct and prefers sub-type extraction over splitting; no correctness impact, so this is style.
• "patchPVCGroup arms a fresh cooldown when LastExpansionAt is zero" — real but immaterial: the normal partial-patch path stamps ledger.formance.com/last-expansion-at on the same patch as the size (volume_expansion.go:497-512), so a converge always sees the original timestamp; the substitution only bites if the annotation is stripped externally, and the converge itself just applied the capacity.

Reviewed independently by Claude (claude-opus-5) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by Claude.