Repository navigation
Path-less queries go live on partitioned Postgres — revocations reach readers without a restart (#697) - #905
Merged
Merged
Conversation
… readers without a restart Fixes #697. Removed users kept reading protected data until the portal restarted. Mechanism: PostgreSqlPartitionedMeshQuery.FanOutQuery was one-shot by design — it emitted a single Initial and completed, with no change-feed subscription (the class doc declared 'live deltas across partitions are out of scope'). PermissionEvaluator.ObserveAllMembershipNodes opens a PATH-LESS query (nodeType:GroupMembership scope:subtree) — path-less on purpose, because a group and its members can live in a different partition than the grant. On partitioned PG a path-less query pins to no partition, so all three providers went silent: the fan-out completed after one Initial, the pedestrian deferred to the native provider on an empty path, and the static provider completed. With no live subscription anywhere, the process-wide Replay(1) snapshot behind IMeshNodeStreamCache.GetQuery froze at its Initial for the life of the process. Revocation only ever appeared to work through SyncedQueryMeshNodes' un-gated IMeshChangeFeed Deleted fast-path, which several delete routes bypass and which has no Created/Updated counterpart — hence the report that re-creating a membership had no live effect either. The same freeze hit every path-less security query: $security-roles and the ROOT $security-policy. This is a THIRD, distinct member of the family: #868 and #889 were equality/dedup gaps ON a live path; here there was no live source at all. The fix composes with both (the Added it now emits relies on #889's duplicate-Added pass-through). Fix: FanOutQuery honours the Initial-then-deltas contract — subscribes MergedChanges, buffers pre-Initial notifications (the same race-fix its two siblings already carry), re-queries serialised via Concat, diffs into Added/Updated/Removed. Cost is bounded by an EXACT relevance gate rather than a debounce, because a path-less query's path/scope predicate matches every notification and would otherwise run a cross-schema UNION per write: a path already in the result set is relevant; otherwise a Deleted is not (a delete can only remove what we do not hold); otherwise the framework's own QueryEvaluator.Matches decides; unclassifiable re-queries. Erring toward over-notifying is deliberate — under-notifying is a security hole, over-notifying costs a query. livePaths is an ImmutableHashSet swapped wholesale under Volatile so the hot filter takes no lock. Tests (PG-only — the defect cannot exist on in-memory backends, where the pedestrian serves path-less queries live): - PathLessNodeTypeQuery_DeliversLiveAddedAndRemoved pins the root defect at the query layer, both directions. - GroupMembership_GrantThenRevocation_ReachTheLivePermissionPath drives the issue's exact topology through GetEffectivePermissions: denied → join → readable → leave → denied, and the member's own root grant survives. The first barrier is deliberately POSITIVE; a deny-shaped wait is satisfied by a cold stream's first Permission.None and certifies nothing. Fail-without (src reverted, rebuilt, re-run): both tests fail — the query emitted 'Last of 1 emission(s) was: ChangeType = Initial' and nothing ever again; the evaluator folded once to None and never re-folded. Pass-with: 2/2. Regression: full MeshWeaver.Hosting.PostgreSql.Test 641 passed/1 skipped, MeshWeaver.Security.Test 345 passed, AccessRestriction 14 passed; independently re-verified after merging main (12/12 across the new class, EffectivePermissionPostgresTest and PaywallRealGateShapeTests). Closes #697 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #697 where path-less security queries (e.g., nodeType:GroupMembership scope:subtree) could become permanently stale on partitioned PostgreSQL, causing group-membership revocations to not reach the live permission-evaluation path until process restart.
Changes:
- Makes
PostgreSqlPartitionedMeshQuery.FanOutQueryemit live Added/Updated/Removed deltas after its Initial snapshot, instead of completing after Initial. - Adds a relevance gate + snapshot diffing to bound cross-schema re-queries for path-less / unpinned queries.
- Adds new PostgreSQL integration tests covering both (a) the query-layer contract and (b) the end-to-end permission revocation scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/MeshWeaver.Hosting.PostgreSql/PostgreSqlPartitionedMeshQuery.cs |
Extends cross-schema fan-out queries from one-shot Initial to a live Initial-then-deltas stream with relevance gating and snapshot diffing. |
test/MeshWeaver.Hosting.PostgreSql.Test/GroupMembershipLiveRevocationTests.cs |
Adds regression tests ensuring path-less GroupMembership queries and effective-permission folding react live to membership add/remove. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Author
|
🚧 Not merging — the fix does not hold on CI. Run 31168629021 shard 5: That is this PR's own new test, and it failed with the exact pre-fix signature: one Do not merge until that is root-caused. Leading candidates, in order:
The second CI failure on that shard, 🤖 Generated with Claude Code |
rbuergi
marked this pull request as draft
August 7, 2026 10:27
This was referenced Aug 7, 2026
rbuergi
marked this pull request as ready for review
August 8, 2026 06:19
…said otherwise The live cross-schema fan-out this branch introduced was still returning the wrong answer, and CI said so: PathLessNodeTypeQuery_DeliversLiveAddedAndRemoved emitted its Initial and then nothing, for the full 60 s budget. Making the fan-out live is not enough if it re-queries the wrong set of partitions. The UNION spans public.searchable_schemas, and the only writer of that registry was a discovery sync throttled to one run per 30 s and triggered only by query traffic. So a partition provisioned inside that window was absent from the list — and for a LIVE query that is not a delay, it is permanent loss: the re-query fires only when a change notification arrives, so a write into the young partition is looked for exactly once, against a list that cannot contain it, and nothing ever looks again. A GroupMembership created in a just-provisioned Space never reached $security-memberships — the same fail-OPEN revocation hole as #697, one layer down. EnsurePartitionProvisioned — the ONE place that creates a partition — now writes the schema into the registry as part of provisioning, behind the same predicate the sync applies (registering an excluded schema, notably the auth mirror, would double every access object until the next sync deleted it again). The registry is written from knowledge instead of rediscovered by a poll; the poll remains for partitions other processes create. The test was passing locally and failing in CI because it seeded the Space before opening the query, which left the outcome to whichever ran first — the first fan-out or the partition's provisioning. It now opens the query BEFORE the partition exists, which is both the reported shape (a running portal, a new space) and a deterministic pin: without this fix it fails on any machine, with the exact CI message. Review feedback, both real: - changeBuffer has two producers (the live feed and the post-Initial backlog drain), so it is Subject.Synchronize'd; the inner subject is what gets disposed. - A delete racing the initial read was dropped while livePaths was still empty, which could leave a just-deleted row in the Initial with nothing to correct it. Deletes now count as relevant until the Initial snapshot is established. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The schema-registry fix flips the deterministic local repro but CI still fails on the same wait, so a SECOND precondition is missing there and the assertion cannot say which. The three the cross-schema re-query needs are: the partition's mesh_nodes table exists, the schema is registered in public.searchable_schemas, and the row is actually in that schema. This dump prints all three right after the membership write, so the next CI log distinguishes "never provisioned" from "provisioned but unregistered" from "registered but the write went somewhere else" — instead of another 60 s timeout that says only "no Added". To be removed once the CI-only cause is identified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fresh path-less query issued at the same instant separates the two remaining candidates in one CI run: if its Initial carries the membership, the cross-schema SQL and access filter are fine and the live change notification never arrived; if it does not, the re-query itself cannot see the row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI now shows the row IS in the right schema and the schema IS registered, yet a fresh path-less query returns nothing — so the per-schema access gate is what hides it. That gate is a public.partition_access row for the caller plus a Read fold over the partition's user_effective_permissions. Dump both, plus the caller identity, and re-dump AFTER the failed wait to separate 'the projection lands late and nothing re-queries' from 'the caller never gains Read on the new partition'. Locally both are present (partition_access=[Roland], Read=True) and the fresh query sees the row, which is why this is CI-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dump established, on CI itself, that when the feature test fails there the row IS in the right schema, the schema IS registered in public.searchable_schemas, the caller IS Roland, public.partition_access holds Roland for that partition and its user_effective_permissions grants Read — and a FRESH path-less query issued at that instant still returns zero rows, unchanged 60 s later. So the remaining CI-only failure is the cross-schema UNION returning empty with every precondition satisfied, not the registry, not the projection, not identity, and not the change notification. The mechanism is reproducible locally: a partition that ships mesh_nodes WITHOUT the per-partition permission tables makes the union's access clause name a missing relation, Postgres fails to plan the whole statement with 42P01, and the provider swallows 42P01 as "no rows" — one such partition zeroes the ENTIRE union. Left unfixed here: both candidate shapes (treat those schemas as public, or skip them) each break other tests, so the fix needs its own change with room to get the access semantics right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n for everyone This is the second half of #697's CI failure, and the half that is a live security-shaped bug in its own right. The per-schema access clause names {schema}.user_effective_permissions. A partition that ships mesh_nodes WITHOUT that table turns the clause into a reference to a missing relation, so Postgres fails to PLAN the whole UNION with 42P01 — and EnumerateReaderOrEmptyOnMissingRelationAsync absorbs 42P01 as "this satellite table isn't here" and yields nothing. The failure was SILENT and TOTAL: one such partition returned an empty result for every authenticated user's unscoped query, however healthy the other fifty partitions were. On CI such a partition accumulates; on a three-schema local container it never does, which is exactly why the #697 live-query test failed only there with every precondition (row present, schema registered, partition_access and Read granted, right caller) provably satisfied. The fix drops the unfilterable branch instead of letting it end the statement, and that is result-equivalent, not a narrowing: the clause's other half is public.partition_access, and the SAME function that builds a partition's user_effective_permissions populates its partition_access rows — so a schema lacking the table has no partition_access row either and its branch could only ever have matched zero rows. Fail CLOSED, deliberately. Contributing the branch UNFILTERED — what public.search_across_schemas does for this case — publishes every row of an ungranted partition to every authenticated user; PerSchemaAccessClauseLeakTests catches it (FutuRe/Report surfacing for a user with no access), and it is the same class #471/#385 RC3 closed by moving public_read inside the partition gate. The proc's overload has no caller on the fan-out path, so the two do not disagree anywhere live; where they do differ, the proc is the unsafe one. The earlier attempt at this failed because it resolved "which schemas are access-controlled" through GetSchemasWithTableAsync, which starts from searchable_schemas and therefore cannot see the partition-pinned fast path's schema — good partitions were classified unfilterable and granted cross-partition nodes vanished. Resolution is now a direct information_schema lookup over exactly the schemas in play. CrossSchemaMissingPermissionTableTests pins it and is load-bearing: with the resolution disabled it fails, with it restored it passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in PostgreSqlFixture ever drops a schema, and CleanDataAsync locks every (schema, table) pair of the container in ONE transaction between tests — so each partition a test leaves behind permanently raises the lock count every later test pays, until the container answers 53200: out of shared memory. A test that fabricates a partition purely to break it has no business leaving it in the shared container. 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.
Summary
Fixes #697 — removed users kept reading protected data until the portal restarted.
Mechanism.
PostgreSqlPartitionedMeshQuery.FanOutQuerywas one-shot by design: a singleInitial, then complete, with no change-feed subscription (its class doc declared "live deltas across partitions are out of scope").PermissionEvaluator.ObserveAllMembershipNodesopens a path-less query (nodeType:GroupMembership scope:subtree) — path-less deliberately, because a group and its members can live in a different partition than the grant. On partitioned PG a path-less query pins to no partition, so all three providers went silent: the fan-out completed after its Initial, the pedestrian deferred to the native provider on an empty path, the static provider completed. With no live subscription anywhere, the process-wideReplay(1)snapshot behindIMeshNodeStreamCache.GetQueryfroze at its Initial for the life of the process.Revocation only ever appeared to work via
SyncedQueryMeshNodes' un-gatedDeletedfast-path — which several delete routes bypass, and which has no Created/Updated counterpart. That is exactly why the reporter saw re-creating a membership have no live effect either: it failed in both directions.The same freeze silently affected every path-less security query —
$security-rolesand the root$security-policy.Family: a third, distinct member. #868 and #889 were equality/dedup/deserialization gaps on a live path; here there was no live source at all. The fix composes with both — the
Addedit now emits relies on #889's duplicate-Addedpass-through.The fix
FanOutQuerynow honours the Initial-then-deltas contract: subscribesMergedChanges, buffers pre-Initial notifications (the same race-fix its two siblings already carry), re-queries serialised viaConcat, diffs intoAdded/Updated/Removed.Cost is bounded by an exact relevance gate, not a debounce — a path-less query's path/scope predicate matches every notification, so a naive re-query would run a cross-schema UNION per write. In order: a path already in the result set is relevant; otherwise a
Deletedis not (a delete can only remove something we don't hold — which also makes the null-Entitydelete payload a non-problem); otherwise the framework's ownQueryEvaluator.Matchesdecides; unclassifiable re-queries. Erring toward over-notifying is deliberate: under-notifying is a security hole, over-notifying costs one query.livePathsis anImmutableHashSetswapped wholesale underVolatile, so the hot filter takes no lock. No async/await outside the existingIIoPoolleaf, noFromAsync, no statics.Verification
Fail-without (src reverted, rebuilt, re-run): both new tests fail — the query reported "Last of 1 emission(s) was: ChangeType = Initial" and nothing ever again; the evaluator folded once to
Noneand never re-folded. Pass-with: 2/2.Regression: full
MeshWeaver.Hosting.PostgreSql.Test641 passed / 1 skipped,MeshWeaver.Security.Test345 passed,AccessRestriction*14 passed. Independently re-verified after merging current main: 12/12 across the new class +EffectivePermissionPostgresTest+PaywallRealGateShapeTests.-c Release -warnaserrorclean.Residual risks (documented, not hidden)
FanOutQueryno longer completes. Consumers use.Where(Initial).Take(1), and the per-schema delegate serving every scoped query already never completed, so nothing could depend on completion — grepped for terminalToList()/LastAsync/Aggregateon query streams, none found; full PG suite confirms.ItemEqualslogic; flagged in the XML doc as a consolidation candidate. Deliberately not refactored — the other two are the The paywall's deny assertions were passing without asserting anything #849/One disposed subscriber silently starved every other one — including the security fold #853/Permission changes were silently swallowed — compare assignment content, not just paths #868/PaywallRealGateShapeTests: buyer's AccessAssignment update never reaches the cached $security-access stream — 3 PRs redded on shard 5 in one morning (post-#868) #889 flake-sensitive paths.No What's New entry: security fix; the user-visible effect (revocation takes effect immediately) rides the fix.
🤖 Generated with Claude Code