Repository navigation
fix: zero observable-to-Task bridges in src/, and a ratchet that holds it there - #2748
Merged
Merged
Conversation
…s it there The maintainer retracted the test exemption on 2026-08-30 — "totask is forbidden", "strictly", "no totask ever" — and sharpened it to "in src especially we should have zero". This is wave 1: core src/, tools/, samples/ and clients/ land at exactly zero, with no allow-list at all. Why the exemption never held. Rx's bridge completes its TaskCompletionSource from INSIDE the pipeline, without RunContinuationsAsynchronously, so the awaiter resumes inline on the signalling thread, still inside Rx's trampoline (Producer.SubscribeRaw) — and everything the continuation does inherits that. It is also sticky: `await` captures TaskScheduler.Current when there is no SynchronizationContext, so every later await in the method follows. That is #2301 (a grain teardown parking the scheduler its own deactivation needed) and #2377 (a children listing that stayed empty forever, no error, no completion). An assertion is the last thing a test awaits before it tears the mesh down, so "only in a test" was never a containment boundary. What replaces it. Waits that must still produce a Task go through this repo's own ReactiveCompletion.ObserveCompletion, which queues the caller's continuation instead of running it on the producer's thread AND keeps its error arm attached so a late fault is reported rather than orphaned. It was already the documented "ONE sanctioned bridge" here — RoutingQuiescence has carried the comment "never .ToTask()" for some time; this change finishes the job. Converted, 32 code sites across 10 files (51 of the 83 grep hits in src/ were doc-comment prose explaining the ban, and are deliberately untouched): - MonolithMeshTestBase.AwaitResponseAsync + SeedTopLevel — the request/response wait the whole suite runs; - IStorageAdapterTestExtensions (13) and PermissionTestExtensions (3) — the shared helpers behind several hundred test call sites, none of which needed an edit; - ObservableAssertions (3) — MeshWeaver.Reactive.Assertions ships standalone ("depends only on System.Reactive"), so it gets ReactiveWait, the same guarantee expressed locally rather than a reference to the mesh; - IIoPool.InvokeObservable — the one bridge that used to be sanctioned because it was sealed inside the pool. It is the worst place for an inline resumption: the continuation is what RELEASES THE SLOT; - OrleansRoutingService (3), IoPoolSiloTeardown, UserContextMiddleware, McpRemoteMeshClient, GenericUnpartitionedDataSource (2), and the three CLI tools. Fixed on the way: ObservableAssertions.Complete() could never pass. It waited on IgnoreElements(), which completes EMPTY on success, and the old bridge reports an empty sequence by throwing InvalidOperationException — which the method's own catch turned into "but it errored: Sequence contains no elements". It had no call sites and no test, so nothing noticed. Both it and NotEmit (whose empty-source semantics DID depend on that throw, and now keep it via an explicit FirstAsync) are pinned by new tests. The ratchet, ObservableToTaskBridgeGuard, has two deliberately different rules: src/, tools/, samples/ and clients/ are held at zero with no allow file — there is no line to add and no budget to raise; test/ and memex/ carry a seeded inventory (1543 sites) that may only shrink, since their sweeps are later waves. It masks comments and string literals before counting, so the war stories that make the rule teachable are not pressured into deletion. Its non-vacuity is stated rather than inferred, because the zero half's evidence is an EMPTY result and "found nothing" is otherwise indistinguishable from "the scanner is broken": it drives the real scanner over a synthetic file and asserts both directions. Verified against a live negative control — an injected .ToTask( in src/ failed the guard RED naming the file, and counted 1 rather than 2, proving the pre-existing comment in that same file was correctly masked. Verified: full solution `dotnet build -c Release -warnaserror` 0 Errors 0 Warnings; Persistence.Test 1063 passed (the heaviest consumer of the converted storage helpers), Messaging.Hub.Test 332 passed, Reactive.Assertions.Test 23 passed, Documentation.Test guards + link/What's-New integrity 6 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enforces the maintainer’s 2026-08-30 ruling by removing Rx observable-to-Task bridges (.ToTask() from production trees and introducing a test-guard ratchet to prevent reintroduction, while standardizing on ReactiveCompletion.ObserveCompletion (and a local equivalent for the standalone assertions package).
Changes:
- Replaced
.ToTask(call sites insrc/andtools/withReactiveCompletion.ObserveCompletion(andReactiveWaitforMeshWeaver.Reactive.Assertions). - Added
ObservableToTaskBridgeGuardplus a seeded allow-inventory for remaining sites intest/andmemex/. - Added regression tests pinning corrected semantics for
Complete()/NotEmit()in reactive assertions, and added documentation/What’s New describing the policy and migration patterns.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/MeshWeaver.PluginTester/Program.cs | Replaces .ToTask( in the plugin gate CLI with ObserveCompletion. |
| tools/MeshWeaver.ComboVerifier/Program.cs | Replaces .ToTask( in combo verification CLI with ObserveCompletion. |
| tools/MeshWeaver.ComboAssembler/Program.cs | Replaces .ToTask( in combo assembly CLI with ObserveCompletion. |
| test/ObservableToTaskBridgeSites.allow | Adds seeded inventory for remaining .ToTask( sites in test/ + memex/ (shrink-only). |
| test/MeshWeaver.Reactive.Assertions.Test/AssertionTests.cs | Adds regression pins for Complete() and NotEmit() semantics. |
| test/MeshWeaver.Documentation.Test/ObservableToTaskBridgeGuard.cs | Introduces a guard enforcing zero .ToTask( in production roots and a shrink-only ratchet elsewhere. |
| test/MeshWeaver.Documentation.Test/BlockingBridgeInTestRatchetGuard.cs | Updates guard guidance to remove the previously “sanctioned” .ToTask test edge claim. |
| src/MeshWeaver.Reactive.Assertions/ReactiveWait.cs | Adds a local non-inline-resumption wait primitive for the standalone assertions package. |
| src/MeshWeaver.Reactive.Assertions/ObservableAssertions.cs | Replaces .ToTask( waits with ReactiveWait, fixing Complete() on empty completion and preserving NotEmit() semantics. |
| src/MeshWeaver.Mesh.Contract/Threading/IIoPool.cs | Replaces the pool’s internal .ToTask( bridge with LastAsync().ObserveCompletion(...). |
| src/MeshWeaver.Hosting/Persistence/Http/McpRemoteMeshClient.cs | Replaces .ToTask( during MCP client disposal join with ObserveCompletion. |
| src/MeshWeaver.Hosting.Monolith.TestBase/PermissionTestExtensions.cs | Updates permission waits to use ObserveCompletion and adds late-fault reporting. |
| src/MeshWeaver.Hosting.Monolith.TestBase/MonolithMeshTestBase.cs | Updates request/response and seeding waits to use ObserveCompletion. |
| src/MeshWeaver.Hosting.AspNetCore/Portal/UserContextMiddleware.cs | Replaces .ToTask( in bearer token validation wait with ObserveCompletion. |
| src/MeshWeaver.Fixture/IStorageAdapterTestExtensions.cs | Replaces .ToTask( in test adapter helpers with ObserveCompletion + late-fault reporting. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-30-src-holds-zero-observable-to-task-bridges.md | Adds a What’s New entry documenting the change and its motivation. |
| src/MeshWeaver.Documentation/Data/Architecture/RemovingObservableToTaskBridges.md | Adds the detailed policy and migration guide for removing .ToTask(. |
| src/MeshWeaver.Data/GenericUnpartitionedDataSource.cs | Replaces .ToTask( in initialization boundary waits with ObserveCompletion. |
| src/MeshWeaver.Connection.Orleans/OrleansRoutingService.cs | Replaces .ToTask( at Task-shaped Orleans boundaries with ObserveCompletion. |
| src/MeshWeaver.Connection.Orleans/IoPoolSiloTeardown.cs | Replaces .ToTask( in lifecycle join with ObserveCompletion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
…ption mechanism Copilot review on #2748, both threads. 1. SeedTopLevel used a raw `Observable.Using(access.ImpersonateAsSystem, …)`. That opens the AsyncLocal scope on the SUBSCRIBING thread and disposes it when the inner observable terminates — possibly a different thread — so it can latch System onto the subscriber or restore the previous identity onto the terminating thread. `RunAsSystem` is the sealed boundary and enters/leaves on the one subscription. This was the LAST entry MonolithMeshTestBase held in ImpersonationScopeSites.allow; retired, with that guard's TotalBudget lowered 78 → 77 in the same change, as its own rule requires. 2. The second thread reported `ReportLateInitFault` as a duplicate member. It is not: line 644 is in `TypeSourceBasedUnpartitionedDataSource` (546–702) and line 845 is in `TypeSourceBasedPartitionedDataSource` (712–849) — two different records that happen to share a file. `dotnet build -c Release -warnaserror` on MeshWeaver.Data is 0 Errors / 0 Warnings, which a duplicate member could not be. Also adds InlineResumptionMechanismTest, which MEASURES rather than assumes which waits resume on the signalling thread. It exists to stop a plausible "simplification": removing a bridge by awaiting the observable DIRECTLY reads cleaner and fixes nothing, because Rx's awaiter is an AsyncSubject<T> that completes its continuation from inside OnCompleted — on the signalling thread. Measured: bridge INLINE=True, direct await INLINE=True, ObserveCompletion INLINE=False. Only RunContinuationsAsynchronously breaks the chain. That test necessarily contains the banned shape, so the guard gains ONE named exemption for it — plus TheExemptedPinningTestStillPinsTheShape, which fails if the file stops existing or stops containing the shape, so the exemption cannot decay into a hole with a comment on it. Verified: Hosting.Monolith.Test 764 passed / 0 failed (the heaviest consumer of AwaitResponseAsync), Documentation.Test guards 7 passed, the 3 new mechanism pins pass, and every touched project builds -c Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot a fix either The doc page prescribed the three real conversions but did not warn about the one substitution that looks like a fix and is not: awaiting the observable directly. Rx's awaiter is an AsyncSubject<T> that completes its continuation from inside OnCompleted, on the signalling thread, so it carries the same inline-resume property as the bridge. Measured, not reasoned about, and pinned by InlineResumptionMechanismTest; listed BEFORE the real options because it is the one that would pass review and leave the defect in place. Also fills in the Plugins production column now that sweep has landed (64). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r could not see them Found by sweeping file types rather than trusting the `--include='*.cs'` grep the inventory was built from. `src/MeshWeaver.Graph/Templates/Mirror.csx` and `NodeCopy.csx` are REAL PRODUCTION CODE — embedded resources that seed mesh script nodes and compile at RUNTIME in the portal. No `dotnet build` and no test compiles them, which is exactly the blind spot AGENTS.md names: green CI does not mean the mesh compiles. Both now wait through ReactiveCompletion.ObserveCompletion. Mirror.csx also swaps `.Take(1)` for `.FirstAsync()`: the next line dereferences `response.Message`, and `.Take(1)` completes EMPTY where the old bridge threw — ObserveCompletion would have settled with null instead, turning a missing response into a NullReferenceException inside the portal rather than a named fault. Resolution verified against the runtime import set rather than assumed: MeshScriptEnvironment.Imports supplies `Microsoft.Extensions.Logging` (so `Log.LogWarning` resolves, as `LogInformation`/`LogError` already did), `MeshWeaver.Messaging` (ObserveCompletion) and `System.Reactive.Linq` (FirstAsync/Timeout). Neither file uses `Task.ToObservable()`, so dropping `System.Reactive.Threading.Tasks` is safe. SourceScan now scans `.csx` alongside `.cs`/`.razor`, so this class of file cannot hide a bridge again. That widens the file set for EVERY guard built on it, so the whole MeshWeaver.Documentation.Test suite was re-run: 104 passed, no other guard's inventory shifted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 30, 2026
test: no ToTask in the test tree either — 1,538 bridges become the deadlock-safe Await wrapper
#2750
Merged
Contributor
Contributor
Contributor
…the merge ref Shard 3 red on the previous push, and the failing test was my own guard: NoNewBridgeInTheTreesStillBeingSwept reported two NEW SITEs. Not a false positive — the seed was stale. CI checks out the MERGE REF (main merged into the PR head), so files main gained AFTER this branch was cut are present when the guard runs but absent from a seed taken locally. Main merged PodHubNotHereRefusalTest.cs and RecycledHubRefusalTest.cs mid-review, adding 5 sites the inventory had never seen. The guard was right and the baseline was wrong. Merged main, re-seeded with the guard's own masking scanner, TotalBudget 1543 → 1548 (348 files). The allow file's header now states the rule, so the next person hits the explanation instead of the failure. Note what this does NOT change: the sweep PRs that only REMOVE sites still do not touch this file — a shrunken count is reported as STALE, never failed — so concurrent batches cannot collide here. Only a PR that ADDS a bridge trips it, which is the ratchet working. Verified: MeshWeaver.Documentation.Test 104 passed, Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
rbuergi
added a commit
that referenced
this pull request
Aug 30, 2026
Rx's `.ToTask()` completes its TaskCompletionSource from INSIDE the Rx pipeline, without RunContinuationsAsynchronously, so `TrySetResult` resumes the awaiter INLINE on the signalling thread — still inside Rx's trampoline — and `await` then pins TaskScheduler.Current for every later continuation in the same method. In an ASP.NET request pipeline that means the rest of the request, the response write included, runs on a mesh hub's single-threaded action block. That is the shape that wedges a partition hub (#2301, #2377). All 33 code sites in `memex/Memex.Portal.Shared` move to `ReactiveCompletion.ObserveCompletion` (MeshWeaver.Messaging), which completes through a TCS created with RunContinuationsAsynchronously and keeps its error arm attached after the task settles, so a late fault is reported rather than becoming an unobserved exception. Semantics preserved, site by site: * Every one of the 33 already had a `First*` reducer immediately upstream (`.FirstAsync()`, in most cases on the source line above the `.ToTask(...)`), so first-vs-last is a non-question and NO `.LastAsync()` was added anywhere. * Empty sources behave identically: `FirstAsync()` turns an empty completion into an OnError, which ObserveCompletion faults the task with — exactly what `.ToTask()` did. No endpoint that relied on the throw to produce a 404/500 can now return a silent default. * Every cancellation token is carried through unchanged (`http.RequestAborted`, `Context.RequestAborted`, `ct`). The three sites that never had one (SeoResolver.ResolveAsync, UserRoleResolver.LoadDbRolesAsync, OnboardingMiddleware.InvokeAsync) still have none, and OnboardingMiddleware says why in a comment: abandoning that wait would leave the request with no outcome at all. `reportLateFault` is never an empty lambda — each call site logs through a logger it already had, or through a small `LateFault` helper that resolves the logger EAGERLY while the request scope is alive (a late fault arrives after `HttpContext.RequestServices` is disposed, so resolving inside the lambda would throw exactly when the report is needed). `using System.Reactive.Threading.Tasks;` is dropped everywhere it is now dead. It stays in MeshApiEndpoints.cs, which still uses `Task<T>.ToObservable()` — the safe direction, and the reason that using cannot be removed blind. Comments that PRESCRIBED `.FirstAsync().ToTask(ct)` as the sanctioned edge pattern are repointed at ObserveCompletion and the 2026-08-30 ruling. Comments that already PROSCRIBE `.ToTask()` (AdminMenuGate, UserOnboardingService) are left as they are. Verified: `dotnet build -c Release -warnaserror` on Memex.Portal.Shared and on Memex.Portal.Shared.Test, both `0 Warning(s)` / `0 Error(s)`; Memex.Portal.Shared.Test 481 tests, 0 errors, 0 failed, 0 skipped. No What's New entry — the umbrella entry ships with #2748. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Wave 1 of the maintainer's 2026-08-30 ruling — "totask is forbidden" · "strictly" · "no totask ever", sharpened to "in src especially we should have zero". The doctrine itself is #2743; this is the code.
src/,tools/,samples/,clients/now contain zero.ToTask(call sites, with no allow-list at all.Why the test exemption never held
Rx's bridge completes its
TaskCompletionSourcefrom inside the pipeline, withoutRunContinuationsAsynchronously, so the awaiter resumes inline on the signalling thread, still inside Rx's trampoline — and everything the continuation does inherits that. It is sticky, too:awaitcapturesTaskScheduler.Currentwhen there is noSynchronizationContext. That mechanism is #2301 (a grain teardown parking the scheduler its own deactivation needed) and #2377 (a children listing that stayed empty forever, no error, no completion). An assertion is the last thing a test awaits before it tears the mesh down, so "only in a test" was never a containment boundary.What replaces it
ReactiveCompletion.ObserveCompletion— already this repo's documented "ONE sanctioned bridge … for the signatures that are not ours to change", andRoutingQuiescencehas carried the comment "never.ToTask()" for some time. It queues the caller's continuation instead of running it on the producer's thread, and keeps its error arm attached so a late fault is reported rather than becoming an unobserved exception.What changed
32 code sites across 10 files. 51 of the 83
grephits insrc/were doc-comment prose explaining the ban, and are deliberately untouched — see the guard note below.MonolithMeshTestBase.AwaitResponseAsync+SeedTopLevelIStorageAdapterTestExtensions(13),PermissionTestExtensions(3)ObservableAssertions(3)MeshWeaver.Reactive.Assertionsships standalone ("depends only on System.Reactive"), so it getsReactiveWait— the same guarantee expressed locally rather than a reference to the meshIIoPool.InvokeObservableOrleansRoutingService(3),IoPoolSiloTeardown,UserContextMiddleware,McpRemoteMeshClient,GenericUnpartitionedDataSource(2), 3 CLI toolsBug found and fixed on the way
ObservableAssertions.Complete()could never pass. It waited onIgnoreElements(), which completes EMPTY on success, and the old bridge reports an empty sequence by throwingInvalidOperationException— which the method's owncatchturned into "but it errored: Sequence contains no elements". It had no call sites and no test, so nothing noticed. Verified experimentally, not assumed. Both it andNotEmit(whose empty-source semantics did depend on that throw, and now keep it via an explicitFirstAsync) are pinned by new tests.The ratchet
ObservableToTaskBridgeGuard, modelled onBlockingBridgeInTestRatchetGuard, with two deliberately different rules:src/,tools/,samples/,clients/— zero, no allow file. There is no line to add and no budget to raise; the only way past is to fix the site.test/andmemex/— a seeded inventory (1543 sites) that may only SHRINK, since their sweeps are later waves. Each root moves to the zero list when its wave lands.Two properties are load-bearing and both are pinned:
Confirmed with a live negative control: an injected
.ToTask(insrc/failed the guard RED naming the file, and counted 1, not 2 — proving the pre-existing comment in that same file was correctly masked.BlockingBridgeInTestRatchetGuard's remarks named.FirstAsync().ToTask(ct)as the sanctioned test edge; corrected, since a guard that teaches the banned pattern is a defect of its own.Verification
dotnet build -c Release -warnaserror— 0 Errors, 0 Warnings (confirmed it actually compiled, not an up-to-date no-op).MeshWeaver.Persistence.Test1063 passed — the heaviest consumer of the converted storage helpers.MeshWeaver.Messaging.Hub.Test332 passed (includesDisposalWaitBridgeTest, which pinsObserveCompletion).MeshWeaver.Reactive.Assertions.Test23 passed (21 + the 2 new regression pins).MeshWeaver.Documentation.Testguards + link/What's-New integrity 6 passed.Follow-ups (named, not hidden)
test/(1510 sites), batched per project. Wave 3 —memex/(33). Both already ratcheted.MeshWeaver.Pluginsproduction sweep is in flight as a separate PR in that repo.StreamConfigurationhas anIObservable-shapedWithInitializationoverload that would drop theTaskfromGenericUnpartitionedDataSource's framework edge entirely. It routes initialization down a different path (per-emissionSetCurrentinstead of the hub-init boundary), so it is a behavioural change that belongs in its own commit rather than in a mechanical de-bridging. Recorded in the code and in the doc page.Full account — what to write at each kind of boundary, the five conversion traps met in practice, the measured inventory, and how the guard proves it is not passing on an empty scan — is committed as
Doc/Architecture/RemovingObservableToTaskBridges.🤖 Generated with Claude Code