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) {