ChangeFeedProcessor: Fixes lease de-duplication for /partitionKey-partitioned lease containers

[kirankumarkolli]

Research report (gist): https://gist.github.com/kirankumarkolli/8bf1cbb9c3ee45dfa64511c4a0cb35ad

Problem statement

In V3 SDK 3.20.0+, when the lease container is partitioned on /partitionKey (used for Gremlin/Graph API accounts and any customer-chosen layout), DocumentServiceLeaseManagerCosmos.CreateLeaseIfNotExistAsync sets the new lease's partition-key value to Guid.NewGuid().ToString(). Because Cosmos DB's id uniqueness is scoped per logical partition, two racing hosts that compute the same deterministic lease id land in different logical partitions and both succeed with 201 Created instead of the 409 Conflict the dedup logic relies on.

Root cause: V3 PR #2491 (2021-05-27) and its V2 predecessor PR #158 silently violated the original V2 PR #105 (2018) invariant that "lease collection PK must be a function of monitored collection PK Range. We already have that function as lease id."

Fix (minimal, env-gated with safe default)

New env var

Variable	Default	Purpose
AZURE_COSMOS_CHANGE_FEED_LEASE_ID_AS_PARTITION_KEY_ENABLED	true	When true, lease docs in a /partitionKey-partitioned lease container use the deterministic lease id as the partition-key value (restores the 409-based dedup invariant). When false, falls back to legacy Guid.NewGuid() behavior.

Code changes

ConfigurationManager.cs — New env-var constant and accessor IsChangeFeedLeaseIdAsPartitionKeyEnabled(), mirroring the established AZURE_COSMOS_*_ENABLED pattern.

DocumentServiceLeaseManagerCosmos.cs — New useDeterministicPartitionKey field snapshotted in the constructor (ensures consistency for the manager's lifetime). New GetLeasePartitionKeyValue(leaseId) helper used at both CreateLeaseIfNotExistAsync call sites, replacing the Guid.NewGuid().ToString() calls.

Why this is the right fix

Property	Status after fix
PK path	Unchanged (/partitionKey) — Gremlin still works
Public API surface	Unchanged
Default behavior	New deterministic PK (fix is on by default)
Operational escape hatch	AZURE_COSMOS_CHANGE_FEED_LEASE_ID_AS_PARTITION_KEY_ENABLED=false reverts per-process
Dedup invariant (PR #105)	Restored for all three lease-container layouts
Back-compat read path	Unchanged — TryGetLeaseAsync uses stored PK value

Not required (explicitly)

• No schema migration — old GUID-PK leases coexist with new id-PK leases; both read/update paths route through the stored PartitionKey field.
• No change to PartitionedByPartitionKeyCollectionRequestOptionsFactory (already routes by stored PK value).

Risks and mitigations

#	Risk	Likelihood	Mitigation
1	Existing customers carry a mix of GUID-PK (old) and id-PK (new) leases	Certain	Safe by design — TryGetLeaseAsync reads stored lease.PartitionKey; old leases remain readable/updatable
2	Duplicate leases already written under old code persist	Certain	Out of scope — not self-clearing; manual cleanup needed
3	Mixed-fleet window during rolling upgrade	Certain during rollout	Dedup fully restored only after all hosts upgraded. Not a new failure mode.
4	Hot-partition risk	Very low	Each lease has a different PK value (per-leaseToken), so distribution is similar to /id layout

Upgrade semantics — mixed-fleet behavior

During rolling upgrade (old + new SDK hosts coexist):

• Read/update/delete/checkpoint of existing leases: works on both sides (routes through stored lease.PartitionKey).
• Concurrent create of the same missing lease: still produces duplicates if one host is old SDK (GUID PK) and another is new (id PK). Dedup is fully restored only after rollout completes.

Known limitation — pre-existing duplicates not auto-cleaned

Duplicate leases written by older SDK versions persist until the corresponding PKRange undergoes a split/merge. Manual cleanup (SELECT * FROM c WHERE c.id = @id + DELETE extras) is needed for existing duplicates.

Test coverage

Unit tests — DocumentServiceLeaseManagerCosmosTests.cs

Test	What it proves
CreateLeaseIfNotExistAsync_PartitionKeyBehavior (4 DataRows)	Parameterized across PKRange/EPK × default/opt-out: captures the PartitionKey sent in the create request and asserts deterministic (PK = lease.Id) or legacy (PK = GUID ≠ lease.Id) behavior
CreateLeaseIfNotExistAsync_DuplicatePkId_Returns409 (2 DataRows)	Simulates Cosmos DB's (PK, id) uniqueness constraint via ConcurrentDictionary — first create succeeds, second with same (PK, id) gets 409
AcquireCompletes_WithPreExistingGuidPartitionKey	Back-compat: lease with old random-GUID PK can still be acquired (stored PK preserved)
RenewCompletes_WithPreExistingGuidPartitionKey	Back-compat: lease with old random-GUID PK can still be renewed (stored PK preserved)
Existing CreatesEPKBasedLease / CreatesPartitionKeyBasedLease	Strengthened: added Assert.AreEqual(lease.Id, lease.PartitionKey) for /partitionKey factory

Infrastructure: [DoNotParallelize] + [TestInitialize]/[TestCleanup] for env var safety. Shared helpers eliminate duplication across overload types.

Emulator integration test — GremlinSmokeTests.cs

Test	What changed
Schema_DefaultsToHavingPartitionKey	Converted to [DataTestMethod] with two rows: (a) default env → asserts partitionKey == lease.id (dedup invariant), (b) env var false → asserts partitionKey is a GUID ≠ lease.id (legacy behavior)

Infrastructure: [DoNotParallelize] added (env var mutation is process-global).

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[NaluTripician]

@sdkReviewAgent

[kirankumarkolli]

@copilot follow issue agent fix instrucitons and implement it.

[Copilot]

Copilot wasn't able to review any files in this pull request.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

@copilot follow issue agent fix instrucitons and implement it.

Implemented the fix and tests per the issue-agent plan, including deterministic lease-id partition-key behavior with env-var opt-out and expanded coverage for both lease-creation overloads and both env-var branches. Latest addressing commit: 9bd4c8e (plus preceding implementation commits on this branch).

[xinlian12]

Plan Review ChangeFeed Lease De-duplication Fix

Overall assessment: The root-cause analysis is excellent and the proposed code fix is correct. The bug at lines 131 and 156 of DocumentServiceLeaseManagerCosmos.cs is real, well-researched, and the fix (using deterministic LeaseId as the partition key value) restores the V2 PR #105 invariant. All read/update/delete paths use the stored lease.PartitionKey, so old GUID-PK leases remain fully operational. The DocumentServiceLeaseStoreCosmos marker/lock documents already use this exact pattern the fix is restoring consistency, not inventing something new.

However, the test plan has significant design issues that should be addressed before implementation begins.

---

Finding 1: Proposed emulator test will not trigger the duplicate lease bug

The proposed emulator test starts two ChangeFeedProcessor instances concurrently via Task.WhenAll(p1.StartAsync(), p2.StartAsync()) and expects them to race on CreateLeaseIfNotExistAsync. This will not work because BootstrapperCore.InitializeAsync() serializes lease creation via a lock document:

Host A: AcquireInitializationLockAsync()  201 (lock acquired)
Host A: CreateMissingLeasesAsync()  creates all leases
Host A: MarkInitializedAsync() + ReleaseInitializationLockAsync()
Host B: AcquireInitializationLockAsync()  409 (lock held)  sleeps 15s
Host B: IsInitializedAsync()  true  breaks without creating any leases

Only one host ever calls CreateLeaseIfNotExistAsync during bootstrap. The test will pass on both pre-fix and post-fix code, providing false confidence.

Suggested alternative: Test the invariant directly at the lease-manager level construct two DocumentServiceLeaseManagerCosmos instances pointing at the same real /partitionKey-partitioned lease container, call CreateLeaseIfNotExistAsync concurrently for the same PartitionKeyRange, then query the container and assert exactly one lease document exists.

---

Finding 2: No unit test proves the full 409 dedup chain with the new PK value

The proposed tests verify that the partition key value is deterministic and equals the lease ID. This is necessary but not sufficient. No proposed test mocks a second concurrent create returning 409 to verify the dedup chain end-to-end. The existing tests at lines 68154 only exercise the happy path (200 OK).

Suggested addition: Add a unit test where the mock returns OK on the first CreateItemStreamAsync and 409 on the second call for the same lease token. Assert the first returns a lease and the second returns null.

---

Finding 3: Missing back-compat test for pre-existing GUID-PK leases

The plan claims old leases with GUID partition keys "remain readable/updatable" because all downstream operations use the stored lease.PartitionKey. This is a critical upgrade-safety claim with no proposed test coverage.

Suggested addition: A unit test that creates a lease with a known GUID PK value (simulating a pre-fix lease), then verifies TryGetLeaseAsync, RenewAsync, and AcquireAsync still route correctly through the stored GUID value.

---

Finding 4: Strengthen existing GremlinSmokeTests as a cheap regression guard

GremlinSmokeTests.Schema_DefaultsToHavingPartitionKey (line 98) already exercises a /partitionKey-partitioned lease container but only asserts Assert.IsNotNull(lease.partitionKey). After the fix, add Assert.AreEqual((string)lease.id, (string)lease.partitionKey) a one-line change that makes the existing emulator test validate the dedup invariant. This is more reliable than the concurrent-startup test.

---

Finding 5: Emulator test comment about instanceName is incorrect

The plan says .WithInstanceName("host-A") // same host name forces same deterministic lease id. This is wrong instanceName maps to options.HostName which determines lease ownership, not the lease ID. Lease ID = prefix + leaseToken where prefix includes processorName, cosmosHost, dbRid, containerRid not instanceName.

---

Observation: Env-var toggle for a correctness fix

The env var preserves the ability to re-enable known-broken behavior. The mixed-fleet window during rollout is unavoidable regardless (old/new hosts still race with different PK strategies). The env var only helps if reverting all hosts, which means reverting to the bug. The complexity cost is modest but perpetual the broken code path must be maintained and tested indefinitely. Worth deciding explicitly before implementation.

---

AI-generated review may be incorrect. Agree? resolve the conversation. Disagree? reply with your reasoning.

[xinlian12]

✅ Review complete (33:23)

No new comments — existing review coverage is sufficient.

_{Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage}

[NaluTripician]

Minor readability/refactor nit on the two CreateLeaseIfNotExistAsync overloads: the PK-value selection block is duplicated verbatim. Since both sites must stay in lockstep to preserve the dedup invariant, consider extracting a small helper so they can't drift:

private string GetLeasePartitionKeyValue(string leaseId)
{
    return this.useDeterministicPartitionKey
        ? leaseId
        : Guid.NewGuid().ToString();
}

Each call site then collapses to:

this.requestOptionsFactory.AddPartitionKeyIfNeeded(
    (string pk) => documentServiceLease.LeasePartitionKey = pk,
    this.GetLeasePartitionKeyValue(documentServiceLease.LeaseId));

Keeps the snapshotted-flag guarantee intact and gives a single source of truth for the "PK value = lease id when deterministic" invariant.

Optional bigger refactor (probably out of scope for this PR): push the policy onto RequestOptionsFactory itself as a virtual GetCreatePartitionKeyValue(string leaseId), defaulting to Guid.NewGuid().ToString() and overridden by PartitionedByPartitionKeyCollectionRequestOptionsFactory to return leaseId. That would localize the per-layout behavior where it belongs and remove the env-var branch from the lease manager.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[kirankumarkolli]

@sdkReviewAgent

[xinlian12]

✅ Review complete (31:09)

Posted 3 inline comment(s).

_{Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage}

[kushagraThapar]

Overall looks good, requesting changes on missing changelog entry.
I am also curious, is there any way to fix the existing leases for existing customers? Maybe some guidance?

[kirankumarkolli]

Overall looks good, requesting changes on missing changelog entry. I am also curious, is there any way to fix the existing leases for existing customers? Maybe some guidance?

.NET changelog is included part of release not with PR.
No real need to update existing entries and its still functional with no side-affect

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Minor readability/refactor nit on the two CreateLeaseIfNotExistAsync overloads: the PK-value selection block is duplicated verbatim. Since both sites must stay in lockstep to preserve the dedup inva...

Done — the two CreateLeaseIfNotExistAsync overloads now share a single GetLeasePartitionKeyValue helper so the PK-selection invariant has one source of truth. This is in commit cb93af1 (still present in the branch).

[kirankumarkolli]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

I resolved the merge conflict locally and finalized it in commit 5619f7d (conflict in ConfigurationManager.cs resolved by keeping both upstream HTTP/2 env vars and the change-feed env var). Push back to this branch is currently blocked by repo rules (GH013: Cannot update this protected ref), so I can’t publish the merge commit from this environment.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[xinlian12]

LGTM, thanks