feat: l1/l2 entity and root-field caching on the defer engine - #1560
Open
jensneuse wants to merge 61 commits into
Open
feat: l1/l2 entity and root-field caching on the defer engine#1560jensneuse wants to merge 61 commits into
jensneuse wants to merge 61 commits into
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jensneuse
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/resolve/node_object.go (1)
141-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
ParentOnTypeNamesinField.Copy()
Field.Copy()dropsParentOnTypeNames, so copied object trees lose the parent type-condition metadata used byresolvable.goto decide whether a field should be skipped. Add it to the returned struct to keep copies semantically equivalent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/node_object.go` around lines 141 - 158, Field.Copy() is dropping ParentOnTypeNames, so copied fields lose type-condition metadata used later by resolvable.go. Update the Field.Copy() return value in node_object.go to preserve ParentOnTypeNames alongside OnTypeNames and the other copied properties, ensuring copied object trees remain semantically equivalent.
🧹 Nitpick comments (7)
execution/cachingtesting/normalization_e2e_test.go (1)
43-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTautological key assertion — doesn't actually verify the stored key.
Key: ops[1].Keycompares the operation's key against itself, so this assertion can never fail on a wrong/mismatched key — onlyKind/Value/TTLare effectively checked.♻️ Suggested fix — assert fields independently instead of round-tripping the key
ops := store.Ops() require.Len(t, ops, 2) -assert.Equal(t, cachetesting.StoreOp{ - Kind: "Set", - Key: ops[1].Key, - Value: `{"stock":5,"__typename":"Product"}`, - TTL: time.Minute, -}, ops[1]) +assert.Equal(t, "Set", ops[1].Kind) +assert.NotEmpty(t, ops[1].Key) +assert.Equal(t, `{"stock":5,"__typename":"Product"}`, ops[1].Value) +assert.Equal(t, time.Minute, ops[1].TTL)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/cachingtesting/normalization_e2e_test.go` around lines 43 - 50, The StoreOp assertion in normalization_e2e_test is tautological because cachetesting.StoreOp.Key is compared to ops[1].Key itself, so it never validates the stored key. Update the assertion around store.Ops() to compare the Set operation’s Key against an independently constructed expected value or a separately captured key from the tested code path, and keep the existing checks on Kind, Value, and TTL in the same assertion.v2/pkg/engine/cache/observer.go (1)
40-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-deterministic
ShadowComparesordering from map iteration.
h.ShadowStashis amap[int]ShadowCacheEntry; ranging over it directly meanscompares(and the resultingCacheTrace.ShadowCompares) gets a randomized item order on every call, since Go intentionally randomizes map iteration order.♻️ Suggested fix — iterate ShadowStash in deterministic (index) order
+ indices := make([]int, 0, len(h.ShadowStash)) + for itemIndex := range h.ShadowStash { + indices = append(indices, itemIndex) + } + sort.Ints(indices) compares := make([]resolve.CacheShadowCompareTrace, 0, len(h.ShadowStash)) - for itemIndex, entry := range h.ShadowStash { + for _, itemIndex := range indices { + entry := h.ShadowStash[itemIndex] freshValue := fresh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/observer.go` around lines 40 - 72, The TraceObserver.CompareShadow method is building ShadowCompares by ranging directly over h.ShadowStash, so the order is randomized each call. Update CompareShadow to iterate ShadowStash deterministically by sorting the stash keys (or otherwise walking indexes in order) before appending CacheShadowCompareTrace entries. Keep the existing freshValue lookup logic tied to h.Items and batch indices, but make the compare slice order stable before storing it in o.compares.docs/caching/reviews/20-art-observability.md (1)
36-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: consider a size cap on
TraceObserver.compares.The doc itself flags this cross-request hygiene gap and invites a decision. Given the leak only manifests on a process-killing panic (deferred
EndRequestotherwise always drains the entry), a defensive size cap is optional rather than required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/caching/reviews/20-art-observability.md` around lines 36 - 39, Add a defensive size cap to TraceObserver.compares and enforce it in the drain-on-observe path inside TraceObserver/observer.go, even though EndRequest normally clears entries from controller.go. Keep the existing handle-based controller↔observer contract unchanged, and make the cap logic safe for cross-request hygiene so stale entries cannot grow unbounded if a process-killing panic bypasses deferred EndRequest.v2/pkg/engine/resolve/loader.go (1)
440-455: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify
BeginRequestnever legitimately returns nil.
cacheRequest()keys its lazy-once init purely onl.ctx.requestCache == nil. IfCacheController.BeginRequestcan legitimately returnnil(e.g. a per-request opt-out), this branch re-invokesBeginRequeston every subsequent cache-configured fetch of the same request instead of once, violating the documented "called once per request" contract onCacheController.BeginRequest.cache_noop_test.go'scountingCacheControllerreturnsnilbut never exercises this path (its test has no cache-configured fetch, socacheRequest()is never reached).If
nilis a valid "opt out for this request" signal, consider a separate started-flag instead of overloading the nil check.🔍 Possible fix using a dedicated started flag
func (l *Loader) cacheRequest() RequestCache { if l.ctx.cacheController == nil { return nil } l.dataBuffer.Lock() defer l.dataBuffer.Unlock() - if l.ctx.requestCache == nil { + if !l.ctx.requestCacheStarted { l.ctx.requestCache = l.ctx.cacheController.BeginRequest(l.ctx) + l.ctx.requestCacheStarted = true } return l.ctx.requestCache }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/resolve/loader.go` around lines 440 - 455, cacheRequest() currently uses l.ctx.requestCache == nil as the only “not started” check, so if CacheController.BeginRequest can legitimately return nil it will be called repeatedly for the same request. Update the lazy init in Loader.cacheRequest to track whether BeginRequest has already been attempted with a separate started/initialized flag on the request context instead of relying on requestCache being non-nil, and keep the BeginRequest call itself in that path so the once-per-request contract is preserved.docs/caching/specs/2026-06-30-rfc-02-caching-planner.md (1)
142-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language hint to the fenced blocks for markdownlint (MD040) compliance.
Flagged by static analysis at Line 142 and Line 398.
📝 Proposed fix
-``` +```text(apply to both fenced blocks at lines 142 and 398)
Also applies to: 398-398
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/caching/specs/2026-06-30-rfc-02-caching-planner.md` at line 142, The fenced markdown blocks in this document are missing a language hint, which triggers MD040. Update both fenced sections in the caching planner spec to use a tagged fence (for example, the markdown-safe text hint) so the blocks are compliant, and apply the same fix to both occurrences identified in the review.Source: Linters/SAST tools
v2/pkg/engine/plan/cache_provides_data_visitor.go (1)
141-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
FieldNameBytescall — reuse the existingfieldNamelocal.
fieldNameis already computed at line 103; line 147 recomputes the identical value instead of reusing it.♻️ Proposed fix
if fetchResponseKey != string(fieldName) { - field.OriginalName = v.operation.FieldNameBytes(fieldRef) + field.OriginalName = fieldName }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/plan/cache_provides_data_visitor.go` around lines 141 - 148, The redundant name conversion in cache_provides_data_visitor.go should be removed by reusing the existing fieldName local instead of calling v.operation.FieldNameBytes(fieldRef) again inside the resolve.Field construction. Update the logic around the field assignment in the cacheProvidesDataVisitor path so OriginalName is derived from the already-computed fieldName value when fetchResponseKey differs, keeping the behavior unchanged while avoiding duplicate work.v2/pkg/engine/cache/controller.go (1)
349-410: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant per-target lookup in root-field entity reuse.
prepareItemStateis called once perin.Itemselement with the identicallookupItem/templates, repeating the L2 read/parse (and L1 populate) for every merge target of the same fetch instead of computing the shared entity state once and fanning it out. This also causesServedFromLayerto differ across targets of the same fetch (first: "l2", rest: "l1") purely from that iteration's cache-warming side effect, which will surface as a confusing/incorrect ART trace value for otherwise-identical merge targets.♻️ Suggested direction
lookupItem := entityLookupItem(r.ctx, cfg.KeySpec.EntityKeyMappings) tx := in.Arena.Begin() defer tx.Commit() // The coverage/selection walks run against the FIELD subtree — the cached // value is the entity, not the whole response. reuseCfg := *cfg reuseCfg.ProvidesData = subtree + sharedState, missed, itemMustWriteBack := r.prepareItemState(tx, &reuseCfg, templates, lookupItem) items := make([]resolve.ItemCacheState, 0, len(in.Items)) missedByItem := make([][]string, 0, len(in.Items)) - allCovered := true - mustWriteBack := false + allCovered := true + mustWriteBack := itemMustWriteBack for _, item := range in.Items { - state, missed, itemMustWriteBack := r.prepareItemState(tx, &reuseCfg, templates, lookupItem) + state := sharedState state.Item = item🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller.go` around lines 349 - 410, The root-field entity reuse path in requestCache.prepareRootFieldEntityReuse is redundantly recomputing the same shared entity lookup for every in.Items entry via prepareItemState, which also makes ServedFromLayer vary across identical merge targets due to cache warming side effects. Compute the shared lookup/state once for the fetch using the common lookupItem and templates, then fan out copies of that result to each item while preserving item-specific fields like Item and EntityMergePath. Make sure the cached decision and ART trace values are derived from the shared read rather than per-iteration side effects, and keep ShadowMode behavior applied per item after cloning the shared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@execution/cachingtesting/cachingtesting.go`:
- Around line 49-53: PlanResult.Inputs and PlanResult.Gate currently hide
unknown subgraph names by passing an empty ID through to Fakes, which can make
typos look like successful setup; update both methods to resolve names the same
way as PlanResult.LoadCount by falling back to the literal name when nameToID
misses, or otherwise fail fast using PlanResult’s existing validation path. Keep
the behavior consistent across Inputs, Gate, and LoadCount so unrecognized
subgraph names cannot silently register unusable gates or fetch paths.
In `@execution/cachingtesting/defer_l1_e2e_test.go`:
- Around line 212-225: The concurrency test in defer_l1_e2e_test.go can hang
indefinitely on channel receives if the resolver regresses, so add short
timeout-based selects around the waits on arrived, writer.Flushed, and done in
the relevant test block. Update the defer-l1 e2e test flow near the current
channel ordering checks to fail fast with a clear assertion on timeout instead
of blocking forever, keeping the same expectations around Frames() and the gated
fetch behavior.
In `@v2/pkg/engine/cache/cache_key_template.go`:
- Around line 83-118: The numeric normalization in renderRepresentationValue
still uses the original JSON literal, so equivalent numbers can produce
different cache keys. Update the default scalar handling in
renderRepresentationValue to canonicalize astjson.TypeNumber values into a
normalized numeric representation before returning, instead of re-serializing
with value.MarshalTo(nil), while keeping the object recursion and __typename
skipping behavior unchanged.
In `@v2/pkg/engine/cache/controller.go`:
- Around line 939-975: The writeFetchedValue path in requestCache is labeling
pure backfill item traces as refresh. Update the final item.WriteReason
assignment so it reflects CacheWriteReasonBackfill when all written keys come
from PendingCandidates (for example, when backfillFrom is 0 and no RenderedKeys
exist), and only use CacheWriteReasonRefresh when there was an original rendered
key. Keep the per-key deferSet reason logic unchanged.
- Around line 135-148: The fetchLoadTrace helper currently violates the
fetch-type switch rule by branching on concrete resolve.Fetch implementations.
Update fetchLoadTrace to use the Fetch interface’s tracing accessors instead of
type-switching on *resolve.SingleFetch, *resolve.EntityFetch, and
*resolve.BatchEntityFetch, and keep the nil behavior for tracing-disabled
requests. Also scan nearby cache/controller code for similar concrete fetch
switches and replace them with interface-based access where found.
- Around line 606-674: The multi-key freshness loop in controller.go incorrectly
lets any null cached value set NegativeHit because it only checks
state.FromCache == nil, which is never set for positive hits in this loop, so a
later stale negative sentinel can override a fresher positive candidate. Fix the
hit-selection logic so only the freshest candidate can determine negative-hit
status, or otherwise defer negative-sentinel promotion until after the freshest
candidate is chosen via selectMultiCandidateCacheValue. Use the lookupHit
processing, state.FromCache, NegativeHit, and selectMultiCandidateCacheValue
flow in controller.go to guide the change, and add a regression case in
TestMultiKeyFreshnessRows for a fresh positive plus stale negative sentinel
across sibling `@key` candidates.
In `@v2/pkg/engine/cache/coverage.go`:
- Around line 18-32: The covers function currently treats every field in
resolve.Object.Fields as required, which causes type-conditional fields to fail
coverage for polymorphic results. Update covers to detect and skip fields
guarded by OnTypeNames unless the cached __typename matches, and keep the
recursive checks in coversNode for only the applicable fields. Use the existing
normalizedFieldName, covers, and coversNode logic to locate where to gate the
field lookup before recursing.
In `@v2/pkg/engine/cache/optimize_l1_cache.go`:
- Around line 344-355: The union contribution gate in objectSharesAnyField is
too shallow because it only checks top-level field overlap. Update this helper
to recurse through nested object fields so a provider only counts as
contributing when it shares a field path the consumer actually needs, using
objectSharesAnyField and the existing field lookup helpers to walk nested
selections instead of stopping at the first top-level match.
In `@v2/pkg/engine/cache/partial.go`:
- Around line 99-100: The partial fetch handling in reduced batch processing is
silently skipping short `_entities` responses instead of surfacing a contract
violation. Update the logic around the batch consumption check in partial.go so
the code in the partial reduction path fails with an error when the consumed
entity count does not match the expected fetched count, rather than continuing;
use the relevant fetch/reduction flow and the `_entities` handling branches as
the place to enforce this guard.
In `@v2/pkg/engine/cache/transform.go`:
- Around line 162-168: The response shaping in denormalizeToSelection is
incorrectly reusing the cached object visit/write-back path, which can append
cache-only extras into client-facing values. Update the logic around obj.Visit
and out.Set so that only fields present in the requested selection are
materialized into the response, while any write-back preservation for cached
supersets is handled separately from response construction. Use
denormalizeToSelection and the surrounding Visit/out.Set flow to keep
response-shaped values free of unselected normalized fields.
---
Outside diff comments:
In `@v2/pkg/engine/resolve/node_object.go`:
- Around line 141-158: Field.Copy() is dropping ParentOnTypeNames, so copied
fields lose type-condition metadata used later by resolvable.go. Update the
Field.Copy() return value in node_object.go to preserve ParentOnTypeNames
alongside OnTypeNames and the other copied properties, ensuring copied object
trees remain semantically equivalent.
---
Nitpick comments:
In `@docs/caching/reviews/20-art-observability.md`:
- Around line 36-39: Add a defensive size cap to TraceObserver.compares and
enforce it in the drain-on-observe path inside TraceObserver/observer.go, even
though EndRequest normally clears entries from controller.go. Keep the existing
handle-based controller↔observer contract unchanged, and make the cap logic safe
for cross-request hygiene so stale entries cannot grow unbounded if a
process-killing panic bypasses deferred EndRequest.
In `@docs/caching/specs/2026-06-30-rfc-02-caching-planner.md`:
- Line 142: The fenced markdown blocks in this document are missing a language
hint, which triggers MD040. Update both fenced sections in the caching planner
spec to use a tagged fence (for example, the markdown-safe text hint) so the
blocks are compliant, and apply the same fix to both occurrences identified in
the review.
In `@execution/cachingtesting/normalization_e2e_test.go`:
- Around line 43-50: The StoreOp assertion in normalization_e2e_test is
tautological because cachetesting.StoreOp.Key is compared to ops[1].Key itself,
so it never validates the stored key. Update the assertion around store.Ops() to
compare the Set operation’s Key against an independently constructed expected
value or a separately captured key from the tested code path, and keep the
existing checks on Kind, Value, and TTL in the same assertion.
In `@v2/pkg/engine/cache/controller.go`:
- Around line 349-410: The root-field entity reuse path in
requestCache.prepareRootFieldEntityReuse is redundantly recomputing the same
shared entity lookup for every in.Items entry via prepareItemState, which also
makes ServedFromLayer vary across identical merge targets due to cache warming
side effects. Compute the shared lookup/state once for the fetch using the
common lookupItem and templates, then fan out copies of that result to each item
while preserving item-specific fields like Item and EntityMergePath. Make sure
the cached decision and ART trace values are derived from the shared read rather
than per-iteration side effects, and keep ShadowMode behavior applied per item
after cloning the shared state.
In `@v2/pkg/engine/cache/observer.go`:
- Around line 40-72: The TraceObserver.CompareShadow method is building
ShadowCompares by ranging directly over h.ShadowStash, so the order is
randomized each call. Update CompareShadow to iterate ShadowStash
deterministically by sorting the stash keys (or otherwise walking indexes in
order) before appending CacheShadowCompareTrace entries. Keep the existing
freshValue lookup logic tied to h.Items and batch indices, but make the compare
slice order stable before storing it in o.compares.
In `@v2/pkg/engine/plan/cache_provides_data_visitor.go`:
- Around line 141-148: The redundant name conversion in
cache_provides_data_visitor.go should be removed by reusing the existing
fieldName local instead of calling v.operation.FieldNameBytes(fieldRef) again
inside the resolve.Field construction. Update the logic around the field
assignment in the cacheProvidesDataVisitor path so OriginalName is derived from
the already-computed fieldName value when fetchResponseKey differs, keeping the
behavior unchanged while avoiding duplicate work.
In `@v2/pkg/engine/resolve/loader.go`:
- Around line 440-455: cacheRequest() currently uses l.ctx.requestCache == nil
as the only “not started” check, so if CacheController.BeginRequest can
legitimately return nil it will be called repeatedly for the same request.
Update the lazy init in Loader.cacheRequest to track whether BeginRequest has
already been attempted with a separate started/initialized flag on the request
context instead of relying on requestCache being non-nil, and keep the
BeginRequest call itself in that path so the once-per-request contract is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba065fe5-dc2d-4729-afae-df2deab6fded
📒 Files selected for processing (135)
docs/caching/CODING_GUIDELINES.mddocs/caching/PLAN.mddocs/caching/PROGRESS.mddocs/caching/reviews/01-representation-variable-extraction.mddocs/caching/reviews/02-runtime-contract-and-loader-seam.mddocs/caching/reviews/03-planner-wiring-and-engine-config.mddocs/caching/reviews/04-test-infrastructure.mddocs/caching/reviews/05-provides-data-visitor.mddocs/caching/reviews/06-entity-cache-configuration.mddocs/caching/reviews/07-entity-l2-controller-core.mddocs/caching/reviews/08-multi-key-freshness-reorder.mddocs/caching/reviews/09-store-normalization.mddocs/caching/reviews/10-batch-entity-caching.mddocs/caching/reviews/11-negative-caching.mddocs/caching/reviews/12-shadow-mode.mddocs/caching/reviews/13-root-field-l2.mddocs/caching/reviews/14-root-field-isolation.mddocs/caching/reviews/15-entity-cache-reuse.mddocs/caching/reviews/16-optimize-l1-pass.mddocs/caching/reviews/17-l1-runtime-store.mddocs/caching/reviews/18-defer-concurrency-coverage.mddocs/caching/reviews/19-partial-fetching.mddocs/caching/reviews/20-art-observability.mddocs/caching/specs/2026-06-30-rfc-01-appendix-testing-strategy.mddocs/caching/specs/2026-06-30-rfc-01-loader-cache-abstraction.mddocs/caching/specs/2026-06-30-rfc-02-caching-planner.mddocs/caching/specs/2026-06-30-rfc-03-per-root-field-cache-isolation.mddocs/caching/tasks/01-representation-variable-extraction.mddocs/caching/tasks/02-runtime-contract-and-loader-seam.mddocs/caching/tasks/03-planner-wiring-and-engine-config.mddocs/caching/tasks/04-test-infrastructure.mddocs/caching/tasks/05-provides-data-visitor.mddocs/caching/tasks/06-entity-cache-configuration.mddocs/caching/tasks/07-entity-l2-controller-core.mddocs/caching/tasks/08-multi-key-freshness-reorder.mddocs/caching/tasks/09-store-normalization.mddocs/caching/tasks/10-batch-entity-caching.mddocs/caching/tasks/11-negative-caching.mddocs/caching/tasks/12-shadow-mode.mddocs/caching/tasks/13-root-field-l2.mddocs/caching/tasks/14-root-field-isolation.mddocs/caching/tasks/15-entity-cache-reuse.mddocs/caching/tasks/16-optimize-l1-pass.mddocs/caching/tasks/17-l1-runtime-store.mddocs/caching/tasks/18-defer-concurrency-coverage.mddocs/caching/tasks/19-partial-fetching.mddocs/caching/tasks/20-art-observability.mdexecution/cachingtesting/art_e2e_test.goexecution/cachingtesting/batch_e2e_test.goexecution/cachingtesting/cachingtesting.goexecution/cachingtesting/cachingtesting_test.goexecution/cachingtesting/compose.shexecution/cachingtesting/config.jsonexecution/cachingtesting/defer_l1_e2e_test.goexecution/cachingtesting/entity_config_test.goexecution/cachingtesting/entity_l2_test.goexecution/cachingtesting/entity_reuse_e2e_test.goexecution/cachingtesting/graph.yamlexecution/cachingtesting/isolation_e2e_test.goexecution/cachingtesting/l1_e2e_test.goexecution/cachingtesting/multikey_e2e_test.goexecution/cachingtesting/negative_e2e_test.goexecution/cachingtesting/normalization_e2e_test.goexecution/cachingtesting/partial_e2e_test.goexecution/cachingtesting/provides_data_test.goexecution/cachingtesting/rootfield_e2e_test.goexecution/cachingtesting/shadow_e2e_test.goexecution/cachingtesting/subgraphs/deals.graphqlexecution/cachingtesting/subgraphs/inventory.graphqlexecution/cachingtesting/subgraphs/products.graphqlexecution/cachingtesting/subgraphs/reviews.graphqlexecution/cachingtesting/subgraphs/users.graphqlexecution/engine/engine_caching_config_test.goexecution/engine/engine_config.goexecution/engine/execution_engine.gov2/pkg/engine/cache/cache_key_builder.gov2/pkg/engine/cache/cache_key_builder_test.gov2/pkg/engine/cache/cache_key_template.gov2/pkg/engine/cache/cachetesting/fakes.gov2/pkg/engine/cache/cachetesting/fakes_test.gov2/pkg/engine/cache/cachetesting/realish.gov2/pkg/engine/cache/configure_caching.gov2/pkg/engine/cache/configure_caching_test.gov2/pkg/engine/cache/controller.gov2/pkg/engine/cache/controller_batch_test.gov2/pkg/engine/cache/controller_l1_test.gov2/pkg/engine/cache/controller_negative_test.gov2/pkg/engine/cache/controller_rootfield_test.gov2/pkg/engine/cache/controller_shadow_test.gov2/pkg/engine/cache/controller_test.gov2/pkg/engine/cache/coverage.gov2/pkg/engine/cache/entity_reuse_test.gov2/pkg/engine/cache/fetch_cache_configurator.gov2/pkg/engine/cache/fetch_cache_configurator_rootfield_test.gov2/pkg/engine/cache/fetch_cache_configurator_test.gov2/pkg/engine/cache/multikey.gov2/pkg/engine/cache/multikey_test.gov2/pkg/engine/cache/observer.gov2/pkg/engine/cache/observer_test.gov2/pkg/engine/cache/optimize_l1_cache.gov2/pkg/engine/cache/optimize_l1_cache_test.gov2/pkg/engine/cache/partial.gov2/pkg/engine/cache/partial_test.gov2/pkg/engine/cache/transform.gov2/pkg/engine/cache/transform_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/plan/cache_provides_data_visitor.gov2/pkg/engine/plan/cache_provides_data_visitor_port_test.gov2/pkg/engine/plan/cache_provides_data_visitor_test.gov2/pkg/engine/plan/cacheconfig/cacheconfig.gov2/pkg/engine/plan/cacheconfig/cacheconfig_test.gov2/pkg/engine/plan/configuration.gov2/pkg/engine/plan/datasource_configuration.gov2/pkg/engine/plan/path_builder_visitor.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/representationvariable/representation_variable.gov2/pkg/engine/plan/representationvariable/representation_variable_test.gov2/pkg/engine/plan/root_field_isolation.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/resolve/cache_config.gov2/pkg/engine/resolve/cache_config_test.gov2/pkg/engine/resolve/cache_controller.gov2/pkg/engine/resolve/cache_controller_test.gov2/pkg/engine/resolve/cache_fetch_test.gov2/pkg/engine/resolve/cache_node_copy_test.gov2/pkg/engine/resolve/cache_noop_test.gov2/pkg/engine/resolve/cache_transaction.gov2/pkg/engine/resolve/context.gov2/pkg/engine/resolve/fetch.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/node_object.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/response.gov2/pkg/engine/resolve/subgraph_request_singleflight_test.gov2/pkg/engine/resolve/tainted_objects_test.go
Member
Author
|
Re the outside-diff comment and the nitpicks from the review body (addressed in 0e9e572):
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
execution/cachingtesting/loader_bench_test.go (1)
58-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
b.ResetTimer()calls.b.Loop()already handles benchmark timing, so these lines in both helpers can be removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@execution/cachingtesting/loader_bench_test.go` around lines 58 - 72, Remove the redundant benchmark timing reset from benchResolve, since b.Loop() already manages timing; update the benchResolve helper to drop b.ResetTimer() while keeping the resolve.New, b.ReportAllocs(), and ResolveGraphQLResponse loop unchanged. If there is a matching helper in the same benchmark file, apply the same cleanup there so both benchmark helpers are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@execution/cachingtesting/loader_bench_test.go`:
- Around line 58-72: Remove the redundant benchmark timing reset from
benchResolve, since b.Loop() already manages timing; update the benchResolve
helper to drop b.ResetTimer() while keeping the resolve.New, b.ReportAllocs(),
and ResolveGraphQLResponse loop unchanged. If there is a matching helper in the
same benchmark file, apply the same cleanup there so both benchmark helpers are
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae7c090a-e7f8-4d0c-ac43-8211f8df774d
📒 Files selected for processing (13)
execution/cachingtesting/art_e2e_test.goexecution/cachingtesting/cachingtesting.goexecution/cachingtesting/defer_l1_e2e_test.goexecution/cachingtesting/entity_config_test.goexecution/cachingtesting/isolation_e2e_test.goexecution/cachingtesting/loader_bench_test.gov2/pkg/engine/cache/cache_key_template.gov2/pkg/engine/cache/cachetesting/fakes.gov2/pkg/engine/cache/controller.gov2/pkg/engine/cache/partial.gov2/pkg/engine/plan/root_field_isolation_test.gov2/pkg/engine/resolve/fetchtree.gov2/pkg/engine/resolve/loader.go
🚧 Files skipped from review as they are similar to previous changes (7)
- v2/pkg/engine/cache/partial.go
- execution/cachingtesting/art_e2e_test.go
- execution/cachingtesting/cachingtesting.go
- execution/cachingtesting/defer_l1_e2e_test.go
- v2/pkg/engine/cache/cache_key_template.go
- v2/pkg/engine/cache/cachetesting/fakes.go
- v2/pkg/engine/cache/controller.go
Member
Author
|
Re the |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/cache/controller_batch_test.go (1)
168-195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse a fresh
RequestCachefor the post-EndRequestlookup.Sibling subtests (I1-I3) end a request via
primeBatch/rc.EndRequest()and then create a brand-newrc := newRC(store)for the follow-up assertions. This subtest instead reuses the samercafter callingEndRequest()on it (Line 180) to check the "next lookup" behavior at Line 192.EndRequestis documented incontroller.goas running "once, single-threaded, after the root tree and every defer group have resolved," so callingPrepareFetchon the same instance afterward relies on unspecified behavior rather than proving the entity was never persisted to L2.Proposed fix
- decisionB, handleB := prepare(t, rc, cfg, productItem(t, "1")) + rcB := NewController(store, nil).BeginRequest(nil) + decisionB, handleB := prepare(t, rcB, cfg, productItem(t, "1")) assert.Equal(t, resolve.DecisionFetch, decisionB) assert.False(t, handleB.Items[0].NegativeHit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/cache/controller_batch_test.go` around lines 168 - 195, The post-`EndRequest` assertion is reusing the same `RequestCache` instance, which depends on unspecified behavior after request finalization. In `controller_batch_test.go`, update the `t.Run("[I6] a null batch element writes nothing...")` flow to create a fresh `rc := newRC(store)` for the follow-up lookup, matching the sibling subtests and using `PrepareFetch` on the new cache instance before checking `NegativeHit`. This keeps the test focused on verifying persistence behavior through `store` rather than reusing finalized state from the original `RequestCache`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v2/pkg/engine/cache/controller_batch_test.go`:
- Around line 168-195: The post-`EndRequest` assertion is reusing the same
`RequestCache` instance, which depends on unspecified behavior after request
finalization. In `controller_batch_test.go`, update the `t.Run("[I6] a null
batch element writes nothing...")` flow to create a fresh `rc := newRC(store)`
for the follow-up lookup, matching the sibling subtests and using `PrepareFetch`
on the new cache instance before checking `NegativeHit`. This keeps the test
focused on verifying persistence behavior through `store` rather than reusing
finalized state from the original `RequestCache`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2e9ae5d-ad53-45ee-b0e2-c3b18ae4b379
📒 Files selected for processing (21)
execution/cachingtesting/cachingtesting.goexecution/cachingtesting/defer_l1_e2e_test.goexecution/cachingtesting/loader_bench_test.goexecution/cachingtesting/partial_e2e_test.gov2/pkg/engine/cache/cachetesting/fakes.gov2/pkg/engine/cache/cachetesting/fakes_test.gov2/pkg/engine/cache/controller.gov2/pkg/engine/cache/controller_batch_test.gov2/pkg/engine/cache/controller_l1_test.gov2/pkg/engine/cache/controller_shadow_test.gov2/pkg/engine/cache/fetch_cache_configurator.gov2/pkg/engine/cache/fetch_cache_configurator_test.gov2/pkg/engine/cache/multikey_test.gov2/pkg/engine/cache/optimize_l1_cache.gov2/pkg/engine/cache/optimize_l1_cache_test.gov2/pkg/engine/plan/cacheconfig/cacheconfig.gov2/pkg/engine/plan/root_field_isolation.gov2/pkg/engine/plan/root_field_isolation_test.gov2/pkg/engine/resolve/cache_config.gov2/pkg/engine/resolve/cache_controller.gov2/pkg/engine/resolve/resolve.go
🚧 Files skipped from review as they are similar to previous changes (13)
- v2/pkg/engine/cache/optimize_l1_cache_test.go
- v2/pkg/engine/cache/fetch_cache_configurator_test.go
- v2/pkg/engine/cache/multikey_test.go
- execution/cachingtesting/partial_e2e_test.go
- v2/pkg/engine/cache/cachetesting/fakes_test.go
- execution/cachingtesting/loader_bench_test.go
- v2/pkg/engine/cache/optimize_l1_cache.go
- v2/pkg/engine/cache/controller_l1_test.go
- v2/pkg/engine/cache/fetch_cache_configurator.go
- execution/cachingtesting/cachingtesting.go
- v2/pkg/engine/cache/controller.go
- execution/cachingtesting/defer_l1_e2e_test.go
- v2/pkg/engine/cache/cachetesting/fakes.go
Member
Author
|
Re the I6 nitpick: fixed — the post- |
Member
Author
|
@coderabbitai full review |
1 similar comment
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Unified PLAN.md with the maintainer-feedback revision (R2) folded in, one work-order task file per subtask (tasks/01-20), regenerated CODING_GUIDELINES.md, PROGRESS.md as the durable execution state, and the four RFC specs copied as background reference. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…resentationvariable package Problem: the @key -> representation-node builder was unexported in graphql_datasource; cache-key construction (task 06) needs it, and copying it would create two divergent @key walkers — the silent key-skew hazard the key model exists to prevent. Task file: docs/caching/tasks/01-representation-variable-extraction.md (PLAN.md phase 0). - Pure move of representation_variable.go to v2/pkg/engine/plan/representationvariable (new exported package); buildRepresentationVariableNode -> BuildRepresentationVariableNode, mergeRepresentationVariableNodes -> MergeRepresentationVariableNodes; visitor and merge helpers stay unexported; logic verbatim. - graphql_datasource refactored in place: both call sites (build + merge in buildRepresentationsVariable) now use the exported names; no copy remains. - Tests moved with the code and extended with one "with entity interface" case (entityInterface __typename OnTypeNames remap was previously uncovered). - No import cycle: representationvariable imports plan; plan does not import it. Verification: representationvariable + graphql_datasource + plan suites pass; full v2 suite (40 pkgs) and execution module (5 pkgs) pass; golangci-lint (v2.5.0, config minus modernize which the local version lacks) reports 0 issues; gci/gofmt clean. Reviewer guidance: diff should read as relocation + rename + doc comments, not rewrite; MergeRepresentationVariableNodes is exported for the datasource only — the task 06 cacheKeyBuilder will NOT call it (multi-key keeps candidates separate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the loader had no cache abstraction; the controller package and the planner passes need the runtime OUTPUT contract to exist and be wired as a strict no-op first, so every later task plugs into stable seams. Task file: docs/caching/tasks/02-runtime-contract-and-loader-seam.md (PLAN.md phase 0). Reviewer notes: docs/caching/reviews/02-runtime-contract-and-loader-seam.md (includes the backfilled reviews/01-*.md per the new per-commit reviewer-doc convention). - resolve/cache_controller.go: CacheController, RequestCache, Decision, PrepareFetchInput, MergeInput (incl. MergePath per D4 and all five write-gate signals), CacheObserver, FetchCacheHandle/ItemCacheState/CacheCandidate/ ShadowCacheEntry. - resolve/cache_transaction.go: TransactionBeginner + concrete CacheTransaction (D2; Begin takes DataBuffer.Lock once, Commit releases; arena ops only as methods on a held transaction). - resolve/cache_config.go: FetchCacheConfig (nil-safe Equals/String), CacheKeySpec, CacheKeyCandidate, CacheScope, CacheWriteReason, EntityKeyMapping/EntityFieldMapping. - fetch.go: Cache field on FetchConfiguration/EntityFetch/BatchEntityFetch; Fetch interface gains CacheConfig/SetCacheConfig/IsEntityFetch/ IsBatchEntityFetch (D8 — no switch over concrete fetch types anywhere in the new code); nil-safe cache clause in FetchConfiguration.Equals. - loader.go: cachePrepare/cacheMerge call sites in resolveSingle OUTSIDE the phase locks; lazy once-per-request cacheRequest under DataBuffer.Lock; full-hit sets skipLoad + fetchSkipped; result carriers response/responseData/ responseHasErrors assigned where mergeResult already computes them. - context.go / resolve.go: SetCacheController (mirrors SetAuthorizer), idempotent endCacheRequest deferred at all four entry functions, clone resets requestCache, Free tears down defensively. - Tests (dedicated files, full-value assert.Equal): Equals mutation table (every field) + P1-P5 dedup rows, exact String renders, fetch polymorphism table, observable no-op gates (real loader run; BeginRequest never called), lifecycle idempotency + clone isolation. Verification: full v2 suite (40 pkgs) ok; execution module (5 pkgs) ok; go test -race ./pkg/engine/resolve ok; golangci-lint (config minus modernize, absent in local v2.5.0) 0 issues; gci/gofmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the planner had no caching producer, no policy model, and no ProvidesData carrier; these must exist as additive wiring that produces NO config until caching configuration is supplied, with the engine Configuration as the only public entry point. Task file: docs/caching/tasks/03-planner-wiring-and-engine-config.md (PLAN.md phase 0). Reviewer notes: docs/caching/reviews/03-planner-wiring-and-engine-config.md. - NEW plan/cacheconfig (leaf; imports only time): CachingConfiguration + policy structs (no L1/L2 bools, D3) + CacheConfigProvider; the configuration itself implements the provider. The first-pass KeySpecs external key input is dropped per D10 (keys derive structurally in cacheKeyBuilder, task 06). - NEW engine/cache (D5/D6): Configurator.ConfigureCaching facade holding the single no-op gate; inert skeletons cacheKeyBuilder / fetchCacheConfigurator / optimizeL1Cache (bodies land in tasks 06/13/16). - postprocess: EnableCaching option + three thin ConfigureCaching calls (sync, defer incl. every defer-group tree via deferTrees, subscription), each after createConcreteSingleFetchTypes and before organizeFetchTree/buildDeferTree. - plan: Configuration.CacheConfigProviders (keyed by datasource ID); dataSourceConfiguration.Caching() accessor seam; cacheProvidesDataVisitor skeleton + the gated SECOND filter-free walk in Planner.Plan (D9; never re-runs the planning visitor, never rebuilds the plan). - resolve: Object.HasAliases, Field.OriginalName/CacheArgs (+CacheFieldArg) carried through Copy (CacheArgs cloned); GraphQLResponse cacheProvidesData side-table + accessors (off the response tree, unreachable by defer Copy). - execution/engine: Configuration.SetCaching(map[dsID]CachingConfiguration); NewExecutionEngine validates IDs (unknown id errors), force-enables FetchInfo (DisableIncludeInfo=false), builds providers+federation maps, threads postprocess.EnableCaching into every per-execution processor. Tests: facade no-op gate rows (full-tree equality, providers off/on-but-inert); provider lookup hit/miss rows for all four policy kinds; P1 gating + second-walk determinism (plan with inert caching == plan without, pretty-printed); Copy carrier + clone semantics; engine wiring on/off/unknown-id rows. Verification: full v2 suite (42 pkgs) ok; execution module (5 pkgs) ok; golangci-lint (config minus modernize) 0 issues in both modules; gci/gofmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: caching needs provably-composable test subgraphs, drift-proof real-planner inputs, and a shared fake set, so the loader glue and the controller reach high coverage without a network — built on the execution module and the federationtesting approach, with no golden snapshots (D7). Task file: docs/caching/tasks/04-test-infrastructure.md (PLAN.md phase 0). Reviewer notes: docs/caching/reviews/04-test-infrastructure.md. - NEW execution/cachingtesting fixtures: products (multi-key Product upc+sku, by-key root fields, sibling root fields, User.favoriteProduct), inventory (Product.stock/warehouse — mixed-TTL siblings vs products), reviews (batch entities), users (me/user(id)); composed with real wgc (config.json committed), cross-validated with rover. The defer fixture gap is closed and PINNED: TestDeferSupersetShape proves a defer group whose inventory entity fetch selects a strict subset of the initial fetch's selection. - NEW harness (execution/cachingtesting): Plan(tb, query, caching, responses) loads the committed config, wires caching like NewExecutionEngine (via the new exported Configuration.PlannerConfig() accessor), runs the REAL planner + postprocess (query plans always included for inline plan-shape asserts), and swaps transports for fakes; ResolveResponse drives the public sync entry. Caching/response keys use subgraph NAMES (translated to ID-named datasources). - NEW v2/pkg/engine/cache/cachetesting fakes: FakeCacheController, FakeRequestCache (normalized Call log incl. MergePath, scripted decisions, handle identity), RecordingController, FakeStore (absolute ExpiresAt, ordered StoreOp log), GatedDataSource (gate channels), RecordingObserver, FakeRegistry + SwapDataSources. No custom clock: TTL/time via testing/synctest only. - Fetch.SetDataSource added to the Fetch interface (all concrete types): the first-pass swap util switched over concrete fetch types — the D8 pattern this port removes. - NOT ported from the first pass: RealishCache/Mode/CacheStage/storeAdapter (they need the task-07 controller; would be dead code today) and all goldens. Tests: no-op e2e baseline (byte-identical with a recording controller set and caching unconfigured; zero BeginRequests), six fixture smoke rows with complete response bodies, defer-superset plan pin, fake self-tests (TTL expiry in a synctest bubble, gate ordering via synctest.Wait, full Call-log round-trip, registry fallback + load counts). Verification: wgc + rover composition clean; full v2 suite (43 pkgs) ok; execution module (6 pkgs) ok; golangci-lint (config minus modernize) 0 issues in both modules; gci/gofmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the runtime coverage walk needs, per fetch, the exact field tree the
fetch returns — alias-aware and argument-aware; deriving it from the merged
response tree is lossy (per-fetch attribution lost, arg-blind).
Task file: docs/caching/tasks/05-provides-data-visitor.md (PLAN.md phase A).
Reviewer notes: docs/caching/reviews/05-provides-data-visitor.md.
- Full port of the ProvidesData builder into cacheProvidesDataVisitor (from the
first-pass port of the OLD builder): entity-boundary reset, __typename dedup,
inline-fragment OnTypeNames (union/interface expansion with grandparent
narrowing, interfaceObject remap), alias->OriginalName, sorted CacheArgs
(variable-bound only; root-operation-field args excluded).
- D9 seam completed in planner.go: the gated SECOND filter-free walk receives
operation/definition/config/planners/fieldPlanners (complete, because the
main walk populated fieldPlanners in LeaveField) and attaches the
map[*FetchInfo]*Object side-table via SetCacheProvidesData, planner IDs
sorted for determinism. Deviation from the first pass: P1 is NOT additionally
registered on the main walk (wasted work; reset() discarded it).
- ComputeHasAliases deliberately deferred to task 06 (its first caller).
Tests: fidelity rows ported 1:1 + a NEW inline-fragment row; adversarial rows
beyond the OLD set (irrelevant provider no-leak; partial overlap with per-fetch
trees; empty selections: boundary-only planner => EMPTY tree i.e. zero
coverage, unrouted planner absent); determinism (same op twice, identical
side-tables); execution-module fidelity over REAL plans (full side-table for a
root+batch-entity plan; the DEFERRED inventory fetch owns its {stock} entry).
Verification: full v2 suite (43 pkgs) ok; execution module (6 pkgs) ok;
golangci-lint (config minus modernize) 0 issues; gci/gofmt clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntity arm) Problem: entity fetches carried no cache config — no keys, no coverage tree, no policy; federation @key data must cross into runtime config exactly once, by value, in one reviewable unit. Task file: docs/caching/tasks/06-entity-cache-configuration.md (PLAN.md phase A). Reviewer notes: docs/caching/reviews/06-entity-cache-configuration.md. - cacheKeyBuilder.buildEntitySpec (the SOLE federation reader): one candidate per resolvable @key set via the shared representationvariable builder, deterministically ordered by selection-set string, copied out by value. HARDENING vs the first pass: candidates whose representation carries no field beyond __typename are rejected as malformed (unknown-field @keys silently degrade to __typename-only nodes — a cross-entity key-collision hazard); zero usable keys => not cached. - fetchCacheConfigurator entity arm: walks the finished flat trees, and for entity/batch-entity fetches (D8 predicates, no type switch — the first-pass fetchIsEntity switch was not ported) assembles FetchCacheConfig: L1=true (eligibility; task 16 narrows), L2 = TTL>0 || NegativeCacheTTL>0 (D3), scalar policy fields, KeySpec, ProvidesData from the task-05 side-table (the tree itself, not a copy) with ComputeHasAliases folded in; SetCacheConfig via the Fetch interface; nil on no provider / no policy / no key / nil info / all-flags-false (the last unreachable for entities, serves task 13). - resolve.ComputeHasAliases lands with its first caller (deferred from task 05). Tests: full CacheKeySpec literals (single, composite+nested, MULTIPLE sets sorted), malformed-candidate skip + all-broken bail, the ALIASING GATE (mutate source federation after building), builder-node == datasource representation node; full FetchCacheConfig on entity + batch fetches, zero-TTL L1-only row, five nil rows; plan-level rows over REAL sync AND defer plans (deferred-group inventory fetch configured too) + determinism. Verification: full v2 suite (43 pkgs) ok; execution module (6 pkgs) ok; golangci-lint (config minus modernize) 0 issues in both modules; gci/gofmt clean; no freeze/stamp vocabulary in the cache package (D6). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the defer x caching interaction is where the first pass could not complete its proofs — cross-defer-group L1 SERVING was never proven end to end. The task-04 fixtures carry the required shapes; this lands the proofs (N1-N4, M3, M5) plus the one runtime fix they flushed out. Task file: docs/caching/tasks/18-defer-concurrency-coverage.md (PLAN.md phase D). Reviewer notes: docs/caching/reviews/18-defer-concurrency-coverage.md. - FLUSHED-OUT FIX: the narrowing pass learns defer-group ANCESTRY as an ordering source — ConfigureCaching takes treeParents (postprocess derives them from DeferDescriptors.ParentID, mirroring deferTrees) and treeEncloses generalizes root-before-defers: ancestor-tree fetches execute before every descendant-tree fetch (the resolver resolves a parent group fully before its children); SIBLING groups stay unordered. Without it, N2's nested consumer (no dependency edge to the deferred provider) was narrowed off. - Harness: ResolveDeferResponse over a frame-capturing writer (optional Flushed signal channel for gate-based frame ordering) + PlanResult.Gate. - N1+M3: initial-fetch entity serves the DEFERRED group — plan inspected (the group really carries a configured same-entity fetch), both frames pinned as complete strings, deferred subgraph never hit (tampered canned response), zero store ops, ONE BeginRequest across groups. The first-pass carry-forward gap is CLOSED. - N2: a deferred fetch populates L1 served to the NESTED later group — ancestry-only ordering (subset-nested @defer is normalized away, so the fixture routes through the reviews hop back to the same Product). - N3: exactly one EndRequest; the single flush carries the initial AND the group's L2 writes (all Gets before all Sets, values pinned). - N4: a SkipFullHit sibling flushes while the other sibling is gated mid-Load — pure channel synchronization (synctest bubbles deadlock on engine-lifetime goroutines; documented), zero latency dependence. - M5: a cache-hook error lands in THAT group's completed entry as a frame error; the sibling's frame is complete and unaffected. N5/M4 are covered by -race across the rows plus the task-02 loader seam tests. Verification: full execution harness -race clean; v2 cache/postprocess -race clean; full v2 + execution suites ok; golangci-lint (config minus modernize) 0 issues in both modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: a fetch was all-or-nothing — one uncovered batch item refetched
everything. Partial fetching serves what the cache has, sends the subgraph
ONLY the missing representations, and realigns the reduced response to the
original positions.
Task file: docs/caching/tasks/19-partial-fetching.md (PLAN.md phase E).
Reviewer notes: docs/caching/reviews/19-partial-fetching.md.
LOADER TOUCHES (the sanctioned beyond-seam changes, each explained):
(1) cachePrepare swaps prepared.input for handle.PartialInput on FetchPartial
(single-flight dedups on the reduced input) and sets res.cachePartial;
(2) the res.cachePartial bool;
(3) mergeResult returns after response/error processing for partial fetches —
the reduced response is positionally misaligned with batchStats, so the
cache hook owns the data merge (error rendering/parsing unchanged);
(4) cacheMerge dispatches FetchPartial to OnFetchResult (the task-02 seam sent
it to OnFetchSkipped, which could splice but never write fetched values).
Cache side (all partial semantics in the separable cache/partial.go):
- filterBatchInput: best-effort representations filtering in bucket order;
any unexpected shape falls back to a FULL fetch — never a wrong request.
- onPartialBatchResult: ONE hook, one transaction — splice covered buckets
(identical duties to OnFetchSkipped via the extracted spliceCachedItem,
incl. the negative splice-nothing rule), realign the reduced _entities in
fetched-bucket order, merge into every target, write via the extracted
writeFetchedValue. Dispatches BEFORE the failure gate: a failed partial
fetch keeps the covered splice and skips only the fetched subset.
- Gates: EnablePartialCacheLoad (entity policies) or PartialBatchLoad
(root-field policies); shadow wins; all-hit/all-miss degenerate to
SkipFullHit/Fetch; knob off = byte-for-byte all-or-nothing.
- Per-field partial expiry delivered as mixed-TTL semantics ACROSS fetches
(per-request query rewriting is out of scope — interpretation documented).
- cachetesting: exact-input recording (RecordInput/Inputs) for the
"exactly the missing representations" acceptance criterion.
Tests: filterBatchInput byte-exact + fallbacks; partial split with exact
reduced input; realign with full merged targets and fetched-only writes; the
adversarial set (duplicated representations, all-hit, all-miss, single
element); failure-in-fetched-subset; config gate; shadow-wins; e2e batch
partial (recorded subgraph input asserted; canned response only matches a
reduced request) and mixed-TTL expiry (only the expired subgraph refetched);
-race clean; every earlier task's row passes unchanged.
Verification: full v2 + execution suites ok; golangci-lint (config minus
modernize) 0 issues in both modules.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: operators need to SEE caching behave — the playground (via ART) must surface every caching aspect per fetch without the observability concern leaking back onto the lookup/write surface (the coupling that sank the OLD implementation at ~461 call sites). Task file: docs/caching/tasks/20-art-observability.md (PLAN.md phase E, final task). Reviewer notes: docs/caching/reviews/20-art-observability.md. - Production cache.TraceObserver: assembles a per-fetch CacheTrace from the opaque FetchCacheHandle at EndRequest (single-threaded, no lock, no arena; endCacheRequest defers before resolve returns, so the router's trace serialization always sees it) and attaches it to the fetch's existing ART trace — DataSourceLoadTrace gains the additive "cache" JSON section: decision, per-item served_from (l1|l2), rendered keys (hashed under HashAnalyticsKeys — full-key xxhash64), exact remaining TTLs, write reasons (refresh/backfill), negative hits, pending candidates, shadow compares with CacheAge. - ZERO observer calls outside the controller: registerHandle (centralizing four duplicated registration sites) stamps the handle's Trace destination + HashAnalyticsKeys; EndRequest calls OnFetchObserved per handle. Accumulation rides existing state: ItemCacheState.ServedFromLayer stamped at the serve points (cleared by the shadow stash — nothing was served) and WriteReason stamped at the write sites; the per-item helpers now take pointers so the stamps persist. - CompareShadow computes compares EAGERLY (nothing arena-owned survives the transaction) keyed per handle under the observer's own mutex (one instance, many requests); OnFetchObserved drains entries even with tracing off. - Scope guard: OnEntity/OnFieldValue stay no-ops; export pipelines follow up. Tests: 8 observer unit rows with COMPLETE CacheTrace asserts (synctest pins the 40s remaining TTL and 15s CacheAge exactly; HashAnalyticsKeys both ways; tracing-off drains); ART e2e over real plans (L2 miss->hit, L1 chain hit, shadow compare, partial batch — complete cache JSON sections pinned with real-clock TTLs normalized) plus the tracing-off regression row; -race clean; the untouched full suites are the byte-identical no-op proof. Verification: full v2 + execution suites ok; golangci-lint (config minus modernize) 0 issues in both modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… += in a loop CI's modernize analyzer (stringsbuilder) flags string concatenation in loops; the local lint config lacks that analyzer, so this surfaced only on CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Negative sentinel authority (CRITICAL finding): only the FRESHEST @key candidate decides a negative hit — a staler sentinel on a sibling key no longer overrides a fresher positive value, and sentinels never enter the positive selection ladder; regression rows added both ways. - Fetch type-switch removed: resolve.Fetch grew LoadTrace() (implemented by the three concrete fetch types + test stubs); the controller and the ART e2e helper use it, per CODING_GUIDELINES. - covers() now gates type-conditioned fields on the cached __typename: a concrete-type value no longer misses coverage over a SIBLING type's fields (absent __typename stays conservative); unit rows added. - optimizeL1Cache contribution gate is recursive: nested NAME-only overlap no longer counts as contribution; flaw-pin row added. - Partial batch: a reduced _entities count mismatch is now a surfaced contract violation (short and over-long rows added), mirroring the loader's own invalidBatchItemCount check. - writeFetchedValue labels pure-backfill writes as "backfill" in the item trace. - Field.Copy() preserves ParentOnTypeNames (outside-diff finding). - Harness: Inputs/Gate fall back to the literal subgraph name like LoadCount; N4 channel waits carry failsafe timeouts (regression-only, never ordering); normalization e2e key assert de-tautologized (write key == read key). - TraceObserver emits ShadowCompares in deterministic item order. - Docs/comments: renderRepresentationValue comment now states the ACTUAL behavior (number/string unification; 1 vs 1.0 split conservatively — full float canonicalization deliberately avoided); RFC-02 bare fences tagged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e rows The key hashes are deterministic (xxhash64 over canonical preimages), so the whole cache trace sections pin as full-value assert.Equal literals instead of substring checks — consistent with the other ART rows and the repo's full-value assertion rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Package-level query/expected consts and response helper funcs (batchQuery, entityL2Query/Expected/Responses, l1ChainQuery/Expected/Responses, batchResponses) forced readers to jump around; every test now defines its query, responses, and expected body immediately before the Plan call and reuses the locals for repeat requests. Duplication across tests is accepted by design — each test reads top to bottom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ops in isolation Multi-request tests accumulated the fake store's op log, so later assertions re-listed earlier requests' ops and readers had to diff mentally. FakeStore gained ResetOps() (clears the LOG, keeps the data); the v2 unit tests use the existing store.ops = nil idiom. Every cross-request op assertion now states exactly what THAT request did (e.g. an L2 hit is ONE Get, a full-batch hit is one Get per entity), and the L1 mode-matrix second request gained a previously missing exact op assert (sku-hit Get, upc L2 Get, pending-candidate backfill Set). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The isolation, entity-config, and defer-L1 plan tests asserted hand-rolled
per-fetch summary strings (renderFetchIsolation/renderFetchCacheConfigs),
which hid the plan structure. The engine's query-plan printer now emits an
additive 'Cache: {...}' line per fetch when a cache config exists (byte-
identical output when caching is off; the config reads through the Fetch
interface), the harness gained PrettyPlan (initial tree + every defer group,
one string), and every plan assertion pins the COMPLETE pretty-printed plan —
structure, queries, representations, and cache configs in one readable blob.
The two custom renderers are gone. As a side effect the router's query-plan
output (JSON field "cache" + pretty line) now surfaces cache configs to the
playground.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The predicate was covered only indirectly through the plan-level isolation tests in execution/cachingtesting; these rows pin each gate condition next to the file: cached query root isolates; no-providers never isolates (the provable no-op); mutation/subscription/nested parents decline; missing provider and missing coordinate policy decline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BenchmarkLoader measures every fetch type over real plans with a logging-free in-memory store (benchStore): entity/batch/root-field/L1-chain, each as no-cache baseline, steady-state hit, miss+write, and batch-partial — one resolver and controller across iterations (the router's production mode), fresh Context per request, b.Loop(). pprof (alloc_space, memprofilerate 512-1024) attributed ~28% of the chain hit path's allocations to PrepareFetch. Fixes: - cacheKeyTemplate.render writes the canonical key JSON DIRECTLY to a byte buffer instead of building intermediate astjson objects per candidate; the preimage bytes are identical to the former marshal form (the pinned key hashes in the e2e suites prove parity), render's share dropped from ~11% cum to ~4%. - The controller's four per-handle side maps (configs/prefixes/missedKeys/ reuseProvides) collapsed into ONE lazily allocated states map with a handleState struct — BeginRequest allocates no maps at all now. - cachetesting.FakeRegistry caps input recording at 16 per datasource (the recorder copies only under the cap), removing test-double noise from every measurement. Results (Apple M4 Max, 20000x): cache-attributable overhead on the entity L2 hit path fell from +33 to +18 allocs/op vs the no-cache baseline, the L1+L2 chain from +139 to +86 (353 -> 292 total allocs/op, -3.2KB/op). Remaining cache-side costs are inherent (parsing the hit bytes, one map insert + one state struct per cached fetch); the residual profile is dominated by loader input rendering and response walking, which predate caching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every ART subtest now pins the COMPLETE DataSourceLoadTrace of every fetch — raw input, rendered input, output, single-flight flags, load stats, and the cache section — as one indented, path-labeled JSON blob per request (real-clock durations and TTL/age nanos normalized; exact values pinned by the synctest unit rows). This surfaced a trace gap: a cache full hit skipped the load without reporting it — cachePrepare now sets Trace.LoadSkipped like every other skip path, so ART shows load_skipped=true alongside the cache section's SkipFullHit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four parallel agents each owned a disjoint package group, ran Codex (gpt codex-cli) as an independent reviewer, fixed valid findings with fail-first test rows, and had Codex re-review their diffs to a CLEAN verdict. Runtime core (engine/cache controller/partial/multikey/observer + doubles): - A null _entities element in a full-batch response was cached as a positive-TTL negative sentinel, bypassing the NegativeCacheTTL knob — writeFetchedValue now guards TypeNull (aligned with the partial path); I6 row. - A shadow L2 hit populated the shared L1 BEFORE the stash cleared it, so an L1-only sibling policy could serve a never-served probe value — populateL1 is now gated on !ShadowMode; row pins both reads probing L2. - Sentinel filtering misaligned the parsed list with FromCacheCandidates, making the older-single ladder rung report the wrong SelectedRemainingTTL — nil placeholders keep positional alignment; 3-candidate synctest row. - spliceCachedItem could enqueue L2 writes under L1-only configs (nil-store panic at EndRequest) and marshaled unconditionally on pure hits — useL2 gate + no-duties early return; zero-store-ops row. - FakeRegistry.LoadCount returns -1 for never-swapped name/path pairs so typo'd assertions fail loudly (counters created eagerly at swap). Plan side (visitor/isolation/configurator/optimize/cacheconfig): - An INERT root-field policy (no TTL, no shadow) still split planners while the configurator dropped its config — plans changed with zero caching enabled. Gate now requires cacheconfig.RootFieldCachePolicy.EnablesCaching() (TTL > 0 || ShadowMode, the configurator's exact effective-enablement). - Abstract-path entity fetches can carry MIXED entity types; policy and key spec derived from RootFields[0] would mis-key the other types — the entity arm now declines caching on mixed TypeNames (mirror of the root-field all-or-nothing rule). - treeEncloses looped forever on a malformed treeParents cycle — walk bounded by tree count. Resolve surfaces: comment/contract fixes only — EndRequest's post-arena- release heap-data-only contract is now documented at the interface, the ItemCacheState arena-ownership warning added, guard comments at the three arena entry defers, the OnFetchSkipped/OnFetchResult partial-dispatch docs corrected, and the FetchCacheConfig.Equals scope reasoning recorded (dedup runs before ConfigureCaching; Input identity pins alias/arg structure). Execution harness: PlanResult fails tests on unknown subgraph names (no more vacuous LoadCount/Inputs/Gate); translateResponseKeys rejects key collisions; TestDeferHookErrorIsolation rewritten with gate-ordered, fully pinned frames; remaining assert.Contains converted to full-value pins; redundant b.ResetTimer() before b.Loop() removed (CodeRabbit nitpick). Refuted with reasons (in agent transcripts): failed-partial write-backs of covered buckets (served-value bytes, per the task-19 contract); partial count-mismatch "bypass" (validated cache-side both directions); Equals field coverage (no live trigger); interface-object type-name mirror; CacheArgs inline-literal precondition; entity-boundary reset (under-reports only). All fixes verified: both modules test- and race-clean, golangci-lint (config minus modernize) 0 issues, all 10 loader benchmarks pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reusing the finalized instance kept its L1 map alive and relied on unspecified post-EndRequest behavior; a fresh request proves the missing entity was never PERSISTED (its empty L1 forces the lookup through L2), matching the sibling I-rows' structure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g suite ConfigureCaching in the defer arm now runs AFTER organizeFetchTree and buildDeferTree: the group trees and their ancestry come from the AUTHORITATIVE DeferTree the resolver executes (collectDeferCachingTrees walks Single/Sequence/Parallel nodes — a Sequence's first child is the parent group, Parallel children share their enclosing parent, fetchless groups attach children to the nearest fetch-bearing ancestor, and the flat Defers list is the fallback when the tree is not built). This deletes the parallel deferTreeParents derivation from DeferDescriptors.ParentID — one source of truth instead of two that could drift. NEW postprocess-level caching suite (the pipeline had no caching/defer coverage at this level): the FULL Processor over synthetic plans pins - the sync arm configuring a converted EntityFetch (full config string), - the no-op gate (EnableCaching with empty providers produces a plan EQUAL to a plain processor's), - the defer-ancestry semantics end to end through the pipeline: the same L1-only provider/consumer pair KEEPS L1 when the consumer's group is nested under the provider's and is narrowed off (configs re-nil'd) when the groups are unordered siblings, - collectDeferCachingTrees directly: single root, siblings, nested chains, the not-built fallback, and fetchless-parent reattachment. Test-authoring note baked into the suite: DisableResolveInputTemplates also disables createConcreteSingleFetchTypes, so the entity fixtures carry a real ResolvableObjectVariable and run the untrimmed pipeline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ck filtering The partial-batch path rendered the FULL input, then parsed it back, filtered the representations array, and re-marshaled (cache/partial.go filterBatchInput). The filtering now happens while the data is still astjson: - prepareBatchEntityFetch renders header/separator/footer into their own buffers and keeps each UNIQUE representation's rendered SEGMENT (bucket order) instead of building the final input inline; - the cache decision runs on the buckets, and the controller marks the still-missing ones on the new opaque handle.BatchFetchKeep (PartialInput is gone, and with it the input-shape dependence and its fallback); - assembleBatchInput then renders the FINAL input exactly once — header + kept segments + footer — and runs the undefined-variables post-processing and pre-fetch validation on the final bytes. A full fetch assembles ALL segments, byte-identical to the old inline build (the batch e2e suite and the partial e2e's exact recorded-subgraph-input pin prove both paths). BenchmarkLoader/batch/partial-hit: 253 -> 208 allocs/op, -2.5KB/op, ~-8% time; the full-batch paths are unchanged within noise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The caching ProvidesData walk reused the finished planningWalker via ResetVisitors + SetVisitorFilter(nil) and seven field-by-field assignments in planner.go. The visitor now owns a walk() entry that creates its OWN walker (the planning walker keeps its visitors and filter untouched), takes all the state in one call, and runs after every planning walk — fieldPlanners is complete at that point. planner.go shrinks to a single gated call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The response-level e2e tests drove a hand-rolled plan+resolve pipeline; they
now run the REAL engine end to end (the federation_integration_static_test.go
pattern): a new engine.WithCacheController execution option attaches the
runtime controller per request (the counterpart of Configuration.SetCaching,
which was already wired but had no runtime hook), and the harness gained an
engine half (enginetesting.go) — httptest subgraph doubles with body-routed
canned responses, per-double request counters and exact recorded bodies, over
the committed federation config with datasource URLs rewritten to the doubles.
Converted (14 tests across 10 files): batch, negative, normalization,
multikey, rootfield, entity-reuse, shadow, partial, the entity-L2 and
isolation serving tests, and the three L1 rows. Notable semantics:
- the engine renames user variables during normalization, so canned routing
matches rendered bodies ("variables":{"a":1}) and per-fetch-path LoadCounts
became per-rule request counts (same strictness, real transport);
- the partial e2e now pins the COMPLETE reduced subgraph request body
recorded by the reviews double — the representations-filtering proof over
real HTTP;
- one engine per caching configuration, reused across requests (production
plan-cache behavior); TAMPERED responses keep their must-not-serve roles;
- TestEntityL2EndToEnd dropped its LoaderHooks assertions (no engine hook
option; the C7 contract stays pinned by the resolve-level suites).
Plan()/ResolveResponse remain ONLY for what the engine encapsulates: plan
pins (pretty-printed plans), ART trace internals, defer frames, gated
in-process ordering, and the loader benchmarks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Load stats are HTTP connection-timing noise that only bloats the trace pins and golden files — the tests never assert on them. ExcludeLoadStats also drops the redundant per-fetch duration fields at the source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
batchInputAssembly moves to its own file with a single entry point — assemble(arena, keep) returns the final input bytes — and drops the info/fetchItem fields the loader already carries via prepared.item. The loader keeps only orchestration (tracing-skip branch, pre-fetch validation). Unit tests pin the exact final bytes for every keep shape, including separator placement and the undefined-variables post-processing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ART trace extension serializes DURING Resolve, while EndRequest — where the observer used to assemble each fetch's CacheTrace — runs after the response has been written. A trace rendered with the response (extensions.trace) therefore never carried the cache sections. resolve gains the optional CacheTraceFlusher extension; the request cache implements FlushTraces() idempotently (per-handle observed flag, EndRequest reuses the same pass), and both sync resolve paths flush right before rendering when the trace ships in the response extensions. Each handle is observed exactly once either way, pinned by TestFlushTracesBeforeEndRequest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # v2/pkg/engine/resolve/resolve.go
Every executing caching test now runs the federation-integration pattern: a real ExecutionEngine over httptest subgraph doubles, queries executed like a client, and full-response assertions. ART tests pin the entire body including the complete extensions.trace cache sections (ExecuteTraced), defer tests pin complete frame slices with ordering driven by gated subgraph responses (ExecuteDefer + SubgraphRule.Gate), and plan-shape tests pin the queryPlan response extension carrying the canonical one-line cache configs (ExecutePlanned via the new WithIncludeQueryPlanInResponse option). Kept on the plan harness, each with a documented rationale: the defer-plan pins and ProvidesData tests (deferred-group plans and plan-internal objects are not client-visible) and the loader benchmarks (HTTP would drown the hot-path measurements). The orphaned resolve-side helpers (ResolveDeferResponse, deferFrameWriter, resolveWithContext) are removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ysmolski
force-pushed
the
caching-on-defer-v2
branch
from
July 7, 2026 11:45
79d06ea to
10be522
Compare
Contributor
|
I have rebased it on top of master (with defer) |
ysmolski
self-requested a review
July 13, 2026 07:56
6 tasks
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
2 participants
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.
What this PR adds
A clean re-implementation of L1/L2 entity and root-field caching on top of the defer engine, executed as 20 reviewable task commits.
The old implementation (reference:
caching-base) tangled caching intoloader.go(~461 call sites); this port keeps the loader surface to a handful of explained seams and concentrates all caching semantics in a dedicatedv2/pkg/engine/cachepackage.Architecture (one paragraph)
The planner side derives per-fetch
FetchCacheConfigvia additive passes (cacheProvidesDataVisitor,cacheKeyBuilder,fetchCacheConfigurator,optimizeL1Cache) wired through a singlepostprocess.ConfigureCachingfacade; the runtime side is aCacheController/RequestCachepair the loader talks to through two hooks (PrepareFetchbefore the network,OnFetchSkipped/OnFetchResultafter the merge) with an opaqueFetchCacheHandlein between — a nil controller or nil config is a zero-cost no-op, and all arena work rides oneCacheTransaction(oneDataBuffer.Lockacquisition per hook).Feature checklist
@keybest-effort candidates, freshest-first selection, union merge, write-back/backfill, coverage validation againstProvidesDataBatchIndex)EntityKeyMappings)*astjson.Value, zero marshaling, keys shared with L2) with cross-defer-group servingcachesection onDataSourceLoadTrace, assembled bycache.TraceObserverwith zero observer calls outside the controllerReview aids
docs/caching/PROGRESS.md— the task board with per-task commit hashes and the decision logdocs/caching/reviews/01-20-*.md— one reviewer document per task commit: decisions, implementation summary, what to look into, verification evidencedocs/caching/PLAN.md,docs/caching/specs/— the RFCs and the phased plan this executesVerification
v2/pkg/engine/cache,execution/cachingtesting— real wgc-composed fixtures, real planner, in-process fakes;testing/synctestfor all TTL rows, gate channels for ordering, no latency-based tests)-race-clean;golangci-lint0 issues in both modules at every task commit🤖 Generated with Claude Code
Closes #1259