fix(component): stop FlushStatus conflict retries rolling back shared status

docs/component.md

@@ -815,22 +815,28 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request)
 status subresource**, not only the conditions. Every status field the controller staged in memory is sent, and every
 field it did not stage is sent exactly as it stands on the in-memory object. Fetch the owner fresh at the top of each
 reconcile, so the fields nobody staged still hold current server state. An owner carried over from an earlier pass
-writes stale values back over newer ones.
+writes stale values back over newer ones. With
+[`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), the write is a merge patch of the
+declared fields and the conditions instead.
 
 On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call,
 `FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object,
 takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose
-type this flush does not own, and retries. The staged owner is never replaced.
+type this flush does not own, and retries. The staged owner is never replaced, and no other status field is taken from
+the server.
 
 That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the
 manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very
 write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict
 again until the cache catches up. A direct read sees the server as it is.
 
-Two things follow from that. Non-condition status fields staged during the reconcile survive a conflict, because the
-object holding them is the object that gets written. And a condition type this flush does not own keeps the server's
-value rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled
-back.
+Two things follow from that. A status field that the flush writes keeps its staged value across a conflict, because the
+object holding it is the object that gets written. And a condition type this flush does not own keeps the server's value
+rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled back.
+Without `WithOwnedStatusFields`, the flush writes every non-condition status field, so a retry also writes back the
+stale copy of a field that another controller changed after the fetch. When another controller writes non-condition
+fields of the same status, see
+[Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller).
 
 Which types count as owned comes from the components passed to `FlushStatus`:
 
@@ -917,6 +923,61 @@ Server-Side Apply is for managed resources, never for the owner's status subreso
 depends on which call runs last: the `Update` can overwrite the patched condition, or the patch can overwrite what the
 `Update` wrote. Keep every owner-status write on the `meta.SetStatusCondition` plus `FlushStatus` path.
 
+### Sharing the owner's status with another controller
+
+Sometimes two controllers write the status of one CR. For example, a status-only controller owns an `Accepted`
+condition, and a second controller owns a `Ready` condition and `status.observedGeneration`. Each controller still
+writes through `FlushStatus`. Condition ownership comes from the components passed to it. For non-condition fields, pass
+`component.WithOwnedStatusFields` with the fields that this controller writes:
+
+```go
+// In the status-only controller, which writes status.phase and no other non-condition field:
+flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase"))
+
+// In the other controller, which writes status.observedGeneration:
+flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration"))
+```
+
+With the option, every attempt of `FlushStatus` is a JSON merge patch of the status instead of a `Status().Update`. The
+patch holds only:
+
+- the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the
+  owner changed after the controller read it;
+- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the
+  server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that
+  the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too;
+- the conditions list as staged. On the first attempt that includes any unowned condition the controller staged by hand,
+  such as an owner-level aggregate; the precondition makes the patch fail if the server changed after the read. After a
+  409, the unowned conditions are first refreshed from the server.
+
+To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client`
+otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. When that read finds an owner with
+another UID, because the owner was deleted and recreated under the same name after the controller read it, `FlushStatus`
+returns an error and does not write, as `Status().Update` would refuse the write. A field that the controller did not
+declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409,
+`FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the
+patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the
+owner, so a value staged in a field that you did not declare does not survive in memory.
+
+Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`,
+`"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush
+owns no non-condition field, which fits a controller that writes only conditions. Paths from several options are
+combined. Declare every non-condition field that the controller writes, including `observedGeneration` when it is this
+controller's field: a field that you stage but do not declare is not written.
+
+`FlushStatus` returns an error, with no write, when a path:
+
+- is empty or has an empty segment, such as `"summary..ready"`;
+- names the conditions, contains them, or lies below them, because conditions are owned through components.
+  `FlushStatus` finds the JSON path of the list that `GetStatusConditions` returns, so this holds whatever that field is
+  called;
+- runs below a field that the staged owner holds as a value that is not an object, such as `"phase.value"` when `phase`
+  is a string.
+
+The framework does not check a path against the CRD schema. A misspelled path, such as `"phsae"`, sends a field that the
+schema does not declare. With a structural schema the API server prunes that field, so that part of the write does
+nothing and `FlushStatus` returns no error. Check each path against the CRD type when you add it.
+
 ### Where the owner's observedGeneration is set
 
 The framework sets `ObservedGeneration` on every **component condition** it writes, from `owner.GetGeneration()`. It
@@ -925,7 +986,9 @@ never writes `status.observedGeneration` on the owner itself. `OperatorCRD` requ
 
 An owner-level `status.observedGeneration` is the controller's own field. Assign it on the owner before `FlushStatus`
 runs, in the same staging step that writes the owner's own condition, and the single status write persists both. This
-applies to a status-only controller too, as in the validation-only example above.
+applies to a status-only controller too, as in the validation-only example above. When the status is
+[shared with another controller](#sharing-the-owners-status-with-another-controller), only the controller that owns
+`observedGeneration` assigns it, and that controller includes it in `WithOwnedStatusFields`.
 
 ```go
 app.Status.ObservedGeneration = app.Generation

docs/guidelines.md

@@ -190,6 +190,12 @@ returns an error.
 If you do not want condition metrics, leave `ReconcileContext.Metrics` as `nil`; `FlushStatus` tolerates a nil recorder
 and skips metric emission.
 
+When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller
+passes the components that own its conditions and declares its non-condition fields with
+[`WithOwnedStatusFields`](component.md#sharing-the-owners-status-with-another-controller), so its flush patches only
+those fields and the conditions. Without the declaration, the flush writes the whole status, and a conflict retry writes
+the controller's stale copy of the other controller's fields back over newer values.
+
 Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable:
 enabling an optional feature changes which components the resolver returns without touching the reconcile loop.
 

pkg/component/component.go

@@ -655,11 +655,21 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error {
 // validation-only CRD reports, it is unowned. On a conflict an unowned condition
 // follows the server, and the next reconcile stages it again.
 //
-// FlushStatus issues a single Status().Update, which writes the entire status
-// subresource, not only the conditions. Fields the controller never staged are
-// sent exactly as they stand on the in-memory owner. The owner must therefore be
-// fetched fresh at the top of each reconcile: an owner carried over from an
-// earlier pass writes stale status values back over newer ones.
+// By default FlushStatus issues a single Status().Update, which writes the
+// entire status subresource, not only the conditions. Fields the controller
+// never staged are sent exactly as they stand on the in-memory owner. The owner
+// must therefore be fetched fresh at the top of each reconcile: an owner
+// carried over from an earlier pass writes stale status values back over newer
+// ones.
+//
+// With [WithOwnedStatusFields], every attempt is instead a JSON merge patch of
+// the status that holds only the declared fields and the conditions, with the
+// staged resourceVersion as a precondition when the owner has one. A field the
+// controller did not declare is never sent, so it cannot overwrite another
+// writer's value. Use it when another controller writes non-condition fields of
+// the same status. After a successful write rec.Client decodes the server's
+// response into rec.Owner; with the controller-runtime client a staged value of
+// an undeclared field does not survive in memory.
 //
 // Controllers must call FlushStatus exactly once per reconciliation, typically
 // via defer so that conditions set on error paths are still persisted. Declare
@@ -693,7 +703,10 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error {
 // the object being written. It fetches the server's copy into a separate object
 // through rec.APIReader when set (rec.Client otherwise), takes that object's
 // resourceVersion so the retry targets the live object, and restores from it
-// only the conditions whose type this flush does not own. It then retries.
+// only the conditions whose type this flush does not own. It then retries. No
+// other status field is taken from the server. Without [WithOwnedStatusFields]
+// the retry therefore writes every staged non-condition field again, including
+// a stale copy of a field that another writer has changed since the fetch.
 //
 // Set rec.APIReader from the manager's GetAPIReader. The manager's default
 // Client serves reads from the informer cache, which can lag the very write
@@ -714,24 +727,35 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error {
 // the next reconcile.
 //
 // rec.Client and rec.Owner must be populated, and every component in comps must
-// be non-nil; a nil component is a programming error. If rec.Metrics is nil,
-// metric recording is skipped.
-func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component) error {
+// be non-nil; a nil component is a programming error. With
+// [WithOwnedStatusFields], FlushStatus returns an error without writing for an
+// invalid path. If rec.Metrics is nil, metric recording is skipped.
+func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, opts ...FlushOption) error {
+	var options flushOptions
+	for _, opt := range opts {
+		opt(&options)
+	}
+
 	owned := ownedConditionTypes(comps)
 
-	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		updateErr := rec.Client.Status().Update(ctx, rec.Owner)
-		if updateErr == nil {
-			return nil
-		}
-		if !apierrors.IsConflict(updateErr) {
+	var err error
+	if options.fieldsDeclared {
+		err = patchOwnedStatus(ctx, rec, owned, options.ownedFields)
+	} else {
+		err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			updateErr := rec.Client.Status().Update(ctx, rec.Owner)
+			if updateErr == nil {
+				return nil
+			}
+			if !apierrors.IsConflict(updateErr) {
+				return updateErr
+			}
+			if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil {
+				return refreshErr
+			}
 			return updateErr
-		}
-		if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil {
-			return refreshErr
-		}
-		return updateErr
-	})
+		})
+	}
 	if err != nil {
 		return err
 	}
@@ -779,14 +803,7 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m
 	// conditions in place here and they would read as the server's.
 	*server.GetStatusConditions() = nil
 
-	// Read through APIReader when one is set: the manager's Client serves reads
-	// from the informer cache, which can lag the write that caused the conflict
-	// and return the same stale resourceVersion, defeating the retry.
-	reader := client.Reader(rec.Client)
-	if rec.APIReader != nil {
-		reader = rec.APIReader
-	}
-	if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil {
+	if err := liveReader(rec).Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil {
 		return err
 	}
 
@@ -819,6 +836,17 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m
 	return nil
 }
 
+// liveReader returns rec.APIReader when one is set, and rec.Client otherwise.
+// The manager's Client serves reads from the informer cache, which can lag the
+// write that caused a conflict and return the same stale resourceVersion,
+// defeating the retry.
+func liveReader(rec ReconcileContext) client.Reader {
+	if rec.APIReader != nil {
+		return rec.APIReader
+	}
+	return rec.Client
+}
+
 // replaceStatusCondition sets cond by type, taking its value verbatim.
 //
 // meta.SetStatusCondition is deliberately not used here: it preserves the