fix(watch): stop reporting a routine watch reconnect as a failure

.coverage-baseline

@@ -1 +1 @@
-77.9
+78.0

docs/UPGRADING.md

@@ -7,6 +7,48 @@ 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.
 
+## Watch reconnects no longer report as failures
+
+**Not breaking, but two observable surfaces move.** Neither needs a manifest change; both may need
+an alert rule updated.
+
+### `watch_sessions_ended_total` gains `reason="closed"`
+
+The API server ends every watch on its own randomized timeout, and the stream reconnects from its
+cursor in about two seconds. That ending used to be counted as `reason="error"`, so on a healthy
+cluster the `error` series was almost entirely the protocol working. It is now `closed`.
+
+An alert on `reason="error"` gets quieter and more accurate; nothing needs to change for it to be
+correct. A dashboard panel split by reason gains a series. If you want to catch a stream that is
+reconnecting in a hot loop, alert on the `closed` rate — see
+[interpreting-metrics.md](./interpreting-metrics.md).
+
+The reconnect itself no longer touches stream readiness at all. A clean session end says only that
+the session ended; whether anything is wrong is the next open's answer, and a failing open still
+marks the stream `Blocked`/`WatchError` one backoff later. This removes a `Ready=False` flip that
+used to occur roughly every forty minutes per watched type on a completely healthy cluster — which
+also means `kubectl wait --for=condition=Ready` and CI gates built on it are no longer racing it.
+
+A mid-stream `410` is now graded `Replaying`/`ExpiredResourceVersion` rather than
+`Blocked`/`WatchError`, matching how the cursor-resume path has always graded the same expiry.
+
+### Event severity is graded by `Stalled`
+
+Kubernetes Events for a persisted `Ready` transition on `GitTarget`, `WatchRule`,
+`ClusterWatchRule`, `GitProvider` and `ClusterProvider` used to be `Warning` for anything that was
+not `Ready=True`. They are now `Warning` only when `Stalled=True` — a human is needed — and `Normal`
+for everything progressing: streams still replaying after a restart, a rule waiting on a GitTarget
+that is still coming up.
+
+**If you route alerts on Event `type=Warning` for these kinds**, you will stop seeing startup
+replays and dependency waits. That is the intent: previously essentially every `Warning` on a
+healthy cluster was the system working, so a real block arrived indistinguishable from the routine
+ones. Alerting that wants the old breadth should route on the `Ready` condition rather than on
+Event severity, which is the more durable signal in any case. Match on `Ready != True`, not on
+`Ready=False`: a progressing gate that has not been established at all publishes `Ready=Unknown`
+(a GitTarget whose source cluster has not yet reported reachability does exactly this), and
+`kstatus` treats both as in progress.
+
 ## `{namespaceOrCluster}` is gone; `{namespace}` renders `_cluster`
 
 **Breaking for a placement template that names `{namespaceOrCluster}`.** There is now one

docs/interpreting-metrics.md

@@ -189,7 +189,7 @@ boundary, the commit, the push. Background:
 | --- | --- | --- | --- |
 | `watch_events_total` | counter | `gittarget_namespace`, `gittarget_name`, `group`, `version`, `resource`, `outcome` | The ingest census: every delivered watch event, exactly once. `outcome` is `routed` / `unchanged` / `operation_filtered` / `not_object` / `bookmark` / `shutdown` / `stream_error` / `route_failed`. |
 | `watch_event_handling_seconds` | histogram | `group`, `version`, `resource` | How long a stream was **busy** on one event, attribution wait included. Occupancy, not queue delay. |
-| `watch_sessions_ended_total` | counter | `group`, `version`, `resource`, `reason` | `expired` (cursor out of history, forcing a rebuild) / `error` / `stopped`. |
+| `watch_sessions_ended_total` | counter | `group`, `version`, `resource`, `reason` | `closed` (the API server's routine watch timeout: the most common ending on a healthy cluster) / `expired` (cursor out of history, forcing a rebuild) / `error` / `stopped`. |
 | `watch_replay_duration_seconds` | histogram | `group`, `version`, `resource` | Time to `initial-events-end`: what a `410` storm charges. |
 | `watch_recovery_total` | counter | `gittarget_namespace`, `gittarget_name`, `group`, `resource`, `mode` | One per completed recovery. `mode` is `cursor_resume` / `type_reconcile` / `replay` / `list_fallback`. No `version`: a recovery covers a cell. |
 | `watch_types` | gauge | `source_cluster`, `gittarget_namespace`, `gittarget_name`, `state` | Types this target resolves, by `streaming` / `replaying` / `blocked`. `sum` is the resolved total. |
@@ -333,6 +333,20 @@ sum by (resource, reason) (rate(gitopsreverser_watch_sessions_ended_total[15m]))
 histogram_quantile(0.95, sum by (le) (rate(gitopsreverser_watch_replay_duration_seconds_bucket[5m])))
 ```
 
+A steady trickle of `reason="closed"` is health, not a fault: the API server ends every watch on a
+randomized timeout and the stream reconnects from its cursor in seconds. Because that reconnect
+deliberately moves no condition — a two-second reattach must not flip `StreamsRunning` and make
+`kubectl wait --for=condition=Ready` flaky on a healthy cluster — this counter is the **only** place
+a reconnect is visible, and so the only way to catch a stream that is reconnecting in a hot loop:
+
+```promql
+sum by (resource) (rate(gitopsreverser_watch_sessions_ended_total{reason="closed"}[15m])) > 0.05
+```
+
+One close per stream per watch timeout is expected; the threshold above is roughly an order of
+magnitude above that. `error` now means a session that ended on a genuine fault, and `expired`
+remains the one that costs a full rebuild.
+
 `git_commits_total` carries the **`BranchWorker`'s**
 `{provider_namespace, provider_name, branch, author_kind, message_source}` identity, not a GitTarget: one worker can
 serve several GitTargets sharing a provider+branch, coalescing their writes into one commit batch, so

docs/spec/status-conditions-guide.md

@@ -152,6 +152,17 @@ document cannot silently disagree.
    patch lands, not beside each condition write, so a reconcile that writes `Ready` twice (a
    placeholder, then the real outcome) announces only the value that was actually stored.
 
+6. **Grade the Event's severity by `Stalled`, not by "`Ready` is not `True`".** `Warning` means a
+   human is needed — which is what `Stalled=True` already means, and what kstatus calls `Failed`.
+   Everything progressing is `Normal`: a stream still replaying after a restart, a rule waiting on a
+   GitTarget that is still coming up. These are different questions, and conflating them costs the
+   severity its meaning — if every ordinary startup emits a `Warning`, a real block arrives looking
+   exactly like the routine ones and nobody picks it out. The accumulator has already decided this
+   (`readinessProgressing` vs `readinessStalled`) and publishes the verdict as `Stalled`, so reading
+   it back cannot drift from the trio. Alerting that wants every not-ready transition should route
+   on the `Ready` condition itself rather than on Event severity, matching `Ready != True` so it
+   covers the `Unknown` that an unestablished gate publishes as well as `False`.
+
 ## Applied to this project
 
 GitTarget, WatchRule, and ClusterWatchRule use the kstatus trio as the generic layer:

internal/controller/status.go

@@ -244,6 +244,20 @@ func (s *reconcileStatus) commit(ctx context.Context) error {
 // stored. Announcing intermediate values would fill `kubectl describe` with states that never
 // existed. Events are how a transient failure that clears before anyone looks stays visible at all,
 // and they are the only thing an Event-driven alerting pipeline can route.
+//
+// SEVERITY is read off Stalled, not off "Ready is not True". Those are different questions, and
+// conflating them is what made the severity stop carrying information: every progressing state —
+// a stream still replaying after a restart, a rule waiting on a GitTarget that is still coming up —
+// announced itself as a Warning, so on a healthy cluster essentially every Warning was the system
+// working. A real block then arrives looking exactly like the sixty-fourth routine one.
+//
+// The accumulator has already answered the question this needs: readinessProgressing means the gate
+// clears on its own (kstatus InProgress) and readinessStalled means it needs a human (kstatus
+// Failed). That verdict is published as Stalled, so reading it here cannot drift from the trio.
+//
+// An absent Stalled keeps Warning. Nothing in this package publishes Ready without the trio — every
+// beginStatus caller goes through applyReadiness — but the conservative arm means a future one that
+// does is loud rather than silently downgraded.
 func (s *reconcileStatus) recordReadyTransition() {
 	if s.recorder == nil {
 		return
@@ -257,9 +271,10 @@ func (s *reconcileStatus) recordReadyTransition() {
 		return
 	}
 
-	eventType := corev1.EventTypeWarning
-	if after.Status == metav1.ConditionTrue {
-		eventType = corev1.EventTypeNormal
+	eventType := corev1.EventTypeNormal
+	if stalled := findCondition(*s.conditions, ConditionTypeStalled); stalled == nil ||
+		stalled.Status == metav1.ConditionTrue {
+		eventType = corev1.EventTypeWarning
 	}
 	s.recorder.Event(s.object, eventType, after.Reason, after.Message)
 }

internal/controller/status_event_severity_test.go

@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package controller
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3"
+)
+
+// Severity is read off Stalled, not off "Ready is not True".
+//
+// The two are different questions, and conflating them is what made the severity stop carrying
+// information: a stream still replaying after a restart, or a rule waiting on a GitTarget that is
+// still coming up, announced itself as a Warning. On a healthy cluster essentially every Warning
+// was then the system working, so a real block arrived looking exactly like the routine ones.
+func TestRecordReadyTransition_SeverityFollowsStalled(t *testing.T) {
+	for name, tc := range map[string]struct {
+		contribute func(*readiness)
+		want       string
+	}{
+		// kstatus InProgress. It clears on its own; nobody needs to be paged.
+		"a progressing gate is Normal": {
+			contribute: func(rd *readiness) {
+				rd.progressing(metav1.ConditionFalse, ReasonProgressing, "Waiting for streams to run")
+			},
+			want: corev1.EventTypeNormal,
+		},
+		// Unknown is the other honest way to not be ready yet, and it is equally not a failure.
+		"an unestablished gate is Normal": {
+			contribute: func(rd *readiness) {
+				rd.progressing(metav1.ConditionUnknown, ReasonProgressing, "Nothing observed yet")
+			},
+			want: corev1.EventTypeNormal,
+		},
+		// kstatus Failed. Waiting changes nothing, which is exactly what a Warning should mean.
+		"a stalled gate is a Warning": {
+			contribute: func(rd *readiness) {
+				rd.stalled(GitTargetReasonBranchNotAllowed, "branch is not allowed")
+			},
+			want: corev1.EventTypeWarning,
+		},
+		"reaching Ready is Normal": {
+			contribute: func(*readiness) {},
+			want:       corev1.EventTypeNormal,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			recorder := record.NewFakeRecorder(4)
+			st := newSeverityTestStatus(t, recorder)
+
+			rd := newReadiness("converged", "GitTarget is not stalled")
+			tc.contribute(rd)
+			st.applyReadiness(rd)
+			require.NoError(t, st.commit(context.Background()))
+
+			assert.Equal(t, tc.want, eventTypeOf(t, recorder))
+		})
+	}
+}
+
+// The conservative arm. Nothing in this package publishes Ready without the trio — every
+// beginStatus caller goes through applyReadiness — so this pins the behaviour for a future one
+// that does not: it stays loud rather than being silently downgraded to Normal.
+func TestRecordReadyTransition_ReadyWithoutTheTrioStaysAWarning(t *testing.T) {
+	recorder := record.NewFakeRecorder(4)
+	st := newSeverityTestStatus(t, recorder)
+
+	st.set(ConditionTypeReady, metav1.ConditionFalse, ReasonProgressing, "no trio written")
+	require.NoError(t, st.commit(context.Background()))
+
+	assert.Equal(t, corev1.EventTypeWarning, eventTypeOf(t, recorder))
+}
+
+func newSeverityTestStatus(t *testing.T, recorder record.EventRecorder) *reconcileStatus {
+	t.Helper()
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme))
+	target := &configbutleraiv1alpha3.GitTarget{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "acme", Namespace: "tenant-acme", ResourceVersion: "1", Generation: 1,
+		},
+	}
+	c := fake.NewClientBuilder().WithScheme(scheme).
+		WithObjects(target).WithStatusSubresource(target).Build()
+	return beginStatus(c, recorder, target)
+}
+
+// eventTypeOf reads the leading word of the recorded event, which is where FakeRecorder puts the
+// type ("Normal Succeeded converged").
+func eventTypeOf(t *testing.T, recorder *record.FakeRecorder) string {
+	t.Helper()
+
+	select {
+	case got := <-recorder.Events:
+		require.NotEmpty(t, got)
+		return strings.Fields(got)[0]
+	default:
+		t.Fatal("expected exactly one Event for the persisted Ready transition")
+		return ""
+	}
+}

internal/watch/target_watch.go

@@ -533,7 +533,9 @@ func (m *Manager) runTargetWatch(
 			return
 		}
 		if err != nil {
-			m.markTargetStreamState(gitDest, stream.key.Cell(), StreamStateBlocked, StreamReasonWatchError, err.Error())
+			if state, reason, mark := targetStreamStateForSessionEnd(err); mark {
+				m.markTargetStreamState(gitDest, stream.key.Cell(), state, reason, err.Error())
+			}
 			log.Info("target watch session ended; reconnecting",
 				"gvr", stream.key.GVR.String(), "namespace", stream.key.Namespace, "err", err.Error())
 		}
@@ -543,6 +545,35 @@ func (m *Manager) runTargetWatch(
 	}
 }
 
+// targetStreamStateForSessionEnd grades one watch session ending, and reports whether that
+// grading is worth publishing at all.
+//
+// A session ENDING says only that it ended. Whether anything is WRONG is the next open's answer,
+// and every open-failure path below already marks the cell Blocked/WatchError one backoff later.
+// Reporting a clean end as Blocked cost a Warning event, a Ready flip and a cadence change on a
+// perfectly healthy cluster, every time the API server hit its randomized watch timeout — roughly
+// every forty minutes, per type. The protocol working is not a failure.
+//
+// It deliberately does NOT report a distinct non-stalled reason for the reconnect either, which is
+// the other shape this could take. Any state other than Streaming flips StreamsRunning to False
+// and so flips Ready, which would make `kubectl wait --for=condition=Ready` and every CI gate
+// built on it intermittently fail against a healthy cluster. The reconnect stays observable where
+// it cannot flap a condition: watch_sessions_ended_total{reason="closed"}.
+//
+// An expired cursor is graded exactly as the resume path already grades it — Replaying, not
+// Blocked. It is routine watch-history pressure that forces a rebuild, and which session happens
+// to observe the 410 must not change how it reads.
+func targetStreamStateForSessionEnd(err error) (StreamState, string, bool) {
+	switch {
+	case errors.Is(err, errTargetWatchClosed):
+		return "", "", false
+	case errors.Is(err, errTargetWatchExpired):
+		return StreamStateReplaying, StreamReasonExpiredResourceVersion, true
+	default:
+		return StreamStateBlocked, StreamReasonWatchError, true
+	}
+}
+
 // followFactsForWatch takes one reference on the attribution fact stream for the (audit route,
 // group/resource) this watch covers, and returns the release for it. It is a no-op returning a
 // no-op in configured-author mode, where no follower runs and no subscription is meaningful.