You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The built-in membership manager reads coherent full-table snapshots. A separate point-row read contract adds provider behavior and test complexity without a runtime consumer.
Solution
Retain and deprecate IMembershipTable.ReadRow and ReadRowAsync, preserving their signatures and wire identity. First-party implementations report NotSupportedException after observing pre-cancellation. The default interface adapter remains available to custom legacy providers.
Migrate first-party tests and conformance observations to ReadAllAsync with MembershipTableData.TryGet, selecting the row, ETag and table version from the same snapshot. Keep the 959 generated histories, 18 internal operation identities, and exact hosted partition multiplicities; present/absent observations now use snapshots. Public G15/G22 scenario names remain obsolete aliases while normal discovery avoids duplicate full-read scenarios.
Scope and setup cost
Per review feedback, the proposed receipt-returning mutation APIs, DTOs, provider plumbing and receipt-only tests have been removed. Existing bool-returning mutation APIs, native write behavior, and RPC identities remain unchanged. No artificial runtime consumers or replacement test hooks are added.
Large conformance setup uses existing bool inserts and full snapshots to obtain the current ETag. For N rows it performs N inserts and N+3 full reads, returning N(N+1)/2 + 2N membership rows. This explicitly accepted test-only cost is 8,398,848 returned rows at N=4096. Expected canonical entries remain independently constructed, with per-insert and final verification.
The landed provider read fences, ZooKeeper single-attempt mutation/session ownership, Firestore transactional full reads, cancellation behavior and fixture ownership remain intact. Migration guidance and regenerated API surfaces accompany the behavioral change. No database/catalog or package-validation policy changes are introduced.
The receipt token is computed only after the native insert has completed. With an expected ETag of int.MaxValue, the SQL write can commit the next database version and then CreateWriteResult throws OverflowException, so the caller receives a deterministic failure despite a committed mutation and no receipt (the new unit test explicitly exercises this). Validate that the next token is representable before issuing the write, then return the precomputed receipt only when the write succeeds.
This issue also appears on line 158 of the same file.
Review 5395407025 contains one medium Previously missed finding recommending prevalidation, despite its top-level “Findings: None”. I reviewed that finding and am retaining the approved caller/native boundary rather than adding another provider guard.
The risk is real for manually fabricated, out-of-protocol candidates: Oracle's version column is NUMBER(*,0), and its successful native mutation can commit a number beyond Int32. SQL Server/MySQL use INT, and PostgreSQL uses integer; their native numeric limits apply instead. Oracle here is source/scripted evidence, not a live Oracle result.
Existing bool entrypoints still return their existing native outcome. Rich entrypoints await the same operation, then construct the actual committed expected-ETag-plus-one receipt. MembershipReceipt_UnrepresentableOracleCommitToken_ThrowsAfterNativeSuccess explicitly verifies the post-native exception and unchanged bool outcome. False writes carry no receipt. There is no wrapped token, replay, or later read presented as originating-commit metadata.
Prevalidation would introduce a new malformed-input policy: for example, a stale maximum-valued ETag which the database would reject with false would instead throw before the native condition is evaluated; native error ordering would also change. The approved scope deliberately retains caller-valid successors and native failure semantics rather than introducing that guard or changing SQL catalogs.
Disposition: retain the explicit overflow outcome for this out-of-protocol/unrepresentable native result, acknowledge that it does not imply rollback, and preserve the unchanged bool contract. The finding exists only in the review body (no inline thread to resolve). Final native CI evidence will be reported separately and will not be described as Oracle execution.
The newest successful coverage run tested 6bf11ad, not current main 9bb744a.
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.
Head: 793181549c79f26c141353559b4df2677930554a. The exact-head gate is 85 successful checks, one expected Pages deployment skip, zero failed/pending checks. Native/full CI, documentation, static analysis and CodeQL all completed. The PR is ready for human review and remains unmerged.
Requirement
Evidence
Optional receipts without breaking existing providers or bool RPCs
DefaultWriteResult_InvokesBoolOnceWithoutReading, generated receipt roundtrips, GeneratedProxyCalls_PreserveLegacyWireCompatibility, and additive rich-RPC tests.
Exact originating-commit metadata
Actual native receipt cases passed for Azure Table, Cosmos no-content batches, DynamoDB, Firestore, Consul, Redis, Cassandra and ZooKeeper; ADO.NET frozen-original/current-fresh cases passed on SQL Server, PostgreSQL and MariaDB. Selected exact-head TRX outcomes were checked directly. Oracle remains source/scripted coverage only.
ZooKeeper ambiguous outcomes remain exceptions
Rich and bool wrapper commit/close-loss cases, callback-plus-close failure cases, owned Multi/close lifetime cases, and the landed sequential/read-fence regressions passed. Rich writes use the single-attempt session owner.
Row-read retirement and compatibility
All built-in signatures remain and return unsupported after pre-cancellation; zero-I/O/zero-RPC cases pass. The custom legacy-provider default adapter and old generated request identities remain covered. Ordinary callers use full snapshots and TryGet.
Linear setup and explicit absent-receipt behavior
ConcurrentReadSetup_UsesCommitReceiptsAndIndependentFinalView passes at N=5, 1001 and 4096, including provider-token variants: N actual richer inserts, three full snapshots, 2N returned membership rows, zero point reads. Missing-receipt coverage proves one successful write/one initial read, then an explicit prerequisite error without replay or readback.
Preserve history coverage while changing observations
Manifest_DefaultGenerationPreservesOriginal959Cases and exact partition/multiplicity checks pass. All 18 internal operation identities remain; present/absent observations now use snapshot lookup. Normal discovery has 26 direct scenarios plus generated conformance (four generated partitions for the hosted target); G15/G22 remain tested obsolete aliases without duplicate normal discovery.
Generated surfaces and compatibility
Core, TestKit, ADO.NET, Consul and ZooKeeper surfaces regenerated via GenAPI. Normal Release pack checks passed with no new compatibility suppressions. Documentation validation passed, including 240 site tests and link/output audits.
Review disposition: review 5395407025 contains a medium “Previously missed” ADO.NET overflow finding despite its top-level “Findings: None”. The source/test-linked disposition records the checked Next()/runtime caller boundary, unchanged bool behavior, and the explicit manual out-of-protocol Oracle post-commit exception caveat. No prevalidation guard, SQL expansion, replay or readback receipt was added. There are no inline review threads to resolve; this is not a claim that no concern was raised.
ReubenBond
changed the title
feat(membership): return commit receipts and retire row reads
refactor(membership): retire row reads in favor of snapshots
Oct 3, 2026
Add the result contract, compatibility defaults, runtime cores, and disjoint provider receipts while retaining existing bool APIs and row reads. Azure/Dynamo receipt patches and ADO receipt tests remain deferred for the separately owned wiring fixes; final retirement integration follows the wiring landing.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Retiring public read behavior across providers requires maintainer validation of compatibility and backend test results.
Review effort: Balanced Findings: None
This branch has not been deployed
No deployments
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
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.
Problem
The built-in membership manager reads coherent full-table snapshots. A separate point-row read contract adds provider behavior and test complexity without a runtime consumer.
Solution
Retain and deprecate
IMembershipTable.ReadRowandReadRowAsync, preserving their signatures and wire identity. First-party implementations reportNotSupportedExceptionafter observing pre-cancellation. The default interface adapter remains available to custom legacy providers.Migrate first-party tests and conformance observations to
ReadAllAsyncwithMembershipTableData.TryGet, selecting the row, ETag and table version from the same snapshot. Keep the 959 generated histories, 18 internal operation identities, and exact hosted partition multiplicities; present/absent observations now use snapshots. Public G15/G22 scenario names remain obsolete aliases while normal discovery avoids duplicate full-read scenarios.Scope and setup cost
Per review feedback, the proposed receipt-returning mutation APIs, DTOs, provider plumbing and receipt-only tests have been removed. Existing bool-returning mutation APIs, native write behavior, and RPC identities remain unchanged. No artificial runtime consumers or replacement test hooks are added.
Large conformance setup uses existing bool inserts and full snapshots to obtain the current ETag. For N rows it performs N inserts and N+3 full reads, returning N(N+1)/2 + 2N membership rows. This explicitly accepted test-only cost is 8,398,848 returned rows at N=4096. Expected canonical entries remain independently constructed, with per-insert and final verification.
The landed provider read fences, ZooKeeper single-attempt mutation/session ownership, Firestore transactional full reads, cancellation behavior and fixture ownership remain intact. Migration guidance and regenerated API surfaces accompany the behavioral change. No database/catalog or package-validation policy changes are introduced.