Skip to content

Fix health check query not executed#4927

Open
JC-wk wants to merge 14 commits into
microsoft:mainfrom
JC-wk:api-health-check
Open

Fix health check query not executed#4927
JC-wk wants to merge 14 commits into
microsoft:mainfrom
JC-wk:api-health-check

Conversation

@JC-wk

@JC-wk JC-wk commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Resolves #4926

What is being addressed

The existing code container.query_items("SELECT TOP 1 * FROM c") does not actually execute the query so the health check can falsely return OK even if Cosmos is down or inaccessible.

How is this addressed

  • Ensure the query runs
  • Add tests
  • Update CHANGELOG.md
  • Increment version

* Health check can falsely return OK even if Cosmos is down or inaccessible. ([microsoft#4926](microsoft#4926))
Copilot AI review requested due to automatic review settings June 8, 2026 10:45
@JC-wk
JC-wk requested a review from a team as a code owner June 8, 2026 10:45

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

This pull request fixes the Cosmos DB health check so it actually executes a query (preventing false “OK” results when Cosmos is unreachable), and updates the API’s unit tests, changelog, and version accordingly.

Changes:

  • Execute the Cosmos query by iterating the async result (async for ... break) with max_item_count=1.
  • Add unit tests covering Cosmos HTTP errors and query-time request errors.
  • Update CHANGELOG.md and bump api_app version.

Reviewed changes

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

File Description
api_app/services/health_checker.py Forces Cosmos query execution during health check and maps CosmosHttpResponseError to “not accessible”.
api_app/tests_ma/test_services/test_health_checker.py Adds tests for Cosmos HTTP/query-time failures (but one existing “responding” test needs updating to reflect the new iteration behavior).
CHANGELOG.md Documents the bug fix under Unreleased “BUG FIXES”.
api_app/_version.py Bumps API version from 0.25.16 to 0.25.17.

Comment thread api_app/tests_ma/test_services/test_health_checker.py
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Unit Test Results

681 tests   681 ✅  8s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 11ab4c0.

♻️ This comment has been updated with latest results.

James Chapman and others added 4 commits June 8, 2026 10:52
api.dependencies.database.Database.
  get_container_proxy  instead of  azure.cosmos.aio.ContainerProxy.query_items .
  • Configured the return value of  get_container_proxy_mock  to be a mock container whose
  query_items  function returns a real async iterator:  AsyncIterator([{"id": "item"}]) .
  • Verified that all unit tests pass, and successfully ran the entire test suite (all 675 tests
  passed).
@rudolphjacksonm

Copy link
Copy Markdown
Collaborator

/test

@github-actions

Copy link
Copy Markdown

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/28439249321 (with refid 4403460f)

(in response to this comment from @rudolphjacksonm)

@marrobi marrobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From Opus 4.8:

Blocking concerns: (1) create_state_store_status() still has ambiguous success semantics—ok is returned whenever iteration does not raise, including the zero-result path, which can mask real Cosmos connectivity/auth/partition misconfiguration depending on SDK behavior; please make success depend on a definitive successful probe operation (not just “no exception while iterating”) and add an explicit test for empty results.

(2) The new tests rely on custom async iterator doubles + MagicMock that may drift from actual azure.cosmos.aio.ContainerProxy.query_items behavior and create false confidence across SDK changes; please tighten fidelity (e.g., AsyncMock/shared fixture with realistic async iterable behavior) and assert the exact query_items call contract ("SELECT TOP 1 * FROM c", max_item_count=1) so regressions are caught.

@JC-wk

JC-wk commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

From Opus 4.8:

Blocking concerns: (1) create_state_store_status() still has ambiguous success semantics—ok is returned whenever iteration does not raise, including the zero-result path, which can mask real Cosmos connectivity/auth/partition misconfiguration depending on SDK behavior; please make success depend on a definitive successful probe operation (not just “no exception while iterating”) and add an explicit test for empty results.

(2) The new tests rely on custom async iterator doubles + MagicMock that may drift from actual azure.cosmos.aio.ContainerProxy.query_items behavior and create false confidence across SDK changes; please tighten fidelity (e.g., AsyncMock/shared fixture with realistic async iterable behavior) and assert the exact query_items call contract ("SELECT TOP 1 * FROM c", max_item_count=1) so regressions are caught.

In health_checker.py:

• Added an explicit await container.read() call before iterating over container.query_items .
• Calling container.read() acts as a definitive probe of the state store container (validating credentials, endpoints, and partition existence). It will raise a standard database error (
ServiceRequestError / CosmosHttpResponseError ) if misconfigured, avoiding ambiguous success status when query_items returns an empty result set.

2. High-Fidelity Mocking & Exact Call Contract Assertions

In test_health_checker.py:

• Replaced the custom class-based async iterator double ( AsyncIteratorWithError ) with Python's built-in AsyncMock to configure asynchronous iteration. This uses native mocking behavior (
aiter.return_value = query_results or aiter.side_effect = query_error ) that matches the Azure SDK iterator interface.
• Added assertions for the exact query_items call contract: container_mock.query_items.assert_called_once_with("SELECT TOP 1 * FROM c", max_item_count=1) .
• Added assertions verifying that the new probe operation is invoked: container_mock.read.assert_called_once() .
• Added a new explicit test test_get_state_store_status_empty_results to verify that when the container is empty, the probe completes successfully and returns StatusEnum.ok .

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 1 comment.

Comment thread api_app/services/health_checker.py Outdated
Comment on lines +22 to +24
await container.read()
async for _ in container.query_items("SELECT TOP 1 * FROM c", max_item_count=1):
break

@JC-wk JC-wk Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This extra read was added in response to opus feedback to "make success depend on a definitive successful probe operation".
implementation with await container.read() + query_items("SELECT TOP 1 * FROM c") is the only approach that fully satisfies the Opus 4.8 code review. It ensures we have a definitive probe that prevents masked failures, while still allowing the health check to succeed on an, empty database.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wonder what the RU consumption would be related to this... how often does it run etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Opus:

The extra await container.read() isn't needed to satisfy the "definitive probe" requirement — iterating the query (async for ... break) already forces a network round-trip to Cosmos, and any failure surfaces as ServiceRequestError/CosmosHttpResponseError and is caught below. An empty container is still a successful probe (the request goes out; the loop body just doesn't run), so read() adds no correctness value.

It does add cost: an extra round-trip on every poll and potentially broader RBAC requirements, which could fail the check for permission reasons unrelated to actual availability. I'd drop read() and use a minimal projection:

async for _ in container.query_items("SELECT TOP 1 c.id FROM c", max_item_count=1):
    break

The unit tests will need updating to match.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The costs were only pennies but I'm happy to revert it if Opus has now changed it's mind

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Leave it to you to decide.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

reverted

Copilot AI review requested due to automatic review settings July 23, 2026 12:09

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.

Comments suppressed due to low confidence (1)

api_app/tests_ma/test_services/test_health_checker.py:23

  • create_mock_container configures query_items_mock.return_value.__aiter__.return_value with a plain list on a MagicMock return value. async for expects an async iterator (__anext__), so this setup will raise TypeError when create_state_store_status() iterates the results. Use an AsyncMock (or a small async-iterator helper) as the query_items() return value so async iteration works.
    query_items_mock = MagicMock()
    if query_error:
        query_items_mock.return_value.__aiter__.side_effect = query_error
    else:
        query_items_mock.return_value.__aiter__.return_value = query_results or []

Copilot AI review requested due to automatic review settings July 23, 2026 12:13

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.

Comments suppressed due to low confidence (1)

api_app/tests_ma/test_services/test_health_checker.py:23

  • container.query_items() returns an async iterable; in these tests the mocked query_items() currently returns a plain MagicMock, which can behave differently from an async iterable and is inconsistent with other repo tests (they typically return an AsyncMock with __aiter__). Making query_items_mock return an AsyncMock will better match the Cosmos SDK contract and avoid brittle async-iteration behavior.
    query_items_mock = MagicMock()
    if query_error:
        query_items_mock.return_value.__aiter__.side_effect = query_error
    else:
        query_items_mock.return_value.__aiter__.return_value = query_results or []

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.

Health check can falsely return OK even if Cosmos is down or inaccessible

4 participants