fix: condition counter writes on the read-time ETag to prevent duplicate ids under multi-instance concurrency#30
Conversation
…ate ids TryOptimisticWrite conditioned its upload on the blob ETag fetched at write time (GetProperties), not on the ETag of the value GetData returned. A competing writer that committed between the read and the write therefore went undetected: its commit changed the ETag, the late writer fetched that new ETag, and the If-Match condition passed. Both writers succeeded and both processes handed out the same id range. GetData(Async) now uses DownloadContent, which returns the content and its ETag from a single response, and records that read-time ETag per block. TryOptimisticWrite(Async) consumes it for the If-Match condition, turning the read->write cycle into a correct compare-and-swap: an intervening commit fails the write with 412 and UniqueIdGenerator re-reads and retries. Writers without a preceding read keep the previous current-ETag behaviour. Adds an Azurite integration test with four independent generators allocating concurrently from one scope (BatchSize 1), which reproduces the duplicate ids on the previous implementation.
|
@0x414c49 Can you check this? |
|
Nice find, and the repro test is great, really shows the bug clearly (231/400 unique on main before this fix, all 400 unique after). Tested it locally against main and against this branch to confirm. One thing worth a look before merging: Not something I'd block on since it can't happen with how the store is registered/used in this repo, but maybe worth a short comment on the class or the field noting that read+write for the same block need to stay serialized per instance, so nobody hits this if they use Also, small side benefit worth calling out in the description: switching to Logic itself looks right to me, good to go from my side. |
…obOptimisticDataStore The read-time ETag captured by GetData is stored per block name and consumed by the next TryOptimisticWrite. That requires the read->write cycle for a given block to stay serialized per instance (as UniqueIdGenerator does via its per-scope lock). Add a class-level XML doc comment noting the invariant and the silent duplicate-id fallback that occurs if a shared instance interleaves concurrent read+write for the same block, so future direct users of the class stay within it. Doc-only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
added the comment |
2 participants
Problem
BlobOptimisticDataStore.TryOptimisticWriteconditions its upload on the blob's currentETag, fetched via
GetProperties()at write time — not the ETag of the value the precedingGetDatareturned. A competing writer that commits between the read and the write goesundetected: its commit changes the ETag, the late writer fetches that new ETag, and the
If-Match passes. Both writers succeed and both processes hand out the same id range.
GetData→ "500"GetData→ "500"GetProperties→ E2; writes "501" (If-Match: E2) → OK; also dispenses 500We hit this in production-like testing: 100 parallel entity creates against a two-instance
deployment produced duplicate ids (used as primary keys). This appears to have regressed in
the Azure.Storage.Blobs v12 migration — the legacy
CloudBlockBlobcachedProperties.ETagfrom the preceding read, so the old
AccessCondition.GenerateIfMatchCondition(...)comparedagainst the read-time state;
BlockBlobClientdoesn't cache, andGetProperties()became alive call. Related earlier attempt: #25.
Fix
GetData(Async)usesDownloadContent(Async), which returns the content and its ETag froma single response, and records the read-time ETag per block name.
TryOptimisticWrite(Async)consumes that ETag for the If-Match condition, making theread→write cycle a correct compare-and-swap: an intervening commit fails the write with 412
and
UniqueIdGeneratorre-reads and retries.current-ETag behaviour.
Test
New Azurite integration test (
ShouldNotGenerateDuplicateIdsAcrossGeneratorsUnderContention):four independent generators allocate 100 ids each from one scope with
BatchSize = 1and all400 must be distinct. This reproduces duplicates on the previous implementation.