Repository navigation
Release the permission-fold gate — kill the #899 deadlock at its generator - #918
Merged
Merged
Conversation
…rator Fixes #899. Two prior attempts (#910, #917) removed the CONDUIT by making change-feed fan-out asynchronous; CI proved that breaks three documented synchronous cache-invalidation contracts (PathResolutionService._resolutionCache, MeshNodeStreamCache's failure reset, Workspace._remoteStreamCache), so SearchResultStormTest and friends went red. This removes the GENERATOR instead and leaves delivery synchronous. The cycle needs two ingredients. Ingredient 2 is that CheckDeletePermissionForNode is GetEffectivePermissions().Take(1) .SelectMany(<entire delete pipeline>), and the fold's CombineLatest emits synchronously during Subscribe (cached ReplaySubject sources) — so storage delete, cache invalidation and change-feed publish all ran while holding that gate. New HubPermissionExtensions.TakeDecisionOutsideGate<T>() takes the decision inside the fold and re-emits via Observable.Return(v, Scheduler.Default) so only the continuation leaves it. Both of #917's claims were re-measured rather than inherited: ObserveOn takes 64 subscriptions from 13 to 77 threads while the Return form goes 13 to 16; and the hop preserves the AsyncLocal AccessContext (Scheduler.Default queues with ExecutionContext capture), pinned as a test. Five dangerous sites fixed: CheckDeletePermissionForNode; AccessControlPipeline.cs:123 (broadest in the repo — the onCompleted branch invokes the whole downstream handler for every [RequiresPermission] message, and the hop sits after the Catch so the fail-closed path leaves the gate too); RlsNodeValidator.Validate's final Take(1) (every validated create/update/delete, placed at the end so the custom-rule path is covered); EnsurePartitionBootstrap; SkipNoOpIfAuthorized. Change-feed delivery is untouched, and a new test pins that it stays synchronous so a future async attempt fails loudly. Also lands the Text terminal-lock guard in ThreadExecution .PushToResponseMessage from #910 — Status, ToolCalls and UpdatedNodes were already protected against a late push regressing them; Text was not, so a delayed placeholder could overwrite a finished answer. A regression I introduced, found and root-caused rather than papered over: the delete handler posts DeleteNodeResponse INSIDE the Observable.Using subtree-deletion guard, and Rx disposes a Using resource only after OnCompleted propagates, so the guard outlived the response by ~5 ms. That latent bug was masked because the delete used to run inline on the per-node hub's turn, which queued a caller's follow-up create behind it; leaving the fold's gate also leaves the turn, so the race became near-certain (WorkspaceCacheEvictionTest 0/15 on main, 14/15 with the hop alone). Fixed by releasing the guard explicitly just before posting success — Dispose is Interlocked-idempotent, so the enclosing Using stays the error/timeout safety net. Back to 0/15. The test was not adapted and the pipeline was not pinned back onto the turn. Soak 15/15 consecutive NodeOperations runs at 85/85; Monolith x4; Hosting 115, Query 361, Data 303, Graph 818+2, Orleans 136, AI 1037+5. The six tests the async approach disturbed all pass UNMODIFIED — zero existing test files touched. Closes #899 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #899 by preventing permission-evaluation “fold gate” lock-order inversions from enclosing downstream handler work, while keeping change-feed delivery synchronous (to preserve cache invalidation contracts). It introduces TakeDecisionOutsideGate<T>() and applies it at several high-leverage call sites, plus adds regression tests and tightens thread message Text terminal-locking.
Changes:
- Add
HubPermissionExtensions.TakeDecisionOutsideGate<T>()to move post-decision continuations off the permission fold’s synchronous emission gate (while still taking the decision inside the fold). - Apply the new operator to multiple permission-gated pipelines (delete permission check, access-control pipeline, RLS validator, partition bootstrap / no-op skip sites) and fix subtree-deletion guard disposal ordering around delete success.
- Add regression tests/harnesses that pin both the deadlock class and the synchronous change-feed delivery contract; extend AI thread message terminal-locking to
Text.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/MeshWeaver.Hosting.Test/RxFanOutInversionHarness.cs | Adds a reusable concurrency harness to deterministically probe the lock-order inversion cycle. |
| test/MeshWeaver.Hosting.Test/PermissionFoldGateTests.cs | Adds regression tests for #899, including a pinned contract that in-process change-feed delivery stays synchronous. |
| src/MeshWeaver.Mesh.Contract/MeshExtensions.cs | Switches key permission checks to TakeDecisionOutsideGate and fixes delete guard disposal ordering before posting success. |
| src/MeshWeaver.Mesh.Contract/HubPermissionExtensions.cs | Introduces TakeDecisionOutsideGate<T>() implemented via Observable.Return(..., Scheduler.Default) to avoid ObserveOn thread costs. |
| src/MeshWeaver.Hosting/Security/AccessControlPipeline.cs | Moves the broad [RequiresPermission] pipeline off the permission fold gate via TakeDecisionOutsideGate. |
| src/MeshWeaver.Graph/Security/RlsNodeValidator.cs | Ensures validated operations don’t run subsequent writes under the fold gate by using TakeDecisionOutsideGate. |
| src/MeshWeaver.AI/ThreadExecution.cs | Extends terminal-lock semantics to Text to prevent late streaming/placeholder pushes from clobbering completed output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+106
to
+109
| // Both bodies must be live at the same instant, or no cycle can form. | ||
| bothInsideTheirBody.SignalAndWait(DeadlockBound); | ||
| publish(tag); | ||
| completed.TrySetResult(); |
Contributor
Test Results (shard 0)672 tests 672 ✅ 4m 49s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results (shard 5)1 629 tests 1 438 ✅ 5m 49s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results (shard 4)1 075 tests 1 070 ✅ 5m 42s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results (shard 3) 11 files 11 suites 4m 43s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results (shard 2)1 726 tests 1 724 ✅ 7m 13s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results (shard 1)1 225 tests 1 225 ✅ 6m 58s ⏱️ Results for commit 25abf8d. |
Contributor
Test Results 65 files 65 suites 35m 16s ⏱️ Results for commit 25abf8d. |
This was referenced Aug 7, 2026
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.
Fixes #899. Supersedes #910 and #917, whose approach (making change-feed fan-out asynchronous) CI proved breaks three documented synchronous cache-invalidation contracts. This removes the generator instead and leaves delivery synchronous.
The change
The cycle needs two ingredients. Ingredient 2:
CheckDeletePermissionForNodeisGetEffectivePermissions().Take(1).SelectMany(<entire delete pipeline>), and the fold'sCombineLatestemits synchronously during Subscribe (cachedReplaySubjectsources) — so storage delete, cache invalidation and change-feed publish all ran while holding that gate. NewTakeDecisionOutsideGate<T>()takes the decision inside the fold and re-emits viaObservable.Return(v, Scheduler.Default), so only the continuation leaves it.Both of #917's claims were re-measured, not inherited:
ObserveOntakes 64 subscriptions from 13 → 77 threads while theReturnform goes 13 → 16; and the hop preserves the AsyncLocalAccessContext(Scheduler.Defaultqueues withExecutionContextcapture) — pinned as a test.Five dangerous sites fixed:
CheckDeletePermissionForNode;AccessControlPipeline.cs:123(broadest in the repo — invokes the whole downstream handler for every[RequiresPermission]message; the hop sits after theCatchso the fail-closed path leaves the gate too);RlsNodeValidator.Validate's finalTake(1)(every validated write, placed at the end so the custom-rule path is covered);EnsurePartitionBootstrap;SkipNoOpIfAuthorized.Change-feed delivery is untouched, and a new test pins that it stays synchronous so a future async attempt fails loudly. Also lands #910's
Textterminal-lock guard —Status/ToolCalls/UpdatedNodeswere protected against a late push regressing them,Textwas not, so a delayed placeholder could overwrite a finished answer.A regression I introduced, found, and root-caused
Monolith run 2 failed
WorkspaceCacheEvictionTest. Baselined at 15 iterations per arm: pristine main 0/15, my hop alone 14/15, hop + fix 0/15.Mechanism (instrumented): the delete handler posts
DeleteNodeResponseinside theObservable.Usingsubtree-deletion guard, and Rx disposes aUsingresource only after OnCompleted propagates — so the guard outlived the response by ~5 ms. That latent bug was masked because the delete used to run inline on the per-node hub's turn, queuing a caller's follow-up create behind it; leaving the fold's gate also leaves the turn, so the race became near-certain. Fixed the ordering — release the guard explicitly just before posting success (DisposeisInterlocked-idempotent, so the enclosingUsingstays the error/timeout safety net). The test was not adapted and the pipeline was not pinned back onto the turn.Verification
Soak 15/15 consecutive full
NodeOperationsruns at 85/85 · Monolith ×4 · Hosting 115/115 · Query 361/361 · Data 303/303 · Graph 818+2 · Orleans 136/136 · AI 1037+5. Deadlock proof: same binaries with the idiom's body swapped to a bareTake(1)→ 3 failed (both probes hang to the 10 s bound); with it → 6/6 in 36 ms. The precondition test is kept so the others cannot pass vacuously.The six tests the async approach disturbed all pass UNMODIFIED —
SearchResultStormTest,PandasExplorerLayoutAreaTest,HarnessInstallGateTest,CreateNodesRequestTest,MeshNodeStreamCacheIdleReleaseTest,GrainActivationFailureRegistryTest. Zero existing test files touched.Residual
StaleActivationSeedRollbackTestis red on main at 8/15 (pre-existing, filed via Change feed dispatches off the publisher's thread — kills the delete deadlock (#899) #910); this change makes it less frequent (4/15) but it will still red CI intermittently.NodeTypeReleaseExtensions.cs:93,MeshNodeStreamCache.cs:1406,AccessControlPipeline.HandleGetPermission:245.Observable.Usingwhose resource is a guard and whose inner pipeline posts the done signal. One instance fixed; not audited repo-wide.Closes #899
🤖 Generated with Claude Code