fix(resolve): fix GC-liveness bug in the loadBatchEntityFetch

[ysmolski]

The arena memory is invisible to GC. The method was allocating 1 element slice on the Go heap and referenced from the arena bytes. At some point GC collected that slice and pointer was poisoned.

-race was indicating the problem very well.

The fix is to allocate the slice in the arena memory, so its lifetime is tied to the lifetime of pooled objects.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 77d8b458-2192-4753-92e5-235aa3bc09d0

📥 Commits

Reviewing files that changed from the base of the PR and between c8a2984 and 7427f5c.

📒 Files selected for processing (1)

• v2/pkg/engine/resolve/resolve_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• v2/pkg/engine/resolve/resolve_test.go

---

📝 Walkthrough

Walkthrough

Fixes a defer-capture bug in batch entity fetch cleanup, moves initial batch-stats bucket allocation onto the loader arena, extracts a shared nested-batching fixture, refactors two benchmarks to reuse it, and adds a GC-pressure regression test that runs many concurrent resolver executions.

Changes

Batch entity fetch memory improvements

Layer / File(s)	Summary
Tool pool defer cleanup fix v2/pkg/engine/resolve/loader.go	Deferred return of res.tools to batchEntityToolPool is performed inside an inline defer func() so the pool Put reads res.tools at defer execution time.
Arena allocation for batch stats v2/pkg/engine/resolve/loader.go	Initial per-unique-index targets bucket in loadBatchEntityFetch is allocated via arena.AllocateSlice and appended into batchStats instead of using a heap slice literal.

Nested batching test suite enhancements

Layer / File(s)	Summary
Test imports, input check, and fixture extraction v2/pkg/engine/resolve/resolve_test.go	Adds runtime/debug; introduces _fakeDataSource.checkInput and routes Load/LoadWithFiles through it; extracts nestedBatchingFixture(tb TestingTB) returning the federation GraphQLResponse plan and expected bytes.
Benchmark consolidation v2/pkg/engine/resolve/resolve_test.go	Benchmark_NestedBatching and Benchmark_NestedBatchingArena obtain plan and expected from nestedBatchingFixture() and retain pooling/buffering and parallel execution loops.
GC pressure regression test v2/pkg/engine/resolve/resolve_test.go	Adds TestNestedBatching_GCPressure which sets GC percent to 1 and runs many concurrent ResolveGraphQLResponse calls, validating output bytes on every iteration.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: fixing a GC-liveness bug in loadBatchEntityFetch by reallocating slices in arena memory.
Description check	✅ Passed	The description clearly explains the GC-liveness bug, its root cause (heap-allocated slice collected by GC while referenced from arena), and the solution (allocate slice in arena memory).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch yury/eng-9717-concurrency-bugs-in-the-resolver-via

---

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

[coderabbitai]

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/resolve/resolve_test.go`:
- Around line 7761-7783: The concurrent test uses nestedBatchingFixture which
wires in fakeDataSourceWithInputCheck that calls require.Equal (which invokes
FailNow) from worker goroutines; change fakeDataSourceWithInputCheck so it does
not call require.Equal but instead performs a non-fatal comparison and returns a
descriptive error when inputs mismatch (so the error flows back through
ResolveGraphQLResponse and is reported from the goroutine), or alternatively
replace the require.Equal call with a plain if check that constructs and returns
an error; ensure nestedBatchingFixture still uses that updated
fakeDataSourceWithInputCheck so ResolveGraphQLResponse can propagate the failure
rather than causing FailNow in a non-test goroutine.

🪄 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: 363b42aa-f732-4772-aa5f-9c2cebeadf15

📥 Commits

Reviewing files that changed from the base of the PR and between 542d2af and 68d8c07.

📒 Files selected for processing (2)

• v2/pkg/engine/resolve/loader.go
• v2/pkg/engine/resolve/resolve_test.go

[Noroth]

You could use t.Cleanup

[ysmolski]

fixed