Repository navigation
Fix defer parentNodeId resolution and pooled path aliasing in operation plan traces - #10291
Merged
Merged
Conversation
…e variable-set paths aliasing pooled arrays
Contributor
Patch coverage88.6% of changed lines covered (839/947)
Uncovered changed lines (JSON){
"sha": "f34b125455438e31379fb8476100c5efefc1e502",
"files": [
{ "path": "src/HotChocolate/Fusion/src/Fusion.Execution/Text/Json/CompactPathSegment.cs", "ranges": [[66, 66], [68, 71], [73, 73]] },
{ "path": "src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.cs", "ranges": [[265, 270], [277, 279]] },
{ "path": "src/HotChocolate/Fusion/src/Fusion.Execution/Planning/DeferOperationRewriter.cs", "ranges": [[383, 384], [394, 396], [418, 421], [423, 423], [426, 429], [431, 431], [534, 534]] },
{ "path": "src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.BuildExecutionTree.cs", "ranges": [[206, 206], [265, 266]] },
{ "path": "src/HotChocolate/Fusion/src/Fusion.Execution/Planning/OperationPlanner.Defer.cs", "ranges": [[224, 225], [261, 264], [280, 280], [325, 326], [331, 331], [421, 421], [423, 423], [531, 531], [533, 533], [602, 603], [606, 610], [691, 692], [718, 719], [775, 776], [816, 817], [827, 828], [843, 844], [882, 883], [887, 888], [896, 897], [946, 949], [979, 980], [984, 986], [1110, 1112], [1163, 1166], [1168, 1172], [1175, 1175], [1425, 1426], [1538, 1539], [1546, 1547], [1597, 1598], [1670, 1671], [1733, 1733], [1763, 1764]] }
]
}Project coverage: 57.5% (283895/493493 lines) |
Anchored inside a parent result (a list item, an object), a deferred fragment was planned by re-seeding a fresh operation rooted at Query and re-fetching the whole path from root: nothing tied the deferred selection to the parent fetch beyond position, lists were fetched twice, and mixed-subgraph deferred selections silently dropped same-subgraph fields (OperationPlanner.PlanIncrementalPlan always seeded via CreateQueryPlanBase; ApplyDeferRequirementsToParent assumed a single self-fetch step at index 0). PlanIncrementalPlan now seeds a Lookup work item rooted at the anchor's own type for any non-root-anchored defer (falling back to a NodeFieldWorkItem for a `node(id)` anchor, and to the prior root re-fetch when the anchor type has no reachable lookup at all), letting PlanQueue pick a keyed lookup off the parent's own key. PlanQueue needed one narrow addition: EnqueueLookupPlanNodes's self-reentry exclusion could leave a schema that owns both a list field and the keyed lookup for its own item type with zero candidate plans, since the parent-path walk was skipped along with the lookup attempt; the walk is now still tried for that one case, since it is a plain field re-fetch, never a lookup call. ApplyDeferRequirementsToParent no longer assumes the self-fetch is step 0: it finds every step that exists purely to relay a key value toward the deferred selection (target at the anchor or a bootstrapped root re-fetch, every field it selects consumed by its own dependents), routes each through the existing parent-scope walker, and sees through a chain of such relays to the step that actually grounds the value instead of promoting an intermediate pass-through hop. Pins five new planner shapes (list anchor with a sibling lookup on multiple root fields, same+foreign subgraph split, no-lookup object anchor, root anchor, and a key-only defer where the only lookup is self-cyclic) plus two new end-to-end tests, and updates existing defer snapshots whose defer-operation numbering shifted under the new seeding order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MHsog5kcxQ2rMZ2LiYCuSs
michaelstaib
requested changes
Aug 25, 2026
michaelstaib
left a comment
michaelstaib
left a comment
Member
There was a problem hiding this comment.
I am working on changes ... so do not merge
RemoveReroutedInPlanEdges used to sever a provider-to-dependent edge wholesale whenever any one of the dependent's requirements was rerouted to the parent scope, even when another requirement from the same dependent still needed that provider. Sever the edge only when none of the dependent's remaining requirements still resolves back to the same provider, and keep dropping only the rerouted field once nothing else consumes it. Adds a regression test covering a downstream step with two requirements from the same in-plan provider where only the one at the defer anchor path reroutes to the parent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2TfmiTonT2kyPi3S6Rjjy
Reduce the multi-sentence design-justification comments in PlanQueue's self-reentry candidate-schema check to 1-2 sentences that state what the code does, and trim the matching three-line arrange/assert descriptions in the two newest DeferTests to a single line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2TfmiTonT2kyPi3S6Rjjy
BuildIncrementalPlanOperation forced OperationType.Query on every incremental plan operation, so a mutation-anchored defer's internal operation lied about its own root type. That broke root-type resolution for BuildIncrementalPlans/AddTypeNameToAbstractSelections and made PlannerExtensions.GetPossibleLookupsThroughPath treat the mutation's own root field as reachable through Query, producing a KeyNotFoundException instead of a keyed lookup. Fix: - DeferOperationRewriter no longer forces the incremental operation to Query; it keeps the original operation type, which is all BuildIncrementalPlans and the executor's delivery-path walk need. - A defer wrapping the operation's own root field(s) (Path.IsRoot) on a mutation has no entity to key a lookup off, so planning now throws DeferredMutationLookupRequiredException instead of silently forcing Query and reaching a KeyNotFoundException deep in the query-root machinery. - A defer anchored inside a mutation result's own object graph cannot walk back to the operation root for a lookup key the way a query-anchored defer does: GetPossibleLookupsThroughPath refuses that walk for non-Query operations to avoid re-running the mutation. The incremental plan is instead seeded like the main plan's own mutation root fetch (CreateMutationPlanBase), so the key field is resolved directly on the mutation's own step; the resulting key-only producer step is still recognized and dropped by ApplyDeferRequirementsToParent, which routes it to the parent scope so the mutation runs exactly once. - A mutation-typed step that survives that routing pass carries real deferred output rather than just a key, meaning it could only be produced by invoking the mutation root field a second time. That is now rejected with the same guardrail instead of ever reaching the executor. Un-skips DeferPlannerTests.Defer_OnMutationResult_Should_ProduceDeferredGroup and DeferTests.Defer_On_Mutation_Result_Should_Return_Incremental_Response, and adds two planner tests covering the "no lookup available" guardrail (root-anchored and result-anchored). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2TfmiTonT2kyPi3S6Rjjy
BuildIncrementalPlanOperation built the incremental plan's operation from the root operation and only replaced the selection set, so it copied the root operation's variable definitions wholesale even when the reconstructed deferred selection referenced only a subset (or none) of them. This left unused variables declared on the skeleton operation that BuildDeferredExecutionNodes consumes, and inflated the hasVariables check derived from it. Collect the variable names actually referenced by the reconstructed selection set (field arguments including nested list/object values, and directives on fields and inline fragments) and keep only the matching VariableDefinitionNodes, preserving their original document order. The already-stripped @defer(if: $var) directive cannot keep a definition alive on its own, since collection runs after StripDeferDirective removes it. Add DeferPlannerTests cases asserting the incremental plan operation drops an unused root variable and keeps one referenced by the deferred selection.
The safety-net throw in RouteIncrementalPlans reported the operation's root type name (Mutation) instead of the surviving step's real anchor type, unlike the other DeferredMutationLookupRequiredException throw site. Resolve the anchor type from the descriptor's own path when reachable, falling back to the operation root type name otherwise, and correct the DeferPlannerTests guardrail test that pinned TypeName == "Mutation" for a non-root anchor. Also moves the DeferredMutationLookupRequiredException message into FusionExecutionResources (matching the existing guardrail-exception pattern) and validates path/typeName before the message is formatted. Adds a DeferPlannerTests coverage case for a defer nested one level below the mutation's own root field (createUser.child): the planner already produces a keyed lookup incremental plan that depends on the mutation node, with the mutation executing exactly once, so no guardrail follow-up is needed here. Comment trims adjacent to the anchor-type fix in OperationPlanner.Defer.cs (same hunks) were folded into this commit since git commit -- <pathspec> stages the whole file, not just the touched hunks.
DeferOperationRewriter.BuildIncrementalPlanOperation pruned unused variable definitions only when the used-variable count differed from the definition count, which coincidentally matches in some cases even when the referenced set is not the same. Always build the filtered list and swap it in whenever its length differs from the original. CollectUsedVariables did not visit FragmentSpreadNode directives, an unreachable case today (DeferPartitioner inlines spreads before this runs) but a latent trap for future callers; it now visits them like the other selection kinds. Trims a few multi-sentence, rationale-heavy comments and XML docs down to the 1-2 sentence contract style, and a "// assert: ..." comment in DeferTests down to a bare "// assert" with the detail moved beneath.
A deferred selection whose fields the enclosing plan scope already has to fetch anyway produces no incremental benefit: honoring @defer for it only adds a redundant fetch (or, in the worst case, a wasteful root re-fetch when the anchor type's only lookup is self-cyclic on the deferred key itself). The incremental delivery spec explicitly allows a server to skip deferring in that case: it delivers the fields in the initial data with no pending entry and no delivery group. Add TryAbsorbFullyRedundantDefer, a post-planning check run after each incremental plan descriptor is routed. It recognizes two redundant shapes: a plain (non-lookup) producer step whose fields are exactly the anchor type's own key fields, which any schema exposing the entity resolves for free; and a genuine keyed lookup whose entire output is already selected on the enclosing step as a fusion__requirement for an unrelated reason. In both cases the fields are made visible (any fusion__requirement marker is stripped) on the enclosing step and its internal operation, and the caller skips producing an incremental plan and delivery group for the descriptor. The optimization is restricted to leaf descriptors so nested @defer children keep a valid registered enclosing scope, and skips any step guarded by @skip/@include so a variable-conditioned defer's eager main-plan copy is never mistaken for unconditionally available data. OperationPlanner.cs filters the compiled plan's delivery groups to the set still referenced by a routed descriptor, so an absorbed descriptor's delivery group never surfaces as a pending entry with nothing to complete it. RemoveUnconsumedEntityField is refactored to share the new WithEntitySelectionSet helper with the new code path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2TfmiTonT2kyPi3S6Rjjy
Replace Defer_KeyOnlyField_Should_KeepRootRefetch_When_OnlyReachableLookupIsSelfCyclic (which pinned a wasteful root re-fetch of the whole list to re-obtain a key the parent already reaches) with Defer_KeyOnlyField_Should_NotDefer_When_OnlyReachableLookupIsSelfCyclic, pinning the new not-deferred shape: no incremental plans, no delivery groups, and the key visible directly in the main operation document. Add Defer_FieldAlreadyRequiredByParent_Should_NotDefer, covering the other redundant shape: a field already fetched as a fusion__requirement for an unrelated sibling field's cross-subgraph requirement is not deferred a second time; its fusion__requirement marker is stripped instead so it appears in the initial data. Add the matching end-to-end case, Defer_KeyOnlyField_Should_Return_NonStreamed_Result, pinning a single non-streamed payload with the key in the initial data and no pending entry, comparable to the existing @defer(if: false) shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N2TfmiTonT2kyPi3S6Rjjy
…eaks MakeFieldsVisible could only guess, from selection-set shape, whether a lone __typename in a redundant defer's parent target was the defer split's synthetic placeholder or a field the client actually selected; the guess dropped both, silently discarding client-requested __typename siblings. It also masked a second bug: because DeferOperationRewriter's placeholder carried no marker, it was already leaking into the client-visible response whenever a deferred fragment was a field's only content, independent of the redundant-defer absorb path. DeferOperationRewriter now marks its two synthesized __typename placeholders with the existing internal fusion__empty directive, the same marker OperationCompiler and RemoveInternalDirectives already understand for excluding synthetic selections from outgoing requests and client responses. MakeFieldsVisible no longer needs the shape guess: a client-selected __typename (no directive) survives merges untouched, and the marked placeholder is stripped downstream once real fields join it. Also hardens TryAbsorbFullyRedundantDefer to use the failure-signalling TryLocateSelectionSetAtPath instead of the unchecked LocateSelectionSetAtPath, so a parent step whose definition does not actually reach the anchor path declines the optimization instead of silently dropping the deferred fields. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pins that a client selecting __typename alongside a redundant key-only @defer keeps that field in the initial payload (planner and end-to-end), and reviews the resulting snapshot churn on two existing defer tests now that the synthetic __typename placeholder is correctly excluded from source requests and client responses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the absorb-schema match TryCollectFlatFieldNames unconditionally skipped every __typename, including one the client explicitly selected inside a deferred fragment, so TryAbsorbFullyRedundantDefer dropped it entirely once the incremental plan collapsed into the parent step. It now skips only the synthetic fusion__empty placeholder and declines the optimization for any field carrying a real @skip/@include-style directive instead of silently absorbing it. MakeFieldsVisible can now upgrade an existing placeholder __typename node into a plain one, and __typename is exempted from the already-present/key-field checks since every schema resolves it for free. The parent-step search also required the candidate to sit on the same schema as the surviving incremental step, which meant a defer whose only surviving producer routed to a schema the parent never separately visits (e.g. a self-cyclic lookup falling back to a different schema once __typename widens reachability) could never be absorbed even though its output was already available elsewhere in the parent scope. The search now prefers a same-schema candidate first and falls back to any step covering the anchor path, while the key-field fallback branch (no exact field match) still requires schema identity, since only that schema is guaranteed to resolve the missing key fields for free. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The XML doc on s_placeholderTypeNameField explained why the placeholder exists instead of stating its contract. Reword it to plain, direct sentences per AGENTS.md's doc comment rules. Also clear two analyzer findings in the same file: mark StripDeferFromSelectionSet static since it touches no instance state, and replace a foreach-Add loop with AddRange.
This was referenced Aug 29, 2026
This was referenced Sep 8, 2026
Closed
Closed
This was referenced Sep 15, 2026
Closed
[nuget][SUI_Matcher]- Bump the sui-package-updates group with 20 updates
DFE-Digital/SUI_Matcher#405
Closed
Closed
This was referenced Sep 22, 2026
Closed
Closed
[nuget][SUI_Matcher]- Bump the sui-package-updates group with 20 updates
DFE-Digital/SUI_Matcher#410
Merged
This was referenced Oct 5, 2026
Open
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.
Fixes two @defer-related bugs in Fusion's operation plan output (
extensions.fusion.operationPlan).IncrementalPlan.ParentNodeId resolves to the wrong root node
When a deferred fragment was anchored under a field resolved by the second (or later) root fetch node, the plan reported the first root node as
parentNodeId. All root nodes shareTarget = $, so the deepest-ancestor selection inResolveDeferParentNodeIdalways kept the first one. A candidate now only qualifies if the first field segment of the defer path below its target is a response name in its rootResultSelectionSet; the deepest-target preference among qualifying candidates is kept, with a fallback to the previous behavior when no candidate qualifies (avoids turning exotic shapes intoIncrementalPlanParentNotFound).ExecutionNodeTrace.VariableSets aliases pooled path arrays
Trace variable sets captured
CompactPaths wrappingint[]s rented from the store's path pool. After a node's results merged, the arrays were returned and re-rented by a later node, so serializing the trace at request completion printed the later node's requirement paths under the earlier node'svariableSets. When telemetry is collected,TrackVariableValueSetsnow stores detached copies ofPathandAdditionalPaths(newCompactPath.Detach()/CompactPathSegment.Detach()); the pooled originals still flow through the request/merge path unchanged, so the hot path is unaffected.Tests
DeferTestsasDefer_Anchored_Under_Second_Root_Node_Should_Report_Correct_ParentNode(snapshot now pinsparentNodeId: 2) and added a three-root case pinningparentNodeId: 3.OperationPlanTelemetryTestsasserts an earlier lookup's trace keys are its own target paths after a dependent lookup builds deeper paths; both new tests fail without their fix.Fusion.Execution.TestsandFusion.AspNetCore.Testssuites pass; only the intended snapshot values changed.