From 78f3a130452caaa8a4461d2f8fde6070bafd9c9e Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 27 Oct 2026 14:10:33 +0000 Subject: [PATCH 1/3] fix(commitrequest): keep the creating fact past a no-op write, and order the 0.54 upgrade - The fact index files a creating fact in its own uid-keyed slot. A no-op patch returns the object at the resourceVersion the create stamped, so in the exact tier its fact replaced the creator's, and a request reapplied before its author resolved settled AuthorUnresolved. - The attribution guide and the tuned policy require RequestResponse capture of commitrequests create and patch: the author is matched by the uid and resourceVersion in responseObject, which Metadata level leaves out, and server-side apply creates with a patch. - UPGRADING gains an ordered "Upgrading from 0.54.0" checklist: stop new saves, drain open requests, upgrade, remove the admission webhook's leftovers (Helm keeps cert-manager's Secret; install.yaml removes nothing), and delete and recreate a request still open afterwards. The chart's upgrade section links to it before helm upgrade. - The mutation-lab normalizer writes a list's own resourceVersion as , outside the numbering: on k3s v1.37.1 the deletecollection list returned the last item's revision in some runs and a newer one in others, which made the nightly capture report drift. - configuration.md and the design text say a competing save of the same author is refused with SaveInProgress, not queued; architecture.md drops the Redis records for request authorship. Co-Authored-By: Claude Opus 5.5 --- charts/gitops-reverser/README.md | 5 ++ docs/UPGRADING.md | 53 ++++++++++++++++- docs/architecture.md | 5 +- docs/attribution-setup-guide.md | 10 ++-- docs/configuration.md | 9 ++- docs/design/commit-timing-surface.md | 6 +- docs/spec/commitrequest-design.md | 10 ++-- internal/mutationlab/normalize/normalize.go | 44 +++++++++++++- .../mutationlab/normalize/normalize_test.go | 23 ++++++++ internal/queue/fact_index.go | 6 ++ internal/queue/fact_index_store.go | 57 ++++++++++++------- internal/queue/fact_index_test.go | 21 ++++++- internal/queue/key_prefix.go | 4 +- test/e2e/cluster/audit/policy.yaml | 10 ++-- test/mutationlab/corpus/CLUSTER.md | 10 +++- .../audit.deletecollection.yaml | 2 +- 16 files changed, 224 insertions(+), 51 deletions(-) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 24da08ae..47c3d314 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -345,6 +345,11 @@ curl http://localhost:8080/metrics ## Upgrading +Before upgrading, read the [upgrade guide](../../docs/UPGRADING.md) entries for every release you +cross, and follow their steps in order. Upgrading from 0.54.0 starts with +[draining the `CommitRequest`s](../../docs/UPGRADING.md#upgrading-from-0540-drain-the-commitrequests-first). +Then apply the new release's `crds.yaml` ([CRD lifecycle](#crd-lifecycle)), and upgrade. + ### Standard Upgrade ```bash diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 6c1ae59d..e482c909 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,53 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Upgrading from 0.54.0: drain the `CommitRequest`s first + +Existing providers, branches, targets and claims keep running across this upgrade; none of the +0.53.0 recreation [below](#upgrading-from-0530-recreate-the-connection-objects-in-one-pass) applies. +What needs ordering is the `CommitRequest`s that are in progress: 0.54.0 wrote progress conditions +this release does not take over, so a request still open when it starts can stay open for good. + +1. Stop creating `CommitRequest`s: pause the automation and people that create them. +2. Wait for every request to end. This lists the ones that have not, and should come back empty + once each has passed its `spec.window` bounds: + + ```bash + kubectl get commitrequests -A -o json | jq -r '.items[] + | select([.status.conditions[]? | select((.type == "Ready" or .type == "Stalled") + and .status == "True")] | length == 0) + | "\(.metadata.namespace)/\(.metadata.name)"' + ``` + +3. Adjust the audit policy and values, as the [entry below](#the-admission-webhook-is-gone-a-commitrequests-author-comes-from-audit) + describes: `commitrequests` `create` and `patch` at `RequestResponse` level, and no + `servers.admission` or `--admission-webhook*` settings. +4. Apply the release's `crds.yaml`, then upgrade, as the + [chart's upgrade steps](../charts/gitops-reverser/README.md#upgrading) describe. +5. Remove the admission webhook's leftovers. + - **Helm** deletes the `ValidatingWebhookConfiguration` and the `Certificate` it rendered, but + not the Secret cert-manager wrote for that certificate. Delete it: + + ```bash + kubectl -n gitops-reverser delete secret gitops-reverser-admission-server-cert --ignore-not-found + ``` + + - **`install.yaml`** is applied with `kubectl apply`, which removes nothing. Delete the three + resources yourself: + + ```bash + kubectl delete validatingwebhookconfiguration gitops-reverser-validate-operator-types --ignore-not-found + kubectl -n gitops-reverser delete certificate gitops-reverser-admission-server-cert --ignore-not-found + kubectl -n gitops-reverser delete secret gitops-reverser-admission-server-cert --ignore-not-found + ``` + + The names are those of a release named `gitops-reverser` in namespace `gitops-reverser`; use + yours if they differ. +6. Run the command from step 2 again. A request it still lists was open across the upgrade and may + never end; one made on the cluster GitOps Reverser runs in, for a `GitTarget` that mirrors another + cluster, certainly will not. Delete each, and create it again: on the cluster the `GitTarget` + mirrors, for such a `GitTarget`. Then resume creating requests. + ## The admission webhook is gone; a `CommitRequest`'s author comes from audit GitOps Reverser installs no admission webhook. The chart no longer renders the @@ -22,8 +69,10 @@ those that remain expire on their own. certificate with `servers.metrics.tls.enabled`. The default install needs no cert-manager. - **A request's author.** A `CommitRequest`'s author comes from its own audit event, as a write's does, and is reported in `status.author`. The `AuthorAttributed` condition is gone. Keep `create` - of `commitrequests` in your audit policy; the write catch-all of the - [tuned policy](../test/e2e/cluster/audit/policy.yaml) covers it. A request no audit event names, on + and `patch` of `commitrequests` in your audit policy at `RequestResponse` level: the author is + matched by the uid and resourceVersion in the response, which `Metadata` level leaves out, and + server-side apply creates with a `patch`. The write catch-all of the + [tuned policy](../test/e2e/cluster/audit/policy.yaml) covers both. A request no audit event names, on a cluster that sends audit events, ends `Ready=True` with reason `AuthorUnresolved` and saves nothing. With attribution off, a request names nobody, as it did with the webhook off. - **Status while it runs.** A `CommitRequest` in progress is `Ready=Unknown`, reason `Saving` until the diff --git a/docs/architecture.md b/docs/architecture.md index 4081a098..438712e1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -172,8 +172,9 @@ the Flux `Kustomization` applier and the Argo CD `Application` applier, its auto **Redis/Valkey is optional but advised.** The default configured-author mode runs without it: a plain `helm install` comes up healthy and watches initialize from current state on startup. When an endpoint -is configured, Redis stores cursors for reconnecting running watches and the coordination records used by -CommitRequest author capture and HA. Attribution does not require it on its own: its facts travel on a +is configured, Redis stores cursors for reconnecting running watches. A CommitRequest's author comes from +the audit fact of the write that created it, like any write's, and needs no Redis record. +Attribution does not require it on its own: its facts travel on a selectable transport, Redis Streams by default and an in-process ring with `--author-attribution-transport=memory`, which is refused with more than one replica. HA will require Redis as the shared store across replicas. diff --git a/docs/attribution-setup-guide.md b/docs/attribution-setup-guide.md index 62341e03..0a11b425 100644 --- a/docs/attribution-setup-guide.md +++ b/docs/attribution-setup-guide.md @@ -120,10 +120,12 @@ other subresources. Keep manual scale writes if you want their authors recorded. The example includes Secret writes at `RequestResponse` level. To keep Secret bodies out of the webhook, exclude core `secrets` too. Those writes then have no audit evidence for attribution. -Keep `create` of `commitrequests` in the `configbutler.ai` group covered, at `Metadata` level at -least: a `CommitRequest`'s author comes from its own audit event, as a write's does. Without it a -request names nobody, and cannot save the window of an author the audit names. The write catch-all -covers it. +Keep `create` and `patch` of `commitrequests` in the `configbutler.ai` group covered at +`RequestResponse` level. A `CommitRequest`'s author comes from the audit event that created it, +matched by the uid and resourceVersion in the event's `responseObject`. `Metadata` level leaves +the response out, so the request cannot be matched. `patch` is there because server-side apply +creates an object with a `patch`. Without the creating event, a request names nobody and cannot +save the window of an author the audit names. The write catch-all covers both verbs. The audit policy controls evidence collection; `WatchRule` and `ClusterWatchRule` control what gets written to Git. diff --git a/docs/configuration.md b/docs/configuration.md index 2749da5b..3c83f67f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -3059,9 +3059,12 @@ separates work the worker already collected from work that reaches it afterward. write was made after the request: a write still held for its audit fact arrives later and lands in the next window. -Competing requests from one author are served first come, first served. A window carries at most -one request; the next waits for the next window. A re-sent request is recognized by its identity -and restarts nothing: its deadlines and, for `Next`, its one close are fixed at first registration. +Competing requests from one author are not queued. A window carries one message, so while an +earlier request of the same author on the same `GitTarget` waits for a window or collects one, a +later request is refused at once with [`SaveInProgress`](#its-status). A `Next` request is not +refused by one that is collecting: it closes that window under the earlier request's message, then +waits for its own. A re-sent request is recognized by its identity and restarts nothing: its +deadlines and, for `Next`, its one close are fixed at first registration. #### Sizing the request window diff --git a/docs/design/commit-timing-surface.md b/docs/design/commit-timing-surface.md index a76ff757..d9ee7cca 100644 --- a/docs/design/commit-timing-surface.md +++ b/docs/design/commit-timing-surface.md @@ -166,8 +166,10 @@ Two rules keep the edges honest: never closes a second window. - **`Next` needs time to wait.** CEL refuses `attach: Next` with `attachTimeout: 0s`: the save would close the author's window and give up in the same instant, collecting nothing. -- **Competing saves are served first come, first served,** in the order the worker first registered - them. A window carries at most one save; the next waits for the next window. +- **Competing saves are not queued.** A window carries one message, so a later save of the same + author on the same `GitTarget` is refused with `SaveInProgress` while the earlier one waits or + collects. A `Next` save closes a collecting window under the earlier message, then waits for its + own. What `Next` guarantees is narrower than its name suggests. It separates work the worker already collected from work that reaches the worker afterward. It cannot prove a write was made after the diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index f3d89339..50d16c1e 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -102,9 +102,9 @@ A window is only ever opened by a write; a request attaches to one and never ope registration, the worker sets the attach deadline to registration plus `window.attachTimeout`, and repeated reconciles are idempotent: they restart nothing and, for `attach: Next`, close nothing again. With `CurrentOrNext` the request attaches to the author's window already open at registration, before -any deadline is consulted, so `attachTimeout: 0s` can attach at all. Otherwise it waits, and waiting -requests are served first come, first served. A request whose deadline has passed resolves and never -takes a later window; when its deadline and a matching write are ready on the same wake, expiry wins. +any deadline is consulted, so `attachTimeout: 0s` can attach at all. Otherwise it waits. A request +whose deadline has passed resolves and never takes a later window; when its deadline and a matching +write are ready on the same wake, expiry wins. On attach, the request's `idleTimeout` and `maxDuration` replace the window's timers: `maxDuration` runs from the attach, and so does the first idle interval. A waiting request gets a new window before @@ -129,7 +129,9 @@ its first edits before they reach Git. A Git host's push notice leaves an attach Each request attaches to at most one window and cannot rename a finalized commit, including one waiting for push. A bundle provides no ordering guarantee. The wait does not reserve a transaction. Competing requests are -served in registration order, first come, first served. +not queued: a later request of the same author on the same `GitTarget` is refused with +`SaveInProgress` while the earlier one waits or collects, except that a `Next` request closes a +collecting window under the earlier message and then waits for its own. `spec.message` is literal, including template-like text and surrounding spaces. It is never parsed as a template, so a request author cannot execute one. Omission uses diff --git a/internal/mutationlab/normalize/normalize.go b/internal/mutationlab/normalize/normalize.go index f44e7adc..ea1cd4d8 100644 --- a/internal/mutationlab/normalize/normalize.go +++ b/internal/mutationlab/normalize/normalize.go @@ -56,6 +56,14 @@ func expiredMessageRVs(message string) []string { // stable without losing the ordering that matters. const tsPlaceholder = "" +// listRVPlaceholder replaces a list's own metadata.resourceVersion, which stays out of the +// relational space. It is the revision the API server read the list at, not one any object +// carries: on k3s v1.37.1 a deletecollection's list returned the last deleted object's +// resourceVersion in some runs and a newer one in others, as an unrelated write landed in between. +// Numbered with the rest, it shifted every later placeholder by one, so the corpus alternated +// between two shapes on one cluster. +const listRVPlaceholder = "" + // isTimestampKey reports whether a JSON key's string value is an RFC3339 // timestamp the normalizer collapses to tsPlaceholder. "time" covers // managedFields[].time; lastTransitionTime/lastUpdateTime cover status.conditions[] @@ -188,9 +196,14 @@ func (c *collector) walk(v any) { c.collectGenerateNameSuffix(t) // Walk keys in sorted order so first-appearance indexing is deterministic // regardless of Go's randomized map iteration. + list := isList(t) for _, k := range sortedKeys(t) { - c.collectScalar(k, t[k]) - c.walk(t[k]) + v := t[k] + if list && k == "metadata" { + v = withoutKey(v, "resourceVersion") + } + c.collectScalar(k, v) + c.walk(v) } case []any: for _, e := range t { @@ -398,6 +411,11 @@ func (idx *indices) transform(v any) any { // association key (k:{"ip":"10.42.3.14"}) does not churn the corpus. out[idx.rewriteKey(k)] = idx.transformScalar(k, val, t) } + if meta, ok := out["metadata"].(map[string]any); ok && isList(t) { + if _, has := meta["resourceVersion"]; has { + meta["resourceVersion"] = listRVPlaceholder + } + } return out case []any: out := make([]any, len(t)) @@ -586,6 +604,28 @@ func (idx *indices) rewriteGeneratedName(v any, parent map[string]any) (string, return "", false } +// isList reports whether m is a list object: items beside its own metadata. +func isList(m map[string]any) bool { + _, items := m["items"].([]any) + _, meta := m["metadata"].(map[string]any) + return items && meta +} + +// withoutKey returns v with key left out when v is a map, and v unchanged otherwise. +func withoutKey(v any, key string) any { + m, ok := v.(map[string]any) + if !ok { + return v + } + out := make(map[string]any, len(m)) + for k, val := range m { + if k != key { + out[k] = val + } + } + return out +} + func placeholder(prefix string, n int) string { return fmt.Sprintf("<%s-%d>", prefix, n) } func lookup(m map[string]string, s string) any { diff --git a/internal/mutationlab/normalize/normalize_test.go b/internal/mutationlab/normalize/normalize_test.go index 797f3aab..366e75fc 100644 --- a/internal/mutationlab/normalize/normalize_test.go +++ b/internal/mutationlab/normalize/normalize_test.go @@ -309,6 +309,29 @@ func TestNormalize_DeletecollectionFanoutKeepsDistinctIdentities(t *testing.T) { } } +func TestNormalize_ListResourceVersionStaysOutOfTheRelationalSpace(t *testing.T) { + // Row 9 on k3s v1.37.1: the deletecollection list's resourceVersion is the last item's in some + // runs and a newer one in others. Both must normalize alike, and the watch DELETED placeholders + // after it must not shift. + audit := func(listRV string) string { + return `{"responseObject":{"kind":"ConfigMapList","items":[` + + `{"metadata":{"uid":"ua","resourceVersion":"10"}},{"metadata":{"uid":"ub","resourceVersion":"11"}}],` + + `"metadata":{"resourceVersion":"` + listRV + `"}}}` + } + deleted := `{"type":"DELETED","object":{"metadata":{"uid":"ua","resourceVersion":"13"}}}` + want := []string{ + `{"responseObject":{"items":[{"metadata":{"resourceVersion":"","uid":""}},` + + `{"metadata":{"resourceVersion":"","uid":""}}],"kind":"ConfigMapList",` + + `"metadata":{"resourceVersion":""}}}`, + `{"object":{"metadata":{"resourceVersion":"","uid":""}},"type":"DELETED"}`, + } + for _, listRV := range []string{"11", "12"} { + if got := normJSON(t, audit(listRV), deleted); !reflect.DeepEqual(got, want) { + t.Errorf("list rv %s:\n got %v\nwant %v", listRV, got, want) + } + } +} + func TestNormalize_FinalizerTerminalDeleteSameUIDHigherRV(t *testing.T) { // Row 8: the terminal DELETED keeps the same but a higher , proving // it is the same object at a later resourceVersion. diff --git a/internal/queue/fact_index.go b/internal/queue/fact_index.go index 5a86fbd3..4920e7ef 100644 --- a/internal/queue/fact_index.go +++ b/internal/queue/fact_index.go @@ -536,6 +536,12 @@ func (i *FactIndex) file(facts *scopeFacts, scope factScope, fact AuthorFact, no // tier too, and a removal query already waits on that key. facts.putRemoval(fact.UID, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) } + if fact.Created { + // The creating fact also takes its own slot, which a later write may not overwrite: a + // no-op patch shares the creation's (uid, rv) and would replace it in the exact tier. + // Like the removal slot it needs no waiter key, since the latest tier's key covers it. + facts.putCreation(fact.UID, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) + } return append(keys, factWaiterKey{scope: scope, kind: factKindLatest, value: fact.UID}) case fact.ResourceVersion != "": facts.putRV(fact.ResourceVersion, &indexedFact{fact: fact, at: now, seq: i.nextSeq()}) diff --git a/internal/queue/fact_index_store.go b/internal/queue/fact_index_store.go index bd83e0d3..649cd929 100644 --- a/internal/queue/fact_index_store.go +++ b/internal/queue/fact_index_store.go @@ -8,7 +8,7 @@ import ( "k8s.io/apimachinery/pkg/labels" ) -// factKind names one of the six match structures a fact can land in. The set is closed: a fact +// factKind names one of the seven match structures a fact can land in. The set is closed: a fact // that fits none of them is not stored, because a fact nothing can ever join is only memory. type factKind uint8 @@ -20,6 +20,9 @@ const ( // factKindRemoval is (uid), the sticky removal pointer: the fact that this object was DELETED and // by whom. It is the one structure a later fact may not overwrite — see putRemoval. factKindRemoval + // factKindCreation is (uid), the fact that CREATED this object. Only a creating fact is filed + // there, so a later write cannot overwrite it — see putCreation. + factKindCreation // factKindRV is (rv), the escape hatch for a fact with an rv but no uid. factKindRV // factKindName is (namespace, name), the floor for a fact with neither a uid nor an rv. Only an @@ -148,7 +151,10 @@ type scopeFacts struct { latest map[string]*indexedFact // removals is the sticky removal pointer, keyed by uid: "this object was deleted, and this actor // asked for it". It is deliberately NOT swept by the TTL — see putRemoval and sweep. - removals map[string]*indexedFact + removals map[string]*indexedFact + // creations is the creating fact, keyed by uid. Unlike removals it is swept by the TTL like the + // ordinary tiers: the one lookup that reads it waits only a grace period for its fact. + creations map[string]*indexedFact rvOnly map[string]*indexedFact byName map[nameFactKey]*indexedFact collections []*indexedCollection @@ -160,11 +166,12 @@ type scopeFacts struct { func newScopeFacts() *scopeFacts { return &scopeFacts{ - exact: map[exactFactKey]*indexedFact{}, - latest: map[string]*indexedFact{}, - removals: map[string]*indexedFact{}, - rvOnly: map[string]*indexedFact{}, - byName: map[nameFactKey]*indexedFact{}, + exact: map[exactFactKey]*indexedFact{}, + latest: map[string]*indexedFact{}, + removals: map[string]*indexedFact{}, + creations: map[string]*indexedFact{}, + rvOnly: map[string]*indexedFact{}, + byName: map[nameFactKey]*indexedFact{}, } } @@ -197,6 +204,17 @@ func (s *scopeFacts) putRemoval(uid string, entry *indexedFact) { putKeyed(s, s.removals, uid, entry, factRef{kind: factKindRemoval, uid: uid}) } +// putCreation stores the fact that created an object. Only a fact marked Created is filed here, so +// a later write cannot reach the slot. +// +// The exact tier cannot serve this, and that was measured: a no-op patch (a reapply that changes +// nothing) returns the object at the resourceVersion it already had, so its fact is filed under the +// creating fact's own (uid, rv) and replaces it. A CommitRequest re-applied before its author was +// resolved then lost its creator. +func (s *scopeFacts) putCreation(uid string, entry *indexedFact) { + putKeyed(s, s.creations, uid, entry, factRef{kind: factKindCreation, uid: uid}) +} + // putRV stores the rv-only escape hatch. func (s *scopeFacts) putRV(rv string, entry *indexedFact) { putKeyed(s, s.rvOnly, rv, entry, factRef{kind: factKindRV, rv: rv}) @@ -241,20 +259,9 @@ 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. +// lookupCreation reads the fact that created the object with uid. 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 + return liveFact(s.creations[uid], cutoff) } // lookupRemovalPointer reads the sticky removal pointer. It takes NO cutoff, and it is the only lookup here @@ -341,6 +348,12 @@ func (s *scopeFacts) sweep(cutoff time.Time) int { removed++ } } + for key, entry := range s.creations { + if entry.at.Before(cutoff) { + delete(s.creations, key) + removed++ + } + } for key, entry := range s.rvOnly { if entry.at.Before(cutoff) { delete(s.rvOnly, key) @@ -404,6 +417,8 @@ func (s *scopeFacts) remove(ref factRef) bool { return removeKeyed(s.latest, ref.uid, ref.seq) case factKindRemoval: return removeKeyed(s.removals, ref.uid, ref.seq) + case factKindCreation: + return removeKeyed(s.creations, ref.uid, ref.seq) case factKindRV: return removeKeyed(s.rvOnly, ref.rv, ref.seq) case factKindName: @@ -480,6 +495,8 @@ func (s *scopeFacts) live(ref factRef) bool { return liveKeyed(s.latest, ref.uid, ref.seq) case factKindRemoval: return liveKeyed(s.removals, ref.uid, ref.seq) + case factKindCreation: + return liveKeyed(s.creations, ref.uid, ref.seq) case factKindRV: return liveKeyed(s.rvOnly, ref.rv, ref.seq) case factKindName: diff --git a/internal/queue/fact_index_test.go b/internal/queue/fact_index_test.go index 754b0f8b..096ed823 100644 --- a/internal/queue/fact_index_test.go +++ b/internal/queue/fact_index_test.go @@ -165,7 +165,7 @@ func TestFactIndex_CreationQueryReadsOnlyTheCreatingFact(t *testing.T) { created := objectFact("alice", "101") created.Verb, created.Created = "create", true harness.publish(factIndexTestStream("prod-eu-1"), created, objectFact("label-controller", "102")) - harness.waitForFacts(3) + harness.waitForFacts(4) creation := FactQuery{AuditRoute: "prod-eu-1", GroupResource: deploymentsGroupResource(), UID: factIndexTestUID, Namespace: "team-a", ExactCapable: true, Creation: true} @@ -183,6 +183,25 @@ func TestFactIndex_CreationQueryReadsOnlyTheCreatingFact(t *testing.T) { harness.absent(creation) } +// TestFactIndex_CreationSurvivesANoOpWriteAtItsVersion: a no-op patch returns the object at the +// resourceVersion the create stamped, so its fact shares the creating fact's (uid, rv). It must not +// replace the creator: a CommitRequest re-applied before its author was resolved lost its creator +// that way and settled AuthorUnresolved. +func TestFactIndex_CreationSurvivesANoOpWriteAtItsVersion(t *testing.T) { + harness := newFactIndexHarness(t, FactIndexConfig{}) + created := objectFact("alice", "101") + created.Verb, created.Created = "create", true + noOp := objectFact("ci-bot", "101") + noOp.Verb = "patch" + harness.publish(factIndexTestStream("prod-eu-1"), created, noOp) + harness.waitForFacts(3) + + resolution := harness.resolve(FactQuery{AuditRoute: "prod-eu-1", GroupResource: deploymentsGroupResource(), + UID: factIndexTestUID, Namespace: "team-a", ExactCapable: true, Creation: true}) + require.Equal(t, AttributionExact, resolution.Result) + require.Equal(t, "alice", resolution.Fact.Author, "the creator, not the no-op writer") +} + // TestFactIndex_CreationQueryWaitsForTheCreatingFact: a creation query parked before the fact arrives // is woken by it. func TestFactIndex_CreationQueryWaitsForTheCreatingFact(t *testing.T) { diff --git a/internal/queue/key_prefix.go b/internal/queue/key_prefix.go index 4be44954..26d8ed7d 100644 --- a/internal/queue/key_prefix.go +++ b/internal/queue/key_prefix.go @@ -12,8 +12,8 @@ import ( // dwarfs the key it namespaces. const maxKeyPrefixLength = 128 -// DefaultKeyPrefix is the root namespace every Redis key (cursors, fact streams, and command -// author records alike) carries when --redis-key-prefix is not set. It is also the value every +// DefaultKeyPrefix is the root namespace every Redis key (cursors and fact streams alike) carries +// when --redis-key-prefix is not set. It is also the value every // release before the flag existed used, so the default is a no-op upgrade. const DefaultKeyPrefix = "gitops-reverser" diff --git a/test/e2e/cluster/audit/policy.yaml b/test/e2e/cluster/audit/policy.yaml index 2aa32aca..c538b1eb 100644 --- a/test/e2e/cluster/audit/policy.yaml +++ b/test/e2e/cluster/audit/policy.yaml @@ -52,8 +52,8 @@ rules: - delete - deletecollection # No resources listed = Matches everything else (including CRDs). - # This also captures `commitrequests.configbutler.ai` create events, which - # name a CommitRequest's author, as a write's event names its author. If this - # catch-all is ever narrowed, keep at least Metadata-level capture for - # `commitrequests` create events — the author is joined by uid and - # resourceVersion, which Metadata carries. + # This also captures the `commitrequests.configbutler.ai` event that created + # a CommitRequest, which names its author. If this catch-all is ever narrowed, + # keep RequestResponse capture of `commitrequests` create AND patch (server-side + # apply creates with a patch): the author is joined by the uid and + # resourceVersion in responseObject, which Metadata level leaves out. diff --git a/test/mutationlab/corpus/CLUSTER.md b/test/mutationlab/corpus/CLUSTER.md index 66a26d27..19453d39 100644 --- a/test/mutationlab/corpus/CLUSTER.md +++ b/test/mutationlab/corpus/CLUSTER.md @@ -32,9 +32,13 @@ records it for the same request. Every other scenario reproduced unchanged. Moving from `v1.36.4+k3s1` to `v1.37.1+k3s1` (2026-10-27, the e2e cluster's version) changed three shapes, each reproduced by a second capture: -- `configmap/deletecollection/`: the `deletecollection` response's list - resourceVersion is now the last deleted object's own resourceVersion, where - 1.36 returned a newer one, so the watch DELETED placeholders shift by one; +- `configmap/deletecollection/`: the watch DELETED placeholders shifted by + one. The `deletecollection` response's list resourceVersion is the revision + the API server read the list at, which on 1.37.1 is the last deleted object's + resourceVersion in some runs and a newer one in others (two of four captures + each), where 1.36 returned a newer one in every run. The normalizer now writes + a list's own resourceVersion as ``, outside the numbered `` + placeholders, so both outcomes give the same corpus; - `pod/graceful-delete/`: the deleted pod's `PodReadyToStartContainers` condition now carries `reason: PodSandboxNotReady` and a message; - `configmap/owner-ref-cascade/`: the garbage collector's `userAgent` names the diff --git a/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml b/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml index 64c07d38..ba700a41 100644 --- a/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml +++ b/test/mutationlab/corpus/configmap/deletecollection/audit.deletecollection.yaml @@ -48,7 +48,7 @@ responseObject: uid: kind: ConfigMapList metadata: - resourceVersion: + resourceVersion: responseStatus: code: 200 metadata: {} From 995a9c8717e5717c3cf8ead516e36543b1a571ee Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 27 Oct 2026 16:34:13 +0000 Subject: [PATCH 2/3] fix(commitrequest): time out an apply that does not settle, and set the source streams once A request followed its apply until the worker's record aged out, which only happened when another save on the same worker resolved: an apply running past 15 minutes was then reported Lost, with a message blaming a restart, and one on a quiet worker was followed for ever. The follow now has an explicit deadline, git.SaveApplyTimeout (10 minutes from Ready): past it Applied settles Unknown, reason TimedOut, naming the commit and where the apply last stood. The worker keeps an unsettled record until past that deadline, so Lost again means only a restart. The CommitRequestServer rewrote its fact-stream set under its own mutex on every step while concurrent steps read it without one; it is now set once, as the reconciler already does. Co-Authored-By: Claude Opus 5.5 --- api/v1alpha3/commitrequest_schema_test.go | 2 +- api/v1alpha3/commitrequest_types.go | 3 ++ .../bases/configbutler.ai_commitrequests.yaml | 3 ++ docs/configuration.md | 6 ++- internal/controller/commitrequest_apply.go | 46 ++++++++++++++---- .../controller/commitrequest_apply_test.go | 25 ++++++++++ internal/controller/commitrequest_server.go | 10 ++-- .../controller/commitrequest_server_test.go | 16 +++++++ internal/git/save_apply.go | 31 ++++++++++-- internal/git/save_apply_test.go | 48 +++++++++++++++++++ 10 files changed, 168 insertions(+), 22 deletions(-) create mode 100644 internal/git/save_apply_test.go diff --git a/api/v1alpha3/commitrequest_schema_test.go b/api/v1alpha3/commitrequest_schema_test.go index f6e92dd1..2f8ac120 100644 --- a/api/v1alpha3/commitrequest_schema_test.go +++ b/api/v1alpha3/commitrequest_schema_test.go @@ -19,7 +19,7 @@ import ( // TestCommitRequestSchemaRevision: raise CommitRequestSchemaRevision, and the revision line, and add // the new hash here. var commitRequestSchemaHashes = map[int]string{ - 1: "ea99cba6116e0f562c0be931c13d6580eabdc2e7ba32d486d20a0a5029163e99", + 1: "395df39c86bc4c0350eb2c476dd0ae02db2a7513f03962103416123b0cbb5476", } // commitRequestRevisionLine is the last line of the kind's description. diff --git a/api/v1alpha3/commitrequest_types.go b/api/v1alpha3/commitrequest_types.go index 4bfc6b3c..147ef5c7 100644 --- a/api/v1alpha3/commitrequest_types.go +++ b/api/v1alpha3/commitrequest_types.go @@ -228,6 +228,9 @@ type CommitRequestStatus struct { // holds the saved objects. // - False, reason CommitDropped: the branch no longer holds the commit; // its history was rewritten after the push. Only this is False. + // - Unknown, reason TimedOut: the apply did not settle within 10 minutes + // of Ready, so the request stopped following it. The commit stays + // pushed; the message says where the apply last stood. // - Unknown, reason Lost: GitOps Reverser restarted before it knew. // +optional // +listType=map diff --git a/config/crd/bases/configbutler.ai_commitrequests.yaml b/config/crd/bases/configbutler.ai_commitrequests.yaml index 64e4b8eb..ce638139 100644 --- a/config/crd/bases/configbutler.ai_commitrequests.yaml +++ b/config/crd/bases/configbutler.ai_commitrequests.yaml @@ -289,6 +289,9 @@ spec: holds the saved objects. - False, reason CommitDropped: the branch no longer holds the commit; its history was rewritten after the push. Only this is False. + - Unknown, reason TimedOut: the apply did not settle within 10 minutes + of Ready, so the request stopped following it. The commit stays + pushed; the message says where the apply last stood. - Unknown, reason Lost: GitOps Reverser restarted before it knew. items: description: Condition contains details for one aspect of the current diff --git a/docs/configuration.md b/docs/configuration.md index 3c83f67f..d11e5cf0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -3275,10 +3275,12 @@ already at the tip needs none. The request reports the outcome in its `Applied` | `True` | `Applied` | the applier applied a revision that holds the commit; `status.appliedRevision` names it, as the applier reported it | | `True` | `NothingToApply`, `NoApplierClaim`, `ApplierGone` | no apply was needed: the request pushed no commit, or the `GitTarget` names no applier claim, or stopped naming the one asked. The cluster already holds what was saved | | `False` | `CommitDropped` | the branch no longer holds the commit: its history was rewritten after the push. The only `False` | +| `Unknown` | `TimedOut` | the apply did not settle within 10 minutes of `Ready`, so the request stopped following it. The commit stays pushed, and the applier may still apply it; the message says where the apply last stood, and the applier claim's status reports later applies | | `Unknown` | `Lost` | GitOps Reverser restarted before it learnt how the apply went | -Wait for `Applied=True` for "saved and applied"; `Ready=True` stays "saved". A finished request is -deleted only after its `Applied` condition is settled. +Wait for `Applied=True` for "saved and applied"; `Ready=True` stays "saved". A request follows its +apply for 10 minutes at most: an applier that cannot catch up does not keep it open. A finished +request is deleted only after its `Applied` condition is settled. ### Finished requests are deleted after 48 hours diff --git a/internal/controller/commitrequest_apply.go b/internal/controller/commitrequest_apply.go index 20dfd60e..6c919749 100644 --- a/internal/controller/commitrequest_apply.go +++ b/internal/controller/commitrequest_apply.go @@ -21,12 +21,16 @@ import ( // save's alone and never changes here. // crReasonApplyLost is the Applied reason for an apply the worker knows nothing of: GitOps Reverser -// restarted before it learnt how it went, or the record aged out. +// restarted before it learnt how it went. const crReasonApplyLost = "Lost" +// crReasonApplyTimedOut is the Applied reason for an apply that did not settle within +// git.SaveApplyTimeout of Ready: the request stops following it. +const crReasonApplyTimedOut = "TimedOut" + // applyLostAfter is how long, from Ready, a request may find its worker knowing nothing of its apply -// before Applied says it is lost. The worker records the apply as the request resolves, so only a -// restart, or a record past its TTL, leaves it unknown. +// before Applied says it is lost. The worker records the apply as the request resolves, and keeps +// it past git.SaveApplyTimeout, so only a restart leaves it unknown. const applyLostAfter = time.Minute // followsApply reports whether a finished request still reports its apply: it asked for one, its @@ -36,10 +40,12 @@ func followsApply(cr *configbutleraiv1alpha3.CommitRequest) bool { conditionIsTrue(cr.Status.Conditions, ConditionTypeReady) && !applyReported(cr) } -// applyReported reports whether Applied is settled: True, False, or Unknown because it was lost. +// applyReported reports whether Applied is settled: True, False, or Unknown because it was lost or +// timed out. func applyReported(cr *configbutleraiv1alpha3.CommitRequest) bool { c := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeApplied) - return c != nil && (c.Status != metav1.ConditionUnknown || c.Reason == crReasonApplyLost) + return c != nil && (c.Status != metav1.ConditionUnknown || + c.Reason == crReasonApplyLost || c.Reason == crReasonApplyTimedOut) } // followApply reads where the request's apply stands, writes it to Applied, and comes back until it @@ -48,7 +54,7 @@ func applyReported(cr *configbutleraiv1alpha3.CommitRequest) bool { // // 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. +// otherwise be reported lost. One not settled git.SaveApplyTimeout after Ready times out. func (c *commitRequestCore) followApply( ctx context.Context, p commitRequestPath, cr *configbutleraiv1alpha3.CommitRequest, ) (time.Duration, error) { @@ -66,8 +72,11 @@ func (c *commitRequestCore) followApply( "name", client.ObjectKeyFromObject(cr), "err", err.Error()) return commitRequestPollInterval, nil } - if !known { + switch { + case !known: apply = unknownApply(cr) + case !apply.Done && readyFor(cr) >= git.SaveApplyTimeout: + apply = timedOutApply(cr, apply) } } before := cr.Status.DeepCopy() @@ -97,12 +106,29 @@ func nothingToApply() git.SaveApply { // 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 { - if ready := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeReady); ready != nil && - time.Since(ready.LastTransitionTime.Time) < applyLostAfter { + if readyFor(cr) < applyLostAfter { return git.SaveApply{Reason: git.SaveApplyApplying, Message: "waiting for the branch worker to take the apply"} } return git.SaveApply{Reason: crReasonApplyLost, - Message: "GitOps Reverser does not know how the apply went: it restarted before it learnt, or kept no record"} + Message: "GitOps Reverser does not know how the apply went: it restarted before it learnt"} +} + +// timedOutApply is the apply of a request whose applier did not apply its commit within +// git.SaveApplyTimeout of Ready, last standing at apply. +func timedOutApply(cr *configbutleraiv1alpha3.CommitRequest, apply git.SaveApply) git.SaveApply { + return git.SaveApply{Reason: crReasonApplyTimedOut, + Message: "the applier did not apply commit " + cr.Status.Commit + " within " + git.SaveApplyTimeout.String() + + " of the push, so this request stopped following it; the commit stays pushed, and the GitTarget's " + + "applier claim reports its applies. Last seen: " + apply.Message} +} + +// readyFor is how long the request has been Ready, zero while it is not. +func readyFor(cr *configbutleraiv1alpha3.CommitRequest) time.Duration { + ready := apimeta.FindStatusCondition(cr.Status.Conditions, ConditionTypeReady) + if ready == nil || ready.Status != metav1.ConditionTrue { + return 0 + } + return time.Since(ready.LastTransitionTime.Time) } // setAppliedCondition writes apply to Applied and status.appliedRevision. diff --git a/internal/controller/commitrequest_apply_test.go b/internal/controller/commitrequest_apply_test.go index 933f5b4c..658fe3ae 100644 --- a/internal/controller/commitrequest_apply_test.go +++ b/internal/controller/commitrequest_apply_test.go @@ -119,6 +119,31 @@ func TestCommitRequestReconcile_AnApplyNobodyKnowsIsLost(t *testing.T) { assert.False(t, followsApply(&got), "lost is settled") } +// TestCommitRequestReconcile_AnApplyThatDoesNotSettleTimesOut: an apply still running when +// git.SaveApplyTimeout has passed since Ready stops being followed, and Applied says why. +func TestCommitRequestReconcile_AnApplyThatDoesNotSettleTimesOut(t *testing.T) { + c := newCommitRequestClient(t, nil, catchingUp(newCommitRequest("save-1"))) + f := committedFinalizer() + f.apply, f.applyKnown = git.SaveApply{Reason: git.SaveApplyRetrying, Message: "the apply failed"}, true + 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, ConditionTypeApplied, metav1.ConditionUnknown, git.SaveApplyRetrying) + + ready := apimeta.FindStatusCondition(got.Status.Conditions, ConditionTypeReady) + ready.LastTransitionTime = metav1.NewTime(time.Now().Add(-git.SaveApplyTimeout)) + require.NoError(t, c.Status().Update(context.Background(), &got)) + reconcileCommitRequest(t, r, "save-1") + got = fetchCommitRequest(t, c, "save-1") + requireCondition(t, got, ConditionTypeApplied, metav1.ConditionUnknown, crReasonApplyTimedOut) + applied := apimeta.FindStatusCondition(got.Status.Conditions, ConditionTypeApplied) + assert.Contains(t, applied.Message, "within 10m0s") + assert.Contains(t, applied.Message, "the apply failed", "it says where the apply last stood") + assert.False(t, followsApply(&got), "a timeout is settled") +} + // TestCommitRequestReconcile_NoCatchUpReportsNoApply: the default asks no apply, and has no Applied. func TestCommitRequestReconcile_NoCatchUpReportsNoApply(t *testing.T) { c := newCommitRequestClient(t, nil, newCommitRequest("save-1")) diff --git a/internal/controller/commitrequest_server.go b/internal/controller/commitrequest_server.go index dbc0ba8b..e01cdfde 100644 --- a/internal/controller/commitrequest_server.go +++ b/internal/controller/commitrequest_server.go @@ -108,7 +108,9 @@ type CommitRequestServer struct { watchers map[k8stypes.NamespacedName]*commitRequestWatcher reports map[k8stypes.NamespacedName]CommitRequestsReport events chan event.GenericEvent - streams commitRequestStreams + + streamsOnce sync.Once + streams *commitRequestStreams } // commitRequestWatcher is the watch of one KubeProvider's CommitRequests, in the namespaces it lends. @@ -586,11 +588,9 @@ func (s *CommitRequestServer) consider( } func (s *CommitRequestServer) core() *commitRequestCore { - s.mu.Lock() - s.streams.set = s.FactStreams - s.mu.Unlock() + s.streamsOnce.Do(func() { s.streams = &commitRequestStreams{set: s.FactStreams} }) return &commitRequestCore{ - reader: s.Client, finalizer: s.Finalizer, authors: s.Authors, streams: &s.streams, ttl: s.TTL, + reader: s.Client, finalizer: s.Finalizer, authors: s.Authors, streams: s.streams, ttl: s.TTL, } } diff --git a/internal/controller/commitrequest_server_test.go b/internal/controller/commitrequest_server_test.go index 756f02e7..67215634 100644 --- a/internal/controller/commitrequest_server_test.go +++ b/internal/controller/commitrequest_server_test.go @@ -30,6 +30,7 @@ import ( configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/queue" ) // fakeDiscovery serves resources, and fails with err when it is set. @@ -398,3 +399,18 @@ func (f *fixedCommitRequestsReporter) CommitRequestsReport(types.NamespacedName) } func (f *fixedCommitRequestsReporter) CommitRequestsEvents() <-chan event.GenericEvent { return nil } + +// Requests are served concurrently, and each step reads the server's fact streams: they are set +// once, never rewritten under a lock the readers do not take. Run with -race. +func TestCommitRequestServer_StepsShareTheFactStreams(t *testing.T) { + s := &CommitRequestServer{FactStreams: queue.NewFactStreamSet()} + var wg sync.WaitGroup + for i := range 8 { + wg.Go(func() { + s.core().streams.ensure([]string{"route-a", "route-b"}[i%2]) + }) + } + wg.Wait() + first := s.core().streams + assert.Same(t, first, s.core().streams, "every step shares one set of streams") +} diff --git a/internal/git/save_apply.go b/internal/git/save_apply.go index e9149957..97434096 100644 --- a/internal/git/save_apply.go +++ b/internal/git/save_apply.go @@ -23,6 +23,10 @@ import ( // carries them again. A save that pushed no commit, whose GitTarget names no applier, or whose applier // stops being named is settled as applied: no apply is needed, and the cluster, where the saved objects // came from, already holds them. Only a save whose commit the branch lost is settled as not applied. +// +// A save's apply is followed for SaveApplyTimeout at most: the controller then stops following it +// and says so in Applied. The worker keeps a record that is not settled at least that long, so it +// never ages out while the controller still reads it. // Why a save's apply was settled, or why it still waits: the Applied condition's reasons. const ( @@ -35,6 +39,11 @@ const ( SaveApplyCommitDropped = "CommitDropped" ) +// SaveApplyTimeout is how long, from Ready, the controller follows the apply a CommitRequest asked +// for. A save is not kept waiting on an applier that does not catch up: past it, Applied settles as +// timed out, though the commit stays pushed and the applier may still apply it. +const SaveApplyTimeout = 10 * time.Minute + // SaveApply is how the apply a CommitRequest asked for stands. type SaveApply struct { // Done is whether it is settled: it changes no more. @@ -50,7 +59,9 @@ type SaveApply struct { } type saveApplyEntry struct { - apply SaveApply + apply SaveApply + // askedAt is when the save asked for its apply, and updatedAt when apply was last recorded. + askedAt time.Time updatedAt time.Time } @@ -62,7 +73,12 @@ func (w *BranchWorker) recordSaveApply(id commitRequestID, apply SaveApply) { if w.crApplies == nil { w.crApplies = map[commitRequestID]saveApplyEntry{} } - w.crApplies[id] = saveApplyEntry{apply: apply, updatedAt: time.Now()} + now := time.Now() + askedAt := now + if entry, ok := w.crApplies[id]; ok { + askedAt = entry.askedAt + } + w.crApplies[id] = saveApplyEntry{apply: apply, askedAt: askedAt, updatedAt: now} } // LookupSaveApply reports where the apply a CommitRequest asked for stands, and false while the @@ -74,10 +90,17 @@ func (w *BranchWorker) LookupSaveApply(namespace, name, uid string) (SaveApply, return entry.apply, ok } -// collectSaveApplies drops the entries older than the outcome TTL. The caller holds crOutcomesMu. +// collectSaveApplies drops a settled entry once it is older than the outcome TTL. One not settled is +// kept the outcome TTL past SaveApplyTimeout, counted from when it was asked: the controller reads +// Ready within the outcome TTL of that, and stops following at SaveApplyTimeout from Ready. The +// caller holds crOutcomesMu. func (w *BranchWorker) collectSaveApplies(now time.Time) { for id, entry := range w.crApplies { - if now.Sub(entry.updatedAt) > commitRequestOutcomeTTL { + expired := now.Sub(entry.updatedAt) > commitRequestOutcomeTTL + if !entry.apply.Done { + expired = now.Sub(entry.askedAt) > commitRequestOutcomeTTL+SaveApplyTimeout + } + if expired { delete(w.crApplies, id) } } diff --git a/internal/git/save_apply_test.go b/internal/git/save_apply_test.go new file mode 100644 index 00000000..4521cf32 --- /dev/null +++ b/internal/git/save_apply_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// ageSaveApply moves a save's apply record back by by, as if it was asked and last recorded then. +func ageSaveApply(w *BranchWorker, id commitRequestID, by time.Duration) { + w.crOutcomesMu.Lock() + defer w.crOutcomesMu.Unlock() + entry := w.crApplies[id] + entry.askedAt = entry.askedAt.Add(-by) + entry.updatedAt = entry.updatedAt.Add(-by) + w.crApplies[id] = entry +} + +// An apply that runs longer than the outcome TTL keeps its record while the controller still +// follows it, so another save resolving never turns it into a lost one; it is collected only past +// SaveApplyTimeout. A settled record is collected after the outcome TTL. +func TestSaveApply_AnUnsettledRecordOutlivesTheFollowDeadline(t *testing.T) { + w := &BranchWorker{} + running := commitRequestID{Namespace: "default", Name: "running", UID: "u1"} + settled := commitRequestID{Namespace: "default", Name: "settled", UID: "u2"} + w.recordSaveApply(running, SaveApply{Reason: SaveApplyApplying}) + w.recordSaveApply(settled, SaveApply{Done: true, Applied: true, Reason: SaveApplyApplied}) + collect := func() { + w.recordCommitRequestOutcome(commitRequestID{Namespace: "default", Name: "other"}, FinalizeResult{}) + } + + ageSaveApply(w, running, commitRequestOutcomeTTL+time.Minute) + ageSaveApply(w, settled, 2*commitRequestOutcomeTTL) + collect() + _, kept := w.LookupSaveApply(running.Namespace, running.Name, running.UID) + assert.True(t, kept, "an unsettled apply is kept past the outcome TTL") + _, kept = w.LookupSaveApply(settled.Namespace, settled.Name, settled.UID) + assert.False(t, kept, "a settled apply is collected after the outcome TTL") + + w.recordSaveApply(running, SaveApply{Reason: SaveApplyRetrying}) // a retry keeps when it was asked + ageSaveApply(w, running, SaveApplyTimeout) + collect() + _, kept = w.LookupSaveApply(running.Namespace, running.Name, running.UID) + assert.False(t, kept, "past the follow deadline nothing reads it any more") +} From 3b9628f3c07da4d86cbd444d0dd131d55b746fb9 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 27 Oct 2026 16:40:00 +0000 Subject: [PATCH 3/3] docs(commitrequest): say how long a request lives, stage by stage Every limit a request runs into was documented apart, and the safety window before the attach not at all. One table now lists each stage, what ends it, and what the status says then; and it says why the push alone has no limit. Co-Authored-By: Claude Opus 5.5 --- docs/configuration.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index d11e5cf0..c310b351 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -3258,6 +3258,25 @@ request produced a pushed commit: A request is served again after GitOps Reverser restarts: the path that owns it finds its own claim, and resumes with the author it recorded. +### How long a request lives + +A `CommitRequest` is a single save, not a standing order: every stage it passes through ends on its +own, and the status says which limit ended it. To save again, create a new request. + +| Stage | Ends after | What the request then says | +| --- | --- | --- | +| Picked up, author named, waiting for a branch worker | `attachTimeout` + `maxDuration` + 2 minutes, counted from creation | `Ready=False`, `Stalled=True`, reason `FinalizeFailed`: the `GitTarget` did not start a branch worker, or the request was not acted on, within the safety window | +| Waiting for a window (`WaitingForWindow`) | `attachTimeout` | `Ready=True`, reason `NoWindow` (see [When there is nothing to commit](#when-there-is-nothing-to-commit)) | +| Collecting its window (`CollectingWindow`) | `maxDuration`, at most `5m` | the window closes and is committed | +| Waiting for the push (`WaitingForPush`) | no limit of its own | stays `Ready=Unknown`; `Reconciling` says why the push waits | +| Waiting for its apply, with `catchUp: AfterPush` | 10 minutes from `Ready` | `Applied=Unknown`, reason `TimedOut` (see [Applying a save at once](#applying-a-save-at-once)) | +| Finished | 48 hours, by default | deleted (see [Finished requests are deleted after 48 hours](#finished-requests-are-deleted-after-48-hours)) | + +The push is the one wait without a limit, deliberately. Once the window is committed, the commit is +going to the remote: failing the request while the worker still retries, every 10 seconds doubling +to 5 minutes, would report a save as failed that then lands. A worker that stops before the push +fails the request instead. A request that waits there needs the remote fixed, not a new request. + ### Applying a save at once A save is in Git once it is pushed, and the cluster already runs it, so by default nothing applies @@ -3279,8 +3298,9 @@ already at the tip needs none. The request reports the outcome in its `Applied` | `Unknown` | `Lost` | GitOps Reverser restarted before it learnt how the apply went | Wait for `Applied=True` for "saved and applied"; `Ready=True` stays "saved". A request follows its -apply for 10 minutes at most: an applier that cannot catch up does not keep it open. A finished -request is deleted only after its `Applied` condition is settled. +apply for 10 minutes at most: an applier that cannot catch up does not keep it open, and a request +that ends `TimedOut` needs the applier looked at, not a new request. A finished request is deleted +only after its `Applied` condition is settled. ### Finished requests are deleted after 48 hours