Skip to content

fix(response_cache): entity fields returning null with cache-control: no-cache#9197

Open
OriginLeon wants to merge 4 commits intodevfrom
marcelo/ROUTER-1689
Open

fix(response_cache): entity fields returning null with cache-control: no-cache#9197
OriginLeon wants to merge 4 commits intodevfrom
marcelo/ROUTER-1689

Conversation

@OriginLeon
Copy link
Copy Markdown
Contributor

@OriginLeon OriginLeon commented Apr 15, 2026

Summary

Fixes ROUTER-1689 (escalation TSH-22520).

When response_cache is enabled and a request carries cache-control: no-cache, entity fields resolved via _entities queries (non-owning subgraphs) were returning null. Fields owned by the root subgraph were unaffected. Regression introduced in v2.13.0 via PR #8948.

Root cause:

cache_lookup_entities had an early return when no-cache was detected, returning ResponseCacheResults::default() — an empty Vec<IntermediateResult>. The downstream function insert_entities_in_result iterates over that list to assemble the final response in order, interleaving cache hits with fresh subgraph data. With an empty list the loop never executed, new_entities stayed empty, and _entities: [] was written into the response — causing all entity fields to be null regardless of what the subgraph returned.

The bug was total: no-cache broke entity resolution completely, independently of whether the cache had been populated or not.

Additionally, the early return was placed after cache.fetch_multiple, meaning a Redis round-trip was being made even when the result was going to be discarded.

Changes:

  • Remove the early return and instead treat no-cache as all-cache-miss: produce a Vec<Option<CacheEntry>> of all None entries (one per entity) so that filter_representations builds a properly sized IntermediateResult list and insert_entities_in_result can assemble the response correctly
  • Move cache.fetch_multiple inside the else branch to avoid the unnecessary Redis round-trip when no-cache is set
  • Change cache_keys.into_iter() to cache_keys.iter() in the span attribute (ownership required since cache_keys is now used after the span call)

Tests added:

  • no_cache_from_request: two-phase regression test — phase 1 warms the cache with a normal request, phase 2 sends cache-control: no-cache and asserts entity fields are returned correctly (not null)

Test plan

  • cargo test -p apollo-router --lib plugins::response_cache — 54/54 passed (68 failures are pre-existing Redis integration tests requiring external infra)
  • cargo clippy -p apollo-router --lib -- -D warnings — clean
  • cargo fmt --check — no changes
  • Manual validation on cloud infra against v2.13.1 (bug) and fix build: bug version always returns null on entity fields with no-cache; fix version always returns correct data regardless of cache state

🤖 Generated with Claude Code

… no-cache is set

When no-cache was present, cache_lookup_entities returned early with an empty
IntermediateResult list. insert_entities_in_result iterates over that list to
assemble the final response, so an empty list caused _entities to be [] and all
entity fields to be null.

Fix by treating no-cache as all-cache-miss instead of early returning, so that
insert_entities_in_result receives a properly sized result list and can assemble
the response correctly. Also avoids the unnecessary Redis round-trip when no-cache
is set by moving fetch_multiple inside the else branch.

Fixes ROUTER-1689

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@OriginLeon OriginLeon requested a review from a team as a code owner April 15, 2026 19:38
@apollo-librarian
Copy link
Copy Markdown
Contributor

apollo-librarian bot commented Apr 15, 2026

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: aad5f5caa717d3799eb3d75b
Build Logs: View logs


✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

@github-actions
Copy link
Copy Markdown
Contributor

@OriginLeon, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

Copy link
Copy Markdown
Contributor

@carodewig carodewig left a comment

Choose a reason for hiding this comment

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

Great find, appreciate you fixing this!

I think the same issue will apply to the entity cache, so the fix should be made there as well.

Comment on lines +1521 to +1522
let cache_result: Vec<Option<CacheEntry>> = if cache_control.is_some_and(|c| c.is_no_cache()) {
std::iter::repeat_n(None, keys_len).collect()
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.

The downside of this approach is that things that were never intended to hit the cache (ie requests with no-cache) will end up incrementing cache-miss metrics.

I think the easiest way to fix this is to add a record_metrics: bool argument to filter_representations and use it to skip the metrics insertion in that function (let _ = context.insert(CacheMetricContextKey::new(subgraph_name.to_string()), CacheSubgraph(cache_hit));)

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.

2 participants