From 1d79cc7926026919792e927ec3042595fec2813b Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Sun, 26 Jul 2026 19:32:38 +0200 Subject: [PATCH] resolver: drop the wall-clock ceiling from the LSP pass-budget test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestResolveAllDeferredLSPPassBudgetStopsNewCalls asserted that ResolveAll returned in under 100ms, to show the cumulative breaker pays the helper delay once per pass rather than once per deferred edge. That ceiling has failed in CI four times — twice on main (765ms, 135ms) and once each on two PR branches (422ms, 447ms) — every time as the only failing assertion, with all six counters passing. It cannot hold. `elapsed` spans the whole of ResolveAll, not the LSP pass: spool setup, the import closure, and the tail attribution and dispatch passes are all inside the measured window, so the bound mostly measures work unrelated to the breaker. CI runs a single `go test -race ./...` with coverage instrumentation, where that overhead is unbounded and the observed values ran to 447ms. It also adds no detection power. The invariant is call-shaped: once the budget is spent the loop marks every remaining edge skipped without reaching the helper, and the helper records each call before sleeping, so callNames length is an identity for the number of delays paid. And a breaker that never opens leaves LSPBudgetExhausted false, tripping the require on the first line of the block long before any timing assertion is reached. Drop both timing assertions and the now-unused started/elapsed, keeping the counters that state the invariant exactly, and record in a comment why no wall-clock bound belongs here. --- internal/resolver/lsp_budget_test.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/internal/resolver/lsp_budget_test.go b/internal/resolver/lsp_budget_test.go index fef5120ef..4644f44de 100644 --- a/internal/resolver/lsp_budget_test.go +++ b/internal/resolver/lsp_budget_test.go @@ -123,19 +123,30 @@ func TestResolveAllDeferredLSPPassBudgetStopsNewCalls(t *testing.T) { r.SetLSPHelper(helper) r.SetLSPResolvePassBudget(5 * time.Millisecond) - started := time.Now() stats := r.ResolveAll() - elapsed := time.Since(started) + // The invariant is call-shaped, not time-shaped: once the pass budget is + // spent the resolver must issue no further Definition call, so the helper's + // delay is paid once per pass rather than once per deferred edge. The + // counters below state that exactly, and callNames is an identity for the + // number of sleeps — Definition records every call before sleeping, and it + // is the only place the delay exists. + // + // Deliberately not asserted against wall clock. The obvious bound would + // have to cover all of ResolveAll (spool setup, import closure, the tail + // attribution passes), not just the LSP pass, so it measures mostly + // unrelated work; CI runs this package under -race with coverage, where + // that overhead is unbounded. A 100ms ceiling here failed on main twice and + // on two PR branches, always as the only failing assertion while every + // counter passed — and it added no detection power, because a breaker that + // never opens leaves LSPBudgetExhausted false and trips the require below + // first. require.True(t, stats.LSPBudgetExhausted) assert.Equal(t, len(edges), stats.LSPDeferred) assert.Equal(t, 1, stats.LSPAttempted, "only the already-in-flight call may outlive the pass budget") assert.Equal(t, 1, stats.LSPResolved) assert.Equal(t, len(edges)-1, stats.LSPBudgetSkipped) assert.Len(t, helper.callNames(), 1, "skipped edges must not invoke the helper") - assert.GreaterOrEqual(t, elapsed, helper.delay) - assert.Less(t, elapsed, 100*time.Millisecond, - "the cumulative breaker must avoid paying the delay once per deferred edge") lspResolved := 0 for _, edge := range edges {