feat: schedule fetch trees optimally - #1612
Merged
ysmolski merged 24 commits intoAug 13, 2026
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds optional dependency-aware fetch scheduling with validation and legacy fallback. It adds execution configuration, expands scheduler tests and benchmarks, simplifies fetch-tree test construction, renames the post-processor test helper, updates module dependencies, and revises gRPC test-server setup. ChangesFetch scheduling feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExecutionEngine
participant FetchTreeProcessors
participant scheduleFetches
participant LegacyOrganizers
ExecutionEngine->>FetchTreeProcessors: Enable scheduled fetch processing
FetchTreeProcessors->>scheduleFetches: ProcessFetchTree(root)
scheduleFetches-->>FetchTreeProcessors: Return scheduled tree or error
FetchTreeProcessors->>LegacyOrganizers: Organize tree when disabled, ineligible, or failed
LegacyOrganizers-->>FetchTreeProcessors: Return legacy-organized tree
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ysmolski
force-pushed
the
yury/router-579-reduce-federated-query-latency-by-improving-fetch-grouping
branch
3 times, most recently
from
July 23, 2026 14:25
711710c to
be19712
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/postprocess/order_sequence_by_dependencies.go (1)
55-66: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
nodeDependsOnagainst cyclic dependencies.buildSchedulealready errors on self-dependencies and cycles, andscheduleFetchesfalls back toorderSequenceByDependencieson that path. In the fallback,nodeDependsOnrecurses without a visited/in-progress set, so cyclic input can still blow the stack. Add cycle detection here and re-enable the disabled cycle/self-dependency tests inv2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go#L193-L207.🤖 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/postprocess/order_sequence_by_dependencies.go` around lines 55 - 66, Guard orderSequenceByDependencies.nodeDependsOn against recursive cycles by tracking visited or in-progress fetch IDs and stopping recursion when a dependency is already being traversed, while preserving deduplicated sorted results. In v2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go lines 193-207, re-enable the disabled self-dependency and cycle tests to verify the fallback no longer overflows the stack.
🧹 Nitpick comments (2)
v2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go (1)
193-207: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDisabled cycle/self-dependency tests reflect the unguarded-recursion gap.
See the consolidated comment for details — these stubs are disabled because
nodeDependsOninorder_sequence_by_dependencies.gohas no cycle protection.🤖 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/postprocess/order_sequence_by_dependencies_test.go` around lines 193 - 207, Re-enable and satisfy the cycle and self-dependency tests for orderSequenceByDependencies. Update nodeDependsOn to track visited nodes during recursive dependency traversal, terminating safely when a node is revisited while preserving correct dependency ordering for acyclic graphs.v2/pkg/engine/postprocess/schedule_fetches.go (1)
429-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
cmp.Compareover subtraction in the sort comparator.
minReachableFetchID(a) - minReachableFetchID(b)is the classic subtraction-comparator overflow footgun forslices.SortFunc. Practically safe today sinceFetchIDs stay small, butcmp.Compareremoves the risk for free.♻️ Proposed fix
+ "cmp" "fmt" "math" "slices" ... slices.SortFunc(out, func(a, b *resolve.FetchTreeNode) int { - return minReachableFetchID(a) - minReachableFetchID(b) + return cmp.Compare(minReachableFetchID(a), minReachableFetchID(b)) })🤖 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/postprocess/schedule_fetches.go` around lines 429 - 464, Update the comparator in combineOf’s parallel sorting branch to use cmp.Compare on the two minReachableFetchID results instead of subtracting them, and add the required cmp import if needed.
🤖 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.
Outside diff comments:
In `@v2/pkg/engine/postprocess/order_sequence_by_dependencies.go`:
- Around line 55-66: Guard orderSequenceByDependencies.nodeDependsOn against
recursive cycles by tracking visited or in-progress fetch IDs and stopping
recursion when a dependency is already being traversed, while preserving
deduplicated sorted results. In
v2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go lines 193-207,
re-enable the disabled self-dependency and cycle tests to verify the fallback no
longer overflows the stack.
---
Nitpick comments:
In `@v2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go`:
- Around line 193-207: Re-enable and satisfy the cycle and self-dependency tests
for orderSequenceByDependencies. Update nodeDependsOn to track visited nodes
during recursive dependency traversal, terminating safely when a node is
revisited while preserving correct dependency ordering for acyclic graphs.
In `@v2/pkg/engine/postprocess/schedule_fetches.go`:
- Around line 429-464: Update the comparator in combineOf’s parallel sorting
branch to use cmp.Compare on the two minReachableFetchID results instead of
subtracting them, and add the required cmp import if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4d83f89-bd6b-4ad4-97ed-1104340551cc
📒 Files selected for processing (8)
v2/pkg/engine/postprocess/add_missing_nested_dependencies_test.gov2/pkg/engine/postprocess/create_parallel_nodes_test.gov2/pkg/engine/postprocess/order_sequence_by_dependencies.gov2/pkg/engine/postprocess/order_sequence_by_dependencies_test.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/schedule_fetches.gov2/pkg/engine/postprocess/schedule_fetches_test.gov2/pkg/engine/postprocess/util_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasourcetesting/datasourcetesting.go (1)
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the legacy scheduling bypass explicit in the helper API.
WithPostProcessornow always appendsDisableScheduleFetches(), so its generic name hides that callers cannot use it to exercise the default scheduler. Keep a clearly named legacy helper and provide a scheduled variant, or document this invariant prominently, to prevent future integration tests from silently bypassing scheduler coverage.🤖 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/datasourcetesting/datasourcetesting.go` around lines 60 - 70, Make the scheduling behavior explicit around WithPostProcessor: retain a clearly named legacy helper that appends DisableScheduleFetches(), and add a distinct scheduled variant that does not append it, or prominently document the invariant if the API cannot be split. Ensure callers can intentionally exercise the default scheduler instead of silently bypassing it.
🤖 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/datasourcetesting/datasourcetesting.go`:
- Around line 60-70: Make the scheduling behavior explicit around
WithPostProcessor: retain a clearly named legacy helper that appends
DisableScheduleFetches(), and add a distinct scheduled variant that does not
append it, or prominently document the invariant if the API cannot be split.
Ensure callers can intentionally exercise the default scheduler instead of
silently bypassing it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01c2af0d-a656-4760-9a51-43ad200c3bcc
📒 Files selected for processing (9)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_defer_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_entity_interfaces_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.gov2/pkg/engine/datasourcetesting/datasourcetesting.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/postprocess_test.gov2/pkg/engine/postprocess/resolve_input_templates_test.gov2/pkg/engine/postprocess/schedule_fetches.gov2/pkg/engine/postprocess/schedule_fetches_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/postprocess/schedule_fetches_test.go
- v2/pkg/engine/postprocess/schedule_fetches.go
ysmolski
changed the title
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
devsergiy
requested changes
Aug 5, 2026
devsergiy
reviewed
Aug 5, 2026
This was referenced Aug 7, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@v2/pkg/engine/postprocess/schedule_fetches_test.go`:
- Line 1035: Update the test to invoke the fallback processor directly through
NewProcessor().fetchTreeProcessors.organizeFetchTree(scheduled) instead of
(*scheduleFetches).ProcessFetchTree. Preserve the existing invalid scheduled
input and assertions while verifying the legacy processor fallback contract
rather than discarding ProcessFetchTree’s error.
🪄 Autofix
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: 665deb82-8f8b-40ee-b17a-888658781cf2
📒 Files selected for processing (7)
v2/pkg/engine/postprocess/add_missing_nested_dependencies_test.gov2/pkg/engine/postprocess/create_parallel_nodes_test.gov2/pkg/engine/postprocess/order_sequence_by_dependencies_test.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/schedule_fetches.gov2/pkg/engine/postprocess/schedule_fetches_test.gov2/pkg/engine/postprocess/util_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- v2/pkg/engine/postprocess/create_parallel_nodes_test.go
- v2/pkg/engine/postprocess/add_missing_nested_dependencies_test.go
- v2/pkg/engine/postprocess/postprocess.go
- v2/pkg/engine/postprocess/order_sequence_by_dependencies_test.go
devsergiy
reviewed
Aug 11, 2026
devsergiy
approved these changes
Aug 11, 2026
ysmolski
changed the base branch from
master
to
feat/router-62-engine-batch-entity-resolution-requests-to-the-same-subgraph
August 12, 2026 09:47
ysmolski
force-pushed
the
yury/router-579-reduce-federated-query-latency-by-improving-fetch-grouping
branch
from
August 12, 2026 10:56
5dc42a7 to
5ad74b9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
execution/engine/execution_engine.go (1)
161-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an engine-level regression test for this configuration bridge.
The scheduler option-wiring test exercises
postprocess.NewProcessordirectly. It does not verify thatConfiguration.EnableScheduleFetches()causesNewExecutionEngineto addpostprocess.EnableScheduleFetches(). Add a test that enables the configuration flag and verifies the scheduled fetch tree or the resultingpostProcessorOptions.🤖 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/engine/execution_engine.go` around lines 161 - 163, The existing configuration bridge in NewExecutionEngine lacks engine-level coverage. Add a regression test that enables Configuration.EnableScheduleFetches(), constructs NewExecutionEngine, and verifies that postprocess.EnableScheduleFetches() is present through the scheduled-fetch tree or resulting postProcessorOptions.
🤖 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/engine/execution_engine.go`:
- Around line 161-163: The existing configuration bridge in NewExecutionEngine
lacks engine-level coverage. Add a regression test that enables
Configuration.EnableScheduleFetches(), constructs NewExecutionEngine, and
verifies that postprocess.EnableScheduleFetches() is present through the
scheduled-fetch tree or resulting postProcessorOptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6a3c79f-bab1-4596-96da-9f8e92b300ae
📒 Files selected for processing (6)
execution/engine/engine_config.goexecution/engine/execution_engine.gov2/pkg/engine/datasourcetesting/datasourcetesting.gov2/pkg/engine/postprocess/postprocess.gov2/pkg/engine/postprocess/resolve_input_templates_test.gov2/pkg/engine/postprocess/schedule_fetches_test.go
💤 Files with no reviewable changes (2)
- v2/pkg/engine/postprocess/resolve_input_templates_test.go
- v2/pkg/engine/datasourcetesting/datasourcetesting.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/postprocess/schedule_fetches_test.go
ysmolski
force-pushed
the
yury/router-579-reduce-federated-query-latency-by-improving-fetch-grouping
branch
from
August 13, 2026 08:35
ca0fbbc to
ed0729d
Compare
scheduleFetches is a dependency-aware scheduler that emits nested
Sequence/Parallel trees, collapsing independent chains onto their own
branches instead of synchronizing them at wave barriers.
This scheduler tries to break the top level DAG into weakly connected
components. For each component it picks between two strategies of
scheduling:
- the waves tree, that barriers all ready roots per step,
- the inlined tree, that pulls each root's exclusively-reachable
descendants into that root's branch.
This scheduler implements a strategy that is guarnteed to be not the
worst combination condering all possible makespans of fetches.
# Conflicts: # v2/pkg/engine/postprocess/postprocess.go
When both the scheduler and multi-fetch are enabled, organizeFetchTree runs the legacy wave pipeline as a scratch phase to discover maximal same-wave merge groups, merges them via createMultiFetch, flattens the wave tree back into the flat Sequence the scheduler consumes, and schedules the merged DAG. A MultiEntityFetch participates as one node (min member ID, union dependencies; dependents are rewired by mergeGroup), so this needs no scheduler changes. Rationale: scheduling before merging dissolves the same-wave antichains that batching needs (eBay celestial query: 71 requests vs 43), while merging first keeps maximal batching AND the scheduler's dominance proof then holds against the merged wave tree — the result is provably never slower than the legacy waves + multi-fetch pipeline, usually structurally better (independent chains leave the wave barriers). An adaptive in-scheduler variant (branch experiment/mf-in-scheduler) buys one more request on the eBay query at beta=0.005 but costs a cost model, policy portfolio and greedy search; not worth the complexity. Known follow-ups, unchanged from the multi-fetch review: the sjson v1.0.4 workspace pin (multi-fetch is a no-op in consumer builds without it), and computing waves internally instead of materializing the throwaway legacy tree.
ysmolski
force-pushed
the
yury/router-579-reduce-federated-query-latency-by-improving-fetch-grouping
branch
from
August 13, 2026 09:06
ed0729d to
9af0428
Compare
ysmolski
deleted the
yury/router-579-reduce-federated-query-latency-by-improving-fetch-grouping
branch
August 13, 2026 10:11
This was referenced Aug 13, 2026
ysmolski
pushed a commit
that referenced
this pull request
Aug 13, 2026
🤖 I have created a release *beep* *boop* --- ## [2.16.0](v2.15.1...v2.16.0) (2026-08-13) ### Features * implement multi fetch to the same subgraph ([#1594](#1594)) ([22584e2](22584e2)) * schedule fetch trees optimally ([#1612](#1612)) ([bd03deb](bd03deb)) ### Bug Fixes * check interface implementation in `potentiallySameObject` for nullability relaxation ([#1454](#1454)) ([5bacb9e](5bacb9e)) * improve handling of nullable lists for required fields ([#1631](#1631)) ([0af4dd3](0af4dd3)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
ysmolski
added a commit
to wundergraph/cosmo
that referenced
this pull request
Aug 13, 2026
This PR adds two feature flags for the Cosmo's Query Planner:
`engine.enable_multi_fetch`
This option merges entity fetches that target the same subgraph and
execute at the same point in the query plan
into a single request with aliased `_entities` fields.
This reduces the number of requests sent to subgraphs.
env: ENGINE_ENABLE_MULTI_FETCH
`engine.enable_schedule_fetches`
This feature replaces the wave-based fetch execution
with a dependency-aware fetch scheduler.
Independent fetch chains progress as soon as
their own dependencies complete,
instead of waiting for the slowest fetch in each wave.
env: ENGINE_ENABLE_SCHEDULE_FETCHES
By default, these features are disabled:
```
engine:
enable_multi_fetch: false
enable_schedule_fetches: false
```
Built on top of wundergraph/graphql-go-tools#1612
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.
scheduleFetches is a dependency-aware scheduler that emits nested Sequence/Parallel trees, collapsing independent chains onto their own branches instead of synchronizing them at wave barriers.
This scheduler tries to break the top level DAG into weakly connected components. For each component it picks between two strategies of scheduling:
This scheduler implements a strategy that is guaranteed to be strictly the best condering all possible makespans of fetches.
According to production data, recursive wave scheduling provides the most value. It creates wave tree that is deep and where fetches already ordered in the order of dependencies. Inlining gives about 3-5% improvements on top of that.
Summary by CodeRabbit
New Features
Bug Fixes
Tests