From 9af2397462b3430660a07545f32ac0ac8bd77361 Mon Sep 17 00:00:00 2001
From: Jared Rickert <jaredrickert52@gmail.com>
Date: Thu, 6 Aug 2026 17:18:06 -0500
Subject: [PATCH] fix(tapper): stop retrying telemetry batches the hub rejects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A 400 was treated as retryable, so a client whose payload the hub will
never accept re-sent the same rejected batch on every flush for the life
of the process. The hub's decoder disallows unknown fields, so this is
exactly what a client one version ahead of its hub does — it cannot
negotiate the payload down, and no amount of retrying will help.

Treat 400 like the other terminal statuses and disable the process
reporter. Telemetry degrades to nothing, which is the correct outcome
for a best-effort channel, and it lets the client and the hub release in
either order rather than requiring the hub to go first.

413 stays retryable: batch contents vary, so a too-large batch says
nothing about the next one.
---
 pkg/tapper/invocation_telemetry.go      | 18 ++++++++++++++++--
 pkg/tapper/invocation_telemetry_test.go | 22 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/pkg/tapper/invocation_telemetry.go b/pkg/tapper/invocation_telemetry.go
index fef1122..40ad617 100644
--- a/pkg/tapper/invocation_telemetry.go
+++ b/pkg/tapper/invocation_telemetry.go
@@ -242,10 +242,24 @@ func (r *httpInvocationReporter) send(ctx context.Context, batch []InvocationEve
 	}
 }
 
+// disablesInvocationTelemetry reports whether a status means "stop trying" as
+// opposed to "try again later". Every code here says the hub will never accept
+// this client's events, so retrying only wastes requests.
+//
+// 400 is in the list because a hub older than the client rejects any field it
+// does not know — its decoder disallows unknown fields — and the client cannot
+// negotiate the payload down. Without this, a tap carrying a newly added field
+// would re-send a guaranteed-rejected batch on every flush for the life of the
+// process. Degrading to no telemetry is the correct outcome, and it is what
+// lets the client and the hub release in either order.
+//
+// 413 is deliberately absent: batch contents vary, so a too-large batch says
+// nothing about the next one.
 func disablesInvocationTelemetry(status int) bool {
 	switch status {
-	case http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound,
-		http.StatusMethodNotAllowed, http.StatusGone, http.StatusNotImplemented:
+	case http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden,
+		http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusGone,
+		http.StatusNotImplemented:
 		return true
 	default:
 		return false
diff --git a/pkg/tapper/invocation_telemetry_test.go b/pkg/tapper/invocation_telemetry_test.go
index 19a214d..58ffce6 100644
--- a/pkg/tapper/invocation_telemetry_test.go
+++ b/pkg/tapper/invocation_telemetry_test.go
@@ -171,6 +171,28 @@ func TestHTTPInvocationReporterTimeoutAndOlderHubAreBestEffort(t *testing.T) {
 		r.Close(ctx)
 		require.Equal(t, int64(1), calls.Load())
 	})
+
+	// A hub older than this client rejects any field it does not know, and the
+	// client cannot negotiate the payload down. Retrying a guaranteed-rejected
+	// batch on every flush is pure waste, so 400 stops the process reporter the
+	// same way an unsupported endpoint does.
+	t.Run("rejected payload disables process reporter", func(t *testing.T) {
+		var calls atomic.Int64
+		client := &http.Client{Transport: telemetryRoundTripFunc(func(_ *http.Request) (*http.Response, error) {
+			calls.Add(1)
+			return &http.Response{StatusCode: http.StatusBadRequest, Body: io.NopCloser(strings.NewReader("")), Header: make(http.Header)}, nil
+		})}
+		r := newHTTPInvocationReporter("https://old.example.com/api/v1/telemetry/invocations", "token", "test", invocationReporterOptions{
+			client: client, batchSize: 1, flushInterval: time.Hour,
+		})
+		r.Report(InvocationEvent{Surface: "mcp", Tool: "first"})
+		require.Eventually(t, r.disabled.Load, time.Second, time.Millisecond)
+		r.Report(InvocationEvent{Surface: "mcp", Tool: "second"})
+		ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+		defer cancel()
+		r.Close(ctx)
+		require.Equal(t, int64(1), calls.Load(), "the rejected batch must not be retried")
+	})
 }
 
 func TestHTTPInvocationReporterConcurrentReportAndClose(t *testing.T) {