fix(commitrequest): name the author from the creating write, and settle no-commit applies

docs/architecture.md

@@ -239,7 +239,7 @@ optional and only add *who* did something:
 |---|---|---|---|
 | **Discovery** (CRD/APIService) | *what types exist* | yes | the API surface: served GVRs, scope, preferred version, subresources; rules resolve against it |
 | **Watch** (per claimed `(GVR, scope)`) | *what changed* | yes | the object body, ordered per type; the only object-state source, with deletes reconciled via `sendInitialEvents` replay + mark-and-sweep |
-| **Audit webhook** (`/audit-webhook/<audit-route>`) | *who changed mirrored state, and who created a `CommitRequest`* | no | a post-persist attribution fact, partitioned by source provider and joined to the watch event, or to the `CommitRequest`, by resourceVersion ([Optional Attribution](#optional-attribution)) |
+| **Audit webhook** (`/audit-webhook/<audit-route>`) | *who changed mirrored state, and who created a `CommitRequest`* | no | a post-persist attribution fact, partitioned by source provider and joined to the watch event by resourceVersion, or to the `CommitRequest` by the UID it was created with ([Optional Attribution](#optional-attribution)) |
 
 With the optional source off, the product still mirrors state correctly, and every commit is authored
 by the committer.
@@ -262,11 +262,11 @@ the write reaches etcd**:
 Audit avoids both: kube-apiserver posts a `ResponseComplete` event *after* the write persisted, carrying
 the resourceVersion that joins it to the watch event.
 
-A `CommitRequest`'s author comes from audit too, for the same reasons: its own `ResponseComplete` event,
-joined by the UID and resourceVersion it was created with, from the same index and with the same grace
-as the edits it saves. So the request and its edits are attributed by one pipeline, on the cluster both
-were made on, and a request on a source cluster needs nothing from the control cluster. A
-`CommitRequest` is never mirrored, whatever a `WatchRule` selects. See
+A `CommitRequest`'s author comes from audit too, for the same reasons: the `ResponseComplete` event that
+created it, joined by its UID, from the same index and with the same grace as the edits it saves. A later
+write to the request, such as a label patch, never names its author. So the request and its edits are
+attributed by one pipeline, on the cluster both were made on, and a request on a source cluster needs
+nothing from the control cluster. A `CommitRequest` is never mirrored, whatever a `WatchRule` selects. See
 [CommitRequest Finalize](#commitrequest-finalize).
 
 ### Proving it on new Kubernetes versions

docs/spec/commitrequest-design.md

@@ -148,10 +148,13 @@ pushed commit is required; `Ready=True` also includes successful no-commit outco
 
 ## Authorship
 
-A request's author comes from its own audit event, on the cluster it was created on, the same way a
-watched write's does: the core looks it up in the same fact index, on the audit route of the
-`GitTarget`'s `KubeProvider`, keyed by the request's UID and the resourceVersion it was created with,
-and waits up to `--author-attribution-grace`. It acquires the fact stream of `commitrequests` on that
+A request's author comes from the audit event that created it, on the cluster it was created on, the
+same way a watched write's does: the core looks it up in the same fact index, on the audit route of the
+`GitTarget`'s `KubeProvider`, by the request's UID, and waits up to `--author-attribution-grace`. Only a
+fact marked as the creating write answers: a `create`, or a `patch` answered `201 Created`
+(server-side apply of a new object). A later write to the request, such as a controller patching its
+labels before the request is claimed, names who made that write, so it never names the author and
+never moves the request to that actor's window. It acquires the fact stream of `commitrequests` on that
 route, so the fact follower reads those audit facts at all.
 
 | Outcome | The request |
@@ -163,8 +166,7 @@ route, so the fact follower reads those audit facts at all.
 
 The wait runs whenever attribution is on; whether the provider has received facts is checked only
 after it, so a request is not refused while its provider's first audit event is still in transit.
-The author is written in the claim: every status write moves the resourceVersion the fact is keyed
-by, so a later lookup could never match.
+The author is written in the claim, and read from there afterwards.
 
 The author decides which window the request attaches to. The matched window decides the Git author:
 

internal/controller/commitrequest_apply.go

@@ -45,21 +45,30 @@ func applyReported(cr *configbutleraiv1alpha3.CommitRequest) bool {
 // followApply reads where the request's apply stands, writes it to Applied, and comes back until it
 // is settled; then the request expires as any finished one does. A write that conflicts is decided
 // again on the next step, from the request as it is stored.
+//
+// A request that pushed no commit is settled here, without asking its worker: one that ended before
+// it reached a worker, such as AuthorUnresolved, is one its worker never hears of, and would
+// otherwise be reported lost.
 func (c *commitRequestCore) followApply(
 	ctx context.Context, p commitRequestPath, cr *configbutleraiv1alpha3.CommitRequest,
 ) (time.Duration, error) {
-	attach := git.AttachCommitRequest{
-		Namespace: cr.Namespace, Name: cr.Name, UID: string(cr.UID),
-		GitTargetName: cr.GitTargetKey().Name, GitTargetNamespace: cr.GitTargetKey().Namespace,
-	}
-	apply, known, err := c.finalizer.LookupCommitRequestApply(ctx, attach)
-	if err != nil {
-		p.log.V(1).Info("Cannot read the CommitRequest's apply yet; will retry",
-			"name", client.ObjectKeyFromObject(cr), "err", err.Error())
-		return commitRequestPollInterval, nil
-	}
-	if !known {
-		apply = unknownApply(cr)
+	apply := nothingToApply()
+	if cr.Status.Commit != "" {
+		attach := git.AttachCommitRequest{
+			Namespace: cr.Namespace, Name: cr.Name, UID: string(cr.UID),
+			GitTargetName: cr.GitTargetKey().Name, GitTargetNamespace: cr.GitTargetKey().Namespace,
+		}
+		var known bool
+		var err error
+		apply, known, err = c.finalizer.LookupCommitRequestApply(ctx, attach)
+		if err != nil {
+			p.log.V(1).Info("Cannot read the CommitRequest's apply yet; will retry",
+				"name", client.ObjectKeyFromObject(cr), "err", err.Error())
+			return commitRequestPollInterval, nil
+		}
+		if !known {
+			apply = unknownApply(cr)
+		}
 	}
 	before := cr.Status.DeepCopy()
 	setAppliedCondition(cr, apply)
@@ -79,6 +88,12 @@ func (c *commitRequestCore) followApply(
 	return commitRequestPollInterval, nil
 }
 
+// nothingToApply is the apply of a request that pushed no commit: none was needed.
+func nothingToApply() git.SaveApply {
+	return git.SaveApply{Done: true, Applied: true, Reason: git.SaveApplyNothingToApply,
+		Message: "the request pushed no commit, so there is nothing to apply"}
+}
+
 // unknownApply is the apply of a request its worker knows nothing of: still being taken on, shortly
 // after Ready, and lost after applyLostAfter.
 func unknownApply(cr *configbutleraiv1alpha3.CommitRequest) git.SaveApply {

internal/controller/commitrequest_apply_test.go

@@ -83,6 +83,21 @@ func TestCommitRequestReconcile_ASettledApplyWithoutAnApply(t *testing.T) {
 	}
 }
 
+// TestCommitRequestReconcile_ARequestThatPushedNothingHasNothingToApply: a request that ended with no
+// commit settles Applied=True, NothingToApply, without asking the worker.
+func TestCommitRequestReconcile_ARequestThatPushedNothingHasNothingToApply(t *testing.T) {
+	c := newCommitRequestClient(t, nil, catchingUp(newCommitRequest("save-1")))
+	f := &fakeFinalizer{resolved: true, result: git.FinalizeResult{Outcome: git.FinalizeNoOpenWindow, Branch: "main"}}
+	r := &CommitRequestReconciler{Client: c, APIReader: c, Finalizer: f}
+
+	reconcileCommitRequest(t, r, "save-1")
+	reconcileCommitRequest(t, r, "save-1")
+	got := fetchCommitRequest(t, c, "save-1")
+	requireCondition(t, got, ConditionTypeReady, metav1.ConditionTrue, crReasonNoWindow)
+	requireCondition(t, got, ConditionTypeApplied, metav1.ConditionTrue, git.SaveApplyNothingToApply)
+	assert.Zero(t, f.applyLookups)
+}
+
 // TestCommitRequestReconcile_AnApplyNobodyKnowsIsLost: a worker that knows nothing of the apply
 // shortly after Ready is still taking it on; long after, it restarted, and Applied says it is lost.
 func TestCommitRequestReconcile_AnApplyNobodyKnowsIsLost(t *testing.T) {

internal/controller/commitrequest_core.go

@@ -316,8 +316,9 @@ func (c *commitRequestCore) claim(
 // attribution is on. refused reports a request no fact names on a provider that has received audit
 // facts: its fact went missing, and it must not take the window that stays unattributed.
 //
-// The lookup matches the request's resourceVersion, which every status write moves, so it is made
-// only before the claim, and the claim records the answer.
+// The lookup asks for the fact that created the request, by its uid: a later write to it, such as a
+// label patch, names who wrote that, not who asked for the save, and must not take that actor's
+// window. It is made only before the claim, and the claim records the answer.
 //
 // Whether the provider has received audit facts is read after the wait, not from provider: the first
 // facts may arrive during it, and a request whose own fact is still missing then must be refused
@@ -330,14 +331,13 @@ func (c *commitRequestCore) creator(
 	}
 	c.streams.ensure(provider.AuditRoute())
 	user, outcome := c.authors.ResolveAuthor(ctx, watch.AuthorQuery{
-		AuditRoute:      provider.AuditRoute(),
-		GVR:             commitRequestGVR(),
-		UID:             cr.UID,
-		ResourceVersion: cr.ResourceVersion,
-		Namespace:       cr.Namespace,
-		Labels:          cr.Labels,
-		Name:            cr.Name,
-		ExactCapable:    true,
+		AuditRoute:   provider.AuditRoute(),
+		GVR:          commitRequestGVR(),
+		UID:          cr.UID,
+		Namespace:    cr.Namespace,
+		Name:         cr.Name,
+		ExactCapable: true,
+		Creation:     true,
 	})
 	if outcome.NamesActor() {
 		return user.Username, false, nil

internal/controller/commitrequest_core_test.go

@@ -377,7 +377,9 @@ func TestCommitRequestCore_TheAuthorComesFromAuditAndIsClaimedFirst(t *testing.T
 	query := authors.queries[0]
 	assert.Equal(t, "default.local", query.AuditRoute, "the route of the GitTarget's KubeProvider")
 	assert.Equal(t, commitRequestGVR(), query.GVR)
-	assert.Equal(t, stored.ResourceVersion, query.ResourceVersion, "the version the request was created with")
+	assert.Equal(t, stored.UID, query.UID)
+	assert.True(t, query.Creation, "only the fact that created the request names its author")
+	assert.Empty(t, query.ResourceVersion, "a later write to the request moves it, and must not name the author")
 	assert.True(t, query.ExactCapable)
 	assert.Equal(t, 1, streams.Len(), "the stream of commitrequests on the route is followed")
 	assert.True(t, claimedFirst, "the author is written before the attach")
@@ -445,6 +447,32 @@ func TestCommitRequestCore_ARequestNoFactNames(t *testing.T) {
 	})
 }
 
+// TestCommitRequestCore_ARefusedRequestHasNothingToApply: a request that asked to catch up and was
+// refused before any worker heard of it, as AuthorUnresolved is, settles Applied=True, NothingToApply,
+// without asking the worker, which would know nothing of it and report it lost.
+func TestCommitRequestCore_ARefusedRequestHasNothingToApply(t *testing.T) {
+	provider := &configv1alpha3.KubeProvider{
+		ObjectMeta: metav1.ObjectMeta{Name: localProvider, Namespace: "default"},
+		Spec:       configv1alpha3.KubeProviderSpec{InCluster: &configv1alpha3.KubeProviderInCluster{}},
+		Status: configv1alpha3.KubeProviderStatus{Conditions: []metav1.Condition{{
+			Type: KubeProviderConditionAuditFactsReceived, Status: metav1.ConditionTrue, Reason: "Received",
+		}}},
+	}
+	cr := newCommitRequest("save")
+	cr.Spec.CatchUp = configv1alpha3.CatchUpAfterPush
+	c := newCommitRequestClient(t, nil, cr, mirroringTarget(localProvider), provider)
+	f := committed()
+	core := &commitRequestCore{reader: c, finalizer: f, authors: &fakeAuthors{outcome: git.AttributionUnresolved}}
+	serveStep(t, core, controlPath(c), "save")
+	serveStep(t, core, controlPath(c), "save")
+
+	got := fetchCommitRequest(t, c, "save")
+	requireCondition(t, got, ConditionTypeReady, metav1.ConditionTrue, crReasonAuthorUnresolved)
+	requireCondition(t, got, ConditionTypeApplied, metav1.ConditionTrue, git.SaveApplyNothingToApply)
+	assert.Zero(t, f.applyLookups, "no worker heard of it")
+	assert.False(t, followsApply(&got), "settled")
+}
+
 // factsArriveDuringTheWait names nobody, and while it waits the provider's first audit facts arrive:
 // its AuditFactsReceived turns True.
 type factsArriveDuringTheWait struct {

internal/queue/author_fact.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/http"
 	"net/url"
 	"strings"
 	"time"
@@ -149,6 +150,10 @@ type AuthorFact struct {
 	DisplayName string `json:"displayName,omitempty"`
 	Email       string `json:"email,omitempty"`
 	Verb        string `json:"verb,omitempty"`
+	// Created marks the write that created the object: a create, or a patch the API server answered
+	// 201 Created, which is server-side apply making an object that did not exist. A creation query
+	// reads only facts carrying it, so a later write to the same object never answers who created it.
+	Created bool `json:"created,omitempty"`
 	// AuditID is the one field the join never reads and is kept anyway. It is what ties a commit
 	// authored by the wrong person back to the audit event that named them, which is the single
 	// question a mis-attribution investigation asks and the one thing nothing else in the system
@@ -277,6 +282,7 @@ func AuthorFactFromEvent(
 		DisplayName:     user.DisplayName,
 		Email:           user.Email,
 		Verb:            event.Verb,
+		Created:         createdObject(event),
 		AuditID:         string(event.AuditID),
 		ResourceVersion: resourceVersionFromEvent(event),
 	}
@@ -388,6 +394,15 @@ func deleteCollectionItems(obj *runtime.Unknown) []deleteCollectionItem {
 // object's metadata.resourceVersion; requestObject.resourceVersion is the pre-write RV on
 // update-style requests, so it is intentionally ignored. objectRef.resourceVersion is usually the
 // empty precondition RV on writes, so it is only the last resort.
+// createdObject reports whether the event's write created its object: a create, or any write the
+// API server answered 201 Created.
+func createdObject(event auditv1.Event) bool {
+	if strings.EqualFold(event.Verb, "create") {
+		return true
+	}
+	return event.ResponseStatus != nil && event.ResponseStatus.Code == http.StatusCreated
+}
+
 func resourceVersionFromEvent(event auditv1.Event) string {
 	if rv := rvFromRawObject(event.ResponseObject); rv != "" {
 		return rv

internal/queue/author_fact_test.go

@@ -151,6 +151,30 @@ func TestAuthorFactFromEvent_ObjectWriteCarriesTheIdentityTheJoinNeeds(t *testin
 	require.Nil(t, fact.UIDs)
 }
 
+// TestAuthorFactFromEvent_MarksTheWriteThatCreatedTheObject: a create, or a patch answered 201
+// Created (server-side apply making the object), is marked Created; any other write is not.
+func TestAuthorFactFromEvent_MarksTheWriteThatCreatedTheObject(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		verb    string
+		code    int32
+		created bool
+	}{
+		{name: "create", verb: "create", code: 201, created: true},
+		{name: "server-side apply of a new object", verb: "patch", code: 201, created: true},
+		{name: "patch", verb: "patch", code: 200},
+		{name: "update", verb: "update", code: 200},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			event := mutationEvent(tc.verb, "uid-1", "101", "alice")
+			event.ResponseStatus = &metav1.Status{Code: tc.code}
+			fact, _, ok := AuthorFactFromEvent(t.Context(), event, 0)
+			require.True(t, ok)
+			require.Equal(t, tc.created, fact.Created)
+		})
+	}
+}
+
 // TestAuthorFact_UnmarshalRefusesAFactThatNamesNobody pins the wire contract. A fact exists to name
 // somebody, so `author` is the one required field, and missing, null, and empty are one violation
 // rather than three. Go cannot state that as a type — every type has a constructible zero value, and

internal/queue/fact_index.go

@@ -97,6 +97,10 @@ type FactQuery struct {
 	// the write that took the object out of view (a label exit), and for a terminating one it can be
 	// the finalizer PATCH, which says who cleared a finalizer, not who asked for the deletion.
 	Terminating bool
+	// Creation asks who CREATED the object, and nothing else: only a fact marked Created with the
+	// object's uid answers it, whatever its resourceVersion. A request whose author decides what it
+	// may do asks this, so a later write to it, such as a label patch, never names its writer.
+	Creation bool
 }
 
 // FactIndexConfig configures the index. Every zero field falls back to its Default… constant, so
@@ -255,7 +259,8 @@ func isRemovalVerb(verb string) bool {
 	return strings.EqualFold(verb, "delete") || strings.EqualFold(verb, deleteCollectionVerb)
 }
 
-// Lookup reads the index once. A write (an ADDED or MODIFIED event) tries, strongest first:
+// Lookup reads the index once. A creation query reads only the object's creating fact, by uid
+// (lookupCreation). A write (an ADDED or MODIFIED event) tries, strongest first:
 //
 //  1. the exact (uid, rv) fact;
 //  2. the rv-only escape hatch;
@@ -277,6 +282,12 @@ func (i *FactIndex) Lookup(query FactQuery) AuthorResolution {
 	if !ok {
 		return AuthorResolution{Result: AttributionAbsent}
 	}
+	if query.Creation {
+		if fact, found := facts.lookupCreation(query.UID, cutoff); found {
+			return AuthorResolution{Fact: fact, Result: AttributionExact}
+		}
+		return AuthorResolution{Result: AttributionAbsent}
+	}
 	if !query.ExactCapable {
 		return i.lookupRemoval(facts, query, now, cutoff)
 	}
@@ -637,6 +648,10 @@ func (q FactQuery) scope() factScope {
 // wakes it for nothing.
 func (q FactQuery) waiterKeys() []factWaiterKey {
 	scope := q.scope()
+	if q.Creation {
+		// A creating fact carries the uid, so it always fills the latest tier's key.
+		return []factWaiterKey{{scope: scope, kind: factKindLatest, value: q.UID}}
+	}
 	var keys []factWaiterKey
 	if q.UID != "" && q.ResourceVersion != "" {
 		keys = append(keys, factWaiterKey{scope: scope, kind: factKindExact,

internal/queue/fact_index_store.go

@@ -241,6 +241,22 @@ func (s *scopeFacts) lookupLatest(uid string, cutoff time.Time) (AuthorFact, boo
 	return liveFact(s.latest[uid], cutoff)
 }
 
+// lookupCreation reads the fact that created the object with uid, from the exact tier, where every
+// fact carrying a uid and a resourceVersion is filed under its own version, so no later write
+// replaces it. It walks the tier, which costs one pass over a type's facts: it serves only the
+// CommitRequest author lookup, made once per request.
+func (s *scopeFacts) lookupCreation(uid string, cutoff time.Time) (AuthorFact, bool) {
+	if uid == "" {
+		return AuthorFact{}, false
+	}
+	for key, entry := range s.exact {
+		if key.uid == uid && entry.fact.Created {
+			return liveFact(entry, cutoff)
+		}
+	}
+	return AuthorFact{}, false
+}
+
 // lookupRemovalPointer reads the sticky removal pointer. It takes NO cutoff, and it is the only lookup here
 // that does not: the pointer's horizon is a count rather than a clock.
 //