fix(router): renew APQ TTL without corrupting stored query#3072
Open
Lawal-create wants to merge 1 commit into
Open
fix(router): renew APQ TTL without corrupting stored query#3072Lawal-create wants to merge 1 commit into
Lawal-create wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughChangesAPQ TTL renewal
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
@skip.Tests
Description
Fixes #3062.
Automatic Persisted Queries (APQ) corrupted the stored query on TTL renewal.
When a persisted operation whose store entry has a TTL is re-executed and
resolved from the in-memory normalization cache, the router re-saved it to renew
the expiration but wrote back the normalized representation instead of the
original raw query. Normalization evaluates and strips
@skip/@includeusingthe current request's variables, so the stored query was overwritten with a
variable-specific, directive-stripped version. Subsequent requests resolving the
same hash with different variables then silently lost the conditionally-included
fields (HTTP 200, fields absent). Only TTL-renewal on later cache hits was
affected; initial registration correctly stored the raw query.
Root cause
In
FetchPersistedOperation(router/core/operation_processor.go), the APQcache-hit-with-TTL branch called
SaveOperation(..., NormalizedRepresentation).On the hash-only replay path the raw query is not in memory, so the normalized
(stripped) form was written to the APQ store (Redis or in-memory).
Fix
Renew the TTL at the storage layer without rewriting the body:
apq.Client.RenewTTL— re-writes the raw body already present in the store viathe existing
Get+Set(APQ is content-addressed bysha256(rawQuery), sothe stored body is invariant). No-op when the entry is absent. Covers both the
Redis and in-memory stores.
persistedoperation.Client.RenewOperationTTL— wraps it and preserves theexisting PQL-manifest ownership guard used by
SaveOperation.FetchPersistedOperationnow callsRenewOperationTTLinstead ofSaveOperation(NormalizedRepresentation).No change to the
apq.KVClientinterface, APQ registration, orpersisted-operation lookup semantics. No generated code changes.
Tests
router/core/operation_processor_apq_ttl_test.go): regression tests forthe KV-backed (Redis-style mock) and in-memory APQ stores — asserting the
stored body stays the raw query and that a
skip:falsereplay still resolvesthe conditional field after a
skip:truerenewal. Based on the regression testfrom Failing regression test for APQ TTL renewal corrupting stored query #3061.
router-tests/operations/automatic_persisted_queries_test.go): aRedis-backed end-to-end regression in the existing
redis cachegroup,asserting the Redis entry remains the raw query after renewal and that a
skip:falserequest returns the conditional field.Both were verified to fail on the unpatched code and pass with the fix.
How to test
automatic_persisted_queries.cache.ttl > 0.query Q($skip: Boolean!) { a: __typename b: __typename @skip(if: $skip) }with
{"skip": true}(query + sha in the request).{"skip": true}(triggers TTL renewal).{"skip": false}→ the response must includeb.On the unpatched router,
bis missing and Redis holds the strippedquery Q { a: __typename }.Automated:
Notes for upgraders
Entries already corrupted in Redis by an affected router cannot be reconstructed
from hash-only requests. After upgrading, flush the APQ key prefix or wait for
the TTL to expire; affected clients re-register on the next miss.
Checklist
Open Source AI Manifesto
This contribution follows the principles of the Open Source AI Manifesto.