Pin the divergent-default fallback SLO in the tests that assert its result - #398
Merged
Conversation
…esult TestHandleExplorePromotesDivergentDefaultOwnerFromSQLitePHPIndex fails intermittently on the macOS CI runner with "1 is not greater than or equal to 2" — the explore envelope carries only StreamHandler.write, without the promoted RotatingFileHandler constructor and type. The cause is exploreDefaultOwnerFallbackSLO, the 25ms slice bounding the ranked-callable fallback. That fallback is the only route to the child pair here: promotion enters with a single ranked target, so no base constructor/type pair is pre-ranked and exploreDivergentDefaultBases returns nothing. The slice is a SILENT bound — each expiry check returns "no bases", so the caller promotes nothing and the response is a smaller symbol set with no error and no log. Idle, the batched inbound lookup plus node hydration cost ~150us; under load those SQLite reads are descheduled past 25ms and the guards at explore_divergent_default_owner.go:534 and :557 trip. Production is not wrong. A fail-closed SLO on a best-effort recovery path is deliberate, and widening it was already tried and rejected (d86ef8c0, which deleted the budget outright, was archived unmerged). What is wrong is a test asserting WHICH owner gets promoted while racing that SLO against machine load. So the slice becomes overridable and the assertions pin it, exactly as pinExploreSourceLiteralRecallBudget already does for the 75ms source-literal recall — the same failure class in the same pipeline. Two seams, because two call paths reach the fallback: an optional trailing argument for the direct calls, and Server.divergentDefaultFallbackSLOOverride for the handleExplore path. Both default to the production constant, so nothing ships differently. The argument is variadic so the fifteen call sites that never reach the fallback stay untouched. TestDivergentDefaultOwnerCallableFallbackFailsClosed deliberately passes no override: its nodeBatchDelay is exploreDefaultOwnerFallbackSLO+1ms, so it must keep racing the production budget for its deadline contract to mean anything. Verified by reproducing the failure and confirming the fix under CPU saturation (20 spinners on 10 cores, -race): before 2/10 and 3/12 failed, at the same two assertions as CI after 0/14 failed This flake predates the repo-prefix refactor: the same test failed the same way on macOS in run 29881065044 (2026-07-22), on an unrelated branch.
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 failure
test (macos-latest)on main fails intermittently:The explore envelope carries only
StreamHandler.write, without the promotedRotatingFileHandlerconstructor and type.Cause
exploreDefaultOwnerFallbackSLO— the 25ms slice bounding the ranked-callable fallback inexplore_divergent_default_owner.go.That fallback is the only route to the child pair in this test: promotion enters with a single ranked target, so no base constructor/type pair is pre-ranked and
exploreDivergentDefaultBasesreturns nothing. The slice is a silent bound — each expiry check returns "no bases", the caller promotes nothing, and the response is a smaller symbol set with no error and no log.Idle, the batched inbound lookup plus node hydration cost ~150µs against a 25ms budget. Under load those SQLite reads get descheduled past it and the guards at
:534/:557trip. Instrumented on unmodified main under CPU saturation: 4 failures, 4 guard trips, one-to-one (hydTook=26.87ms,fnTook=46.21ms).Fix
Production is not wrong — a fail-closed SLO on a best-effort recovery path is deliberate, and widening it was already tried and rejected (
d86ef8c0, which deleted the budget outright, was archived unmerged). What is wrong is a test asserting which owner gets promoted while racing that SLO against machine load.So the slice becomes overridable and the assertions pin it — exactly as
pinExploreSourceLiteralRecallBudgetalready does for the 75ms source-literal recall, the same failure class in the same pipeline.Two seams, because two call paths reach the fallback: an optional trailing argument for the direct calls, and
Server.divergentDefaultFallbackSLOOverridefor thehandleExplorepath. Both default to the production constant, so nothing ships differently. The argument is variadic so the fifteen call sites that never reach the fallback stay untouched.TestDivergentDefaultOwnerCallableFallbackFailsCloseddeliberately passes no override: itsnodeBatchDelayisexploreDefaultOwnerFallbackSLO + 1ms, so it must keep racing the production budget for its deadline contract to mean anything.Verification
Reproduced the failure and confirmed the fix under CPU saturation (20 spinners on 10 cores,
-race):372167a2(before the repo-prefix refactor)Also
golangci-lintclean and the fullinternal/mcppackage green.Not caused by #396
This flake predates the repo-prefix refactor. The same test failed the same way on macOS in run 29881065044 (2026-07-22), on an unrelated branch, six days before #396 existed — at the assertion that calls
promoteExploreDivergentDefaultOwnerdirectly, bypassing everything #396 touched. The 2/10 pre-refactor reproduction above confirms it independently.Worth noting separately: the macOS
internal/mcpjob is chronically timing-flaky — 10 macOS test-job failures across 188 CI runs in the 07-19..07-29 window, 6 distinct tests, 4 of them explore wall-clock-budget tests, none repeating on the same SHA. This PR fixes one of those four; the pattern suggests the others deserve the same treatment.