Skip to content

Commit 66d39cf

Browse files
IvanHuntersIvanHunters
committed
fix(operator): hold final backoff window before give-up, note upstream version skew
Address lexfrei's non-blocking follow-ups on the LGTM approval: Follow-up #1 — give-up preempted the final backoff. `decideRecovery` returned `recoveryActionGiveUp` as soon as `attempts >= maxRecoveryAttempts`, without checking whether the final-force backoff window had elapsed. The Owns()-watch re-fire from the N-th force-drift's own writes triggered a reconcile milliseconds after the force, so the effective time-to-give-up was ~7.5m (30s+60s+2m+4m) — not the ~11.5m documented on the constants block. The final 4-minute response window the last nudge was supposed to grant source-watcher was never actually granted. Fix: extend the exhaustion branch to hold `Wait` until `now - lastRecoveryAt >= backoffFor(maxRecoveryAttempts)` before switching to GiveUp. Now the documented ~11.5m budget matches the observed behaviour, and the last force gets a real chance to succeed (Owns watch → source-watcher Ready=True copy-through) before we surface SourceWatcherStalled. Tests: * `TestDecideRecovery` gains a case pinning `Wait` when attempts=maxRecoveryAttempts but the final backoff has not yet elapsed (the exact re-fire shape the previous behaviour tripped on). * `TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending` drives the full updateStatus path against the same shape, asserting the PackageSource stays in Unknown/AwaitingSourceWatcherRecovery and the RequeueAfter equals the remaining backoff, NOT zero (GiveUp). Follow-up #3 — upstream version-skew note. The Go module dependency tracks `fluxcd/source-watcher/api/v2 v2.0.3` (CRD types package only), but the deployed binary this workaround targets is pinned to v2.1.0 at `internal/fluxinstall/manifests/fluxcd.yaml`. Comments referencing `drift.go:52` / `controller.go:164/254` describe the deployed binary, not the compiled API package. Documented on the constants block so a future flux bump re-verifies the anchors AND the watch predicate / drift-detection heuristics, which are the load-bearing upstream assumptions. Follow-up #2 (envtest over full Reconcile→Owns→updateStatus cycle) is architectural and out of scope for this PR — noted for a follow-up. Signed-off-by: Ivan Okhotnikov <[email protected]>
1 parent 3f31c9c commit 66d39cf

2 files changed

Lines changed: 110 additions & 1 deletion

File tree

‎internal/operator/packagesource_reconciler.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,17 @@ import (
4040
// race in source-watcher v2.1.0). See the block comment on
4141
// maybeRecoverArtifactGenerator for the mechanism.
4242
//
43+
// Version-skew note. Comments in this file reference upstream source-watcher
44+
// internals by exact file:line (e.g. `drift.go:52`, `controller.go:164/254`).
45+
// Those anchors are for the DEPLOYED source-watcher binary, currently pinned
46+
// to v2.1.0 at `internal/fluxinstall/manifests/fluxcd.yaml`. The Go module
47+
// dependency in `go.mod` (`github.com/fluxcd/source-watcher/api/v2 v2.0.3`)
48+
// only pulls the CRD types package — behavioural code lives in the deployed
49+
// binary. When the flux distribution bumps source-watcher, re-verify the
50+
// line anchors AND the watch predicate (`ReconcileRequestedPredicate`) and
51+
// drift-detection heuristics against the new version — those are the
52+
// load-bearing upstream assumptions this workaround stands on.
53+
//
4354
// The schedule is deliberately conservative:
4455
//
4556
// - `stuckGracePeriod = 30s` — source-watcher normally settles an
@@ -765,8 +776,24 @@ type recoveryDecision struct {
765776

766777
// decideRecovery is the pure decision function driving maybeRecoverArtifactGenerator.
767778
// Split out so it can be unit-tested without a cluster.
779+
//
780+
// The exhaustion branch (`attempts >= maxRecoveryAttempts`) also respects the
781+
// final backoff window before switching to GiveUp. Without that gate, the
782+
// Owns()-watch re-fire from the N-th force-drift's own writes triggers a
783+
// reconcile milliseconds later, `decideRecovery` sees `attempts == max` and
784+
// returns GiveUp immediately — preempting the response window the final
785+
// force was supposed to grant source-watcher and collapsing the documented
786+
// budget from ~11.5m (30s+60s+2m+4m+4m) down to ~7.5m (30s+60s+2m+4m). The
787+
// wait branch here holds until the full final backoff has elapsed, so the
788+
// last nudge actually gets its chance to succeed (Owns → Ready=True copy-
789+
// through) before the driver surfaces SourceWatcherStalled.
768790
func decideRecovery(attempts int, lastRecoveryAt time.Time, now time.Time) recoveryDecision {
769791
if attempts >= maxRecoveryAttempts {
792+
elapsed := now.Sub(lastRecoveryAt)
793+
needed := backoffFor(maxRecoveryAttempts)
794+
if elapsed < needed {
795+
return recoveryDecision{action: recoveryActionWait, wait: needed - elapsed}
796+
}
770797
return recoveryDecision{action: recoveryActionGiveUp}
771798
}
772799
if attempts == 0 {

‎internal/operator/packagesource_reconciler_test.go‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ func TestDecideRecovery(t *testing.T) {
247247
wantWait: 90 * time.Second, // backoffFor(3)=2m minus elapsed 30s
248248
},
249249
{
250-
name: "attempts exhausted — give up",
250+
name: "attempts exhausted with elapsed >> final backoff — give up",
251251
attempts: maxRecoveryAttempts,
252252
now: referenceTime,
253253
wantAction: recoveryActionGiveUp,
@@ -258,6 +258,21 @@ func TestDecideRecovery(t *testing.T) {
258258
now: referenceTime,
259259
wantAction: recoveryActionGiveUp,
260260
},
261+
{
262+
// lexfrei's follow-up: without holding the final backoff window,
263+
// the Owns()-watch re-fire from the N-th force-drift's own
264+
// writes preempts the response window and give-up fires within
265+
// milliseconds of the last force. Budget collapses to ~7.5m
266+
// instead of the documented ~11.5m. This case pins the fix:
267+
// attempts=max but no time elapsed since the final force,
268+
// expect Wait for the full backoffFor(maxRecoveryAttempts).
269+
name: "attempts exhausted but final backoff not elapsed — wait, do not give up",
270+
attempts: maxRecoveryAttempts,
271+
lastRecoveryAt: referenceTime.Add(-time.Second),
272+
now: referenceTime,
273+
wantAction: recoveryActionWait,
274+
wantWait: backoffFor(maxRecoveryAttempts) - time.Second,
275+
},
261276
}
262277

263278
for _, tt := range tests {
@@ -1350,3 +1365,70 @@ func TestUpdateStatus_OwnMarkerAttemptsExhausted_GivesUp(t *testing.T) {
13501365
t.Errorf("PS Ready = %+v, want False/%s — unresponsive source-watcher must eventually surface as stalled", psReady, reasonSourceWatcherBad)
13511366
}
13521367
}
1368+
1369+
// TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending pins
1370+
// lexfrei's follow-up on the give-up path: the Owns()-watch re-fire from the
1371+
// 5th force-drift's own writes triggers a reconcile milliseconds after the
1372+
// force. Without the "hold final backoff window" gate in decideRecovery,
1373+
// that re-fire would see `attempts >= maxRecoveryAttempts` and immediately
1374+
// return GiveUp, preempting the 4-minute response window the last nudge
1375+
// was supposed to grant source-watcher — effective time-to-give-up
1376+
// collapses from the documented ~11.5m to ~7.5m. Assertion: with
1377+
// attempts=maxRecoveryAttempts AND lastRecoveryAt=now-eps (the exact shape
1378+
// of the re-fire on the 5th force's own writes), updateStatus must return
1379+
// Wait with RequeueAfter equal to the remaining backoffFor(maxRecoveryAttempts),
1380+
// NOT GiveUp.
1381+
func TestUpdateStatus_OwnMarkerAttemptsExhaustedButFinalBackoffPending(t *testing.T) {
1382+
// Simulate the moment right after the 5th force landed: attempts=max,
1383+
// lastRecoveryAt=now-eps, marker still on the AG.
1384+
forceAt := referenceTime.Add(-time.Second)
1385+
ps := &cozyv1alpha1.PackageSource{
1386+
ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "cozy-system", Generation: 1},
1387+
Spec: cozyv1alpha1.PackageSourceSpec{
1388+
SourceRef: &cozyv1alpha1.PackageSourceRef{Name: "src", Kind: "OCIRepository", Namespace: "cozy-system"},
1389+
},
1390+
}
1391+
ag := &sourcewatcherv1beta1.ArtifactGenerator{
1392+
ObjectMeta: metav1.ObjectMeta{
1393+
Name: "example", Namespace: "cozy-system", Generation: 1,
1394+
CreationTimestamp: metav1.NewTime(referenceTime.Add(-time.Hour)),
1395+
Annotations: map[string]string{
1396+
annotationRecoveryAttempts: strconv.Itoa(maxRecoveryAttempts),
1397+
annotationLastRecoveryAt: forceAt.UTC().Format(time.RFC3339Nano),
1398+
},
1399+
},
1400+
Status: sourcewatcherv1beta1.ArtifactGeneratorStatus{
1401+
Inventory: []sourcewatcherv1beta1.ExternalArtifactReference{
1402+
{Name: "one", Namespace: "cozy-system", Digest: "sha256:aaa"},
1403+
},
1404+
ObservedSourcesDigest: "sha256:0e7",
1405+
Conditions: []metav1.Condition{{
1406+
Type: "Ready",
1407+
Status: metav1.ConditionFalse,
1408+
Reason: reasonRecoveryForced,
1409+
ObservedGeneration: 1,
1410+
LastTransitionTime: metav1.NewTime(forceAt),
1411+
}},
1412+
},
1413+
}
1414+
c := fake.NewClientBuilder().WithScheme(testScheme(t)).WithStatusSubresource(ps, ag).WithObjects(ps, ag).Build()
1415+
r := &PackageSourceReconciler{Client: c, Scheme: testScheme(t)}
1416+
1417+
res, err := r.updateStatus(context.Background(), ps, referenceTime)
1418+
if err != nil {
1419+
t.Fatalf("updateStatus: %v", err)
1420+
}
1421+
wantWait := backoffFor(maxRecoveryAttempts) - time.Second
1422+
if res.RequeueAfter != wantWait {
1423+
t.Errorf("RequeueAfter = %v, want %v — final backoff window must not be preempted", res.RequeueAfter, wantWait)
1424+
}
1425+
// PS must still be Unknown/AwaitingSourceWatcherRecovery, NOT False/SourceWatcherStalled.
1426+
persistedPS := &cozyv1alpha1.PackageSource{}
1427+
if err := c.Get(context.Background(), client.ObjectKeyFromObject(ps), persistedPS); err != nil {
1428+
t.Fatalf("Get PS: %v", err)
1429+
}
1430+
psReady := meta.FindStatusCondition(persistedPS.Status.Conditions, "Ready")
1431+
if psReady == nil || psReady.Status != metav1.ConditionUnknown || psReady.Reason != reasonAwaitingRecovery {
1432+
t.Errorf("PS Ready = %+v, want Unknown/%s — give-up must wait for final backoff to elapse", psReady, reasonAwaitingRecovery)
1433+
}
1434+
}

0 commit comments

Comments
 (0)