Repository navigation
test: two more bounds become conditions — the merge queue has been ejecting releases on them (#2777, #2700) - #2817
Merged
Conversation
…ecting releases on them (#2777, #2700) Since the merge queue went live every entry builds on a loaded runner BY CONSTRUCTION, and three releases have now been ejected by three different tests whose assertion is a bound rather than a condition: #2800 PodHubClaimLifetimeTest (fixed, merged) #2803 StreamAttachTransientRetryTest (fixed by #2800) #2803 DataContextInitFaultedTest ← this commit, ejected 17:32Z 1. SelfUpdateAvailabilityGateTest (#2777) NeverRolled_WithTheGateUnavailable_StillHolds waited up to 30s for a HELD marker to land on Admin/UpdatePolicy. The service is event-driven, so "the hold is not there yet" and "the first check has not run yet" are indistinguishable from outside — and on a loaded shard it is the second. The issue's own diagnosis, adopted verbatim: make the first poll deterministic rather than widen the bound. SelfUpdateHostedService now publishes ChecksReported — one emission per check that has been evaluated AND reported, an INSTANCE Subject (never static), completed in StopAsync so a subscriber on a stopped service is released rather than left hanging. The tests await the first emission, then assert on the state it produced. Production never subscribes; nothing about what a check does or when it runs changes. The member is protected internal and Hosting.Monolith.Test is not in Memex.Portal.Shared's InternalsVisibleTo list, so the test's own subclass forwards it (`Evaluations`) — cheaper than widening the production assembly's friend list for one file. 2. DataContextInitFaultedTest.FaultedInit_ReachesTerminalFailedState_AndAnswersFast 🚨 ONE bound was doing TWO jobs that want OPPOSITE values. The test asserts liveness ("is a faulted init answered at all" — wants a generous bound, so a timeout means NEVER) and latency (the AndAnswersFast in its name — wants a tight one). A single 15s wait conflated them: a loaded runner pushed the rejection past it, and the assertion read the resulting TimeoutException as "the gate is shut" — the defect's own signature, for a defect that was not there. It ejected #2803, a PR touching only Mesh.Contract/Security and an operator script, which cannot reach a DataContext. Merely widening to 60s would have DELETED the latency half in silence: the test would still pass if the answer took 55s, which is precisely what its name guards. So they are separated — a generous 60s wait (a timeout now means never), plus an elapsed-time assertion of its own against a threshold DERIVED FROM THE MECHANISM rather than a feeling of "fast": the hub's own 30s request budget, which is what a request that FELL THROUGH the faulted arm waits out. A slow answer and an absent one now fail with different messages, because they are different defects. (The split is owed to the reviewing session, who caught that my first pass would have quietly dropped the latency half.) Verified: Memex.Portal.Shared, Hosting.Monolith.Test and Data.Test each -c Release -warnaserror → 0 Error(s); SelfUpdateAvailabilityGateTest 6/6 and FaultedInit_ReachesTerminalFailedState 1/1 green. 🚨 Trap worth recording: `--filter "FullyQualifiedName~DataContextInitWatchdogTest"` matched NOTHING and exited 0 — the file is DataContextInitWatchdogTest.cs but the CLASS is DataContextInitFaultedTest. A filter that matches nothing is a silent pass; filter on the method name when the class name is inferred from a filename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves CI reliability by rewriting two tests to wait on deterministic conditions (service evaluation / explicit liveness vs latency) rather than conflating timing bounds with correctness, and adds a small self-update hosted-service test seam to support that.
Changes:
- Expose a per-check “evaluated & reported” signal from
SelfUpdateHostedServiceand await it inSelfUpdateAvailabilityGateTestbefore asserting hold state. - Split the DataContext faulted-init watchdog test into (1) a generous liveness wait and (2) an explicit latency assertion.
- Add a What’s New entry documenting the rationale and the failure mode these changes prevent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateAvailabilityGateTest.cs | Awaits a new evaluation signal before asserting hold state to avoid “timer vs poller” races. |
| memex/Memex.Portal.Shared/SelfUpdate/SelfUpdateHostedService.cs | Adds ChecksReported seam and completes it on stop to unblock awaiting tests. |
| test/MeshWeaver.Data.Test/DataContextInitWatchdogTest.cs | Separates liveness and latency assertions for the faulted-init watchdog case. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-30-two-more-tests-measure-the-condition-not-a-stopwatch.md | Documents the “bound != assertion” principle and these two fixes. |
Suppressed comments (2)
test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateAvailabilityGateTest.cs:181
- With a 60s xUnit timeout, waiting up to 45s for Evaluations and then up to 30s for the held marker can exceed the test’s overall timeout and fail as an unhelpful xUnit Timeout; after Evaluations has fired, the hold should already be persisted, so this second wait should be a much smaller backstop.
This issue also appears on line 282 of the same file.
var held = await Mesh.GetWorkspace()
.GetMeshNodeStream(UpdatePolicyNodeType.NodePath)
.Select(node => UpdatePolicyNodeType.Parse(node, Mesh.JsonSerializerOptions))
.Where(content => content.IsHeld(CandidateTag))
.FirstAsync()
.Timeout(TimeSpan.FromSeconds(30))
.Await(ct);
test/MeshWeaver.Hosting.Monolith.Test/SelfUpdateAvailabilityGateTest.cs:288
- Same issue as above: the nested 45s + 30s waits can outrun the 60s test timeout and surface as a non-diagnostic xUnit Timeout; after Evaluations has fired, this should only need a small backstop.
var held = await Mesh.GetWorkspace()
.GetMeshNodeStream(UpdatePolicyNodeType.NodePath)
.Select(node => UpdatePolicyNodeType.Parse(node, Mesh.JsonSerializerOptions))
.Where(content => content.IsHeld(CandidateTag))
.FirstAsync()
.Timeout(TimeSpan.FromSeconds(30))
.Await(ct);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// the same — publish the condition, do not infer it from a bound. Production never | ||
| /// subscribes; nothing here changes what a check does or when it runs.</para> | ||
| /// </summary> | ||
| private readonly Subject<Unit> _checksReported = new(); |
Comment on lines
162
to
164
| .Subscribe( | ||
| _ => { }, | ||
| _ => _checksReported.OnNext(Unit.Default), | ||
| ex => _logger?.LogError(ex, "[SelfUpdate] update watch terminated unexpectedly.")); |
Comment on lines
+141
to
+147
| /// <summary> | ||
| /// The hub's own request budget — the latency a request that FELL THROUGH the faulted arm | ||
| /// would wait out. Derived from the mechanism, not from a feeling of "fast": the rejection | ||
| /// handler answers in milliseconds, so any headroom below this separates the two outcomes | ||
| /// while leaving a loaded runner far more room than it needs. | ||
| /// </summary> | ||
| private static readonly TimeSpan HubRequestBudget = TimeSpan.FromSeconds(30); |
Comment on lines
+36
to
+38
| elapsed time is asserted on its own, against a threshold taken from the mechanism rather than from | ||
| a feeling of "fast" — the hub's own request budget, which is what a request that *fell through* the | ||
| faulted arm would wait out. A slow answer and an absent one now fail with different messages, |
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
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.
Since the merge queue went live, every entry builds on a loaded runner by construction — and three releases have now been ejected by three different tests whose assertion is a bound rather than a condition, each burning a full CI run:
PodHubClaimLifetimeTestStreamAttachTransientRetryTestDataContextInitFaultedTest#2803 touches
Mesh.Contract/Securityand an operator shell script. It cannot reach aDataContext.1.
SelfUpdateAvailabilityGateTest— #2777NeverRolled_WithTheGateUnavailable_StillHoldswaited up to 30 s for a held marker to land onAdmin/UpdatePolicy. The service is event-driven, so "the hold is not there yet" and "the first check has not run yet" are indistinguishable from outside — and on a loaded shard it is always the second. #2777's own diagnosis, adopted verbatim: make the first poll deterministic rather than widen the bound.SelfUpdateHostedServicenow publishesChecksReported— one emission per check that has been evaluated and reported; an instanceSubject(never static), completed inStopAsyncso a subscriber on a stopped service is released rather than left hanging. The tests await the first emission, then assert on the state it produced. Production never subscribes; nothing about what a check does or when it runs changes.2.
FaultedInit_ReachesTerminalFailedState_AndAnswersFast— one bound doing two jobsThis is the interesting one. The test asserts liveness (is a faulted init answered at all — wants a generous bound, so a timeout means never) and latency (the
AndAnswersFastin its own name — wants a tight one). A single 15 s wait conflated them: a loaded runner pushed the rejection past it, and the assertion read the resultingTimeoutExceptionas "the faulted arm left the gate shut" — the defect's own signature, for a defect that was not there.🚨 Merely widening to 60 s would have deleted the latency half in silence — the test would still pass if the answer took 55 seconds, which is precisely what its name guards. So the two are separated:
A slow answer and an absent one now fail with different messages, because they are different defects. (The split is owed to the reviewing session, who caught that my first pass would have quietly dropped the latency half — I had it half-right.)
Verified
Memex.Portal.Shared,MeshWeaver.Hosting.Monolith.TestandMeshWeaver.Data.Testeach-c Release -warnaserror→ 0 Error(s);SelfUpdateAvailabilityGateTest6/6,FaultedInit_ReachesTerminalFailedState1/1.🚨 Trap worth knowing:
--filter "FullyQualifiedName~DataContextInitWatchdogTest"matched nothing and exited 0. The file isDataContextInitWatchdogTest.cs; the class isDataContextInitFaultedTest. A filter that matches nothing is a silent pass — filter on the method name when you inferred the class from a filename.🤖 Generated with Claude Code