Repository navigation
Cold means cold: the credential snapshot stops warming itself on read - #1000
Conversation
RouterIsRecognisedAgainstAColdCatalog asserted a state it could not control. ChatClientCredentialResolver.ReadSnapshot() opened the live catalog subscription as a SIDE EFFECT of reading, so the test's own precondition read started the warm-up it was asserting had not happened. Whether the first emission landed before or after the very next line was a coin flip: on CI run 31305125041 it landed in between, ReadTierCandidates() came back empty and HasReadableCatalog came back true microseconds later, and the test failed its own precondition. The defect is in the API, not the test: HasReadableCatalog exists precisely to report the pre-warm window, and asking it ended that window. A query that mutates what it reports cannot be observed. Reads are now pure. Watching the catalog is an owner's explicit act — EnsureSubscription / WatchPartition / WatchSharedProvider — and the DI registration performs it when it builds the mesh's shared resolver, so every consumer that resolves from DI is warm exactly as before (marginally earlier: at service resolution rather than at first lookup). A caller that constructs its own resolver holds it cold for as long as it likes. The test does that: it builds its own resolver, so nothing else can warm it, and a closing re-check of HasReadableCatalog proves the snapshot was still cold when the last assertion ran — the positive router assertions cannot have been answered off a warm catalog. A sibling test pins the other half: repeated reads of every shape leave it cold, and one EnsureSubscription() makes it readable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The read-never-warms test waited on nothing: "still empty after five reads" is only evidence if enough time passed, which is a sleep in disguise. Warm the mesh's SHARED resolver instead and wait until the catalog is demonstrably readable through it — at that instant a private resolver that has been read repeatedly and never asked to subscribe must still read empty. Against warm-on-read that assertion fails deterministically rather than probabilistically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a nondeterministic test failure by making ChatClientCredentialResolver’s snapshot reads side-effect free: reading no longer implicitly starts the live catalog subscription, and the shared DI-registered resolver is explicitly warmed at construction time instead.
Changes:
- Stop
ReadSnapshot()from lazily opening the catalog subscription; subscription warming is now explicitly owner-driven. - Warm the mesh’s shared
ChatClientCredentialResolverinAddLanguageModelType’s DI registration to preserve existing runtime behavior. - Update and extend
AutoRouterDispatchTestto deterministically assert that reads never warm a private resolver, while explicit subscription does.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/MeshWeaver.AI.Test/AutoRouterDispatchTest.cs | Reworks the cold-catalog test to own resolver warm state and adds a determinism test asserting “reads never warm.” |
| src/MeshWeaver.AI/LanguageModelNodeType.cs | Changes DI registration to create and explicitly warm the shared resolver at resolution time. |
| src/MeshWeaver.AI/ChatClientCredentialResolver.cs | Makes ReadSnapshot() a pure read (no auto-subscribe) and updates docs to reflect the new ownership model. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
"Until EnsureSubscription is called, every lookup is a miss" overstated it. Resolve/HasUsableCredential are read-THROUGH: a miss fires the single-flight authoritative re-read, and that merges into the cache with no subscription involved. So EnsureSubscription governs the long-lived WATCH, not whether the snapshot can ever be populated — a resolver stays cold as long as its owner neither warms it nor asks it for a credential. The cold tests already respect that line: they call only snapshot projections. ResolveDefaultModelId is safe for the same reason spelled out now — it passes HasUsableCredential as a predicate over the candidate list, and an empty list never reaches it (ModelTierCatalog.Eligible). Documented on the resolver and at both call sites so a later edit cannot re-open the window by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correct, and worth being exact about — fixed in 1740076.
The cold tests already sit on the right side of that line — they call only projections — and that is |
Test Results (shard 3) 11 files 11 suites 5m 34s ⏱️ Results for commit 082ae6b. |
Test Results (shard 0)685 tests 685 ✅ 4m 55s ⏱️ Results for commit 082ae6b. |
Test Results (shard 2)1 887 tests 1 885 ✅ 6m 16s ⏱️ Results for commit 082ae6b. |
Test Results (shard 4)1 216 tests 1 208 ✅ 6m 14s ⏱️ Results for commit 082ae6b. |
Test Results (shard 5)1 717 tests 1 526 ✅ 6m 17s ⏱️ Results for commit 082ae6b. |
Test Results (shard 1)1 188 tests 1 188 ✅ 7m 32s ⏱️ Results for commit 082ae6b. |
Test Results 65 files 65 suites 36m 50s ⏱️ Results for commit 082ae6b. |
The failure
MeshWeaver.AI.Test.AutoRouterDispatchTest.RouterIsRecognisedAgainstAColdCataloglanded in #992 andreddened the next unrelated tree that actually ran it — PR #989, run
31305125041, shard 3:
It failed its own precondition.
ReadTierCandidates().Should().BeEmpty()passed andHasReadableCatalog.Should().BeFalse()on the very next line failed — the whole test lived 67 ms(class init
09:20:00.134→ dispose09:20:00.205). So the catalog went from unreadable to readablebetween two adjacent statements.
Root cause — the API, not the test
ChatClientCredentialResolver.ReadSnapshot()opened the live catalog subscription as a side effectof being read:
The test's own precondition read therefore started the warm-up it was asserting had not happened.
Whether the first emission landed before or after the next line was a coin flip; under CI load it
landed in between. (The test's comment — "deliberately NO
EnsureSubscription()— nothing has warmedthe snapshot" — was simply not true of the code.)
That is a defect in the API, not just in the test:
HasReadableCatalogexists precisely to reportthe pre-warm window, and asking it ended that window. A query that mutates what it reports cannot be
observed by anyone — the test just happened to be the first caller to try.
The fix — reads are pure, warming is an owner's act
ReadSnapshot()no longer subscribes. Watching the catalog starts only when an owner says so:EnsureSubscription()/WatchPartition()/WatchSharedProvider().AddLanguageModelTypenow performs that act when it builds the mesh's sharedresolver. Every consumer resolves it from DI (verified: no code anywhere constructs one), so every
consumer is warm exactly as before — marginally earlier, at service resolution rather than at first
lookup. No behavioural change for a running deployment.
so the snapshot is empty at the first assertion, at the last, and at every one in between. A closing
re-check of
HasReadableCatalogbrackets the precondition — proof the router assertions wereanswered by the static rule and not by a catalog that warmed midway.
The
HasReadableCatalog.Should().BeFalse()precondition is kept, unweakened, and now assertedtwice. Nothing was skipped, retried, slept on, or widened.
Coldness is deterministic, not "more likely"
A new sibling test,
ReadingNeverWarms_OnlyTheOwnerDoes, pins it on a positive signal rather thanon the passage of time (no sleep, no window): warm the mesh's shared resolver and wait until the
catalog is demonstrably readable through it — at that instant a private resolver that has been read
repeatedly and never asked to subscribe must still read empty. Then one explicit
EnsureSubscription()makes the same instance readable.Verified by counter-proof: with
ReadSnapshot's warm-on-read temporarily restored, that test failsdeterministically —
— while
RouterIsRecognisedAgainstAColdCatalogstill passed on that same old-behaviour run. That isthe point: on this machine the old race falls the lucky way, which is exactly why it only ever showed
up on CI. The new test converts a probabilistic failure into a deterministic one.
Siblings checked
ModelTierCatalogTest,BuiltInLanguageModelProviderTest— pure POCO suites over an explicitlyconstructed catalog, no mesh, no snapshot. No ambient dependency.
AutoRouterDispatchTesttests go throughSeedCatalogAndWarm(), which warmsexplicitly and then waits on the actual condition (seeded models + router visible). Sound.
Verification
dotnet build src/MeshWeaver.AI -c Release -warnaserror— clean (0 warnings)dotnet build test/MeshWeaver.AI.Test -c Release -warnaserror— cleandotnet test test/MeshWeaver.AI.Test -c Release— 1120 passed, 0 failed, 5 skippeddotnet test test/MeshWeaver.Threading.Test -c Release— 128 passed, 0 failed (the other suitethat drives the resolver)
What's New
Skipped deliberately — internal test-determinism fix plus an internal warm-up seam with no
user-visible effect.
🤖 Generated with Claude Code