fix(controller): publish a refreshed status.remote when the floor lifts, and time the bi-directional loop

config/deployment.yaml

@@ -46,8 +46,11 @@ spec:
         - --author-attribution-grace=10s
         # A short refresh interval so the e2e suite can watch an idle GitTarget notice a branch
         # somebody moved from outside, without waiting out the 10m default. The effective
-        # granularity is still the 5m control-plane reconcile.
-        - --git-refresh-interval=30s
+        # granularity is still the 5m control-plane reconcile. Kept above the 1m status.remote
+        # publication floor, as the default is: below it, a reconcile inside the floor refreshes
+        # an observation that has not been published yet, and our own push is published as a
+        # fetch.
+        - --git-refresh-interval=90s
         # The push receiver, so the e2e suite can deliver a Git host's push webhook through the
         # gitops-reverser-receiver Service (receiver-service.yaml) instead of waiting for a refresh.
         - --receiver-bind-address=:9292

docs/spec/e2e-bi-directional-corner.md

@@ -178,6 +178,25 @@ dedicated CI leg:
 | Task | `task test-e2e-bi-directional` |
 | CI leg | `bi-directional` |
 
+### The timing spec: `bi_directional_timing_e2e_test.go`
+
+The other specs prove what the loop does and only bound how long it may take.
+[`bi_directional_timing_e2e_test.go`](../../test/e2e/bi_directional_timing_e2e_test.go)
+(label `timing`) measures it. It mirrors a labeled ConfigMap into the folder a Flux
+Kustomization or an Argo CD Application applies, then times each sample from the stimulus
+(an API edit, or a Git-side push landing on Gitea) to the effect an operator would look for:
+
+- the commit on the branch;
+- Flux applying it after the default 30s quiet period;
+- the trigger holding its applier again;
+- the live object changing after a Git-side push through the receiver.
+
+Each duration goes into a `gmeasure` experiment, which Ginkgo prints as an N/min/median/max
+table and writes into the JSON report. `task allure-e2e-report` attaches it to the spec as
+"Measurements". Every sample also has a budget, and a sample over it fails the spec. The
+budgets catch a path that fell back to the 5-minute steady tick, not a slow runner.
+`E2E_TIMING_SAMPLES` sets the sample count (default 3).
+
 ### The third spec: `provenance_markers_e2e_test.go`
 
 The corner hosts a third spec, and it is the reason the corner is worth more than the
@@ -605,4 +624,6 @@ namespaces and resource names, and none is `Serial`.
 - [`test/e2e/flux_bi_directional_e2e_test.go`](../../test/e2e/flux_bi_directional_e2e_test.go) and
   [`test/e2e/argocd_bi_directional_e2e_test.go`](../../test/e2e/argocd_bi_directional_e2e_test.go):
   the two specs in this corner
+- [`test/e2e/bi_directional_timing_e2e_test.go`](../../test/e2e/bi_directional_timing_e2e_test.go):
+  the loop's latency, as numbers
 - [`internal/sanitize/types.go`](../../internal/sanitize/types.go): the strip lists

docs/spec/e2e-serial-registry.md

@@ -29,17 +29,22 @@ row below.
 
 ## Serial containers
 
+A Serial container runs alone, after every parallel spec of its CI leg has finished, so its whole
+runtime adds to that leg's wall clock. Place it on a leg with time to spare: `CRD Lifecycle`
+carries the `crd-lifecycle` label rather than `manager` for exactly that reason, which puts it in
+the `full-core` leg beside the restart specs instead of at the end of the longest leg.
+
 | File | Container | Shared state it touches | Why name isolation can't fix it |
 |---|---|---|---|
 | [test/e2e/restart_reconcile_e2e_test.go](../../test/e2e/restart_reconcile_e2e_test.go) | `Restart Reconcile Safety` | Rollout-restarts the controller deployment. | The controller is a singleton; restarting it disrupts in-flight reconciles/commits for every other spec. |
 | [test/e2e/flux_restart_recovery_e2e_test.go](../../test/e2e/flux_restart_recovery_e2e_test.go) | `Flux Restart Recovery` | Rollout-restarts the controller deployment while a Flux apply runs, and labels a node so that apply's Deployment can schedule. | The controller is a singleton, as for `Restart Reconcile Safety`; a node label is cluster-wide. |
 | [test/e2e/image_refresh_test.go](../../test/e2e/image_refresh_test.go) | `image refresh dependency chain` | Changes the controller image / redeploys the controller. | Same singleton controller; an image swap perturbs all concurrent specs. |
-| [test/e2e/crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go) | `Manager CRD Lifecycle` | Installs/deletes a cluster-scoped CRD and watches all `customresourcedefinitions` cluster-wide, asserting exact Git file presence/absence. | A concurrent CRD install/delete bumps the global discovery-catalog generation and re-resolves every GitTarget's watched-type tables, delaying this spec's reconcile (CRD file appears late / post-delete sweep lags — the 649↔673 flake). A cluster-wide catalog bump cannot be scoped by name. |
+| [test/e2e/crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go) | `CRD Lifecycle` | Installs/deletes a cluster-scoped CRD and watches all `customresourcedefinitions` cluster-wide, asserting exact Git file presence/absence. | A concurrent CRD install/delete bumps the global discovery-catalog generation and re-resolves every GitTarget's watched-type tables, delaying this spec's reconcile (CRD file appears late / post-delete sweep lags — the 649↔673 flake). A cluster-wide catalog bump cannot be scoped by name. |
 | [test/e2e/tilt_playground_e2e_test.go](../../test/e2e/tilt_playground_e2e_test.go) | `playground` | Reusable manual-playground fixture: fixed (non-randomized) `tilt-playground` namespace and `playground` repo/provider names, preserved across runs. | A singleton by construction — its resource names are intentionally stable so `task playground-*` can re-attach, so per-run name isolation does not apply. |
 
 ### De-serialized
 
-`Manager CRD Lifecycle` ([crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go))
+`CRD Lifecycle` ([crd_lifecycle_e2e_test.go](../../test/e2e/crd_lifecycle_e2e_test.go))
 **was de-serialized and then re-serialized** — it is in the Serial table above, not
 here. Finding #2's fix ([manager.go:1191](../../internal/watch/manager.go#L1191)) —
 resnapshot a target only when its *resolved* plan hash changes — removed the worst

internal/controller/gittarget_controller.go

@@ -203,9 +203,10 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, providerErr
 	}
 
-	// awaitingAnswer: this reconcile is about to ask the remote where the branch is, so the answer
-	// lands after the status write below. See requeueForRemoteAnswer.
-	awaitingAnswer := r.publishGitObservations(st, &target, providerNS, repoIdentityOf(gitProvider))
+	// remoteWake: whether this reconcile is about to ask the remote where the branch is, so the
+	// answer lands after the status write below, and how long the floor holds back an answer it
+	// already has. See requeueForRemoteAnswer.
+	remoteWake := r.publishGitObservations(st, &target, providerNS, repoIdentityOf(gitProvider))
 
 	// Ahead of every gate, for the reason the layout stanza above is: the join series has to carry
 	// the targets that are NOT working. git_pushes_total names a branch that stopped advancing and
@@ -304,7 +305,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// requeueAfter shortens the cadence when the status write lost a race: the object then holds
 	// the WINNER's older answer and nothing re-enqueues it, because the For() predicate filters
 	// status-only updates by design.
-	requeue := st.requeueAfter(requeueForRemoteAnswer(gitTargetRequeue(rd), awaitingAnswer))
+	requeue := st.requeueAfter(requeueForRemoteAnswer(gitTargetRequeue(rd), remoteWake))
 	// TEMPORARY at Info, while Failure A is open. The data plane can converge and this status not
 	// follow it: a run has shown the render gate reaching True and both this GitTarget and every
 	// WatchRule on it still publishing "Rechecking" two minutes later, with no dropped reconcile
@@ -921,34 +922,57 @@ func (r *GitTargetReconciler) publishGitObservations(
 	target *configbutleraiv1alpha3.GitTarget,
 	providerNS string,
 	repo git.RepoIdentity,
-) bool {
+) remoteWake {
 	layout, scanned := r.observeLayout(target)
 	publishLayout(st, target, layout, scanned)
 
 	now := r.clockNow()
 	remote, remoteSeen := r.observeRemote(target, providerNS)
-	r.publishRemote(st, target, remote, remoteSeen, repo, now)
-	// Whether the refresh this reconcile goes on to enqueue will actually reach the remote: the
-	// worker spends a connection only when what it holds is older than the interval configured.
-	return r.GitRefreshInterval > 0 && (!remoteSeen || remote.Age(now) >= r.GitRefreshInterval)
+	heldFor := r.publishRemote(st, target, remote, remoteSeen, repo, now)
+	return remoteWake{
+		// Whether the refresh this reconcile goes on to enqueue will actually reach the remote: the
+		// worker spends a connection only when what it holds is older than the interval configured.
+		asked:   r.GitRefreshInterval > 0 && (!remoteSeen || remote.Age(now) >= r.GitRefreshInterval),
+		heldFor: heldFor,
+	}
+}
+
+// remoteWake is what status.remote needs from the next reconcile's timing.
+type remoteWake struct {
+	// asked: this reconcile asked the remote where the branch is, and the answer lands after it.
+	asked bool
+	// heldFor: the floor holds back an answer this target already has, for this long.
+	heldFor time.Duration
 }
 
 // requeueForRemoteAnswer shortens the cadence for a target that has just ASKED the remote where
-// its branch is.
+// its branch is, or that holds an answer the floor has not let out yet.
 //
 // It is the one piece of scheduling this status field gets, and it exists because the refresh is
 // enqueued during the reconcile: the worker answers moments later, after the status write, so the
 // answer would otherwise sit undelivered until the next steady tick. An idle target would then
 // spend a connection every interval and publish what it learned a whole tick late — visible to an
 // operator as a target that follows a foreign push ten minutes after noticing it.
 //
-// Nothing else is threaded anywhere. A reconcile that asked nothing keeps its own cadence, and no
-// early return has to carry a deadline.
-func requeueForRemoteAnswer(cadence time.Duration, asked bool) time.Duration {
-	if asked && RemotePublicationInterval < cadence {
-		return RemotePublicationInterval
+// Asking is not enough on its own, because the requeue it returns can be lost. A refresh that
+// finds the branch moved re-enqueues the target while the asking reconcile is still running, and
+// the queue folds a delayed requeue into an item that is already waiting (controller-runtime's
+// priority queue keeps the earlier time). The reconcile that then runs holds a fresh answer, so it
+// asks nothing, and the floor stops it publishing: without heldFor it returned the steady cadence,
+// and the answer waited the whole tick after all. So whichever reconcile meets the held answer
+// comes back when the floor lifts.
+//
+// Nothing else is threaded anywhere. A reconcile that asked nothing and holds nothing keeps its
+// own cadence, and no early return has to carry a deadline.
+func requeueForRemoteAnswer(cadence time.Duration, wake remoteWake) time.Duration {
+	next := cadence
+	if wake.asked && RemotePublicationInterval < next {
+		next = RemotePublicationInterval
+	}
+	if wake.heldFor > 0 && wake.heldFor < next {
+		next = wake.heldFor
 	}
-	return cadence
+	return next
 }
 
 // ensureEventStream wires this GitTarget to the branch worker for the repository its GitProvider

internal/controller/gittarget_remote.go

@@ -118,14 +118,17 @@ func (r *GitTargetReconciler) observeRemote(
 // Two things ignore the floor, because they are correctness rather than throughput: the FIRST
 // observation, which is the difference between "nothing has looked" and an answer, and any
 // correction that involves the repository changing under the target.
+//
+// It returns how long the floor is holding news back, zero when nothing is held. A held answer
+// must not wait for whatever reconcile happens to come next: see requeueForRemoteAnswer.
 func (r *GitTargetReconciler) publishRemote(
 	st *reconcileStatus,
 	target *configbutleraiv1alpha3.GitTarget,
 	observed git.RemoteObservation,
 	seen bool,
 	repo git.RepoIdentity,
 	now time.Time,
-) {
+) time.Duration {
 	ref := types.NewResourceReference(target.Name, target.Namespace)
 	published, everPublished := r.remotePublications.last(ref, target.UID)
 	// What this target PUBLISHED was proved against a repository it no longer points at: its
@@ -137,12 +140,12 @@ func (r *GitTargetReconciler) publishRemote(
 		// published with, and it has to go rather than go stale.
 		target.Status.Remote = nil
 		st.afterPersist(func() { r.remotePublications.forget(ref) })
-		return
+		return 0
 	}
 	if !seen {
 		// Nothing has looked at this branch since the process started. What is published is the
 		// last thing anyone proved about it, and there is nothing better to replace it with.
-		return
+		return 0
 	}
 	next := &configbutleraiv1alpha3.GitTargetRemoteStatus{
 		Commit:         observed.Commit,
@@ -151,11 +154,11 @@ func (r *GitTargetReconciler) publishRemote(
 		Parent:         parentStatusOf(observed),
 	}
 	if !remoteStatusIsNews(target.Status.Remote, next) {
-		return
+		return 0
 	}
 	// The floor, skipped for the two corrections above and for the first publication.
 	if everPublished && !staleRepository && now.Sub(published.at) < RemotePublicationInterval {
-		return
+		return published.at.Add(RemotePublicationInterval).Sub(now)
 	}
 	target.Status.Remote = next
 	// Only once the write has landed, and dated THEN rather than now. A patch refused by the
@@ -166,6 +169,7 @@ func (r *GitTargetReconciler) publishRemote(
 	// so on a slow reconcile a cooldown dated from it can be half spent before the write lands,
 	// and a reconcile queued behind it would publish again straight away.
 	st.afterPersist(func() { r.remotePublications.record(ref, target.UID, r.clockNow(), observed.Repo) })
+	return 0
 }
 
 // withdrawRemote decides whether what is published has to be taken back rather than replaced.

internal/controller/gittarget_remote_test.go

@@ -417,12 +417,55 @@ func TestPublishRemote_ADeletedTargetReleasesItsLedgerEntry(t *testing.T) {
 // it, or an idle one spends a connection every interval and publishes what it learned a whole tick
 // late. A target that asked nothing keeps its own cadence.
 func TestRequeueForRemoteAnswer_ShortensOnlyWhenSomethingWasAsked(t *testing.T) {
-	assert.Equal(t, RemotePublicationInterval, requeueForRemoteAnswer(RequeueSteadyInterval, true))
-	assert.Equal(t, RequeueSteadyInterval, requeueForRemoteAnswer(RequeueSteadyInterval, false))
-	assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, true),
+	assert.Equal(t, RemotePublicationInterval,
+		requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{asked: true}))
+	assert.Equal(t, RequeueSteadyInterval, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{}))
+	assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, remoteWake{asked: true}),
 		"a target already on a faster loop is not slowed down to the publication interval")
 }
 
+// TestRequeueForRemoteAnswer_ComesBackWhenTheFloorLifts is the reconcile after a lost requeue. The
+// asking reconcile's one-minute requeue is folded away when the refresh's answer re-enqueues the
+// target while it runs; the reconcile that follows holds that answer fresh, asks nothing, and is
+// stopped by the floor. It must come back when the floor lifts, not a whole steady tick later.
+func TestRequeueForRemoteAnswer_ComesBackWhenTheFloorLifts(t *testing.T) {
+	held := 59 * time.Second
+	assert.Equal(t, held, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{heldFor: held}))
+	assert.Equal(t, held, requeueForRemoteAnswer(RequeueSteadyInterval, remoteWake{asked: true, heldFor: held}),
+		"the earlier of the two wakes wins")
+	assert.Equal(t, time.Second, requeueForRemoteAnswer(time.Second, remoteWake{heldFor: held}),
+		"a target already on a faster loop is not slowed down to the floor")
+}
+
+// TestPublishRemote_ReportsHowLongTheFloorHoldsAnAnswer: the floor delays news, and says for how
+// long, so the reconcile can come back for it. What is not news, and what is published, holds
+// nothing back.
+func TestPublishRemote_ReportsHowLongTheFloorHoldsAnAnswer(t *testing.T) {
+	r := &GitTargetReconciler{}
+	target := remoteTestTarget()
+	start := time.Now()
+	previous := r.clock
+	r.clock = func() time.Time { return start }
+	defer func() { r.clock = previous }()
+
+	none := git.RepoIdentity{}
+	later := start.Add(2 * time.Second)
+
+	st := &reconcileStatus{}
+	first := observation("aaaa", start, git.ObservedByPush, none)
+	assert.Zero(t, r.publishRemote(st, target, first, true, none, start),
+		"the first observation is published, so nothing is held")
+	st.runPersisted()
+
+	moved := observation("bbbb", later, git.ObservedByFetch, none)
+	assert.Equal(t, 58*time.Second, r.publishRemote(&reconcileStatus{}, target, moved, true, none, later),
+		"news two seconds after a write waits out the rest of the floor")
+	assert.Equal(t, "aaaa", target.Status.Remote.Commit)
+
+	assert.Zero(t, r.publishRemote(&reconcileStatus{}, target, first, true, none, later),
+		"what is already published is not news, so the floor holds nothing back")
+}
+
 // remoteLedgerFixture drives publishRemote through the REAL status session, so what advances the
 // ledger is a patch the API server accepted rather than a callback a test chose to run.
 type remoteLedgerFixture struct {