feat(operator): cut stuck reconciles short and retry the rollouts they interrupt

[BryanFRD]

Part of #241. Bottom of stack #275. This now carries what was #272, folded in so the reconcile timeout can never land without the rollout retry it depends on. #272's branch was a fast-forward of this one, so nothing was rewritten.

1. A reconcile can no longer hold the queue forever

On 2026-08-25 one Get of a Deployment blocked forever, and the operator stopped reconciling every FerrVaultSecret for four hours. #240 removed that cause. This removes the class.

• --reconcile-timeout (default 2m, chart value reconcileTimeout) feeds controller-runtime's ReconciliationTimeout, which was unset, meaning no deadline at all.
• It helps only if the blocking call honours its context. The call that hung was a cached Get waiting on an informer, and in controller-runtime 0.25.1 that wait is cache.WaitForCacheSync(ctx.Done(), …), so it does.
• The operator refuses to start with a timeout below RetryPolicy.LongestCall(RequestTimeout), the longest one FerrVault API call can legitimately take: three 10s attempts plus 100ms and 400ms backoff at +25% jitter, about 30.6s. That rejects 0, which in controller-runtime means no timeout, and any value that would cancel healthy reveals.

2. A failed rollout is retried instead of forgotten

rolloutRestart failures were logged and then lost. The next pass saw the target Secret already holding the new content, decided nothing had changed, and never restarted anything, while pods kept the old values under Ready=True. The timeout above would turn a hang into exactly that failure, which is why the two ship together.

• status.lastRolloutHash records the content the workloads were last restarted for, and a restart is due while it differs from the current content. It is declared in the CRD schema, which does not preserve unknown fields: without that the API server would silently prune it.
• Each workload is restarted at most once per content change. The pod template is stamped with ferrvault.com/content-hash.<resource UID> and skipped when it already carries the current one, so a retry only touches the workloads that missed. Without that, one missing target would restart every healthy one on each retry, forever. The key is per resource so two FerrVaultSecrets restarting the same workload do not overwrite each other's marker. It uses the UID rather than the name because the name segment of an annotation key is capped at 63 characters.
• The pending rollout is written to the status before the restart is attempted, on a resource's first rollout. Otherwise a restart cancelled by the timeout was only held in memory, the end-of-reconcile status write failed on the expired context, and the next pass forgot it.
• A failure returns the error (controller-runtime retries with backoff), increments sync_errors_total{reason="RolloutFailed"}, and sets RolloutRestarted=False with the error. Ready and the last-sync gauge still report the data sync, which did succeed.
• Upgrades restart nothing: an existing resource takes the hash already on its target Secret as the baseline.

Tests

• A full Reconcile whose Deployment read blocks until its context ends, the shape of the incident. It returns at the deadline, asserts the read was actually reached, and fails after 5s if that read's context is swapped for context.Background().
• The timeout floor: 0, negative, 5s and 20s are refused and 2m is accepted. LongestCall is pinned for the default policy and for a schedule shorter than the attempts.
• rolloutDue table: new Secret, upgrade with and without a change, a change, a retry with the Secret already rewritten, already restarted.
• A refused patch, then allowed: the first pass fails, records nothing and leaves the condition False; the second restarts and flips it True. On the old code the second pass restarts nothing, and the test caught a baseline-loss bug in my own first attempt.
• [api, worker] with worker missing: api is restarted once across the failing passes, and worker once when it appears. Without the per-workload skip it fails with "restarted 2 times, want 1".
• Unchanged content restarts once across three passes, and a failed rollout still stamps the sync gauge.
• A rollout cancelled by the timeout is retried on the next pass. The fixture's status writes now fail on an expired context, as the real client's do; the fake client ignored it, which is what hid this.
• Two resources restarting one workload: a retry of the first does not restart it again after the second stamped its own marker.

The reconcile fixture (reconcile_fixture_test.go) builds a complete reconcile against a fake client; #273 reuses it.

go test ./..., go vet ./... and gofmt pass. helm lint and golangci-lint run in CI.

[ferrfleet]

Wiring checks out: config.Controller.ReconciliationTimeout exists in controller-runtime v0.25.1 and controller.New defaults it from mgr.GetControllerOptions(), so both controllers pick it up. Flag, chart value, template and README agree, and the 10s client timeout the docs reference is real (internal/ferrvault/client.go:90). The new test helpers do not collide with the existing ones in the package.

One blocking finding on the interaction with --stall-threshold, plus a nit. Could not compile or run the tests: no Go toolchain in this runner, so the test assertions are reviewed by reading only.

[BryanFRD]

Blocking: agreed on every step, and it is the reason this is a stack. #272, directly on top, is the fix you describe: status.lastRolloutHash is advanced only after triggerRollouts succeeds, the trigger is "this hash differs from the current content" rather than "the Secret just changed", and the rollout error is returned instead of logged, so controller-runtime retries it with backoff. It also sets RolloutRestarted=False on the resource. #273 then alerts on controller_runtime_reconcile_timeouts_total and on sync_errors_total{reason="RolloutFailed"}, which replaces the liveness restart as the signal. The liveness restart never fixed the dropped rollout either: the restarted pod saw an unchanged hash too.

So #271 alone does make the failure quieter, as you say, and it should not merge without #272. I made that explicit in the body when I opened it.

Nit: taken in 5b57d62. --reconcile-timeout is now rejected at startup when it is below the FerrVault API client timeout, which covers 0 (no timeout at all) and anything that would cut a legitimate reveal short. The client timeout became the exported ferrvault.RequestTimeout so the check and the client cannot drift apart. There are tests for 0, a negative value, 5s and the default. I left --stall-threshold alone to keep this PR to one change; the same gap there deserves its own fix.

Title fixed too: gh stack had used the branch name, which is what the PR title check caught.

[ferrfleet]

Follow-up on 5b57d62. The --reconcile-timeout=0 nit is fixed and resolved; that commit touches only cmd/main.go, cmd/main_test.go and the RequestTimeout constant, so the blocking finding on the silently dropped rollout is unchanged and still stands. One nit on the new validation.

Still reviewed by reading only: no Go toolchain in this runner, so nothing here was compiled or run.

[BryanFRD]

Nit: right, and taken in 0566e67. The floor is now the longest one API call can take, computed from the retry policy rather than hard-coded: RetryPolicy.LongestCall(RequestTimeout) is three 10s attempts plus the 100ms and 400ms backoffs at +25% jitter, 30.625s. It mirrors backoffFor, including clamping to the last delay when attempts outnumber the schedule. Tests pin the default and the clamp. 20s, which the old floor accepted, is now rejected. The chart README and the flag help say the same.

On the blocking finding still standing: it does stand for #271 read alone, and #271 is not going to land alone. The stack merges with gh stack merge, which is atomic: everything up to the chosen PR merges in one operation, or nothing does. So the reconcile timeout reaches main in the same merge as #272's lastRolloutHash retry, which is approved and which is the fix you described. Putting that fix into this PR would only undo the split into reviewable layers.

[ferrfleet]

Follow-up on 8f89480 and 8fd6740, which add the rollout retry the earlier blocking finding asked for. That approach holds up: the rollout is tracked separately from the Secret write, the error is returned so the reconcile requeues, the per-workload content-hash annotation keeps a retry from restarting workloads that already took the change, lastRolloutHash is in the chart CRD so it survives pruning, and stamping the sync gauge on a rollout failure keeps a current Secret from looking stale. That thread is resolved.

One residual case of the same failure, blocking: when the pass that hangs is also the first one that would have persisted lastRolloutHash, the record of the pending rollout dies with the cancelled context and the restart is dropped for good. Details and a suggested fix on rolloutIfDue. Plus one nit on the shared annotation key.

Still reviewed by reading only: no Go toolchain in this runner, so nothing here was compiled or run.

[BryanFRD]

Both fixed in 3cb012b.

Blocking: right, and the suite hid it for a second reason besides the missing assertion. The fake client ignores an expired context on status writes, so the end-of-reconcile Status().Update that fails for real succeeded in the test. The fixture's interceptor now fails subresource updates on an expired context, as the real client does. The new test runs the incident shape across two passes: the first blocks on the workload read and times out, the second runs with reads unblocked and must restart the workload. Before the fix it failed with exactly the sequence you described. After it, it passes.

The fix is yours: on a resource's first due rollout, lastRolloutHash is written to the status before triggerRollouts, and an error there fails the reconcile. That is one extra status write per resource, once.

Nit: taken, with the UID rather than the name: ferrvault.com/content-hash.<uid>. The name segment of an annotation key is capped at 63 characters, and a FerrVaultSecret name can be far longer, so content-hash. plus the name would make the patch invalid for long names. content-hash. plus a UID is always 49. The trade-off is that recreating a FerrVaultSecret counts as a new owner, which costs at most one extra restart on a resource that was just deleted and recreated. The new test has two resources restarting one Deployment, and a retry of the first no longer restarts it after the second stamped its own marker. With one shared key it fails, 3 restarts instead of 2.

[ferrfleet]

Follow-up on 3cb012b. Both open threads are fixed and resolved: the pending rollout is persisted before triggerRollouts, and the per-workload marker is now keyed by resource UID, which fits the 63-character annotation name limit where the resource name would not have.

One finding on the new code, blocking. The pending-rollout write lands after ensureTargetSecret has already overwritten the Secret annotation that rolloutDue falls back to, so a failure of that one write, by deadline or by a 409 off a stale cache, drops the rollout for good. Details and the ordering that closes it on rolloutIfDue.

Still reviewed by reading only: no Go toolchain in this runner, so nothing here was compiled or run, and the new tests are assessed from their source.

[BryanFRD]

Fixed in 7aa218e, with your ordering rather than my write placement, which was the actual bug: the record has to be written while the Secret still corroborates it.

Reconcile now reads the target Secret's hash first (targetContentHash, a plain Get, absent Secret reads as ""), then recordPendingRollout persists lastRolloutHash = previous on a resource's first due rollout, and only then does ensureTargetSecret rewrite the Secret. rolloutIfDue no longer writes status at all. So every later failure in the pass leaves a state the next pass can act on:

• the pending-rollout write fails: the Secret still holds the old hash, so the next pass computes the same previous and tries again;
• the Secret write fails: lastRolloutHash equals what the Secret still holds, so nothing is pending and nothing is lost;
• the rollout or the end-of-reconcile write fails: lastRolloutHash is the stale hash in etcd, which is what makes the retry fire.

Both of your entry points are covered by that, the deadline landing on this line and a 409 off a stale informer cache, because neither can now destroy the only record.

New test, the case you asked for: the fixture refuses every status write, so the deadline effectively lands on recordPendingRollout. The first pass fails, and the test asserts the target Secret was not rewritten, which is the property the ordering buys. With writes allowed again, the next pass restarts the workload. Moving recordPendingRollout back after ensureTargetSecret fails it with "the target Secret was rewritten ... before the pending rollout was recorded".

contentChanged and rolloutIfDue now both take the pre-read hash, so the pass has one notion of "what the Secret held before", instead of that fact coming from two places.

[ferrfleet]

Follow-up on 4ebc553. The last open blocking finding is fixed and resolved: the pending rollout is recorded before ensureTargetSecret rewrites the Secret, so no single failure in the pass can leave a rotated Secret with no record that a restart is owed.

I walked the remaining orderings against the new code and did not find another hole. A cancelled triggerRollouts leaves the stale lastRolloutHash in etcd and the next pass retries; a final status write lost to the deadline after a successful restart makes the next pass call triggerRollouts again, but the per-workload content-hash.<uid> annotation already matches, so it patches nothing; a resource whose lastRolloutHash is already set skips the extra write entirely, since the stale value in etcd is the baseline the retry needs. Upgrade from a pre-PR resource still establishes the baseline without restarting when the content has not changed.

No blocking findings, no nits. Approving, though note that nothing here counts toward a required review.

Still reviewed by reading only: no Go toolchain in this runner, so nothing was compiled or run and the tests are assessed from their source.