[Hangyi (HangyiWang)]

Change

Protect az iot adr ns link hub|dps|su remove against concurrent namespace updates. Capture the ETag from the GET response header and send it as If-Match on the replacement PUT. Missing or unusable ETags stop removal before RBAC changes or writes. A conflict returns the original HTTP 412 without retrying; rerun the command to re-read and revalidate the namespace. Existing no-wait behavior is unchanged.

Evidence

• Australia East, 2026-11-02-preview: matching ETag accepted, stale ETag rejected with HTTP 412, concurrent changes preserved. Disposable probe resources were deleted.
• 3,148 ADR unit tests passed; real-SDK HTTP regressions cover all three endpoint kinds, header casing, missing ETags, conflicts, and a successful explicit rerun.
• Flake8 and pylint passed. Integration suites were not run.

Same protection is being applied to #906; GA deployment has not been live-validated.

[Copilot]

🟢 Approval recommended

The implementation safely prevents stale writes and includes comprehensive regression coverage.

0 open findings

What changed in this PR

Protects ADR namespace endpoint removal from overwriting concurrent updates using ETag-based conditional PUTs.

Changes:

• Captures and validates namespace response ETags.
• Sends ETags through If-Match and preserves HTTP 412 conflicts.
• Adds regression coverage, help guidance, and release notes.

File	Description
HISTORY.rst	Documents concurrency protection.
azext_iot/​adr/​providers/​link.py	Implements ETag-conditional endpoint removal.
azext_iot/​adr/​_help.py	Explains conflict and rerun behavior.
azext_iot/​tests/​adr/​test_adr_link_delete_unit.py	Covers ETags, conflicts, and reruns.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ZZ (zzcodework)]

Azure SRE Agent - automated review

Reviewing full diff (head a1458be4, base release/1.1.0-preview, 4 files / +106 -13)

Mechanics check out. I verified the two non-obvious plumbing assumptions against the vendored SDK at this head (azext_iot/sdk/deviceregistry/operations/_operations.py, NamespacesOperations):

• get pops cls and calls cls(pipeline_response, deserialized, {}), so lambda response, resource, _: (resource, response.http_response.headers.get("ETag")) really does receive the pipeline response and can read the header (case-insensitively, via the transport's header mapping).
• begin_create_or_replace does _headers = case_insensitive_dict(kwargs.pop("headers", {}) or {}) and forwards headers=_headers into _create_or_replace_initial, which passes it to build_namespaces_create_or_replace_request. So If-Match genuinely reaches the wire — this is the right workaround given the generated client exposes no etag/match_condition parameter.
• The other self.client.namespaces.get(...) call site in the same file (the read in _wait-adjacent code, ~line 105) is unaffected because cls is per-call.

Blocking

1. azext_iot/adr/providers/link.py (the new if not isinstance(etag, str) ...: raise AzureResponseError(...) guard) — this makes az iot adr ns link hub|dps|su remove unconditionally fail if the DeviceRegistry RP does not emit an ETag response header on namespace GET, and there is no fallback: I checked the generated namespace schema in NamespacesOperations and it declares no etag body property at all (0 occurrences across the whole class span), which is consistent with test_delete_real_sdk_requires_header_even_when_body_has_etag deliberately rejecting a body-level etag. So the header is the single point of failure for the entire remove surface.

   Nothing in the PR demonstrates the live RP sends it. All new coverage is either Mock()-based or responses-based, i.e. the test supplies the header itself; those tests pass identically whether or not ARM behaves this way. The only evidence that settles it is the ADR integration suite — azext_iot/tests/adr/test_adr_link_delete_int.py::TestADRLinkDelete::test_adr_link_hub_dps_delete and ::test_adr_link_su_delete both drive iot adr ns link {kind} remove against the real service — and that suite is not part of the PR checks (the 21 completed checks here are unit/lint/build only). Please link an ADR int run on a1458be4 showing those two tests green before merging.

Suggestions

2. azext_iot/adr/providers/link.py — the ETag is captured before self._rbac_manager().ensure_unlink_reader(...), which is the slowest step in the method (it can create a role assignment and absorb propagation delay), and the PUT then asserts a precondition that has been held across that entire window. Any namespace write in between — including service-driven updates to linkingState/provisioningState on a sibling endpoint — invalidates it, and with retry_total=0 and no automatic retry the user just gets a 412 and has to rerun. Re-reading the namespace immediately before building the PUT body (preflight does not depend on the ETag; only resolve_namespace_outbound_principal(namespace) and the endpoint lookup do, and both can be recomputed) would shrink the race window to the body construction itself at no correctness cost. Your own test_delete_real_sdk_preserves_concurrent_changes_and_can_be_rerun models exactly this window by mutating inside ensure_unlink_reader.

3. azext_iot/adr/providers/link.py — the 412 surfaces as a bare HttpResponseError from the generated client (_create_or_replace_initial's error_map maps 401/404/409/304 only, so 412 falls through to HttpResponseError(response=..., error_format=ARMErrorFormat)). The remedy — "inspect the namespace and rerun" — now lives only in _help.py, which the user does not see at failure time. Catching 412 and re-raising with that one-line remedy would make the new failure mode self-explanatory.

Nits

4. The guard rejects "*" after strip(), which is good, but etag is then sent unstripped (headers={"If-Match": etag}). For a value like ' "v1"' the validation and the wire value disagree. Sending etag.strip() would make them consistent.

Checked and clean (recording the negatives so they do not get re-derived):

• AzureResponseError is already imported in link.py (from azure.cli.core.azclierror) — no import change needed.
• HISTORY.rst has a matching entry under ADR bug fixes, and _help.py's remove text was updated to describe 412 instead of the old "avoid concurrent namespace updates" advice. Both are the conventions this repo enforces.
• Unit-test migration is complete: every client.namespaces.get.return_value in test_adr_link_delete_unit.py was moved to the (resource, ETAG) tuple shape, the assert set(arguments) == {...} kwarg-set assertion was widened to include headers, and the real-SDK tests now assert If-Match on network.calls[1]. The etag parametrization [None, "", " ", "*", 7] covers the whole guard including the non-str branch.
• polling=False / retry_total=0 semantics are unchanged, so the "NoPolling returns the initial PUT response" contract and the no-retry-on-backend-error tests still hold.

Automated review — may be incomplete; please treat findings as input rather than as a gate.

[Copilot]

🟡 Changes recommended

ETag validation remains incomplete, and failed conditional writes can leave persistent RBAC grants.

2 open findings

• Clean up Reader assignments after failed precondition · New
• Reject weak or malformed ETags before RBAC changes · New

🧠 Review effort: Balanced

[ZZ (zzcodework)]

Azure SRE Agent - automated review

Reviewing changes since a1458be4 (head 6ecf05cb, base release/1.1.0-preview)

The increment is a single commit — 6ecf05cb Merge release/1.1.0-preview into fix/adr-namespace-unlink-etag. The PR's product change is unchanged: azext_iot/adr/providers/link.py and azext_iot/adr/_help.py are byte-identical to a1458be4, and the diff against the new base is still the same 4 files. So this review is scoped to the merge resolution, which did touch two of the PR's own files.

Blocking

None.

Suggestions

None new.

Nits

None.

---

Checked and clean (the merge resolution, verified end to end):

1. HISTORY.rst — the entry moved to the right heading. The base branch added a new 1.1.0b1 (Preview) section (**Wait command bug fixes**). The resolution moved this PR's bullet — "Namespace link removal uses the namespace response ETag…" — out of 1.0.0b2 (Preview) and under 1.1.0b1, leaving the two pre-existing 1.0.0b2 ADR bullets (--validity-days, link add/update exception cause) in place. That matches azext_iot/constants.py at head, which now declares VERSION = "1.1.0b1", so the unreleased entry sits under the version it will ship in. Nothing from the base branch's own 1.1.0b1 section was dropped in the resolution.

2. azext_iot/tests/adr/test_adr_link_delete_unit.py — the import removal is correct, not a lost dependency. The resolution dropped from azext_iot.tests.adr.test_adr_validation_scenarios_unit import offline_cli # noqa: F401 and the two # noqa: F811 comments on test_delete_real_cli_returns_initial_response and test_delete_rejects_composite_and_wait_options. This is safe because the base branch now defines offline_cli as a fixture in azext_iot/tests/conftest.py, so pytest resolves it by conftest discovery for the whole azext_iot/tests tree — the cross-module re-import (and therefore the F811 redefinition warning it caused) is genuinely obsolete. test_adr_validation_scenarios_unit.py itself consumes offline_cli the same way at head, with no local definition.

3. No other PR content changed in the merge. All of this PR's new test cases survive intact at head: test_delete_missing_etag_stops_before_rbac_and_put (the [None, "", " ", "*", 7] parametrization), test_delete_real_sdk_requires_header_even_when_body_has_etag, test_delete_real_sdk_preserves_concurrent_changes_and_can_be_rerun, plus the assert network.calls[1].request.headers["If-Match"] == ETAG and assert put.call_args.kwargs["headers"] == {"If-Match": ETAG} assertions added to the pre-existing cases.

Carried over from the a1458be4 review, still open (restated in one line each, not re-argued — no code changed, so they stand as they were):

• The ETag is captured before ensure_unlink_reader, so the precondition is held across a step that can create a role assignment and absorb propagation delay; a re-GET immediately before building the PUT body costs nothing, since the preflight does not depend on the ETag.
• 412 is not in _create_or_replace_initial's error_map (401/404/409/304 only), so a precondition failure surfaces as a bare HttpResponseError; the "inspect the namespace and rerun" remedy currently lives only in _help.py.
• The validation strips (etag.strip()) but the PUT sends the unstripped etag.
• Unit tests cannot prove the RP actually emits the ETag response header, and the PR check set is unit/lint/build only — the ADR integration suite (azext_iot/tests/adr/test_adr_link_delete_int.py::test_adr_link_hub_dps_delete / ::test_adr_link_su_delete) on this exact head is the only evidence that the hard-fail path does not break the whole remove surface.

Note: Copilot posted two inline comments on this head a few minutes ago (weak-ETag acceptance of W/"v1", and the 412-after-RBAC-mutation window). I am deliberately not restating either; the weak-ETag point is new and not something my earlier review covered.

This is an automated review and may be incomplete.

[Copilot]

🟢 Approval recommended

The implementation matches the stated behavior and has thorough regression coverage.

0 open findings

2 resolved since last review

• Clean up Reader assignments after failed precondition
• Reject weak or malformed ETags before RBAC changes

🧠 Review effort: Balanced

[ZZ (zzcodework)]

Azure SRE Agent - automated review

Reviewing changes since 6ecf05cb (head 82282465, base release/1.1.0-preview)

One commit, 82282465 fix(adr): validate unlink ETags and explain conflicts — 4 files, +90/-14. It closes both of Copilot's inline comments from the previous head and two of the four findings I carried, so most of this is confirmation rather than new findings.

Closed by this commit (verified, nothing further needed)

• Weak/malformed ETags accepted (Copilot 4274409395, and my own "validate and send the same value" nit). azext_iot/adr/providers/link.py now normalizes with etag.strip(" \t") and then sends that same normalized value in headers={"If-Match": etag}, so the validate/send mismatch is gone. The gate is re.fullmatch(r'"[\x21\x23-\x7e\x80-\xff]*"', etag), which is the RFC 9110 entity-tag character set with \x22 correctly excluded from the inner range and the W/ prefix correctly rejected — right call, since If-Match is strong comparison only. The parametrized rejection list covers the cases that matter: W/"v1", w/"v1", unquoted v1, half-quoted "v1 / v1", embedded quote, control chars \x00/\x1f/\x7f, non-Latin-1 \u0100, and header-folding \n"v1" / "v1"\r / "v1"\r\n (correctly not stripped, since strip is " \t" only).
• 412 is not in the SDK error_map, so the remedy only lived in _help.py (my carried finding). The new except HttpResponseError / if error.status_code == 412 branch surfaces the remedy at failure time and raises the original exception unmodified. test_delete_conflict_warns_and_preserves_original_exception asserts raised.value is error and parametrizes stage over both begin_create_or_replace and .result(), which is the right split given polling=False — either can be the raiser. logger is defined at azext_iot/adr/providers/link.py:73, so the call is live.
• Residual Reader grant after a conflict (Copilot 4274409314). The warning now states it explicitly, which is the "at minimum" half of what that comment asked for. See Suggestion 2 for the half that is still missing.

Suggestions

1. The strong-quoted-ETag requirement has no evidence from the real RP, and the failure mode is total. azext_iot/adr/providers/link.py _remove_endpoint now raises AzureResponseError before RBAC and before the PUT unless the ETag response header matches a strong quoted entity-tag. That is strictly narrower than the previous head's not etag.strip() or etag.strip() == "*" check — an unquoted ETag (some ARM RPs emit bare hex/GUID values) passed before and hard-fails now. If the Device Registry RP does not quote, the entire az iot adr ns link hub|dps|su remove surface breaks with a message blaming the service. Nothing in the check set can catch that: the 24 checks on 82282465 are unit/lint/index-compatibility/build only, and every unit test supplies the header itself (Mock return tuple, or the responses mock in test_delete_real_sdk_*). The only real evidence is azext_iot/tests/adr/test_adr_link_delete_int.py::test_adr_link_hub_dps_delete / ::test_adr_link_su_delete. Please post an ADR integration run on this exact head, or capture one raw ETag header from a live namespace GET, before merge.

2. The conflict guidance names the residual grant but gives no way to act on it. The warning says "Any Reader grant created during preflight is retained," which is accurate, but a user who abandons the unlink has no command to undo it. One concrete az role assignment delete --assignee <principal> --scope <target rg> in the warning (or in the _help.py paragraph, which already has the surrounding context) would make it actionable. Copilot's comment asked for cleanup remediation specifically; this is the gap left.

3. Carried, still open: the precondition is captured before the slow step. The namespaces.get that reads the ETag still runs before self._rbac_manager().ensure_unlink_reader(...), which can create a role assignment and absorb propagation delay. Any write in that window — including service-driven linkingState/provisioningState updates — yields a 412 with retry_total=0 and no retry. A re-GET immediately before building the PUT body costs one request and nothing else: the preflight does not consume the ETag, and writable_namespace_properties / sanitize_arm_identity would then operate on the fresher body. The new warning makes the failure comprehensible, which is a real improvement, but it does not narrow the window.

Nits

• The re.fullmatch pattern is rebuilt per call. re caches compiled patterns so this is not a performance issue, but a module-level _STRONG_ETAG = re.compile(...) next to the RFC comment would keep the rationale and the pattern together.

Summary

Mechanically correct and well tested at the unit level; the commit does exactly what the two open inline threads asked for, and the HISTORY.rst and _help.py wording were both updated to match (bullet stays in the 1.1.0b1 section, consistent with constants.py VERSION). My only real ask is Suggestion 1 — the new validation is a tightening on a header nobody has observed from the live service, and a wrong assumption there fails closed for every unlink.

Automated review — may be incomplete. I did not restate Copilot's two inline comments beyond noting that this commit resolves them.

[Copilot]

🟢 Approval recommended

The focused changes have comprehensive regression coverage and no identified blocking issues; tests were not executed during this review.

0 open findings

🧠 Review effort: Balanced

[ZZ (zzcodework)]

Azure SRE Agent - automated review

Reviewing changes since 82282465 (head 27c89195, base release/1.1.0-preview)

One commit, one file — azext_iot/tests/adr/test_adr_link_delete_unit.py. It gives the accepted and rejected entity-tag cases explicit pytest IDs and hoists the character-boundary tag into a named constant, answering Yuelin Zhao (@cheatsheet1999)'s inline note that the raw values were hard to read. No product code changed.

Blocking

None.

Verified — the commit message's "without changing coverage" claim holds

I checked this mechanically rather than by eye, since the values are mostly escape sequences. Parsing both revisions of the file and comparing every @pytest.mark.parametrize argument set after resolving module constants: all 14 test functions and all of their parametrize value-sets are identical between 82282465 and 27c89195. The accepted list stays at 4 cases, the rejected list stays at 25, in the same order. The change is purely cosmetic.

The extracted constant is also faithful. BOUNDARY_ETAG = '"!#~\\\x80\xff"' is byte-identical to the inline literal it replaced, and its comment is accurate against the gate in link.py (re.fullmatch(r'"[\x21\x23-\x7e\x80-\xff]*"', etag)): ! is \x21, # and ~ are the inclusive ends of \x23-\x7e, the literal backslash \x5c sits inside that range, and \x80/\xff are the obs-text boundaries. So the one case that exercises every edge of the character class is now self-describing.

Spot-checking the rejection IDs against their values, they are all correct rather than approximate — c0-control-upper-boundary for \x1f, del-control-character for \x7f (correctly excluded from \x23-\x7e), outside-latin1-range for \u0100, and the three header-folding cases named for the exact whitespace they carry, which matters because strip is " \t" only and deliberately does not remove them.

Suggestions

None for this increment.

Nits

None.

Summary

Clean readability-only change; the no-coverage-change claim is verified rather than taken on trust. Nothing to act on here. The two residuals from my previous review are untouched by this commit and still stand on the product code: the ETag is still captured before ensure_unlink_reader, so the precondition is held across a step that can create a role assignment and absorb propagation delay (a re-GET immediately before building the PUT body would cost nothing, since preflight does not depend on the ETag); and the validation is still strictly narrower than what shipped previously, so the evidence that actually matters is one live capture of the namespace GET response headers from the RP — not more unit cases, since every test here supplies the header itself. Note also that #927 edits this same begin_create_or_replace call on the same base branch and does not carry the If-Match work, so whichever lands second needs a deliberate hand-merge.

This is an automated review and may be incomplete; please verify findings before acting on them.