fix(managed): requeue absent observe-only resources after the poll interval

[erikmiller-gusto]

Description of your changes

Problem

A managed resource with spec.managementPolicies: [Observe] whose external object
does not exist is re-observed on the error backoff schedule rather than on the
configured poll interval. Because the condition is a stable state — nothing the
reconciler does can resolve it; only the external object appearing can — the
resource is retried indefinitely at the rate limiter's ceiling, and the operator's
configured poll interval has no effect on it whatsoever.

For a provider whose upstream API enforces a shared per-installation rate limit,
a handful of absent observe-only resources can consume a meaningful share of the
budget and starve unrelated reconciles.

Evidence

pkg/reconciler/managed/reconciler.go returned Requeue: true for this branch:

if !observation.ResourceExists && policy.ShouldOnlyObserve() {
    record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist)))
    status.MarkConditions(xpv2.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve)))

    return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
}

Requeue: true causes controller-runtime to AddRateLimited, and
pkg/controller/options.go's ForControllerRuntime() hardcodes
RateLimiter: ratelimiter.NewController() — which is

workqueue.NewTypedItemExponentialFailureRateLimiter[reconcile.Request](1*time.Second, 60*time.Second)

So the retry schedule is 1s, 2s, 4s, 8s, 16s, 32s, 60s, 60s, …. The queue item
is never Forget()-ten while the condition persists, so it pins at the 60s ceiling
forever.

Two consequences, both independent of how the interval is configured:

1. The configured poll interval is ignored. Neither the reconciler's
   --poll-interval nor the per-resource poll-interval annotation
   (effectivePollInterval) influences this path at all. Providers commonly
   configure 10m; the resource was polled at 60s regardless.
2. A stable state is driven by a failure-backoff limiter. The initial
   1s→32s burst is pure waste: no amount of prompt retrying can make an
   external object exist.

Note for reviewers: defaultPollInterval is 1 * time.Minute
(reconciler.go:52), which coincides with the limiter's 60s ceiling. With stock
defaults the steady-state cadence is therefore unchanged. The wins are (a) the
initial 1s–32s retry burst is eliminated, (b) an operator-configured poll interval
is now actually honored, and (c) the per-resource poll-interval annotation now
works on this path.

This also aligns the branch with controller-runtime's own guidance. Result.Requeue
is deprecated as of the version in go.mod (v0.23.1), and its doc comment
describes precisely this case:

When waiting for an external event to happen, either the duration until it is
supposed to happen or an appropriate poll interval should be used, rather than
an interval emitted by a ratelimiter whose purpose it is to control retry on
error.

The change

Requeue after the poll interval, using the same idiom as every other steady-state
path in this reconciler (so the jitter hook and the per-resource annotation
override both apply):

reconcileAfter := r.pollIntervalHook(managed, r.effectivePollInterval(managed))
log.Debug("External resource does not exist", "requeue-after", time.Now().Add(reconcileAfter))

return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(updateStatus(), errUpdateManagedStatus)

The errors.Wrap(updateStatus(), errUpdateManagedStatus) return is retained
deliberately: if the status update itself fails, that is a transient error and
should still back off. controller-runtime discards Result when err != nil, so
this matches the contract used by the other RequeueAfter paths in this file.

Why the event and condition are retained

Unchanged, byte for byte: the reasonCannotObserve Warning event and the
ReconcileError condition (Synced=False with external resource does not exist). Operators must not lose visibility that an observe-only resource is
missing — this PR changes only the cadence of the retry, not its diagnostics.
Anything alerting on the condition or the event is unaffected.

Blast radius

Confined to the single !observation.ResourceExists && policy.ShouldOnlyObserve()
branch. errExternalResourceNotExist has no other non-test reference in the repo.

• Detection latency. The appearance of the external object is now noticed
  within the poll interval — the same guarantee already relied upon for drift
  detection on an observe-only resource that does exist. crossplane.io/reconcile-request
  still forces an immediate reconcile for operators who want to poll now
  (covered by TestReconcileRequestAnnotation).
• The genuine-Observe-error branch is untouched. An error returned from
  external.Observe still returns Requeue: true and still backs off, which is
  correct: that is a transient failure.
• No API, option, or default changes; no behavioral change for resources under
  any other management policy.

Tests

Extended the existing table-driven cases in both the modern and legacy suites
rather than adding parallel ones. Both cases now configure
WithPollInterval(10 * time.Minute), which makes the pair a controlled
experiment on the same input:

• ObserveOnlyResourceDoesNotExist — asserts RequeueAfter: 10m (was
  Requeue: true), and continues to assert the ReconcileError condition via
  the existing MockStatusUpdate.
• ExternalObserveError — still asserts Requeue: true despite the same
  configured poll interval, proving the fix is surgical and the error path still
  rate-limits.

Verified the assertion discriminates: with the test changes applied but the
source change reverted, both ObserveOnlyResourceDoesNotExist cases fail with
-RequeueAfter: s"10m0s" / +RequeueAfter: s"0s", while both
ExternalObserveError cases pass. With the source change, the full package and
the full repo suite pass, along with go vet and gofmt -l.

I have:

• Read and followed Crossplane's contribution process.
• Run ./nix.sh flake check to ensure this PR is ready for review. Nix is
  not available in my environment. I ran instead, all clean:
  go build ./..., go test ./... (full repo), go test -race ./pkg/reconciler/managed/...,
  go vet ./pkg/reconciler/managed/..., gofmt -l (repo-wide), and
  golangci-lint run ./pkg/reconciler/managed/... against the repo's
  .golangci.yml (0 issues). Happy to rerun under Nix if that gate catches
  something these did not.
• Added or updated unit tests.
• Linked a PR or a docs tracking issue to [document this change]. This
  changes retry cadence for one internal branch, with no API or option
  change. Glad to file a docs tracking issue if you consider the cadence
  change user-visible enough to warrant one.
• Added backport release-x.y labels to auto-backport this PR. Deferring
  the backport decision to maintainers; I can add labels if you tell me which
  release branches you want this in.

Opened as a draft: happy to adjust the approach before review if you would rather
solve this differently (for example by making the rate limiter configurable
instead).

🤖

[coderabbitai]

📝 Walkthrough

Walkthrough

The observe-only missing-resource path now persists its error condition and schedules reconciliation after the configured poll interval. Legacy and modern tests distinguish this behavior from rate-limited retries for external observation errors.

Changes

Observe-only polling retry behavior

Layer / File(s)	Summary
Schedule missing-resource retries pkg/reconciler/managed/reconciler.go	Missing external resources in observe-only mode return a RequeueAfter based on the configured, effective poll interval while preserving warning and status updates.
Validate retry distinctions pkg/reconciler/managed/reconciler_legacy_test.go, pkg/reconciler/managed/reconciler_modern_test.go	Tests verify timed poll retries for missing resources and rate-limited requeues for external observe errors.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: adamwg, jbw976

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Breaking Changes	❌ Error	Exported Reconciler.Reconcile changed observe-only missing-resource retries from Requeue to RequeueAfter, altering public behavior without a breaking-change label.	Add the breaking-change label or explicitly document/feature-flag the retry-cadence change before merging.
Title check	⚠️ Warning	Title is descriptive but exceeds the 72-character limit required by the PR guidelines.	Shorten it to 72 characters or fewer while keeping the same focus on observe-only resource requeue timing.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description clearly matches the change to requeue missing observe-only resources after the poll interval.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/reconciler/managed/reconciler_legacy_test.go (1)

1671-1716: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for jitter and per-resource poll-interval overrides.

Both observe-only missing-resource cases assert only the configured base interval. Add cases covering the interval hook/jitter and a per-resource override; otherwise a regression that drops those calculations could still pass in both suites.

• pkg/reconciler/managed/reconciler_legacy_test.go#L1671-L1716: extend the legacy observe-only case or add a focused variant covering the hook and override.
• pkg/reconciler/managed/reconciler_modern_test.go#L1677-L1722: add the equivalent modern-suite coverage.

🤖 Prompt for AI Agents

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

In `@pkg/reconciler/managed/reconciler_legacy_test.go` around lines 1671 - 1716,
Extend the observe-only missing-resource coverage in
pkg/reconciler/managed/reconciler_legacy_test.go:1671-1716 and add equivalent
coverage in pkg/reconciler/managed/reconciler_modern_test.go:1677-1722.
Configure each test to exercise the interval hook/jitter and a per-resource
poll-interval override, then assert RequeueAfter uses the calculated overridden
interval rather than only the base WithPollInterval value; preserve the existing
conditioned status error assertions.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@pkg/reconciler/managed/reconciler_legacy_test.go`:
- Around line 1671-1716: Extend the observe-only missing-resource coverage in
pkg/reconciler/managed/reconciler_legacy_test.go:1671-1716 and add equivalent
coverage in pkg/reconciler/managed/reconciler_modern_test.go:1677-1722.
Configure each test to exercise the interval hook/jitter and a per-resource
poll-interval override, then assert RequeueAfter uses the calculated overridden
interval rather than only the base WithPollInterval value; preserve the existing
conditioned status error assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d11ba87-ffa5-42fb-a8b1-0c32b6de4e75

📥 Commits

Reviewing files that changed from the base of the PR and between b864aea and 4ee6295.

📒 Files selected for processing (3)

• pkg/reconciler/managed/reconciler.go
• pkg/reconciler/managed/reconciler_legacy_test.go
• pkg/reconciler/managed/reconciler_modern_test.go