Drop the wall-clock ceiling from the LSP pass-budget test - #358
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The flake
TestResolveAllDeferredLSPPassBudgetStopsNewCallsasserts thatResolveAllreturns in under 100 ms, to demonstrate that the cumulative pass budget makes the resolver pay the helper's 20 ms delay once per pass rather than once per deferred edge.That ceiling has failed in CI four times, always as the only failing assertion while all six counters passed:
mainmainThe most recent one re-ran green on the identical commit, which is the usual signature of a load-dependent timing assertion.
Why it cannot hold
elapsedspans the whole ofResolveAll, not the LSP pass. Inside the measured window are the deferred-LSP spool (a real temp SQLite file with WAL and aCREATE TABLE), the whole-graph import closure, and the tail file-attribution and language-dispatch passes. So the bound mostly measures work that has nothing to do with the breaker it names.CI runs a single
go test -race -timeout=20m -coverprofile=coverage.out ./...— race detector and coverage instrumentation, every package in one invocation, no-count, no retry. Under that, the non-helper portion of the call is unbounded; measured at rest it is a few milliseconds with a 3–4× spread, and the observed CI values reach 447 ms.Why removing it loses nothing
The invariant is call-shaped, not time-shaped. Once the budget is spent,
resolveDeferredLSPWithPassBudgetsetsstoppedpermanently and every remaining edge takes theresult.skipped++path without reaching the helper. The helper appends tocallsbefore sleeping, and that sleep is the only place the delay exists — solen(helper.callNames())is an identity for the number of delays paid.assert.Len(t, helper.callNames(), 1)already states the invariant exactly.And on a genuine breaker regression the timing assertion is never even reached: with the breaker never opening,
stats.LSPBudgetExhaustedis false and therequire.Trueat the top of the block aborts the test first.Worth being precise about one thing, because the obvious argument for this change is wrong: it is not true that the regression would slip under the ceiling. Simulating it (
SetLSPResolvePassBudget(0), so the breaker never opens) putselapsedat 135–154 ms under-race, above the 100 ms bar — the naive "4 × 20 ms = 80 ms" arithmetic ignores the same non-LSP overhead that causes the flake. The ceiling is redundant becauserequire.Truefires first, not because it is blind.The change
Delete both timing assertions and the now-unused
started/elapsed, keep the six counter assertions, and leave a comment recording why no wall-clock bound belongs here.timeandrequireare still used elsewhere in the file, so there is no import churn.Verification
go vet ./internal/resolver/clean.go test -race ./internal/resolver/ -count=5— whole package, green.-raceruns of the budget tests specifically, green.Follow-up, not included here
A sweep turned up ~18 other absolute wall-clock upper bounds across 10 test files, several with the same shape (a short internal deadline paired with a ceiling only 5–10× larger, running under
-racewith coverage). An adversarial re-check found material errors in 7 of those 18 claims and a ranking that was wrong in both directions, so the list needs re-verification before anyone acts on it. Flagging it as a known area rather than a vetted list.