Repository navigation
MCP analyze_server: overlapping calls each analyze their own server (#4726) - #4742
Merged
Merged
Conversation
…ls each analyze their own server (#4726) Both MCP hosts registered a single shared analysis service. While a pass ran, AnalyzeAsync returned an empty list and the tool read the running pass's state, so a second overlapping analyze_server call answered 'No significant findings' for a server it never analyzed. Darling and Lite now register the service transient, one instance per call as the worker builds one per pass, and still hand every instance the one shared BaselineCache. McpServiceParameterDiSeatCensusTests now accepts a transient seat, and McpToolLatencyRecordingTests registers the service per call like the production host, both on purpose for the new registration.
…pping-call test (#4726) Each MCP host registers its analysis service through an internal static RegisterAnalysisService that the host calls and a test also calls, so a test resolves the production registration instead of a copy. Both analysis services expose the shared baseline cache they were handed (SharedBaselineCache, internal). Darling.Tests and Lite.Tests resolve the service twice from a fresh collection and assert NotSame instances that share the Same baseline cache. Lite.Tests also overlaps two analyze_server calls for different servers behind a held store write lock (non-parallel collection, lock held on a dedicated thread) and asserts the second call is not the busy-path all-clear. The two SharedBaselineCacheTests source pins follow the registration into the extracted method.
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.
Fixes #4726
Why
Both MCP hosts (Darling and Lite) registered ONE analysis service for the life of the process. While a pass is running,
AnalyzeAsyncreturns an empty list at its busy check, and theanalyze_servertool then reads the running pass's state. A second, overlapping call therefore answered "No significant findings. All metrics are within normal ranges" for a server it never analyzed. MCP clients often call tools in parallel, so the window is the whole length of the first pass. Scheduled analysis was not affected.What changes
Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpHostService.cs: the analysis service is registered per call, in a newinternal static RegisterAnalysisService(services, postgres, planFetcher, logger, baselineCache, analyzer)that the host calls. It doesAddTransient<DarlingAnalysisService>(_ => new DarlingAnalysisService(postgres, planFetcher, logger, baselineCache, analyzer ?? AnalyzerConfig.Default)): one instance per MCP call, the way the worker builds one per pass. The host still passes its ONE shared_baselineCache(Every scheduled analysis pass recomputes every 30-day baseline: a fresh DarlingAnalysisService per pass never reuses (or shares with MCP) the bucket cache #3941), so baselines are not recomputed per call.Lite/Mcp/McpHostService.cs: the same change.RegisterAnalysisService(services, duckDb, planFetcher, serverManager, schedules)doesAddTransient<AnalysisService>and still passesBaselineCache.For(duckDb); the host calls it with its own store. Lite's scheduler and Recommendations tab keep their own instances and are untouched.DarlingAnalysisServiceand Lite'sAnalysisServicekeep the shared baseline cache they were handed and expose it as aninternalread-onlySharedBaselineCache, so a test can compare the caches of two per-call instances. Nothing else about the services changes.AnalyzeAsync, so it is left alone.McpServiceParameterDiSeatCensusTestsnow accepts anAddTransient<T>seat as well as the singleton forms (a transient seat still means the parameter is resolved from DI and never served as a client argument). The six-type pin is unchanged.McpToolLatencyRecordingTestsregisters the analysis service per call, with one sharedBaselineCache, like the production host.SharedBaselineCacheTests(inDarling.Testsand inLite.Tests): two source pins followed the registration into the extracted method. Darling's now pins both halves, the construction insideRegisterAnalysisServiceand the host's call that passes_baselineCache. Lite's per-file check now looks forBaselineCache.For(duckDb)inMcpHostService.cs(the parameter name in the extracted method); the host's call that passes_duckDbis pinned inAnalyzeServerPerCallAnalysisServiceTests.New tests:
Darling.TestsAnalyzeServerPerCallAnalysisServiceTests.TwoResolutionsOfTheHostRegistration_AreDistinctServices_ThatShareTheOneBaselineCache: registers throughDarlingMcpHostService.RegisterAnalysisServiceon a freshServiceCollection, resolves the service twice, and asserts the instances areNotSameand theirSharedBaselineCacheisSame(and is the cache passed in). No store is needed: creating the data source connects to nothing. The existing source pin also requires the host to callRegisterAnalysisServicewith its_baselineCache.Lite.TestsAnalyzeServerPerCallAnalysisServiceTests.TwoResolutionsOfTheHostRegistration_AreDistinctServices_ThatShareTheStoresOneBaselineCache: the same throughMcpHostService.RegisterAnalysisService, and the shared cache must beBaselineCache.For(duckDb). The source pin now also requires the host to call the method with_duckDb.Lite.TestsAnalyzeServerOverlappingCallsTests.ASecondCallWhileTheFirstIsAnalyzing_AnalyzesItsOwnServer_InsteadOfTheBusyAllClear, in its own non-parallel collection (lite-analyze-server-overlap) because the store's database lock is one process-wide lock. A dedicated thread takes the store's write lock (the lock is thread-affine, so the same thread releases it, and afinallyreleases it if an assertion fails). Analyze_server call A (server 1) starts and the test waits until its service reportsIsAnalyzing. Call B (server 2) starts, and the test waits until B has either returned (the shared-instance shape) or started analyzing on its own service. Then the lock is released and both calls are awaited. B must not be the "No significant findings. All metrics are within normal ranges" all-clear. No data is collected for either server, so each call answersinsufficient_datafor its own server, and the test asserts that too.Darling.Testsgets no end-to-end overlap test: its analyze_server resolves the server through the store's registry before it analyzes, so that test would need a live store; the registration test above covers Darling's registration.Test plan
Darling.TestsandLite.Testsbuild with 0 warnings, 0 errors.AddSingletonfor one run (not committed),Lite.Testsfailed 3 of 3 (the two-resolution test withAssert.NotSame() Failure: Values are the same instance, the source pin, and the overlap test, where call B returned the all-clear andAssert.DoesNotContainfailed) andDarling.Testsfailed 2 of 2 (the two-resolution test onNotSame, and the source pin). Restored toAddTransient, all pass.Lite.Tests(AnalyzeServerPerCallAnalysisServiceTests,AnalyzeServerOverlappingCallsTests,SharedBaselineCacheTests) Total 10, Failed 0.Darling.Tests(AnalyzeServerPerCallAnalysisServiceTests,McpServiceParameterDiSeatCensusTests,McpToolLatencyRecordingTests,SharedBaselineCacheTests,SharedBaselineCacheLiveTests) Total 18, Failed 0, Skipped 6 (the live tests needDARLING_TEST_PG).Lite.Testssuite: Total 5588, Failed 0, Skipped 0.Darling.Testssuite, run withoutDARLING_TEST_PG: Total 17086, Failed 0, Skipped 1111 (the live tests skip without it), Not Run 1 (a test marked explicit-only).CHANGELOG
SECTION: Fixed
ENTRY:
REF:
[Overlapping analyze_server calls return "No significant findings" for a server that was never analyzed #4726]: Overlapping analyze_server calls return "No significant findings" for a server that was never analyzed #4726