fix(adonet): preserve membership consistency - #11317
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Hardens ADO.NET membership consistency across relational providers with atomic writes, heartbeat preservation, exact token validation, and safe cleanup.
Changes:
- Updates provider SQL and migrations for atomic membership updates.
- Adds captured-value cleanup for stale Dead entries.
- Expands relational and membership test coverage.
| File | Description |
|---|---|
test/Orleans.Runtime.Internal.Tests/MembershipTests/MembershipTableTestsBase.cs |
Updates cleanup expectations and fixtures. |
test/Extensions/Orleans.AdoNet.Tests/StorageTests/Relational/RelationalOrleansQueriesUnitTests.cs |
Adds relational behavior and cleanup tests. |
test/Extensions/Orleans.AdoNet.Tests/StorageTests/Relational/AdoNetMembershipSqlTests.cs |
Validates SQL and migration compatibility. |
src/AdoNet/Shared/Storage/RelationalOrleansQueries.cs |
Implements token validation and captured cleanup. |
src/AdoNet/Shared/Storage/DbStoredQueries.cs |
Registers the new cleanup query. |
src/AdoNet/Orleans.Clustering.AdoNet/SQLServer-Clustering.sql |
Updates SQL Server membership operations. |
src/AdoNet/Orleans.Clustering.AdoNet/PostgreSQL-Clustering.sql |
Updates PostgreSQL membership operations. |
src/AdoNet/Orleans.Clustering.AdoNet/Oracle-Clustering.sql |
Updates Oracle membership operations. |
src/AdoNet/Orleans.Clustering.AdoNet/MySQL-Clustering.sql |
Updates MySQL membership operations. |
src/AdoNet/Orleans.Clustering.AdoNet/Migrations/SQLServer-Clustering-AtomicWrites.sql |
Adds SQL Server upgrade procedures. |
src/AdoNet/Orleans.Clustering.AdoNet/Migrations/PostgreSQL-Clustering-AtomicWrites.sql |
Adds PostgreSQL upgrade procedures. |
src/AdoNet/Orleans.Clustering.AdoNet/Migrations/Oracle-Clustering-AtomicWrites.sql |
Adds Oracle upgrade procedures. |
src/AdoNet/Orleans.Clustering.AdoNet/Migrations/MySQL-Clustering-AtomicWrites.sql |
Adds MySQL upgrade procedures. |
src/AdoNet/Orleans.Clustering.AdoNet/Messaging/AdoNetClusteringTable.cs |
Enforces matching row and table tokens. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage
Report-only conclusion: current-main baseline stale. The newest successful coverage run tested f278e80, not current main 2994764. Coverage combines every CI test matrix job, including providers, CodeGen, .NET 8/10, Linux, Windows, and macOS, using canonical physical source and branch identities. The comparison remains report-only while normal line and branch variance is calibrated. Coverage details |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The documented migration rollout can leave older processes using cached unsafe queries during the upgrade window.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical lock-order deadlocks remain possible, and negative version tokens are not rejected locally.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (5)
Resolved since last review (1)
Problem
Some existing ADO.NET membership definitions commit a version increment when the membership row is missing, rely on optional PostgreSQL assertions for rollback, or clean up non-Dead rows using heartbeat age alone. Improving these definitions should preserve operational simplicity and compatibility with existing deployments.
Solution
Keep the SQL enhancements in the four existing
*-Clustering.sqlinstallation scripts. This PR adds no operational scripts and leaves the historical migrations unchanged. Existing silos and ADO.NET gateway-discovery clients can upgrade the provider package while continuing to use their installed SQL catalog; no database update or new query key is required.The script enhancements include atomic rollback on failed membership updates, PostgreSQL rollback independent of optional assertions for two-write mutations, and improved Dead-row cleanup. Schemas, stored formats, and existing query parameter sets remain unchanged. Fresh installations receive the definitions through the usual database setup.
Each silo owns its heartbeat: the dedicated operation remains one blind, primary-key-targeted
IAmAliveTimeassignment, with no pre-read, version/status check, or provider retry. Canonical updates can use the original logical row/table tokens captured before a heartbeat. Full-row SQL retains the inexpensive inline heartbeat maximum within its already-required update statement.SQL Server membership read queries remain unchanged from the original scripts.
SQLServer-Main.sqlalready enablesREAD_COMMITTED_SNAPSHOT, providing coherent statement snapshots without added read-lock hints. Speculative version-overflow guards are omitted.Existing-deployment compatibility
Original catalogs keep using their installed
CleanupDefunctSiloEntriesKeyquery and behavior. The captured-rowCleanupDefunctSiloEntryKeyis optional: when installed, it enables exact exclusive start/heartbeat/suspicion eligibility and a native condition protecting concurrent changes. Both paths preserve native errors and cancellation. Cleanup remains unversioned.Providers cache definitions during initialization. Operators choosing to adopt revised definitions can use their normal database change-management process and reinitialize providers afterward. The enhanced SQL behavior applies to callers using those definitions; older installed definitions retain their existing behavior. The provider never applies DDL or rewrites the catalog during startup.
Compatibility coverage
Native scenarios install either frozen pre-change definitions or the current installation scripts for SQL Server, PostgreSQL, and MySQL. Both cases initialize current silo and gateway providers, read and mutate membership, publish a blind heartbeat, reuse original tokens, and execute the installed cleanup path. Enhanced cases cover atomic conditional failures and retention-aware cleanup. SQL Server reader/writer coverage verifies the existing snapshot configuration and coherent observations.
Fresh clustering test setup uses the current installation definitions directly, so historical upgrade scripts cannot overwrite current routines. Oracle native execution remains outside the available test infrastructure.
Split prerequisite
Includes only the tiny legacy cleanup-test preparation from the first commit of #11308 (
bede893d290a9035f196f4c3ed58213d102efc6c), retained as a separate cherry-pick. The provider work was carved from frozen commit1dfe8b54cda3582ddf3795113eaa2f4e9d1b6254ontod515d75eaaa3a0390a74cb11c96aa758358b5a67, then simplified to existing-script enhancements with original-catalog compatibility. Shared conformance-kit wiring remains separate.Microsoft Reviewers: Open in CodeFlow