fix(target-health): scrub lastError URLs and fix unknown-state gaps

Combines three closely-coupled target_health.go fixes that all touch targetHealthTracker.record/stateForLocked in the same commit, since splitting them would leave an intermediate commit that fails to compile or a test asserting behaviour its own commit does not yet implement. F1: record gains a rawURL parameter and scrubs err's text through sanitizeProviderErr before storing lastError, so a target URL configured with an embedded credential can never leak through GET /admin/api/targets. The scrub runs before truncateRunes so a long embedded URL cannot survive truncation unscrubbed. Every probe, the per-server proxy hook (mcp_a2a.go), and both federation record sites (mcp_federation.go) now pass their target's configured URL through. F3: maybeSweepTargetHealth re-checks the elapsed interval after winning the single-flight CAS, closing a window where a caller's own stale interval read could still win the CAS once a separate, faster sweep had already started and finished within the same interval. F4: targetHealthEntry gains everOK, set on record's success branch. stateForLocked now reads a target that has never once succeeded as unknown rather than healthy, even below failureThreshold — the threshold still protects against a single transient first failure, it just does not promote a target this tracker has never seen answer to healthy. targetHealthSnapshot gains observed so the admin view can still tell apart never-observed (omit every other field) from observed-but-never-succeeded (keep them) despite both reading state unknown. The now-redundant lastOK field is removed. Also included: target_health_test.go's own test-hygiene fixes (F6) — an idempotent, t.Cleanup-registered unblock ahead of srv.Close so a failing assertion cannot leave a released server handler blocked until the test binary's own timeout, and an atomic hit counter in the disabled-sweep test — since they land in the same file as the tests above. Claude-Session: https://claude.ai/code/session_01EebV5jyWvYocByNQ3oss1w
2026-10-22 06:06:59 +00:00 · 2026-09-03 02:43:40 +01:00
parent e9eac5248e
commit d2152b169c
7 changed files with 387 additions and 65 deletions
@@ -27,7 +27,7 @@ func TestTargetHealthTracker_NilReceiver_Safe(t *testing.T) {
 	t.Parallel()
 	var tr *targetHealthTracker
 	assert.NotPanics(t, func() {
-		tr.record(targetKindMCP, "alpha", true, nil, time.Millisecond, targetHealthSourceTraffic)
+		tr.record(targetKindMCP, "alpha", "", true, nil, time.Millisecond, targetHealthSourceTraffic)
 	})
 	assert.Equal(t, targetHealthUnknown, tr.stateOf(targetKindMCP, "alpha"))
 	snap := tr.snapshot(targetKindMCP, "alpha")
@@ -37,7 +37,7 @@ func TestTargetHealthTracker_NilReceiver_Safe(t *testing.T) {
 func TestTargetHealthTracker_SingleSuccess_ReadsHealthy(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(3)
-	tr.record(targetKindMCP, "alpha", true, nil, 5*time.Millisecond, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", true, nil, 5*time.Millisecond, targetHealthSourceTraffic)
 	assert.Equal(t, targetHealthHealthy, tr.stateOf(targetKindMCP, "alpha"))
 }

@@ -48,10 +48,10 @@ func TestTargetHealthTracker_SingleSuccess_ReadsHealthy(t *testing.T) {
 func TestTargetHealthTracker_BelowThreshold_StaysHealthy(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(3)
-	tr.record(targetKindMCP, "alpha", true, nil, 0, targetHealthSourceTraffic)
-	tr.record(targetKindMCP, "alpha", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", true, nil, 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
 	assert.Equal(t, targetHealthHealthy, tr.stateOf(targetKindMCP, "alpha"), "1 failure of threshold 3 must still read healthy")
-	tr.record(targetKindMCP, "alpha", false, errors.New("boom again"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom again"), 0, targetHealthSourceTraffic)
 	assert.Equal(t, targetHealthHealthy, tr.stateOf(targetKindMCP, "alpha"), "2 failures of threshold 3 must still read healthy")
 }

@@ -59,19 +59,50 @@ func TestTargetHealthTracker_ReachesThreshold_ReadsUnhealthy(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(3)
 	for i := 0; i < 3; i++ {
-		tr.record(targetKindMCP, "alpha", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+		tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
 	}
 	assert.Equal(t, targetHealthUnhealthy, tr.stateOf(targetKindMCP, "alpha"))
 }

+// TestTargetHealthTracker_FirstObservationFailure_ReadsUnknown proves F4:
+// a target whose only observations so far are failures below
+// failureThreshold reads "unknown", not "healthy" — the tracker has
+// never actually seen it succeed, so "healthy" would be a false claim
+// even though the failure-threshold gate has not fired. The threshold
+// still protects against a transient first failure: this stays
+// "unknown", it does not jump straight to "unhealthy".
+func TestTargetHealthTracker_FirstObservationFailure_ReadsUnknown(t *testing.T) {
+	t.Parallel()
+	tr := newTargetHealthTracker(3)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	assert.Equal(t, targetHealthUnknown, tr.stateOf(targetKindMCP, "alpha"), "never having succeeded must read unknown, not healthy, below threshold")
+
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom again"), 0, targetHealthSourceTraffic)
+	assert.Equal(t, targetHealthUnknown, tr.stateOf(targetKindMCP, "alpha"), "still unknown after a second failure, still below threshold")
+}
+
+// TestTargetHealthTracker_SuccessThenFewFailures_ReadsHealthy re-proves
+// TestTargetHealthTracker_BelowThreshold_StaysHealthy's claim in F4
+// terms: once a target HAS succeeded at least once, 1-2 subsequent
+// failures below threshold still read "healthy" — F4 only changes the
+// NEVER-succeeded case.
+func TestTargetHealthTracker_SuccessThenFewFailures_ReadsHealthy(t *testing.T) {
+	t.Parallel()
+	tr := newTargetHealthTracker(3)
+	tr.record(targetKindMCP, "alpha", "", true, nil, 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom again"), 0, targetHealthSourceTraffic)
+	assert.Equal(t, targetHealthHealthy, tr.stateOf(targetKindMCP, "alpha"))
+}
+
 func TestTargetHealthTracker_SuccessAfterUnhealthy_RecoversToHealthy(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(2)
-	tr.record(targetKindMCP, "alpha", false, errors.New("boom"), 0, targetHealthSourceTraffic)
-	tr.record(targetKindMCP, "alpha", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
 	require.Equal(t, targetHealthUnhealthy, tr.stateOf(targetKindMCP, "alpha"))

-	tr.record(targetKindMCP, "alpha", true, nil, 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "alpha", "", true, nil, 0, targetHealthSourceTraffic)
 	assert.Equal(t, targetHealthHealthy, tr.stateOf(targetKindMCP, "alpha"))
 	snap := tr.snapshot(targetKindMCP, "alpha")
 	assert.Equal(t, 0, snap.consecutiveFailures, "a success must reset consecutiveFailures to 0")
@@ -82,7 +113,7 @@ func TestTargetHealthTracker_ErrorString_BoundedLength(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(3)
 	long := strings.Repeat("x", targetHealthMaxErrorRunes+500)
-	tr.record(targetKindMCP, "alpha", false, errors.New(long), 0, targetHealthSourceProbe)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New(long), 0, targetHealthSourceProbe)
 	snap := tr.snapshot(targetKindMCP, "alpha")
 	assert.LessOrEqual(t, len([]rune(snap.lastError)), targetHealthMaxErrorRunes)
 	assert.Equal(t, strings.Repeat("x", targetHealthMaxErrorRunes), snap.lastError)
@@ -91,15 +122,65 @@ func TestTargetHealthTracker_ErrorString_BoundedLength(t *testing.T) {
 func TestTargetHealthTracker_ShortError_Unchanged(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(3)
-	tr.record(targetKindMCP, "alpha", false, errors.New("dial tcp: connection refused"), 0, targetHealthSourceProbe)
+	tr.record(targetKindMCP, "alpha", "", false, errors.New("dial tcp: connection refused"), 0, targetHealthSourceProbe)
 	snap := tr.snapshot(targetKindMCP, "alpha")
 	assert.Equal(t, "dial tcp: connection refused", snap.lastError)
 }

+// TestTargetHealthTracker_Record_ScrubsCredentialsFromLastError proves F1:
+// record scrubs an error's embedded target URL through sanitizeProviderErr
+// before storing lastError, so a target URL configured with an embedded
+// credential (an "api-key" query parameter, or userinfo) never survives
+// into GET /admin/api/targets.
+func TestTargetHealthTracker_Record_ScrubsCredentialsFromLastError(t *testing.T) {
+	t.Parallel()
+
+	t.Run("query parameter credential", func(t *testing.T) {
+		t.Parallel()
+		tr := newTargetHealthTracker(3)
+		rawURL := "https://svc.internal/mcp?api-key=SECRETVALUE"
+		err := errors.New(`Get "` + rawURL + `": dial tcp: connection refused`)
+		tr.record(targetKindMCP, "alpha", rawURL, false, err, 0, targetHealthSourceProbe)
+		snap := tr.snapshot(targetKindMCP, "alpha")
+		assert.NotContains(t, snap.lastError, "SECRETVALUE")
+		assert.NotContains(t, snap.lastError, "api-key=")
+	})
+
+	t.Run("userinfo credential", func(t *testing.T) {
+		t.Parallel()
+		tr := newTargetHealthTracker(3)
+		rawURL := "https://[email protected]/mcp"
+		err := errors.New(`Get "` + rawURL + `": dial tcp: connection refused`)
+		tr.record(targetKindMCP, "alpha", rawURL, false, err, 0, targetHealthSourceProbe)
+		snap := tr.snapshot(targetKindMCP, "alpha")
+		assert.NotContains(t, snap.lastError, "tok3n")
+	})
+}
+
+// TestTargetHealthTracker_Record_ScrubsBeforeTruncate proves F1: the URL
+// scrub runs against err.Error() BEFORE truncateRunes bounds it to
+// targetHealthMaxErrorRunes — even when the embedded URL straddles the
+// truncation point, no fragment of the secret survives.
+func TestTargetHealthTracker_Record_ScrubsBeforeTruncate(t *testing.T) {
+	t.Parallel()
+	tr := newTargetHealthTracker(3)
+	rawURL := "https://svc.internal/mcp?api-key=" + strings.Repeat("S", 60)
+	prefix := strings.Repeat("p", 180)
+	require.Less(t, len([]rune(prefix)), targetHealthMaxErrorRunes, "prefix must start before the truncation point")
+	require.Greater(t, len([]rune(prefix))+len([]rune(rawURL)), targetHealthMaxErrorRunes, "the url must straddle the truncation point")
+	errText := prefix + " " + rawURL + " connection refused"
+
+	tr.record(targetKindMCP, "alpha", rawURL, false, errors.New(errText), 0, targetHealthSourceProbe)
+	snap := tr.snapshot(targetKindMCP, "alpha")
+	assert.LessOrEqual(t, len([]rune(snap.lastError)), targetHealthMaxErrorRunes)
+	assert.NotContains(t, snap.lastError, "SSSS", "no fragment of the secret must survive truncation")
+	assert.NotContains(t, snap.lastError, "api-key=")
+}
+
 func TestTargetHealthTracker_KeyedByKindAndName_Independent(t *testing.T) {
 	t.Parallel()
 	tr := newTargetHealthTracker(1)
-	tr.record(targetKindMCP, "shared", false, errors.New("boom"), 0, targetHealthSourceTraffic)
+	tr.record(targetKindMCP, "shared", "", false, errors.New("boom"), 0, targetHealthSourceTraffic)
 	assert.Equal(t, targetHealthUnhealthy, tr.stateOf(targetKindMCP, "shared"))
 	assert.Equal(t, targetHealthUnknown, tr.stateOf(targetKindAgent, "shared"), "an mcp server and an agent sharing a name must track independently")
 }
@@ -109,7 +190,7 @@ func TestTargetHealthTracker_Snapshot_FieldsPopulated(t *testing.T) {
 	now := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC)
 	tr := newTargetHealthTracker(3)
 	tr.nowFn = func() time.Time { return now }
-	tr.record(targetKindAgent, "beta", true, nil, 42*time.Millisecond, targetHealthSourceProbe)
+	tr.record(targetKindAgent, "beta", "", true, nil, 42*time.Millisecond, targetHealthSourceProbe)

 	snap := tr.snapshot(targetKindAgent, "beta")
 	assert.Equal(t, targetHealthHealthy, snap.state)
@@ -127,7 +208,7 @@ func TestTargetHealthTracker_ConcurrentRecord_NoRace(t *testing.T) {
 		wg.Add(1)
 		go func(i int) {
 			defer wg.Done()
-			tr.record(targetKindMCP, "alpha", i%2 == 0, errors.New("x"), time.Millisecond, targetHealthSourceTraffic)
+			tr.record(targetKindMCP, "alpha", "", i%2 == 0, errors.New("x"), time.Millisecond, targetHealthSourceTraffic)
 		}(i)
 	}
 	wg.Wait()
@@ -226,9 +307,14 @@ func newTargetHealthSweepGateway(t *testing.T, enabled bool, probeInterval time.

 func TestMaybeSweepTargetHealth_Disabled_NoOutboundRequests(t *testing.T) {
 	t.Parallel()
-	hits := 0
+	// F6, feat/target-health review: atomic even though a disabled sweep
+	// must never actually invoke this handler — a regression that DID
+	// invoke it would otherwise race this counter's read below under
+	// -race, obscuring the real bug (an unexpected outbound probe) behind
+	// a data-race report instead of the intended assert.Equal failure.
+	var hits int32
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		hits++
+		atomic.AddInt32(&hits, 1)
 		w.WriteHeader(http.StatusOK)
 	}))
 	defer srv.Close()
@@ -240,7 +326,7 @@ func TestMaybeSweepTargetHealth_Disabled_NoOutboundRequests(t *testing.T) {
 	// wait on — a brief sleep would only prove absence of a signal we
 	// never expect to see.
 	time.Sleep(20 * time.Millisecond)
-	assert.Equal(t, 0, hits, "a disabled targetHealth must never send an outbound probe")
+	assert.Equal(t, int32(0), atomic.LoadInt32(&hits), "a disabled targetHealth must never send an outbound probe")
 	assert.Equal(t, targetHealthUnknown, gw.targetHealth.stateOf(targetKindMCP, "alpha"))
 }

@@ -273,7 +359,20 @@ func TestMaybeSweepTargetHealth_ConcurrentTriggers_SingleFlight(t *testing.T) {
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":"federated","result":{}}`))
 	}))
-	defer srv.Close()
+	// F6, feat/target-health review: registered in this order so
+	// t.Cleanup's LIFO order unblocks the handler (closing release)
+	// BEFORE srv.Close runs — httptest.Server.Close blocks until
+	// outstanding requests complete, and the assertions below can
+	// t.Fatalf/t.Errorf before ever reaching the explicit close(release)
+	// further down; without this a failing assertion left the handler
+	// permanently blocked and srv.Close hung until the whole test
+	// binary's 10-minute timeout, rather than failing fast. unblock is
+	// idempotent (sync.Once) since the happy path below also calls it
+	// directly, before waiting on the sweep's resulting state.
+	var unblockOnce sync.Once
+	unblock := func() { unblockOnce.Do(func() { close(release) }) }
+	t.Cleanup(srv.Close)
+	t.Cleanup(unblock)

 	gw := newTargetHealthSweepGateway(t, true, time.Hour, srv.URL)

@@ -292,11 +391,64 @@ func TestMaybeSweepTargetHealth_ConcurrentTriggers_SingleFlight(t *testing.T) {
 	// maybeSweepTargetHealth concurrently, at most one outbound request
 	// must have been sent.
 	waitForCondition(t, func() bool { return atomic.LoadInt32(&hits) > 0 }, time.Second)
-	close(release)
+	unblock()
 	waitForCondition(t, func() bool { return gw.targetHealth.stateOf(targetKindMCP, "alpha") != targetHealthUnknown }, time.Second)
 	assert.Equal(t, int32(1), atomic.LoadInt32(&hits), "concurrent triggers within one interval must produce exactly one sweep")
 }

+// TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls proves F3:
+// the interval load (top of maybeSweepTargetHealth) and the CAS that wins
+// the single-flight flag are not atomic with each other. Without a
+// re-check under the CAS, a caller whose own interval read predates an
+// entirely separate sweep's start-and-finish could still win the CAS
+// once that sweep clears the flag, and spawn a second sweep inside the
+// same interval — worst at startup, when every caller's first read sees
+// last == 0. A fast-completing first sweep, immediately followed by a
+// burst of concurrent callers, must still yield exactly one sweep within
+// probeInterval; a call after the interval has genuinely elapsed must
+// start a second one.
+func TestMaybeSweepTargetHealth_IntervalEnforcedAcrossRapidCalls(t *testing.T) {
+	t.Parallel()
+	var hits int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atomic.AddInt32(&hits, 1)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":"federated","result":{}}`))
+	}))
+	defer srv.Close()
+
+	probeInterval := 150 * time.Millisecond
+	gw := newTargetHealthSweepGateway(t, true, probeInterval, srv.URL)
+
+	// First sweep: the probe answers immediately, so this completes fast
+	// and clears targetHealthSweeping well inside probeInterval.
+	gw.maybeSweepTargetHealth()
+	waitForCondition(t, func() bool { return atomic.LoadInt32(&hits) > 0 }, time.Second)
+	waitForCondition(t, func() bool { return gw.targetHealth.stateOf(targetKindMCP, "alpha") != targetHealthUnknown }, time.Second)
+
+	// A burst of concurrent callers immediately after — still well inside
+	// probeInterval — must not start a second sweep.
+	var wg sync.WaitGroup
+	for i := 0; i < 50; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			gw.maybeSweepTargetHealth()
+		}()
+	}
+	wg.Wait()
+	time.Sleep(30 * time.Millisecond) // let any wrongly-spawned sweep reach the server
+	if got := atomic.LoadInt32(&hits); got != 1 {
+		t.Errorf("hits = %d, want exactly 1 (a burst of concurrent callers inside probeInterval must not start a second sweep)", got)
+	}
+
+	// Once probeInterval has genuinely elapsed, a new call starts a
+	// second sweep.
+	time.Sleep(probeInterval)
+	gw.maybeSweepTargetHealth()
+	waitForCondition(t, func() bool { return atomic.LoadInt32(&hits) == 2 }, time.Second)
+}
+
 // TestMaybeSweepTargetHealth_LegacySSE_ClosesBodyWithoutReading proves a
 // legacy "/sse" MCP server's probe uses a plain GET and closes the body
 // immediately: a test server that never closes its stream must not hang
@@ -304,6 +456,7 @@ func TestMaybeSweepTargetHealth_ConcurrentTriggers_SingleFlight(t *testing.T) {
 func TestMaybeSweepTargetHealth_LegacySSE_ClosesBodyWithoutReading(t *testing.T) {
 	t.Parallel()
 	headersSent := make(chan struct{})
+	release := make(chan struct{})
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "text/event-stream")
 		w.WriteHeader(http.StatusOK)
@@ -312,9 +465,18 @@ func TestMaybeSweepTargetHealth_LegacySSE_ClosesBodyWithoutReading(t *testing.T)
 		if ok {
 			flusher.Flush()
 		}
-		<-r.Context().Done() // never closes on its own; relies on the probe not reading
+		<-release // never closes on its own; relies on the probe not reading
 	}))
-	defer srv.Close()
+	// F6, feat/target-health review: previously relied on the PROBE'S OWN
+	// 5s context timeout (targetTimeout below) to unblock this handler,
+	// so srv.Close (deferred) silently added a real ~5s stall to every
+	// run of this test. An explicit release channel, unblocked via
+	// t.Cleanup BEFORE srv.Close (LIFO — see
+	// TestMaybeSweepTargetHealth_ConcurrentTriggers_SingleFlight's own
+	// comment for the identical ordering reasoning), makes this test
+	// fast AND immune to a failing assertion ever leaving it blocked.
+	t.Cleanup(srv.Close)
+	t.Cleanup(func() { close(release) })

 	adapters := map[string]providerAdapter{"openai": newFakeAdapter("openai")}
 	cfg := &Config{