Skip to content

Web read latency: each web server records into its own accumulator (#4782) - #4839

Merged
erikdarlingdata merged 5 commits into
devfrom
fix/4782-read-latency-per-app
Sep 29, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
fix/4782-read-latency-per-app

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Completes #4782: the #4785 fix covered the test-only dispatch entry but not the accumulator the dispatch records into.

Why

CI run 36592791128, on #4829's branch, failed ReadLatencyWebRecordingTests.ADispatchEntryThatThrowsA57014PostgresException_RecordsOneWebSample_WithOutcomeTimeout. The response was the expected 503, but Assert.Single (line 193) found no sample in the test's own accumulator.

DarlingWebEndpoints kept the read-latency accumulator and its logger in two statics, s_readLatency and s_readLatencyLogger. Every MapAll call overwrote them, and the record sites read them at request time. Six test classes call MapAll, and xUnit runs classes in parallel. When another class built its server between this test's BuildServer and its request, the static pointed at that class's accumulator and the sample landed there. The test's class comment said that capturing its own accumulator "sidesteps that race entirely". It did not, because the product code never read the captured instance.

Production calls MapAll once per process, so only test runs were exposed to the race.

What changes

  • ReadLatencyRecorder.cs (new): a small public sealed class holding the accumulator and the logger a server was given.
  • DarlingWebEndpoints.cs: both statics are deleted. MapAll builds one recorder from the accumulator and logger it was given, and both record sites close over it. The /api/read/* dispatch loop records through it, and the /api/compose/run route passes it to RunComposedPanelAsync (new optional last parameter, so direct callers in tests still compile and record nothing). RecordWebReadLatency and RecordComposeLatency take the recorder.
  • MCP run_custom_view_panel: the tool calls the same RunComposedPanelAsync, which used to find the accumulator in the static. Deleting the static would have dropped that tool's compose sample, so the tool now takes the recorder as an optional DI service parameter (never in the advertised schema), and ConfigureMcpServices registers one over the same accumulator its per-call latency filter already records into. One production difference follows: only the web startup used to fill the static, so with the web dashboard off (its default) these runs recorded nothing. They are now counted as compose samples. With the dashboard on, the accumulator is the same process-wide instance as before.
  • McpServiceParameterDiSeatCensusTests: the pinned set of DI service parameter types gains ReadLatencyRecorder, and the comment names Darling tests: a test-only read route leaks into other test classes running at the same time #4782.
  • ReadLatencyWebRecordingTests.cs: the class comment now says what makes each server's samples its own. ExtraDispatchEntryScope became internal so the new class reuses it. Every assertion is unchanged.
  • New tests, none of them needing a database (the pool is over a dummy connection string and never opened, because the route throws or the body fails validation before any read):
    • ReadLatencyPerServerRecordingTests, two facts. Each builds server A with accumulator A, then server B with accumulator B, then sends the request to A. One drives a 57014 dispatch entry through the /api/read/* loop and asserts exactly one Web sample with outcome Timeout in A and none in B. The other posts a body with no panel to /api/compose/run and asserts exactly one Compose sample (outcome Error) in A and none in B.
    • McpToolLatencyRecordingTests.RunCustomViewPanel_RecordsOneComposeSample_IntoTheAccumulatorTheHostWasGiven: the MCP tool's run lands as one Compose sample in the accumulator the MCP host was built with, and no Mcp sample.
  • Polish found on the way, in the same test class: RunCustomViewPanel_RecordsNoMcpSample sent a view_id argument that the tool does not declare. The unknown-argument guard refuses that before the tool or the latency filter runs, so the test held without the filter's skip for this tool ever being exercised. It now sends the tool's spec argument. The assertion is unchanged.

Test plan

  • RED first: the tests are committed ahead of the fix (dbf667a). On the old code, ReadLatencyPerServerRecordingTests (both facts) and the new McpToolLatencyRecordingTests fact fail with Assert.Single() Failure: The collection did not contain any matching items. Total: 7, Failed: 3 for the two classes; the other four pass.
  • Plant for the polished skip test: changing McpToolLatencyFilter.SkippedToolName to another string fails RunCustomViewPanel_RecordsNoMcpSample and the new MCP fact (Total: 5, Failed: 2). The plant is not in the branch.
  • Darling.Tests builds with 0 Warning(s), 0 Error(s), after merging the latest origin/dev.
  • New class, every class that calls MapAll or names ReadLatencyAccumulator, DocCommentHygieneTests, and the MCP tool, schema and pin classes (McpServiceParameterDiSeatCensusTests, McpToolsListBudgetTests, McpAlwaysLoadedToolsTests, McpUnknownArgumentGuardTests, McpReadCommandTimeoutTests, DarlingMcpCustomViewToolsTests, DarlingComposeTests), with no DARLING_TEST_PG, on the final build: Total: 706, Errors: 0, Failed: 0, Skipped: 12.
  • Full Darling.Tests suite, once, with no DARLING_TEST_PG: Total: 18004, Errors: 0, Failed: 0, Skipped: 1166, Not Run: 1. The skips are the live PostgreSQL classes, which need DARLING_TEST_PG. The one not-run test is explicit-only. The suite ran before the last two commits: one changes a single test's request, the other rewraps a comment. The classes those commits touch were re-run afterwards (the 706 above, then Total: 7, Failed: 0 for the MCP-latency and per-server classes, then Total: 78, Failed: 0 for the seat census and DocCommentHygiene).
  • The live ReadLatencyWebRecordingTests facts were not run locally (no DARLING_TEST_PG); CI's PostgreSQL jobs run them.

CHANGELOG

SECTION: Fixed
ENTRY:

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 29, 2026 17:22
@erikdarlingdata
erikdarlingdata merged commit 6179609 into dev Sep 29, 2026
21 of 22 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4782-read-latency-per-app branch September 29, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant