App Configuration Provider - Bug Fixes

[mrm9084]

Description

• Added missing locks in iter, len, and contains.
• DNS discover now throws a TimeoutError when timed out, previously there was no difference between timing out and no endpoints being found.
• When endpoints are found via DNS, old ones are removed (i.e. replicas were deleted)
• Added missing close mocks on tests for clean test results.

All SDK Contribution checklist:

• The pull request does not introduce [breaking changes]
• CHANGELOG is updated for new features, bug fixes or other significant changes.
• I have read the contribution guidelines.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[Copilot]

Pull request overview

This PR delivers a set of targeted bug fixes to the azure-appconfiguration-provider data-plane package, improving auto-failover replica handling and thread safety. It distinguishes a DNS SRV lookup timeout from an empty replica result, cleans up replica clients that are no longer part of the failover set, and guards additional mapping operations with the update lock.

Changes:

• DNS discovery now raises TimeoutError on SRV lookup timeout (instead of returning None for both timeout and empty results), and refresh_clients catches it to apply the longer fallback refresh interval; an empty replica list now refreshes at the normal interval.
• refresh_clients (sync + async) now closes replica clients whose endpoints are no longer in the failover set, preventing a resource leak; the original client is always retained.
• __iter__, __len__, and __contains__ are now guarded by self._update_lock, consistent with __getitem__/keys()/items().
• Test doubles gain close()/async close() methods so refresh-driven cleanup runs cleanly.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
azure/appconfiguration/provider/_discovery.py	Raise TimeoutError on SRV timeout rather than returning None.
azure/appconfiguration/provider/aio/_async_discovery.py	Async equivalent of the timeout-vs-empty distinction.
azure/appconfiguration/provider/_client_manager.py	Catch TimeoutError for fallback interval; close stale replica clients.
azure/appconfiguration/provider/aio/_async_client_manager.py	Async equivalent of timeout handling and stale-client await close().
azure/appconfiguration/provider/_azureappconfigurationproviderbase.py	Guard __iter__/__len__/__contains__ with the update lock.
tests/test_configuration_client_manager.py	Add close() to mock client.
tests/test_configuration_client_manager_load_balance.py	Add close() to mock client.
tests/aio/test_configuration_async_client_manager.py	Add async close() to mock client.
tests/aio/test_configuration_async_client_manager_load_balance.py	Add async close() to mock client.
CHANGELOG.md	Document the three bug fixes under 2.5.1 (Unreleased).

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (7)

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_discovery.py:44

• A timeout resolving the origin SRV record is still collapsed into an empty result: _find_origin returns None for both _request_record(...) is None and [], so line 38 returns before this replica-only timeout check. That path therefore uses the minimal refresh interval instead of the new fallback interval. Preserve the no-record case, but surface None as TimeoutError here as well.

    if replicas is None:
        raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/aio/_async_discovery.py:29

• A timeout resolving the origin SRV record is still collapsed into an empty result: _find_origin returns None for both _request_record(...) is None and [], so line 23 returns before this replica-only timeout check. That path therefore uses the minimal refresh interval instead of the new fallback interval. Preserve the no-record case, but surface None as TimeoutError here as well.

    if replicas is None:
        raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_client_manager.py:490

• A successful empty discovery still returns at lines 493-496 before the new cleanup runs. If the service previously advertised replicas and then removes all of them, every old replica remains open and in _replica_clients, so the resource leak and stale routing persist for this boundary case. Let an empty list continue through the replacement/cleanup path; the common code already assigns the minimal interval.

            # SRV record resolution timed out, so we should refresh after a longer interval
            self._next_update_time = time.time() + FALLBACK_CLIENT_REFRESH_EXPIRED_INTERVAL

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/aio/_async_client_manager.py:494

• A successful empty discovery still returns at lines 497-500 before the new cleanup runs. If the service previously advertised replicas and then removes all of them, every old replica remains open and in _replica_clients, so the resource leak and stale routing persist for this boundary case. Let an empty list continue through the replacement/cleanup path; the common code already assigns the minimal interval.

            # SRV record resolution timed out, so we should refresh after a longer interval
            self._next_update_time = time.time() + FALLBACK_CLIENT_REFRESH_EXPIRED_INTERVAL

sdk/appconfiguration/azure-appconfiguration-provider/tests/test_configuration_client_manager.py:35

• This no-op allows the new cleanup path to run, but the refresh tests only assert that the removed client disappears from the list; they never verify that close() was called. The resource-leak regression would therefore pass even if the close call were deleted. Record closure on each mock client and assert the removed replica is closed while retained/original clients remain open.

    def close(self):
        pass

sdk/appconfiguration/azure-appconfiguration-provider/tests/aio/test_configuration_async_client_manager.py:36

• This no-op allows the new async cleanup path to run, but the refresh tests only assert that the removed client disappears from the list; they never verify that close() was awaited. The resource-leak regression would therefore pass even if the close call were deleted. Record closure on each mock client and assert the removed replica is closed while retained/original clients remain open.

    async def close(self):
        pass

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationproviderbase.py:74

• The existing string test uses "test_key", which already passed the old implementation because unpacking raised ValueError; the regression occurs specifically for a two-character key, which the old code silently unpacked as (key, label). Add a test such as _build_watched_setting("ab") == ("ab", NULL_CHAR) so this bug fix is actually covered.

    if isinstance(setting, str):
        key, label = setting, NULL_CHAR

[Copilot]

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (4)

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_discovery.py:44

• Origin lookup timeouts are still collapsed into an empty discovery result: _find_origin() returns None for both _request_record() timeout and NXDOMAIN, so execution returns at line 38 before this new timeout check. With the new empty-result pruning, a transient origin DNS timeout can close every healthy replica and use the 30-second interval instead of preserving clients with the fallback interval. Preserve the None timeout signal (or raise from _find_origin) separately from an empty DNS answer.

    if replicas is None:
        raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/aio/_async_discovery.py:29

• The async origin lookup still maps both a DNS timeout and no records to None, causing this function to return [] before reaching the new replica-timeout check. The client manager then treats a transient origin timeout as successful empty discovery, closes existing replicas, and schedules the short interval. Keep the origin timeout distinct and surface TimeoutError here as well.

    if replicas is None:
        raise TimeoutError("Timed out while resolving auto-failover replica endpoints.")

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationproviderbase.py:74

• Add a regression test using a two-character string key (for example, _build_watched_setting("ab")). The existing test_string_input uses "test_key", which already took the old fallback path; only a two-character string was successfully unpacked character-by-character by the old implementation, so the current test would not detect this bug returning ("a", "b").

    if isinstance(setting, str):
        key, label = setting, NULL_CHAR

sdk/appconfiguration/azure-appconfiguration-provider/CHANGELOG.md:13

• This release note (and the PR description) does not match the implementation: __iter__, __len__, and __contains__ remain unlocked in _azureappconfigurationproviderbase.py:268-279; the actual change uses copy-on-write only for secret refresh. Either add the claimed locking or revise the release note and PR description to accurately describe the copy-on-write fix.

- Fixed a thread-safety issue where iterating, checking membership, or getting the length of the provider's configuration mapping was not guarded by the update lock.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/_azureappconfigurationprovider.py:215

• The copy-on-write invariant stated here is still violated during normal refresh. _attempt_refresh initializes processed_settings from self._dict, then _process_feature_flags mutates that same mapping in place by first assigning an empty feature_management dict and then its contents (_azureappconfigurationproviderbase.py:380-382). Because readers do not share the refresh lock, they can observe that intermediate empty mapping and get a KeyError. Please make the feature-flag refresh path operate on a copy (or make _process_feature_flags return a fresh mapping) before claiming all writes are published by rebinding.

                # Publish a new dict rather than mutating in place. Readers may hold a live view or
                # iterator over self._dict, and an in-place update could raise "dictionary changed
                # size during iteration". Every write to self._dict must rebind to a new object.
                updated_settings = dict(self._dict)
                updated_settings.update(self._secret_provider.refresh_secrets())
                self._dict = updated_settings

sdk/appconfiguration/azure-appconfiguration-provider/azure/appconfiguration/provider/aio/_azureappconfigurationproviderasync.py:225

• The copy-on-write invariant stated here is still violated during async refresh. _attempt_refresh aliases self._dict as processed_settings, and _process_feature_flags mutates it in place in two steps (_azureappconfigurationproviderbase.py:380-382). A concurrent reader can therefore observe an empty feature_management mapping and raise KeyError. Please copy before that mutation or change the shared helper to construct a fresh mapping.

                # Publish a new dict rather than mutating in place. Readers may hold a live view or
                # iterator over self._dict, and an in-place update could raise "dictionary changed
                # size during iteration". Every write to self._dict must rebind to a new object.
                updated_settings = dict(self._dict)
                updated_settings.update(await self._secret_provider.refresh_secrets())
                self._dict = updated_settings