fix(router): do not reuse planners in the plan generator

[ysmolski]

Engine 253 has introduce a breaking change that requires that planners should not be reused. Engine did it to simplify certain actions in the planner. Reusing the state make planner more complicated.

PG was the last place where we reused planners. Things change remove the reuse. It does not affect performance of the planners. Added benchmark shows that internal overhead is minimal.

BenchmarkPlanGenerator-14    1364    2,582,604 ns/op    6,734,461 B/op    11,191 allocs/op

That's ~2.6ms per full plan generation cycle (parsing + normalizing + validating + planning the full.graphql query against 7 subgraphs).

benchstat:

                 │   old.txt.   │            new.txt         │
                 │   sec/op     │   sec/op     vs base       │
PlanGenerator-14   2.107m ± 1%   2.090m ± 1%  ~ (p=0.187 n=15)

Summary by CodeRabbit

• Refactor

  • More robust plan generation with improved initialization and error propagation for more reliable, fail-fast behavior and clearer error signals.

• Tests

  • Added a performance benchmark using comprehensive query test data to exercise plan generation.

• Chores

  • Bumped a tooling dependency to the next patch release.

[coderabbitai]

Walkthrough

Planner acquisition moved from goroutine bootstrap into the per-query loop; error returns were standardized with %w across I/O, parsing, planner, and marshal paths. Added a benchmark and large GraphQL test query. Two module dependencies bumped to github.com/wundergraph/graphql-go-tools/v2 v2.0.0-rc.253.

Changes

Cohort / File(s)	Summary
Plan generator core router/pkg/plan_generator/plan_generator.go	Moves planner retrieval into the per-query processing loop (planners no longer reused across iterations). Standardizes error wrapping using %w, adjusts cancellation/error propagation, ensures default output format, and tightens error handling for file/JSON operations.
Tests & testdata router/pkg/plan_generator/plan_generator_test.go, router/pkg/plan_generator/testdata/queries/bench/full.graphql	Adds BenchmarkPlanGenerator and a comprehensive GraphQL benchmark query payload used by the new benchmark.
Module dependency bumps router/go.mod, router-tests/go.mod	Bumps github.com/wundergraph/graphql-go-tools/v2 from v2.0.0-rc.252 to v2.0.0-rc.253 in both go.mod files.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat: view query plan as a json #2236 — Related changes to plan generator output format handling and passing output-format behavior through the planner.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: removing planner reuse from the plan generator. It is specific, concise, and directly reflects the primary modification.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-6429864705d7f8d5f9b25b1c55a0a6d59a87c507

[endigma]

lgtm, 2 nits:

[codecov]

Codecov Report

❌ Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.09%. Comparing base (5ba48f6) to head (f519041).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
router/pkg/plan_generator/plan_generator.go	31.25%	10 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2537      +/-   ##
==========================================
+ Coverage   61.68%   62.09%   +0.41%     
==========================================
  Files         239      239              
  Lines       25423    25424       +1     
==========================================
+ Hits        15681    15788     +107     
+ Misses       8422     8293     -129     
- Partials     1320     1343      +23     

Files with missing lines	Coverage Δ
router/pkg/plan_generator/plan_generator.go	51.32% <31.25%> (-21.89%)	⬇️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[coderabbitai]

🧹 Nitpick comments (1)

router/pkg/plan_generator/plan_generator.go (1)

118-123: Use sync.WaitGroup.Go for consistency with codebase patterns

Lines 118–123 use manual wg.Add(cfg.Concurrency) followed by go func() { defer wg.Done() }(). The codebase already standardizes on wg.Go(func() { ... }) throughout production code (e.g., router/core/router.go). Since the router targets Go 1.25+, refactor to use wg.Go() to align with established patterns and reduce error-prone lifecycle management.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router/pkg/plan_generator/plan_generator.go` around lines 118 - 123, Replace
the manual lifecycle management that does wg.Add(cfg.Concurrency) and go func(i
int) { defer wg.Done(); ... }(i) with the codebase-standard wg.Go(...) usage:
call wg.Go(func() { ... }) inside the for i := 0; i < cfg.Concurrency; i++ loop
and move the worker body into that closure, ensuring the loop index is correctly
captured (e.g., assign local := i before using it in the closure or accept i as
parameter) and remove the explicit defer wg.Done() / wg.Add call; keep the same
worker logic and references to cfg.Concurrency and wg.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@router/pkg/plan_generator/plan_generator.go`:
- Around line 118-123: Replace the manual lifecycle management that does
wg.Add(cfg.Concurrency) and go func(i int) { defer wg.Done(); ... }(i) with the
codebase-standard wg.Go(...) usage: call wg.Go(func() { ... }) inside the for i
:= 0; i < cfg.Concurrency; i++ loop and move the worker body into that closure,
ensuring the loop index is correctly captured (e.g., assign local := i before
using it in the closure or accept i as parameter) and remove the explicit defer
wg.Done() / wg.Add call; keep the same worker logic and references to
cfg.Concurrency and wg.

[coderabbitai]

🧹 Nitpick comments (1)

router/pkg/plan_generator/plan_generator.go (1)

121-127: Consider using sync.WaitGroup.Go() for cleaner concurrency.

Go 1.25+ provides wg.Go(func()) which manages Add/Done automatically. This would simplify the goroutine spawning pattern.

♻️ Suggested refactor

-	wg.Add(cfg.Concurrency)
 	for i := 0; i < cfg.Concurrency; i++ {
-		go func(i int) {
-			defer wg.Done()
+		wg.Go(func() {
 			for {
 				select {
 				case <-ctxError.Done():
 					return

And at the end of the goroutine:

 				}
 			}
-		}(i)
+		})
 	}

Note: The loop variable i appears unused inside the goroutine, so the closure parameter can be removed entirely.

Based on learnings: "In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router/pkg/plan_generator/plan_generator.go` around lines 121 - 127, Replace
the manual Add/Done goroutine spawning for wg in generatePlan (the block
creating wg := sync.WaitGroup{} and looping i := 0; i < cfg.Concurrency) with
the Go 1.25+ pattern using wg.Go(func() { ... }) so the WaitGroup handles
Add/Done automatically; remove the unused closure parameter i from the anonymous
function since it isn't referenced inside the goroutine, and keep the existing
select/for loop body intact inside the new wg.Go call.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@router/pkg/plan_generator/plan_generator.go`:
- Around line 121-127: Replace the manual Add/Done goroutine spawning for wg in
generatePlan (the block creating wg := sync.WaitGroup{} and looping i := 0; i <
cfg.Concurrency) with the Go 1.25+ pattern using wg.Go(func() { ... }) so the
WaitGroup handles Add/Done automatically; remove the unused closure parameter i
from the anonymous function since it isn't referenced inside the goroutine, and
keep the existing select/for loop body intact inside the new wg.Go call.