Azure / Azure/azure-sdk-for-rust

[azure_data_cosmos_driver] Hot-path account-metadata cache miss fails steady-state data ops with 503/20010 (no regional fallback, no serve-stale)

Open
#4,589 0 comments 1 reaction 0 assignees View on GitHub
Client Cosmos
Dominant language
Rust
Stars
884
Forks
365
Avg merge
2d 19h
Merged PRs (30d)
109

Description

## Summary

In `azure_data_cosmos_driver`, a hot-path account metadata cache miss can fail a user data-plane operation with `503/20010` even when:
1. The pod has been running for hours (cache was previously populated, no restart),
2. Previously-cached `AccountProperties` are available and could be used for regional fallback,
3. The background account-metadata refresh loop is silently swallowing the same failure via `tracing::warn!`.

This was observed during a centralus blackhole drill: a `ReadItem` op on a pod with 0 restarts and ~7h uptime failed with `503/20010: AccountProperties fetch from https://.documents.azure.com/: error sending request for url (...)`. Only 4 such failures occurred across 24h in steady-state — the cache is mostly working, but when it misses the error path is wrong.

## Observed diagnostic

```json
{
"activity_id": "0a523fff-3968-4289-8f64-bf2d09199e8d",
"request_count": 1,
"requests": [{
"duration_ms": 5002,
"endpoint": "https://.documents.azure.com/",
"error": "503/20010: error sending request for url (https://.documents.azure.com/)",
"events": [],
"execution_context": "initial",
"pipeline_type": "metadata",
"region": null,
"request_sent": "not_sent",
"status": "503/20010",
"transport_kind": "gateway"
}],
"total_duration_ms": 5002
}
```

`request_count: 1` — no regional fallback was attempted. `pipeline_type: metadata` — this is the metadata pipeline, not the data pipeline, but the error surfaces as the user op's terminal error.

## Root cause

### Bug A — hot-path passes `previous_props = None`, disabling regional fallback

`sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs:608-613`:

```rust
async fn fetch_account_properties(
&self,
account: &AccountReference,
) -> crate::error::Result {
Self::refresh_account_properties(&self.runtime, account, &self.transport, None).await
// ^^^^ BUG
}
```

`refresh_via_regional_endpoints` (`cosmos_driver.rs:700-728`) bails immediately when `previous_props` is `None`:

```rust
let Some(cached_props) = previous_props else { return Err(primary_error); };
```

The timer-driven path in `LocationStateStore::refresh_account_properties_inner` (`location_state_store.rs:407-410`) does it correctly:

```rust
let previous_props = self.account_metadata_cache.get(&self.account_endpoint).await;
// ...
let fetched = (refresh_fn)(previous_props).await;
```

The hot path called from `execute_operation_direct` (`cosmos_driver.rs:1562-1566`) should do the same — read the existing cache entry (if any) and pass it down so `refresh_via_regional_endpoints` can iterate through known regional endpoints when the global endpoint is unreachable.

### Bug B — hot-path metadata fetch errors should not fail user ops in steady-state

`cosmos_driver.rs:1562-1566`:

```rust
let account_properties = self
.runtime
.account_metadata_cache()
.get_or_fetch(account_endpoint, || self.fetch_account_properties(account))
.await?; // ← propagates fetch error directly to the user op
```

The timer-driven refresh in `LocationStateStore::refresh_account_properties_inner` already treats account-metadata refresh failures as non-fatal — `tracing::warn!` + return. The hot-path should match: if any cached value exists (even stale), serve it; only bubble the error when there is truly no metadata anywhere (real cold-start bootstrap). Right now the hot path silently couples a metadata-pipeline failure to a data-pipeline failure for an op that should not need a fresh metadata fetch in the first place (the cache had no TTL/eviction reason to invalidate the prior entry).

Other Cosmos SDK families (.NET, Java) explicitly:
1. Treat metadata-refresh errors as non-fatal for data-plane ops while a previous snapshot is available, and
2. Fall back to regional endpoint URLs constructed from the account name + known regions when even the previous snapshot is unavailable (true bootstrap).

(2) is a separate, larger gap — out of scope for this issue, but worth tracking.

## Why the cache misses ~4x/day instead of never

Open question — secondary to the main issue. `AccountMetadataCache` (`cache/account_metadata_cache.rs`) is HashMap-backed via `AsyncCache` (`cache/async_cache.rs:26`) with no TTL, no eviction, and no `invalidate()` callers in the codebase. Plausible miss triggers:

- `AsyncLazy` race: hot-path `get` runs while a `get_or_refresh_with` from the background loop has just inserted a fresh lazy that hasn't yet initialized → `try_get` returns `None` → hot path runs its own fetch through the closure.
- `AccountEndpoint` key normalization differing between `initialize()` and `execute_operation_direct` (unlikely — would happen on every op).
- Workload uses two accounts (workload + results); a path may unexpectedly miss on the "other" account.

The miss-trigger should also be investigated, but the propagation behavior is the bug regardless of frequency. Even a 1-in-a-million miss must not fail a steady-state data-plane op when usable metadata is available locally.

## Repro context

- Account: multi-write, regions `[centralus, eastus2]`, hub = `centralus`
- Cluster: AKS in centralus
- Blackhole: iptables DROP on all 4 ATM IPs for `-centralus.documents.azure.com` AND `.documents.azure.com` (global endpoint resolves to a hub-region ATM IP on multi-write accounts, so the global endpoint is unreachable during a hub-region blackhole)
- Pod with the failure: ~7h uptime, 0 restarts, cache populated at bootstrap

## Proposed fixes

### Bug A (1-line)

```rust
async fn fetch_account_properties(
&self,
account: &AccountReference,
) -> crate::error::Result {
let endpoint = AccountEndpoint::from(account);
let previous_props = self
.runtime
.account_metadata_cache()
.get(&endpoint)
.await;
Self::refresh_account_properties(&self.runtime, account, &self.transport, previous_props).await
}
```

### Bug B (wrapper around `get_or_fetch`)

Add `serve_stale_on_refresh_error` semantics: if `get_or_fetch` fails AND the cache currently holds a value (read fresh after the fetch attempt to catch a concurrent successful refresh), return the cached value. Otherwise propagate the error (preserves true-bootstrap failure behavior).

Open question on **B**: should the data op also fire a `LocationEffect::RefreshAccountProperties` so the background loop picks up retrying the metadata refresh? Probably yes — keeps the recovery driven by the existing rate-limited path rather than retrying on every subsequent op.

## Acceptance

- New unit test in `cosmos_driver.rs` (or its test sibling) where `account_metadata_cache` is pre-populated, the global endpoint is blackholed via fault-injection transport, and `execute_operation_direct` for a `ReadItem` succeeds by serving the prior snapshot + falling back to a regional endpoint.
- New unit test where the cache is empty (true bootstrap) and all endpoints are unreachable — op fails with `503/20010` (preserves bootstrap behavior).
- No change to background timer-driven refresh behavior.

Contributor guide

Open the contributing guide

Research direction

Start in sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs, especially fetch_account_properties and execute_operation_direct, then compare the timer path in location_state_store.rs and the cache implementation under cache/. Run or extend the cosmos_driver.rs test sibling with pre-populated and empty-cache fault-injection cases; done means regional fallback and stale serving work without changing background refresh behavior, while true bootstrap failures still return 503/20010.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.