Skip to content

fix: router @include variable fails with "not provided" on repeated requests (ENG-9772)#3045

Open
gausie wants to merge 9 commits into
mainfrom
sam/eng-9772-router-include-variable-fails-with-not-provided-on-repeated
Open

fix: router @include variable fails with "not provided" on repeated requests (ENG-9772)#3045
gausie wants to merge 9 commits into
mainfrom
sam/eng-9772-router-include-variable-fails-with-not-provided-on-repeated

Conversation

@gausie

@gausie gausie commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Motivation and Context

Fixes ENG-9772: an operation whose Boolean variable is used both as a field argument and in a @skip/@include directive succeeds on the first request, then fails on every subsequent identical request with:

Variable "$flag" of required type "Boolean!" was not provided.

Root cause

  • On a normalization cache miss, the normalizer only strips a skip/include variable from the operation and the request variables when it is not used anywhere else. A dual-use variable (also a field argument) stays declared in the cached normalized representation.
  • On a cache hit, normalizeNonPersistedOperation unconditionally deleted every skip/include variable from the request variables. The cached operation still declares the dual-use variable as Boolean!, so validation fails.

The first request misses the cache and succeeds; ristretto's Set is buffered, so the entry lands shortly after and every subsequent identical request hits the cache and fails until the router restarts.

Fix

On cache miss, record which skip/include variables normalization actually removed (i.e. are no longer declared in the normalized operation) and store that list in the NormalizationCacheEntry. On cache hit, delete only those from the request variables, keeping dual-use variables intact.

The persisted-operation cache path is not affected: its cache key includes the skip/include variable values, so a dual-use variable stays consistent between key and entry.

Checklist

  • I have written a test that reproduces the bug (TestNormalizationCacheHitWithIncludeVariableAlsoUsedAsArgument fails without the fix)
  • Integration test waits deterministically for the first cache HIT via the debug normalization cache response header

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of conditional GraphQL variables so requests using the same value both as an argument and in an @include directive return consistent results from cache.
    • Fixed cache behavior across repeated requests, persisted operations, and cache warmup scenarios so responses stay correct after a cache hit.

@github-actions github-actions Bot added the router label Jul 3, 2026
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 88c9558e-f6ca-4474-b581-6226966f1ebe

📥 Commits

Reviewing files that changed from the base of the PR and between 96a990d and 12c3fbe.

📒 Files selected for processing (2)
  • router-tests/operations/normalization_cache_skip_include_test.go
  • router-tests/operations/testdata/cache_warmup/skip_include_dual_use/dual_use.gql

Walkthrough

Adds a removedSkipIncludeVariableNames field to NormalizationCacheEntry in the router's operation processor, computed on cache miss and reused on cache hit to correctly strip skip/include variables even when shared with GraphQL arguments. Adds three integration tests and a GraphQL fixture covering normalization cache, persisted-operation cache, and cache warmup scenarios.

Changes

Normalization cache variable removal fix

Layer / File(s) Summary
Cache entry field and write-path computation
router/core/operation_processor.go
Adds removedSkipIncludeVariableNames field to NormalizationCacheEntry, computed on cache miss by filtering skip/include variables not declared in the normalized operation.
Cache hit variable stripping using cached list
router/core/operation_processor.go
On cache hits, variable stripping uses the cached removedSkipIncludeVariableNames list instead of recomputing from the request's current variables.
Regression tests for shared skip/include variable
router-tests/operations/normalization_cache_skip_include_test.go, router-tests/operations/testdata/cache_warmup/skip_include_dual_use/dual_use.gql
Adds three integration tests (normalization cache, persisted-operation cache, cache warmup) and a mutation fixture verifying correct behavior when a variable is used both as an argument and an @include condition.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • wundergraph/cosmo#2824: Both PRs modify skip/include variable-name handling in router/core/operation_processor.go, one fixing cache key corruption and the other tracking removed variable names for correct cache-hit stripping.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the bug being fixed and matches the dual-use @include variable cache-hit issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-dd1ba71b5cfa35b220810c50729a12dc41aaff42

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.47%. Comparing base (3ed663b) to head (12c3fbe).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
router/core/http_server.go 60.00% 1 Missing and 1 partial ⚠️
router/core/router.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
+ Coverage   61.46%   61.47%   +0.01%     
==========================================
  Files         261      261              
  Lines       30596    30615      +19     
==========================================
+ Hits        18805    18820      +15     
- Misses      10277    10280       +3     
- Partials     1514     1515       +1     
Files with missing lines Coverage Δ
router/core/graph_server.go 85.43% <100.00%> (+0.05%) ⬆️
router/core/operation_processor.go 85.91% <100.00%> (+0.08%) ⬆️
router/core/http_server.go 84.72% <60.00%> (-1.85%) ⬇️
router/core/router.go 71.02% <50.00%> (+0.34%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gausie gausie marked this pull request as ready for review July 3, 2026 10:30
@gausie gausie requested a review from a team as a code owner July 3, 2026 10:30
@gausie gausie changed the title fix: router @include variable fails with "not provided" on repeated requests fix: router @include variable fails with "not provided" on repeated requests (ENG-9772) Jul 3, 2026
Comment thread router-tests/operations/normalization_cache_skip_include_test.go Outdated
Comment thread router-tests/operations/normalization_cache_skip_include_test.go Outdated
Comment thread router/core/operation_processor.go
gausie added 4 commits July 3, 2026 12:39
Expose Router.WaitForCacheWrites (backed by the existing graphMux
waitForCaches) and a testenv helper so tests can deterministically
observe async normalization cache writes.
…vided-on-repeated' of github.com:wundergraph/cosmo into sam/eng-9772-router-include-variable-fails-with-not-provided-on-repeated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants