[Cosmos][Spec] Restore PATCH (Partial Document Update) for Rust SDK#4338
Conversation
This commit adds a design spec for restoring the Cosmos PATCH API removed in PR Azure#3765. The spec proposes a phase-1 design that exposes only the unconditionally-idempotent subset of operations (set / replace / remove) and explicitly disallows add / increment / move at the SDK boundary so that the wire request is always safe under the auto-retry-all-writes default. A phase-2 follow-up will revisit non-idempotent operations via Read-Modify-Write. The spec is informed by a full Landscape Analysis covering call graph, retry semantics (including PPAF/PPCB), driver/SDK split state, and cross-SDK behavioral parity with .NET. Six side effects were classified; the central one (idempotency interaction with auto-retry) is mitigated by narrowing the public PatchOperation enum to the idempotent subset. This is a spec-only change. No source code changes are made. Refs: Azure#4048 Refs: Azure#3765 (the removal we are reversing) Refs: Azure#4327 (Feed Operations Spec — Planner alignment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician
requested review from
LarryOsterman,
RickWinter,
heaths and
ronniegeraghty
as code owners
…st, AC/test gaps) Addresses PR Deep Reviewer iteration 1 findings: - 1/2/3: Rewrite section 4.4 idempotency soundness. The driver retry surface has at least four pathways in retry_evaluation.rs and only two consult is_idempotent(); try_handle_server_error (5xx, line 457) and try_handle_write_forbidden (403/3, line 289) retry cross-region unconditionally, and PPAF was designed for non-idempotent retries. Soundness now rests on document-level wire-body idempotency, not on a // SAFETY: comment over a single retry gate. - 4/5: Section 4.2 PatchItemOptions concrete shape and ItemWriteOptions applicability table. - 6: Remove-op unit test now requires byte-exact assertion. - 7: Section 4.5 corrects PR Azure#4128 precedent characterization (added zero new variants; precedent is leave-legacy-alone, not only-add-to-driver). - 8: Adds 4 acceptance criteria + integration tests (etag x predicate conjunction, soft-delete-of-soft-deleted, partition split, PPCB). - 9: Section 11.2 open question rewritten for FabianMeiswinkel. - Polish 10/11/12: rename_all serde note, README example with filter_predicate + content_response_on_write=false, phase-2 constraints subsection in Out of Scope. - Section 7.2 Alternative B rejection extended to mention the 5xx and 403/3 retry paths that bypass is_idempotent(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a Cosmos PATCH design spec for the Rust SDK and documents a proposed phase-1 restore of PATCH using a restricted operation set intended to fit the current driver/retry architecture. It also adds a repo-level ignore for local spec-generation artifacts referenced during the design process.
Changes:
- Add
PATCH_OPERATIONS_SPEC.mddescribing the proposed PATCH API surface, retry/idempotency model, driver integration, risks, and test plan. - Outline the future implementation work across SDK, driver, tests, README, and changelog files.
- Ignore local
.coding-harness/artifacts in the repository root.gitignore.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
sdk/cosmos/azure_data_cosmos_driver/docs/PATCH_OPERATIONS_SPEC.md |
New PATCH restoration spec covering requirements, API shape, driver wiring, tests, and rollout plan. |
.gitignore |
Ignores local .coding-harness/ spec-generation artifacts. |
| - Operation `op` tokens are: `set`, `replace`, `remove` (lowercase). | ||
| - The serde representation is preserved from the implementation removed in | ||
| PR #3765, recovered at `.coding-harness/removed_patch_operations.rs.diff`. |
| - **M9.** Per-call options support `if_match_etag` (existing | ||
| `ItemWriteOptions` pattern) so customers can build classic |
| ### 2.2 Should | ||
|
|
||
| - **S1.** The public Rust name for the wire-level `condition` field is | ||
| `filter_predicate` on the options struct (matching .NET's customer-facing |
| pub fn with_remove<P>(self, path: P) -> Self | ||
| where P: Into<Cow<'static, str>> { ... } | ||
| pub fn with_filter_predicate<C>(self, condition: C) -> Self | ||
| where C: Into<Cow<'static, str>> { ... } | ||
| } |
| conversion on the user's value can fail without panicking. (This matches the | ||
| prior API's design intent.) | ||
| - The trait `ToJsonNumber` from the prior code is **not** restored — it was | ||
| only used by `with_increment`, which is deferred. |
|
|
||
| A reviewer can confirm "done" by checking: | ||
|
|
| listed in M6. | ||
| 2. Add a `CosmosOperation::patch_item(reference, patch_document) -> CosmosOperation` | ||
| factory in `azure_data_cosmos_driver::models::cosmos_operation` following the | ||
| existing `replace_item` factory pattern (line ~441). |
Addresses PR Deep Reviewer iteration 2 findings:
- N1: AC12 / section 6.3 conjunction test now enumerates all four
etag x predicate combinations including (no-match, no-match), and
notes the server-side eval-order tie-break is observable but not pinned.
- N2 + N3: Remove-op byte-exact assertion now includes the
{"operations":[...]} wrapping array, which also pins the
"operations" top-level wire field name. M5 invariants list extended
to call out "operations" as plural lowercase.
- N4: AC13 / soft-delete-idempotent test reworded to be honest
about what it tests (application-level repeat safety, not
transparent retry). Real retry-pathway property tracked as a
fault-injection follow-up under M8.
- N5: Section 10 phase-2 constraint 2 "major-version bump" replaced
with concrete language about driver-boundary observable retry
behavior; driver crate has no formal semver contract.
- N6: PatchItemOptions::item_options changed from pub to pub(crate)
with rationale comment so phase 2 can add patch-specific invariants
without a breaking change. Mandates pass-through builders for every
applicability-table field.
- N7: Header-block reference to .coding-harness/landscape-report.md
notes the PR description (Azure#4338) is the durable copy.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Captures discussion raised by @FabianMeiswinkel on the time-savings of phase-1 op-narrowing vs. starting with RMW, and the alternative of requiring If-Match etag on every patch instead. - Section 7.5 (Alternative E): mandatory-etag for phase-1 — explicitly rejected for phase 1 because it regresses the canonical `set /deleted = true` soft-delete workload from one RU to two (extra read for the etag), and because 412 on transparent retry is ambiguous between duplicate-success and real conflict — the driver cannot disambiguate without a re-read, which is itself the RMW alternative we rejected. - Section 10 phase-2 options: add option (d) mandatory-etag-for- non-idempotent-ops. Trade-offs differ from phase-1: most counter/move workloads already have an etag from a prior read, so the extra-read cost vanishes, and the caller is in a better position to disambiguate 412 than the SDK. - Section 11 question 5: explicit open question to FabianMeiswinkel on RMW vs. mandatory-etag preference for phase 2. The key insight: the right idempotency answer differs across the op set, which itself justifies phase-1 narrowing rather than a one-size-fits-all retry policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@FabianMeiswinkel — picking up your question on whether phase-1 actually saves time vs. starting with RMW, and your suggestion of disallowing conditions initially. Short version: phase-1 saves the RMW loop subsystem specifically, not the whole spec; and disallowing the What phase-1 actually defers vs. what it doesn'tThe bulk of the work —
That's the work phase-1 saves. It's real, but it's also the easiest part to design wrong without a spec, which is why I'd argue it's exactly the right thing to defer. "Disallow conditions initially" — two readingsReading 1 — disallow Reading 2 — require This is captured in spec §7.5 as Alternative E. For phase 1 I rejected it for two reasons:
For phase 2 the trade-offs invert and I think mandatory-etag becomes genuinely attractive — added as option (d) in §10 alongside the existing RMW / Bottom lineTime savings of phase-1 over starting-with-RMW: real, on the order of "the entire RMW loop subsystem and its retry/observability story", but not "the whole spec". Mandatory-etag-as-phase-1-alternative-to-narrowing: rejected, see §7.5. Mandatory-etag-as-phase-2-alternative-to-RMW: live and now spec'd as option (d) in §10. Open question §11.5 is explicitly directed to you on whether you'd prefer it over RMW for phase 2. Happy to switch §10's framing to recommend (d) if that matches your gut, or to keep the spec position-neutral and decide when phase-2 is scoped. Either is fine by me. |
Summary
Adds a design spec for restoring the Cosmos
PATCH(Partial Document Update) API to the Rust SDK. The previous public surface was removed in PR #3765 because the original design did not safely interact with the Rust SDK's auto-retry-all-writes default — a transparent retry of aPATCHcontaining anincrementoraddoperation could silently double-apply the change. This PR proposes a phase-1 design that ships PATCH only for the unconditionally-idempotent subset (set/replace/remove) and disallowsadd/increment/moveat the SDK boundary, with a phase-2 follow-up reserved for non-idempotent operations via Read-Modify-Write.This PR contains the spec only — no source code changes. The spec lives at
sdk/cosmos/azure_data_cosmos_driver/docs/PATCH_OPERATIONS_SPEC.md, mirroring the location and format of the Feed Operations spec (PR #4327) that PATCH must align with.Issue
Implements #4048.
Related: #3765 (the removal we are reversing); #4327 (Feed Operations Spec — the planner model PATCH must align with); #4156 (PPAF/PPCB landed the second write-retry pathway PATCH has to coexist with).
Root Cause / Background
Cosmos PATCH is conditionally idempotent. A patch body containing only
set/replace/removeoperations is safe to apply twice — applying the same body again yields the same final document. A patch body containingadd/increment(incron the wire) /moveis not idempotent: a silent retry after the server has already applied the patch will double-apply the operation.The Rust SDK retries all writes by default. There are at least four retry pathways in
retry_evaluation.rsthat interact with PATCH — and only two of them gate onis_idempotent():retry_evaluation.rslines 406–408 — a three-way OR (is_read_only() || is_idempotent() || ppaf_write_retry_allowed).retry_evaluation.rslines 523–525 — same three-way OR.try_handle_server_erroratretry_evaluation.rs:457(5xx and 408 responses) — fires unconditionally; nois_idempotent()check at all. The reasoning encoded there is "an internal error in one region is unlikely to repeat in another," so the request is replayed cross-region.try_handle_write_forbiddenatretry_evaluation.rs:289(403 sub-status 3) — fires unconditionally; nois_idempotent()check at all.A correct phase-1 design therefore cannot rely on flipping
is_idempotent()to gate retry — the request body itself must be safe to replay under (3) and (4). PPAF (ppaf_write_retry_allowed) was introduced specifically to allow non-idempotent write retries cross-region, per the doc comment atpipeline/components.rs:92— so reading the OR as "PPAF is gated by is_idempotent" inverts its design.PR #3765 removed the public PATCH API rather than ship a design that handled both pathways correctly. The driver-side plumbing (
Method::Patchin auth signing,FaultOperationType::PatchItem, the SDK-sideOperationType::Patchlegacy variant) was deliberately retained — there is even an explicit// PatchItem will be mapped when OperationType::Patch is added to the driver.TODO atazure_data_cosmos_driver/src/fault_injection/mod.rs:191— because the team understood PATCH would return.This spec settles the design.
Fix / Implementation
The proposed phase-1 design makes
OperationType::Patch::is_idempotent()returntrueunconditionally, and makes that claim sound by narrowing the public API:PatchOperationis a#[non_exhaustive]enum exposing onlySet/Replace/Remove, with noDeserializeimpl. Customers cannot construct a non-idempotent patch because no public symbol exists by which they could expressAdd,Increment, orMove. The load-bearing property is document-level wire-body idempotency — everyPatchDocumentvalue that can ever be constructed produces a wire body that is safe to replay, which is the property required by all four retry pathways above (including the two that don't consultis_idempotent()at all).The maintainer guardrail is an in-crate exhaustive
matchtest overPatchOperation(no_arm).#[non_exhaustive]does not suppress exhaustiveness checks inside the defining crate, so adding a new variant fails to compile that test, forcing a phase-2 redesign before any non-idempotent op can ship.The full design — type model, options struct shape, driver wiring (
OperationType::Patchvariant,CosmosOperation::patch_itemfactory, fault-injection mapping), test plan, and rollout — is in the spec document.Alternatives considered (full reasoning in §7 of the spec):
#[non_exhaustive]enum keeps the door open.is_idempotent()downgrade. Rejected because it loses the auto-retry safety net for the very workloads (counters, telemetry rollups) that benefit most, AND because it would only gate two of the four+ retry pathways —try_handle_server_error(5xx) andtry_handle_write_forbidden(403/3) retry cross-region without consultingis_idempotent(). Defeating those is a cross-cutting retry-policy redesign, not a one-line change. Also requires changingOperationTypefromCopyto per-instance state.unsafe_allow_non_idempotent_patchopt-in flag. Rejected: anunsafe_*flag is a smell that the underlying primitive isn't designed yet. No motivating use case (OpenAI's primary PATCH use issetfor soft-delete, which phase 1 covers).Problem Space Analysis
Landscape confidence: HIGH (full report in
.coding-harness/landscape-report.md— note: this file is gitignored as a session artifact; full text is included as a collapsed section below)Blast Radius
The full call graph and cross-cutting concern sweep is in §1 and §3 of the landscape report. Highlights:
Confirmed unaffected:
Method::Patchis already mapped in bothauthorization_policy.rs:131and SDKsignature_target.rs:57.replace_item.Interacts with this change:
retry_evaluation.rs—is_idempotent()gate at 406–408 / 523–525,try_handle_server_errorat 457,try_handle_write_forbiddenat 289) — phase-1 wire body is document-level idempotent so all four pathways are safe regardless of which one fires. (SE-001)ppaf_write_retry_allowed) — designed for non-idempotent writes; same wire-body argument applies. (SE-001)OperationTypeenums — driver-canonical enum needs thePatchvariant; legacy SDK enum already has it. (SE-002)incr/ separateset/add(NOT RFC 6902); wire field iscondition, notfilterPredicate. (SE-004, SE-005)Decision Log
add/increment/moveat the SDK boundaryis_idempotent()downgrade; (C)unsafe_*flag; (D) restore unchangedset-for-soft-delete is the largest known use case), keepsis_idempotent() == truesound (the type system enforces it), and preserves the phase-2 path via#[non_exhaustive].OperationType::Patchadded to the driver enum (canonical)conditionisfilter_predicateconditiondirectlycondition.Full decision reasoning is in §4 and §7 of the spec document.
[SE-001] 🟡 Idempotency × auto-retry-all-writes interaction
Affected component:
azure_data_cosmos_driver::driver::pipeline::retry_evaluation+OperationType::is_idempotentConditions: A naïve PATCH design that allowed
add/increment/moveand returnedtruefromis_idempotent()would silently double-apply on retry. There are at least four retry pathways inretry_evaluation.rs(the standardis_idempotent()gate at 406–408 / 523–525, PPAF non-idempotent retry,try_handle_server_error5xx at 457,try_handle_write_forbidden403/3 at 289) — three of them retry without consultingis_idempotent(). The wire body itself must be safe to replay.Assessment: Potential, not breaking — phase 1 ships an additive API and we choose the design to avoid the failure mode.
Mitigation: Public
PatchOperationenum exposes onlySet/Replace/Remove(noDeserializeimpl). Every constructiblePatchDocumentproduces a wire body that is unconditionally idempotent at the document level — the property required by every retry pathway. Maintainer guardrail: an exhaustivematchtest in the defining crate fails to compile when a newPatchOperationvariant is added (#[non_exhaustive]does not suppress exhaustiveness checks inside the defining crate), forcing a phase-2 redesign before any non-idempotent op can ship. Phase-2 RMW for non-idempotent ops is explicit follow-up work.[SE-002] 🟡 Two
OperationTypeenums diverge on Patch supportAffected component:
azure_data_cosmos::operation_context::OperationType(legacy) vsazure_data_cosmos_driver::models::OperationType(canonical)Conditions: SDK-side enum has the
Patchvariant; driver-side does not. Implementer reading only one enum may miss the other.Assessment: Potential — consistent with the in-progress driver cutover but a known correctness hazard.
Mitigation: Spec adds the variant to the driver enum and leaves the legacy enum's existing variant unchanged. Adding a variant to the driver enum is a new decision (PR #4128 added zero new variants); the cited precedent only covers the "leave the legacy enum alone" half. Full reconciliation is tracked as a separate follow-up.
[SE-003] 🟢 Driver fault-injection PatchItem mapping is a known TODO
Affected component:
azure_data_cosmos_driver::fault_injectionConditions: Customer using
FaultOperationType::PatchItemexpects fault rules to matchpatch_itemcalls.Assessment: Minor — the TODO at
fault_injection/mod.rs:191was placed for exactly this work item.Mitigation: Resolved in the same change (M8 / AC9). TODO comment removed in the same commit.
[SE-004] 🟡 Wire op tokens are NOT RFC 6902
Affected component:
PatchOperationserde representationConditions: Implementer copying RFC 6902 conventions removes the
#[serde(rename)]attributes and emits the wrong wire tokens.Assessment: Potential — caught only by integration tests against the service.
Mitigation: Spec §4.1 references the prior implementation (preserved at
.coding-harness/removed_patch_operations.rs.diff) as the source of truth. A unit test asserts byte-exact wire body including op tokens.[SE-005] 🟢 Wire field is
condition, notfilterPredicateAffected component:
PatchDocumentserde representationConditions: Implementer takes .NET's
FilterPredicatepublic name as the wire name.Assessment: Minor.
Mitigation: Public Rust name is
filter_predicate;#[serde(rename = "condition")]annotates the wire form. Unit test asserts the wire field name explicitly.[SE-006] 🟢 Transactional batch PatchItem is unsupported
Affected component:
azure_data_cosmos::transactional_batchConditions: Customer expects feature parity with .NET's
TransactionalBatchPatchItemRequestOptions.Assessment: Minor — explicit feature gap.
Risk accepted: Out of scope for this work item; a follow-up issue will be filed.
What Reviewers Should Focus On
add/increment/moveat the SDK boundary acceptable, or should phase 1 ship the RMW loop instead? Ashley's task-description guidance and OpenAI's primaryset-for-soft-delete use case point to disallow; @analogrelay should confirm.is_idempotent() == true(spec §4.4). The argument is "the type system makes the wire request unconditionally idempotent at the document level; therefore every retry pathway inretry_evaluation.rsis safe — including the three (PPAF,try_handle_server_error5xx,try_handle_write_forbidden403/3) that retry without consultingis_idempotent()at all." @FabianMeiswinkel should confirm there are no phase-1 retry pathways where document-level wire-body idempotency is insufficient.OperationType-enums posture (SE-002). We are deliberately leaving them divergent in this PR and tracking reconciliation separately. Confirm this matches the rolling-cutover plan rather than fighting it.Lower-risk areas (fully traced, safe to skim):
Method::Patchalready wired correctly.ContainerClient::patch_itemshape: identical toreplace_item.Testing
Integration tests not included: spec PR; will be added in the implementation PR per the work-item directive that this PR be spec-only.
Risk Assessment
Cross-SDK Impact
FilterPredicate) and wire tokensLandscape Report
The full landscape report (
landscape-report.md) and machine-readable side-effect catalog (landscape.json) are session artifacts under.coding-harness/(gitignored). Key findings inlined in the Problem Space Analysis and Side Effects sections above. Happy to paste the full report into a PR comment if reviewers prefer.