From 8108bfdf3ebcd5dabe5948f305a829709da0bc03 Mon Sep 17 00:00:00 2001 From: Andrey Kumanyaev Date: Tue, 28 Jul 2026 22:24:02 +0200 Subject: [PATCH] Pin the divergent-default fallback SLO in the tests that assert its result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../mcp/explore_divergent_default_owner.go | 27 +++++++++++++++++-- .../explore_divergent_default_owner_test.go | 25 ++++++++++++++--- internal/mcp/server.go | 9 +++++++ internal/mcp/tools_explore.go | 2 +- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/internal/mcp/explore_divergent_default_owner.go b/internal/mcp/explore_divergent_default_owner.go index d2b020c49..6140a5cc8 100644 --- a/internal/mcp/explore_divergent_default_owner.go +++ b/internal/mcp/explore_divergent_default_owner.go @@ -20,6 +20,19 @@ const ( exploreDefaultOwnerFileCap = 3 exploreDefaultOwnerFileNodeCap = 64 exploreDefaultOwnerTotalCap = 96 + // exploreDefaultOwnerFallbackSLO bounds the ranked-callable fallback that + // runs when no base constructor/type pair was already ranked. It is a + // SILENT bound: every check inside + // exploreDivergentDefaultBasesFromRankedCallables returns "no bases" on + // expiry, so the caller simply promotes nothing and the response is a + // smaller symbol set with no error and no warning. + // + // 25ms is ample for the batched inbound lookup + node hydration on an idle + // machine and is routinely exceeded on a loaded one, where those SQLite + // reads are descheduled. That makes WHICH owner gets promoted a function + // of machine load — fine for production (the fallback is best-effort by + // design) and fatal for a test that asserts the promoted identity, which + // is why promoteExploreDivergentDefaultOwner takes an overridable slice. exploreDefaultOwnerFallbackSLO = 25 * time.Millisecond ) @@ -62,7 +75,13 @@ type exploreDivergentDefaultProjection struct { // lookup and one batched node hydration, and fails closed at every cap. A unique // match replaces its represented base pair before being moved to the head, so // promotion never evicts an unrelated protected candidate. -func promoteExploreDivergentDefaultOwner(task string, targets []exploreTarget, store graph.Store, maxSymbols int, readSource func(*graph.Node) string) []exploreTarget { +// The optional fallbackSLO overrides the wall-clock slice the ranked-callable +// fallback may spend. Omitted (or zero) means the production default. Only a +// caller that ASSERTS which owner gets promoted passes one, so its answer +// cannot depend on machine load — see the note on +// exploreDefaultOwnerFallbackSLO. Left variadic so the fifteen call sites that +// never reach the fallback stay untouched. +func promoteExploreDivergentDefaultOwner(task string, targets []exploreTarget, store graph.Store, maxSymbols int, readSource func(*graph.Node) string, fallbackSLO ...time.Duration) []exploreTarget { if store == nil || readSource == nil || len(targets) == 0 || !exploreQueryIsConceptTask(task) { return targets } @@ -72,7 +91,11 @@ func promoteExploreDivergentDefaultOwner(task string, targets []exploreTarget, s return targets } if len(bases) == 0 { - deadline := time.Now().Add(exploreDefaultOwnerFallbackSLO) + slo := exploreDefaultOwnerFallbackSLO + if len(fallbackSLO) > 0 && fallbackSLO[0] > 0 { + slo = fallbackSLO[0] + } + deadline := time.Now().Add(slo) bases, ok = exploreDivergentDefaultBasesFromRankedCallables(taskTerms, targets, store, deadline) if !ok || len(bases) == 0 { return targets diff --git a/internal/mcp/explore_divergent_default_owner_test.go b/internal/mcp/explore_divergent_default_owner_test.go index dc6c9da77..ba06f0230 100644 --- a/internal/mcp/explore_divergent_default_owner_test.go +++ b/internal/mcp/explore_divergent_default_owner_test.go @@ -408,11 +408,16 @@ func TestHandleExplorePromotesDivergentDefaultOwnerFromSQLitePHPIndex(t *testing task := `StreamHandler::write reports "could not be opened: Permission denied" after a rotating handler applies chmod; find the divergent filePermission default and owning type` direct := promoteExploreDivergentDefaultOwner(task, []exploreTarget{{node: write}, {node: baseType}, {node: baseCtor}}, store, 3, func(node *graph.Node) string { return server.manifestSymbolSource(context.Background(), node) - }) + }, pinnedDivergentDefaultFallbackSLO) require.Equal(t, childCtor.ID, direct[0].node.ID, "real store projection did not promote child constructor") + // The fallback path is the one bounded by exploreDefaultOwnerFallbackSLO: + // with only `write` ranked there is no base constructor/type pair, so the + // ranked-callable projection runs and its batched SQLite reads must finish + // inside the slice. This assertion is about WHICH owner is promoted, not + // about how fast the machine is, so the slice is pinned. fallback := promoteExploreDivergentDefaultOwner(task, []exploreTarget{{node: write}}, store, 3, func(node *graph.Node) string { return server.manifestSymbolSource(context.Background(), node) - }) + }, pinnedDivergentDefaultFallbackSLO) require.Equal(t, childCtor.ID, fallback[0].node.ID, "real store callable-owner fallback did not promote child constructor") req := mcpgo.CallToolRequest{} @@ -515,7 +520,21 @@ class RotatingFileHandler extends StreamHandler { _, err = idx.Index(root) require.NoError(t, err) server := NewServer(query.NewEngine(store), store, idx, nil, zap.NewNop(), nil) - return server, store + return pinExploreDivergentDefaultFallbackSLO(server), store +} + +// pinnedDivergentDefaultFallbackSLO is the wall-clock slice tests give the +// divergent-default-owner fallback. Generous on purpose: these tests assert +// WHICH owner is promoted, and the production 25ms default makes that a +// function of machine load rather than of the ranking under test. +const pinnedDivergentDefaultFallbackSLO = 30 * time.Second + +// pinExploreDivergentDefaultFallbackSLO widens the fallback slice for a server +// whose explore responses are asserted symbol-by-symbol. Mirrors +// pinExploreSourceLiteralRecallBudget, for the same reason. +func pinExploreDivergentDefaultFallbackSLO(s *Server) *Server { + s.divergentDefaultFallbackSLOOverride = pinnedDivergentDefaultFallbackSLO + return s } func requirePHPFixtureNode(t *testing.T, store graph.Store, fileSuffix, name string, kind graph.NodeKind) *graph.Node { diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 790dd1946..5897119ac 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -465,6 +465,15 @@ type Server struct { // pinExploreSourceLiteralRecallBudget. sourceLiteralRecallBudgetOverride time.Duration + // divergentDefaultFallbackSLOOverride replaces the wall-clock slice + // explore's divergent-default-owner fallback may spend. Test-only, same + // contract as sourceLiteralRecallBudgetOverride above: production leaves + // it zero and gets exploreDefaultOwnerFallbackSLO. A test that asserts + // WHICH owner the fallback promotes widens the slice so the answer cannot + // depend on how loaded the machine is. See + // pinExploreDivergentDefaultFallbackSLO. + divergentDefaultFallbackSLOOverride time.Duration + // critiqueLLMGenOverride substitutes the critique_review tool's LLM seam. // Test-only: production leaves it nil and the handler builds the closure // over llmService.Generate. A non-nil override stands in for the real diff --git a/internal/mcp/tools_explore.go b/internal/mcp/tools_explore.go index f98651ed2..f5ca0f2b3 100644 --- a/internal/mcp/tools_explore.go +++ b/internal/mcp/tools_explore.go @@ -2571,7 +2571,7 @@ func (s *Server) handleExplore(ctx context.Context, req mcp.CallToolRequest) (*m if exploreQueryIsConceptTask(task) && len(targets) > len(artifactTargets) { symbolTargets := promoteExploreDivergentDefaultOwner(task, targets[len(artifactTargets):], s.graph, maxSymbols, func(node *graph.Node) string { return s.manifestSymbolSource(ctx, node) - }) + }, s.divergentDefaultFallbackSLOOverride) targets = append(targets[:len(artifactTargets):len(artifactTargets)], symbolTargets...) } // Direct retrieval owns the ranked head. Once graph promotion has selected