Skip to content

Fixing _FlowStorageClient write cache usage - #528

Merged
Rodrigo Brandão (rodrigobr-msft) merged 3 commits into
mainfrom
users/robrandao/flow-storage-client-cache
Aug 7, 2026
Merged

Fixing _FlowStorageClient write cache usage#528
Rodrigo Brandão (rodrigobr-msft) merged 3 commits into
mainfrom
users/robrandao/flow-storage-client-cache

Conversation

@rodrigobr-msft

@rodrigobr-msft Rodrigo Brandão (rodrigobr-msft) commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This pull request introduces a bug fix to the OAuth flow state storage logic to avoid unnecessary writes when the cached state is unchanged. It also includes minor improvements to typing and documentation, as well as a corresponding test to ensure the fix works as intended.

Bug Fixes and Tests:

  • Updated _FlowStorageClient.write in libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_oauth/_flow_storage_client.py to skip redundant writes to cache and storage when the flow state has not changed.
  • Added a test test_write_skips_unchanged_cached_state in tests/hosting_core/_oauth/test_flow_storage_client.py to verify that writes are skipped if the cached state is unchanged.
  • Documented the bug fix in the changelog.md under a new "Bug Fixes" section.

Code Quality Improvements:

  • Updated type hints to use | None syntax for Python 3.10+ in _FlowStorageClient methods and constructor. [1] [2]
  • Cleaned up unused imports and improved doc wording in README.md and _flow_storage_client.py. [1] [2]

Copilot AI lite review requested due to automatic review settings August 6, 2026 17:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes _FlowStorageClient.write() to avoid redundant cache/storage writes when the existing cached flow state for a key is unchanged.

Changes:

  • Compare the cached flow state at the specific key (instead of comparing the whole cache dict) before writing.
  • Add an async test ensuring writes are skipped when cached state is unchanged.
  • Update changelog entry and fix a README typo (“internal”).

Reviewed changes

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

File Description
tests/hosting_core/_oauth/test_flow_storage_client.py Adds coverage for avoiding redundant writes when the cached state matches.
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_oauth/_flow_storage_client.py Fixes write-avoidance logic to compare the cached state for the specific key.
changelog.md Documents the bug fix in release notes.
README.md Removes a “Debugging” subsection and corrects “intenral” → “internal”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/hosting_core/_oauth/test_flow_storage_client.py
Comment thread tests/hosting_core/_oauth/test_flow_storage_client.py
Comment thread README.md Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 15:47
@rodrigobr-msft
Rodrigo Brandão (rodrigobr-msft) marked this pull request as ready for review August 7, 2026 15:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_oauth/_flow_storage_client.py:83

  • The new write-skip check can silently miss persisting updates if a cache implementation stores and returns the same _FlowState object by reference: in-place mutations would also mutate the cached value, making cached_state.get(key) != value evaluate false and skipping the storage write. To make the optimization robust, store a defensive copy in the cache and compare against the cached snapshot value (not the live object).
        key: str = self.key(value.auth_handler_id)
        cached_state = await self._cache.read([key], target_cls=_FlowState)
        if not cached_state or cached_state.get(key, None) != value:
            await self._cache.write({key: value})
            await self._storage.write({key: value})

tests/hosting_core/_oauth/test_flow_storage_client.py:101

  • This test hard-codes the storage key string instead of using _FlowStorageClient.key(), which makes the test brittle if the key format changes (even though the behavior under test is cache skip logic). Building the key from the client improves maintainability without changing test intent.
        flow_state = _FlowState(auth_handler_id="handler")
        key = f"auth/{DEFAULTS.channel_id}/{DEFAULTS.user_id}/handler"
        cache.read.return_value = {key: flow_state.model_copy()}
        client = _FlowStorageClient(
            DEFAULTS.channel_id,
            DEFAULTS.user_id,
            storage,
            cache_class=mocker.Mock(return_value=cache),
        )

Copilot AI review requested due to automatic review settings August 7, 2026 18:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (2)

libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/_oauth/_flow_storage_client.py:83

  • In _FlowStorageClient.write, the cache is updated before the backing storage. With the new “skip write when cached state is unchanged” logic, a partial failure (cache write succeeds, storage write raises) can leave cache ahead of storage; subsequent retries on the same client instance will see an unchanged cached value and incorrectly skip the storage write, leaving the durable state stale. Writing to storage first avoids this inconsistency while preserving the optimization.
        key: str = self.key(value.auth_handler_id)
        cached_state = await self._cache.read([key], target_cls=_FlowState)
        if not cached_state or cached_state.get(key, None) != value:
            await self._cache.write({key: value})
            await self._storage.write({key: value})

tests/hosting_core/_oauth/test_flow_storage_client.py:101

  • The test hard-codes the flow-state key string, duplicating _FlowStorageClient’s key format. If base_key/key() formatting changes, this test may fail for an unrelated reason. Construct the key via client.key(...) instead.
        flow_state = _FlowState(auth_handler_id="handler")
        key = f"auth/{DEFAULTS.channel_id}/{DEFAULTS.user_id}/handler"
        cache.read.return_value = {key: flow_state.model_copy()}
        client = _FlowStorageClient(
            DEFAULTS.channel_id,

@rodrigobr-msft
Rodrigo Brandão (rodrigobr-msft) merged commit 16b1c0e into main Aug 7, 2026
11 checks passed
@rodrigobr-msft
Rodrigo Brandão (rodrigobr-msft) deleted the users/robrandao/flow-storage-client-cache branch August 7, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants