fix(component)!: stop FlushStatus from writing stale status back on a conflict

[sourcehawk]

Description

Fixes #238

On a 409 Conflict, FlushStatus wrote the stale owner again with a fresh resourceVersion, so it rolled back status fields that another controller had written after the read. This PR removes the retry: FlushStatus makes one Status().Update, and on a conflict it writes nothing and returns the error. Callers now see a conflict as a reconcile error and a requeue, and FlushStatus loses its comps argument, which is a breaking change.

Changes

• Breaking: FlushStatus(ctx, rec, comps) is now FlushStatus(ctx, rec). To migrate, remove the third argument at each call site. Nothing else changes for a caller.
• FlushStatus makes one status update. It returns any error wrapped with %w, so apierrors.IsConflict still reports true for a conflict. Condition metrics are recorded only after a successful write, as before.
• The conflict path is gone: the condition ownership by component, the refresh of unowned conditions, and the refetch through APIReader.
• The ReconcileContext.APIReader doc now names what still uses it: the live reads for BlockOnForeignController and DeleteOnlyIfOwned.
• The FlushStatus GoDoc, docs/component.md, docs/getting-started.md, docs/guidelines.md, the plugin skills, the reviewer agent, the examples and the e2e reconcilers are updated. Text about owned conditions, unowned conditions that revert on a conflict, and the two-step removal of a retired component's condition is removed.
• docs/component.md adds a short optional pattern: in the deferred call, turn a conflict into RequeueAfter with a nil error, so it is not logged as a reconciler error. It is documentation only, with no new framework API.

Challenges

Why removing the retry fixes #238 for every controller, with no list of owned fields: the stale write was the retry itself. The first Status().Update carries the resourceVersion from the read at the top of the reconcile, so the API server rejects it when anyone else wrote in between. Without a retry, nothing stale lands. The next reconcile starts from the current object, so it writes back only what it computes itself, plus the other writers' values exactly as the server holds them. The first acceptance criterion of #238 ("declare owned fields, or write only owned paths") is not needed, because nothing stale is written. The retry came in as a convenience, so that an external write would not fail the reconcile. The ownership machinery existed only to make that retry safe, so it goes with it.

Related

• Replaces fix(component): stop FlushStatus conflict retries rolling back shared status #240, which took the merge-patch approach with an opt-in list of owned fields.

Testing

• New envtest specs in pkg/component/flush_status_test.go run the FlushStatus conflict retry writes back stale non-condition status fields of other writers #238 scenario against a real API server. The test reads the owner. A second writer then sets status.observedGeneration and a Ready condition. A flush of the stale owner returns an error for which apierrors.IsConflict is true. The server still holds the second writer's values, the stale Accepted condition did not land, and no condition metrics were recorded. A second spec then fetches fresh, stages Accepted again and flushes. The server holds the controller's Accepted and the other writer's Ready and observedGeneration.
• Fake-client unit tests cover the success path with metrics, a nil recorder, and a non-conflict error that is returned without metrics.
• The tests of the deleted ownership and refresh behavior are removed, because that behavior no longer exists.
• make all passes, and so does make run-examples. The e2e reconcilers already fetch the owner on every reconcile, and no e2e spec checks for reconcile errors. A conflict now causes one more requeue, which the Eventually assertions absorb. I did not run e2e locally, because CI runs it.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

The implementation, caller migrations, regression coverage, and documentation consistently support the intended conflict behavior, with no blocking issues identified.

No open issues 🎉

What changed in this PR

Fixes #238 by removing FlushStatus conflict retries, preventing stale status from overwriting another controller’s updates.

Changes:

• Removes the comps argument and returns wrapped errors after one status update.
• Adds regression coverage for conflicts, shared status, and metrics.
• Migrates callers and documents conflict-driven requeueing.

File	Description
plugin/​skills/​structuring-operators/​SKILL.md	Updates controller and aggregate guidance.
plugin/​skills/​structuring-operators/​references/​guidelines.md	Syncs revised reconciliation guidance.
plugin/​skills/​building-components/​SKILL.md	Explains single-write conflict handling.
plugin/​skills/​building-components/​references/​component.md	Syncs updated status documentation.
plugin/​agents/​reviewer.md	Updates conflict-handling review checks.
pkg/​component/​zz_mock_crd_test.go	Clarifies the test status field’s purpose.
pkg/​component/​flush_status_test.go	Adds conflict and shared-status regression tests.
pkg/​component/​conditions_test.go	Removes obsolete retry tests; relocates flush tests.
pkg/​component/​component.go	Removes retries and simplifies FlushStatus.
pkg/​component/​component_test.go	Migrates test calls.
pkg/​component/​aggregate.go	Corrects status-persistence GoDoc.
examples/​mutations-and-gating/​app/​controller.go	Migrates flush calls and APIReader comments.
examples/​grace-inconsistency/​app/​controller.go	Migrates flush calls and APIReader comments.
examples/​extraction-and-guards/​app/​controller.go	Migrates flush calls and APIReader comments.
examples/​custom-resource/​app/​controller.go	Migrates flush calls and APIReader comments.
examples/​component-prerequisites/​app/​controller.go	Removes obsolete component ownership tracking.
e2e/​framework/​reconciler.go	Migrates the namespaced test reconciler.
e2e/​framework/​cluster_reconciler.go	Migrates the cluster-scoped test reconciler.
docs/​guidelines.md	Updates conflict and condition-retirement guidance.
docs/​getting-started.md	Updates introductory API usage.
docs/​component.md	Documents single-write behavior and optional quiet requeueing.

_{Review effort: Balanced}

[sourcehawk]

Review round 1: the Copilot review of aa3fae8 has no posted comments and no suppressed comments, so there is nothing to change. The review body states Review effort: Balanced.

Comment check against origin/main (review base aa3fae8):

Declaration	Lines (main, now)	Sentence	Verdict	Source	Judge
APIReader field in the five example controllers and the two e2e reconcilers	2, 3	"APIReader reads straight from the API server."	KEEP	ReconcileContext.APIReader doc, pkg/component/component.go	evaluator
same	2, 3	"The component reads live objects through it for resources registered with BlockOnForeignController or DeleteOnlyIfOwned."	KEEP	observeController in create.go, the DeleteOnlyIfOwned read in suspend.go	evaluator

The old text of these comments said that FlushStatus reads through APIReader on a conflict. That is false now, so it was removed. The other removed comments explained the deleted conflict retry, ownership code and tests, and they went with that code.

CI is green, e2e included.