test(resolve): add race detector test for parallel entity fetches#1436
Merged
jensneuse merged 3 commits intofeat/add-caching-supportfrom Mar 7, 2026
Merged
test(resolve): add race detector test for parallel entity fetches#1436jensneuse merged 3 commits intofeat/add-caching-supportfrom
jensneuse merged 3 commits intofeat/add-caching-supportfrom
Conversation
…h L2 caching Add TestResolveParallel_NoConcurrentArenaRace to verify that parallel entity fetches with L2 caching do not race on arena memory. This test exercises goroutine code paths in resolveParallel Phase 2 that allocate from per-goroutine arenas, catching regressions if someone accidentally uses the shared l.jsonArena from a goroutine. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jensneuse
requested review from
Noroth,
StarpTech,
devsergiy,
dkorittki and
ysmolski
as code owners
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 selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Go test that stresses concurrent loader Phase 2 operations (extractCacheKeysStrings, populateFromCache, denormalizeFromCache) with shared L1/L2 caches and a reused arena. It runs two scenarios (L2 miss and partial L2 hit) across repeated iterations and provides a thread-safe staticDataSource for deterministic loads. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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: 1
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader_parallel_race_test.go (1)
191-211: Consider clarifying the test scenario description.The comment states "pre-populated L2 cache" but only product entity data is pre-populated (lines 197-198). The inventory fetch will miss the cache (no matching inventory data cached) and will fetch from
inventoryDS.This tests a hybrid scenario (products cache hit + inventory cache miss), which is valid for race detection, but the description could be clearer to avoid confusion.
📝 Suggested comment clarification
t.Run("parallel batch entity fetches with L2 cache hit", func(t *testing.T) { - // Scenario: Same as above but with pre-populated L2 cache. - // Goroutines exercise populateFromCache (parsing cached JSON on goroutine arena). + // Scenario: Root fetch → Parallel( + // BatchEntityFetch (products subgraph, L2 hit → populateFromCache), + // BatchEntityFetch (inventory subgraph, L2 miss → subgraph fetch), + // ) + // Products fetch exercises populateFromCache (parsing cached JSON on goroutine arena). + // Inventory fetch exercises concurrent subgraph fetch alongside cache path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/loader_parallel_race_test.go` around lines 191 - 211, Update the test description and inline comment to reflect that only product entities are pre-populated in the L2 cache while inventory entities will miss and be fetched from inventoryDS; change the t.Run name string (currently "parallel batch entity fetches with L2 cache hit") to something like "parallel batch entity fetches with partial L2 cache hit (products hit, inventory miss)" and update the preceding comment above the cache.SetRawData calls to explain this hybrid scenario; reference the test block containing cache.SetRawData, productCacheKeyTemplate, productsDS and inventoryDS to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/loader_parallel_race_test.go`:
- Around line 3-15: The import block in loader_parallel_race_test.go is
mis-ordered for the gci linter; reorder imports into three groups separated by
blank lines: first standard library packages (context, net/http, sync, testing),
then external packages (github.com/stretchr/testify/assert,
github.com/stretchr/testify/require, github.com/wundergraph/go-arena), and
finally internal project packages
(github.com/wundergraph/graphql-go-tools/v2/pkg/ast,
github.com/wundergraph/graphql-go-tools/v2/pkg/engine/datasource/httpclient,
github.com/wundergraph/graphql-go-tools/v2/pkg/fastjsonext); adjust the import
grouping in the file so gci passes.
---
Nitpick comments:
In `@v2/pkg/engine/resolve/loader_parallel_race_test.go`:
- Around line 191-211: Update the test description and inline comment to reflect
that only product entities are pre-populated in the L2 cache while inventory
entities will miss and be fetched from inventoryDS; change the t.Run name string
(currently "parallel batch entity fetches with L2 cache hit") to something like
"parallel batch entity fetches with partial L2 cache hit (products hit,
inventory miss)" and update the preceding comment above the cache.SetRawData
calls to explain this hybrid scenario; reference the test block containing
cache.SetRawData, productCacheKeyTemplate, productsDS and inventoryDS to locate
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a91109a3-35a4-44ed-ae63-7d505d8c4485
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader_parallel_race_test.go
- Fix gci import grouping: separate external and internal packages - Clarify L2 cache hit test: only products are pre-populated, inventory misses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Separate testify (default) from go-arena (wundergraph prefix) per .golangci.yml gci config: standard / default / prefix(github.com/wundergraph) / prefix(github.com/wundergraph/graphql-go-tools). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/loader_parallel_race_test.go`:
- Around line 200-203: The test currently reuses a single FakeLoaderCache across
all loop iterations and sets product entries once, causing later iterations to
see stale hits; change the subtest to allocate fresh caches per iteration (e.g.,
call NewFakeLoaderCache() inside the loop) and preload only the product cache
entries into a product-specific cache instance via SetRawData, while creating a
separate inventory-specific cache instance (or namespace) for the inventory
loader so inventory misses remain isolated; update the inventory loader setup so
it uses the inventory cache instance instead of the shared `default` cache.
- Around line 183-185: The test currently only asserts
`"id":"prod-1"`/`"id":"prod-2"` which can be satisfied by the root fetch; update
the assertions in loader_parallel_race_test.go (the block using
fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors) and
variable resolvable) to also assert fields that only the parallel entity fetches
produce (e.g. assert the output contains `"name":"..."` and
`"inStock":true/false` for both prod-1 and prod-2), and make the same assertion
upgrade for the other occurrence around the 343-345 region so the test proves
the Phase 2 goroutine branches actually ran.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ac400b5-9ad9-40c2-bf60-a0ec76c88451
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader_parallel_race_test.go
Comment on lines
+183
to
+185
| out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors) | ||
| assert.Contains(t, out, `"id":"prod-1"`) | ||
| assert.Contains(t, out, `"id":"prod-2"`) |
Contributor
There was a problem hiding this comment.
Assert fields that only the parallel entity fetches can produce.
These checks only look for id, but id is already present in the root fetch payload. The test will still pass if the Phase 2 goroutine paths are skipped or misconfigured, which weakens it as a race regression test. Please assert name and inStock too, or otherwise prove those parallel branches ran.
🔧 Minimal assertion upgrade
- assert.Contains(t, out, `"id":"prod-1"`)
- assert.Contains(t, out, `"id":"prod-2"`)
+ assert.Contains(t, out, `"name":"Widget"`)
+ assert.Contains(t, out, `"name":"Gadget"`)
+ assert.Contains(t, out, `"inStock":true`)
+ assert.Contains(t, out, `"inStock":false`)Also applies to: 343-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/pkg/engine/resolve/loader_parallel_race_test.go` around lines 183 - 185,
The test currently only asserts `"id":"prod-1"`/`"id":"prod-2"` which can be
satisfied by the root fetch; update the assertions in
loader_parallel_race_test.go (the block using
fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors) and
variable resolvable) to also assert fields that only the parallel entity fetches
produce (e.g. assert the output contains `"name":"..."` and
`"inStock":true/false` for both prod-1 and prod-2), and make the same assertion
upgrade for the other occurrence around the 343-345 region so the test proves
the Phase 2 goroutine branches actually ran.
Comment on lines
+200
to
+203
| cache := NewFakeLoaderCache() | ||
| // Pre-populate L2 cache with product entities only; inventory entities are NOT cached | ||
| cache.SetRawData(`{"__typename":"Product","key":{"id":"prod-1"}}`, []byte(`{"__typename":"Product","id":"prod-1","name":"Widget"}`), 60_000_000_000) | ||
| cache.SetRawData(`{"__typename":"Product","key":{"id":"prod-2"}}`, []byte(`{"__typename":"Product","id":"prod-2","name":"Gadget"}`), 60_000_000_000) |
Contributor
There was a problem hiding this comment.
Keep the partial-hit scenario isolated per iteration.
This subtest does not reliably stay in the advertised “products hit / inventory miss” state. The cache is preloaded once outside the loop and then reused for all 100 runs, and the inventory fetch also points at default. That means the inventory branch can observe the product cache entries immediately, and after the first iteration the remaining runs will trend toward hit+hit instead of hit+miss. Use a fresh product/inventory cache setup per iteration and give inventory its own cache namespace/object.
Also applies to: 217-217, 295-300, 327-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/pkg/engine/resolve/loader_parallel_race_test.go` around lines 200 - 203,
The test currently reuses a single FakeLoaderCache across all loop iterations
and sets product entries once, causing later iterations to see stale hits;
change the subtest to allocate fresh caches per iteration (e.g., call
NewFakeLoaderCache() inside the loop) and preload only the product cache entries
into a product-specific cache instance via SetRawData, while creating a separate
inventory-specific cache instance (or namespace) for the inventory loader so
inventory misses remain isolated; update the inventory loader setup so it uses
the inventory cache instance instead of the shared `default` cache.
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
1 participant
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.
Summary
Add
TestResolveParallel_NoConcurrentArenaRaceto verify that parallel entity fetches with L2 caching do not race on arena memory. This test exercises goroutine code paths inresolveParallelPhase 2 that allocate from per-goroutine arenas, catching regressions if someone accidentally uses the sharedl.jsonArenafrom a goroutine.Why
The data race described in the arena-data-race bug report was already mitigated by per-goroutine arenas (commit 1ad5a75). This test ensures no regression.
Test Plan
-raceflag:go test -race -run TestResolveParallel_NoConcurrentArenaRace ./v2/pkg/engine/resolve/... -v-race:go test -race ./v2/pkg/engine/resolve/... -count=1🤖 Generated with Claude Code
Summary by CodeRabbit