Skip to content

Darling tests: a test-only read route no longer leaks into other test classes running at the same time (#4782) - #4785

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/4782-dispatch-seam-asynclocal
Sep 29, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/4782-dispatch-seam-asynclocal

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4782.

Why

Dev CI failed at d85dc2f, in the Darling PG tests (2) job. One test failed: DarlingCustomViewsTests.CatalogDescriptors_Keys_EqualTheReadDispatchKeys, with "reads with no catalog descriptor: __test_statement_timeout". That route name belongs to another test class. It is not a real read.

ReadLatencyWebRecordingTests.ADispatchEntryThatThrowsA57014PostgresException_RecordsOneWebSample_WithOutcomeTimeout registers one extra /api/read/* dispatch entry through DarlingWebEndpoints.s_testOnlyExtraDispatchEntry, and BuildReadDispatch() adds that entry whenever it is set. That was a plain static field, so it is process-wide. The class that sets it is in the live-postgres collection, so it only runs on the PostgreSQL CI jobs. About ten other test classes call BuildReadDispatch() outside that collection, so they can run at the same time, and any of them that builds its dispatch while the entry is set sees the extra route. DarlingCustomViewsTests compares the dispatch keys with the Custom Views catalog, so it fails when that happens. That is what failed on dev.

What changes

  • DarlingWebEndpoints.cs: the field is now private static readonly AsyncLocal<(string Name, ReadToolHandler Handler)?>, exposed through an internal TestOnlyExtraDispatchEntry property. Only the async flow that sets the entry, and what that flow starts or awaits, sees it. BuildReadDispatch reads the property. The seam is still null in every production run, and nothing outside Darling.Tests assigns it. This is the only product-file change, and it does nothing outside a test.
  • ReadLatencyWebRecordingTests.cs: the scope class and the two doc references use the property. Every assertion is unchanged. The test builds its server after setting the entry, in the same async method, so its own server still registers the route.
  • DarlingWebEndpointsTests.cs: a new test, TheTestOnlyExtraDispatchEntry_IsSeenBySameFlow_AndNotByAFlowThatDoesNotInheritIt. It sets the entry under a unique route name and asserts BuildReadDispatch() contains it in the same flow. It then runs BuildReadDispatch().ContainsKey(name) in a task started under ExecutionContext.SuppressFlow() and asserts the answer is false. The entry is cleared in a finally.
  • git grep -n "s_testOnlyExtraDispatchEntry" -- Darling now shows only the field and the property.

Test plan

  • RED first: with the field put back to a plain static (behind the same property), the new test fails on its second assertion, "a flow that does not inherit the setter's context must not see the entry". Total: 1, Failed: 1. The plant is not in the branch.
  • Darling.Tests builds with 0 Warning(s), 0 Error(s).
  • The new test, DarlingWebEndpointsTests, DarlingCustomViewsTests and AsOfWindowAnchorTests, with no DARLING_TEST_PG: Total: 172, Failed: 0, Skipped: 0.
  • ReadLatencyWebRecordingTests against a throwaway local PostgreSQL 18.6 with DARLING_TEST_PG set: Total: 3, Failed: 0, Skipped: 0. The 57014 dispatch-entry test ran, so the test's own server still sees the entry.
  • Those four classes together with DARLING_TEST_PG set: Total: 175, Failed: 0, Skipped: 0.
  • Full Darling.Tests suite, once, with no DARLING_TEST_PG: Total: 17199, Errors: 0, Failed: 0, Skipped: 1123, Not Run: 1. The skips are the live PostgreSQL classes, which need DARLING_TEST_PG. The one not-run test is an explicit-only test, which the runner does not start unless asked.
  • The live PostgreSQL classes other than ReadLatencyWebRecordingTests were not run locally; CI's PostgreSQL jobs run them.

CHANGELOG

SECTION: None

Test-only. The one product-file change is a test seam that is null in every production run.

… flow that set it (#4782)

ReadLatencyWebRecordingTests registers one extra /api/read dispatch entry
(__test_statement_timeout) through DarlingWebEndpoints.s_testOnlyExtraDispatchEntry.
That was a plain static, so every test class running at the same time saw the
route in its own BuildReadDispatch() call. On dev,
DarlingCustomViewsTests.CatalogDescriptors_Keys_EqualTheReadDispatchKeys failed
with "reads with no catalog descriptor: __test_statement_timeout".

The entry is now an AsyncLocal behind an internal TestOnlyExtraDispatchEntry
property, so only the async flow that set it (and what that flow starts or
awaits) gets the extra key. The recording test still builds its server in the
same method that sets the entry, so its route is still registered. Every
assertion in both classes is unchanged.

New test: DarlingWebEndpointsTests
.TheTestOnlyExtraDispatchEntry_IsSeenBySameFlow_AndNotByAFlowThatDoesNotInheritIt
sets the entry, asserts the same flow sees it, then reads the dispatch from a
task started under ExecutionContext.SuppressFlow() and asserts it does not.
With the plain static it fails on that second assertion.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 29, 2026 10:16
@erikdarlingdata
erikdarlingdata merged commit 9f4f971 into dev Sep 29, 2026
16 of 18 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4782-dispatch-seam-asynclocal branch September 29, 2026 10:36
erikdarlingdata added a commit that referenced this pull request Sep 29, 2026
…un_custom_view_panel runs count with the web dashboard off (#4782) (#4839)

The web server kept its read-latency accumulator and logger in two process-wide statics. Every MapAll call overwrote them, and the two record sites read them at request time. Production calls MapAll once, but six test classes call it and xUnit runs classes in parallel, so a server that another class built could take a test's samples. That is how ReadLatencyWebRecordingTests.ADispatchEntryThatThrowsA57014PostgresException_RecordsOneWebSample_WithOutcomeTimeout failed in CI run 36592791128, on #4829's branch. #4785 fixed the test-only dispatch entry but not the accumulator.

- ReadLatencyRecorder (new) holds the accumulator and logger one server was given. MapAll builds one per call, and both record sites use it: the /api/read/* dispatch loop and the /api/compose/run route. Both statics are removed.
- The shared composed-panel runner, RunComposedPanelAsync, takes the recorder as an optional last parameter. A direct caller that passes none records nothing.
- MCP run_custom_view_panel takes the recorder as a DI service parameter, which is not in the tool's advertised schema. The MCP host registers one over the same accumulator its per-call latency filter records into. Only the web startup used to fill the static, so with the web dashboard off (the default) these runs were not recorded. They are now recorded as composed-panel reads, the same as a panel run from the web dashboard.
- McpServiceParameterDiSeatCensusTests adds ReadLatencyRecorder to the pinned set of DI service parameter types.
- New tests, none needing a database. ReadLatencyPerServerRecordingTests builds two servers, each with its own accumulator, and checks that a /api/read/* timeout and a /api/compose/run error land only in the server that answered. A new McpToolLatencyRecordingTests fact checks that the MCP tool's run lands as one Compose sample in the host's accumulator, with no Mcp sample.
- RunCustomViewPanel_RecordsNoMcpSample now sends the tool's spec argument. It used to send an argument the tool does not declare, and the unknown-argument guard refused that before the latency filter ran.
- Darling only: Lite has no web server and no read-latency accumulator.
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