feat: improve memory management & request deduplication #1336
Conversation
…to resolve Context
# Conflicts: # go.work.sum # v2/go.mod # v2/go.sum # v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
jensneuse
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
ysmolski
as code owners
WalkthroughAPIs and tests were refactored to pass HTTP headers and return response bytes instead of writing to buffers; arena-based allocation and pooling were introduced and threaded through resolver/loader; sharded single‑flight deduplication for inbound/subgraph requests was added; xxhash UniqueRequestID flows were removed; subscription triggers gained SourceName/SourceID. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant InboundSF as InboundSingleFlight
participant SubgraphSF as SubgraphSingleFlight
participant ArenaPool
participant DataSource
Client->>Resolver: GraphQL request
Resolver->>InboundSF: GetOrCreate(ctx, requestKey)
alt leader
InboundSF-->>Resolver: leader
Resolver->>ArenaPool: Acquire(triggerKey)
ArenaPool-->>Resolver: ArenaItem
Resolver->>SubgraphSF: GetOrCreateItem(fetchItem, input, extraKey)
alt subgraph leader
SubgraphSF-->>Resolver: leader item
Resolver->>DataSource: Load(ctx, headers, input)
DataSource-->>Resolver: (data, err)
Resolver->>SubgraphSF: Finish(item)
else wait/shared
SubgraphSF-->>Resolver: wait for loaded
end
Resolver->>InboundSF: FinishOk(inflightReq, data)
Resolver->>ArenaPool: Release(ArenaItem)
else follower
InboundSF-->>Resolver: wait for leader.Done
end
Resolver-->>Client: merged GraphQL response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
v2/pkg/engine/resolve/inputtemplate.go (3)
161-161: Fix unchecked error to resolve pipeline failure.The
errchecklinter flags this line becauseWriteStringcan return an error. With the interface abstraction, implementations might fail, unlike*bytes.Bufferwhich never returns errors.Apply this diff to handle the error:
- preparedInput.WriteString(value[0]) - return nil + _, err := preparedInput.WriteString(value[0]) + return err
168-168: Handle WriteString error for consistency.Similar to line 161, this
WriteStringcall ignores potential errors. Although not flagged by the pipeline (possibly because the linter stopped at the first error), it should be handled consistently.Apply this diff to handle the error:
- preparedInput.WriteString(value[j]) + if _, err := preparedInput.WriteString(value[j]); err != nil { + return err + }
39-51: Update caller type compatibility in loader.goThe signature change to
InputTemplateWriterintroduces type mismatches at both call sites. The callers in loader.go still initializepreparedInputas*bytes.Bufferand pass it toSetInputUndefinedVariables, which now requiresInputTemplateWriter:
- Line 1340 & 1386:
preparedInput := bytes.NewBuffer(nil)passed to function expectingInputTemplateWriter- Line 1428 & 1500:
preparedInput := bytes.NewBuffer(make([]byte, 0, 64))passed to function expectingInputTemplateWriterEither revert the signature change to
*bytes.Bufferor update the callers in loader.go to useInputTemplateWriter.v2/pkg/engine/resolve/loader_test.go (1)
1029-1049: Fix benchmark assertion to match new response format.The benchmark expects a response with an
"errors"field, but the new response format no longer includes an empty errors array by default. The pipeline failure indicates this assertion is failing.Update the expected output to match the actual response format:
- expected := `{"errors":[],"data":{"topProducts":[{"name":"Table","__typename":"Product","upc":"1","reviews":[{"body":"Love Table!","author":{"__typename":"User","id":"1","name":"user-1"}},{"body":"Prefer other Table.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":8},{"name":"Couch","__typename":"Product","upc":"2","reviews":[{"body":"Couch Too expensive.","author":{"__typename":"User","id":"1","name":"user-1"}}],"stock":2},{"name":"Chair","__typename":"Product","upc":"3","reviews":[{"body":"Chair Could be better.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":5}]}}` + expected := `{"data":{"topProducts":[{"name":"Table","__typename":"Product","upc":"1","reviews":[{"body":"Love Table!","author":{"__typename":"User","id":"1","name":"user-1"}},{"body":"Prefer other Table.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":8},{"name":"Couch","__typename":"Product","upc":"2","reviews":[{"body":"Couch Too expensive.","author":{"__typename":"User","id":"1","name":"user-1"}}],"stock":2},{"name":"Chair","__typename":"Product","upc":"3","reviews":[{"body":"Chair Could be better.","author":{"__typename":"User","id":"2","name":"user-2"}}],"stock":5}]}}`v2/pkg/engine/resolve/resolvable.go (1)
109-121: Non-nil arena precondition across parse/merge/error pathsThese sites assume r.astjsonArena is non-nil (ParseBytesWithArena, MergeValues, SetArrayItem, ArrayValue, AppendError*). This is fine if callers set the arena first; otherwise it’s crash-prone. The fix is in resolve.go (ensure arena is acquired and injected before Init), see my comment there.
Also applies to: 470-479, 862-863, 1198-1212, 1286-1295, 1298-1312
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
252-257: Avoid mutating message.Fields via slice aliasing.validFields := message.Fields followed by append(...) can mutate the underlying slice, causing duplicated fields across calls.
Apply:
- validFields := message.Fields + validFields := append(RPCFields(nil), message.Fields...) if message.IsOneOf() { - validFields = append(validFields, message.FieldSelectionSet.SelectFieldsForTypes( + validFields = append(validFields, message.FieldSelectionSet.SelectFieldsForTypes( message.SelectValidTypes(string(data.Type().Descriptor().Name())))...) }Replace RPCFields with the actual slice type alias (e.g., []RPCField) if different.
🧹 Nitpick comments (32)
v2/pkg/engine/resolve/inputtemplate.go (1)
47-47: Consider handling Write errors consistently.These
Writecalls ignore potential errors. While not flagged by the pipeline, handling errors consistently would improve robustness, especially since the interface abstraction now allows for implementations that might fail.Consider handling errors from all
Writecalls similar to theWriteStringfixes. For example, at line 84:- _, _ = preparedInput.Write(segment.Data) + if _, err = preparedInput.Write(segment.Data); err != nil { + return err + }Apply similar patterns to lines 47, 106, 123, 144, and 166 if error handling is desired.
Also applies to: 84-84, 106-106, 123-123, 144-144, 166-166
v2/pkg/engine/resolve/context.go (2)
39-48: Clarify SubgraphHeadersBuilder contractAdd a short doc that the returned uint64 is the dedup scope key (stable within a request, distinct across requests/users) and that headers must be treated as immutable.
-type SubgraphHeadersBuilder interface { - HeadersForSubgraph(subgraphName string) (http.Header, uint64) -} +// SubgraphHeadersBuilder provides per-subgraph request headers and a dedup scope key. +// The returned header must be treated as immutable by callers. +// The returned uint64 should uniquely scope deduplication (e.g., request ID), +// so concurrent operations with different auth headers do not deduplicate together. +type SubgraphHeadersBuilder interface { + HeadersForSubgraph(subgraphName string) (http.Header, uint64) +}Confirm all call sites of HeadersForSubgraphRequest use the returned uint64 as the SingleFlight extraKey.
211-224: Hygiene: reset Request.ID and SubgraphHeadersBuilder on Free()Minor cleanup to avoid leaking state when Contexts are pooled/reused.
func (c *Context) Free() { c.ctx = nil c.Variables = nil c.Files = nil c.Request.Header = nil + c.Request.ID = 0 c.RenameTypeNames = nil c.RemapVariables = nil c.TracingOptions.DisableAll() c.Extensions = nil c.subgraphErrors = nil c.authorizer = nil c.LoaderHooks = nil + c.SubgraphHeadersBuilder = nil }v2/pkg/engine/resolve/singleflight.go (3)
16-23: Remove unused cleanup channel
cleanup chan func()is never used; drop it to reduce noise.type SingleFlight struct { mu *sync.RWMutex items map[uint64]*SingleFlightItem sizes map[uint64]*fetchSize xxPool *sync.Pool - cleanup chan func() } @@ func NewSingleFlight() *SingleFlight { return &SingleFlight{ items: make(map[uint64]*SingleFlightItem), sizes: make(map[uint64]*fetchSize), mu: new(sync.RWMutex), xxPool: &sync.Pool{ New: func() any { return xxhash.New() }, }, - cleanup: make(chan func()), } }Also applies to: 29-41
118-136: Size hint rollover: consider EMA for smoother adaptation (optional)Current reset at 50 samples turns the moving average into ~50% weight on the last sample. An exponential moving average would be smoother under traffic variance.
If desired, replace the rollover with
avg = alpha*avg + (1-alpha)*len(response)stored as fixed‑point to avoid floats.
75-83: Hash extraKey into the digest instead of adding it to reduce collision riskCurrently,
sfKeyis computed ash.Sum64() + extraKey, which can collide (e.g., 100+50 and 75+75 both yield 150). SinceextraKeycomes from request headers viaHeadersForSubgraph, it should be mixed into the hash to ensure distinct headers produce distinct keys.@@ -import ( - "sync" - - "github.com/cespare/xxhash/v2" -) +import ( + "encoding/binary" + "sync" + + "github.com/cespare/xxhash/v2" +) @@ -func (s *SingleFlight) sfKey(h *xxhash.Digest, fetchItem *FetchItem, input []byte, extraKey uint64) uint64 { +func (s *SingleFlight) sfKey(h *xxhash.Digest, fetchItem *FetchItem, input []byte, extraKey uint64) uint64 { if fetchItem != nil && fetchItem.Fetch != nil { info := fetchItem.Fetch.FetchInfo() if info != nil { _, _ = h.WriteString(info.DataSourceID) _, _ = h.WriteString(":") } } _, _ = h.Write(input) - return h.Sum64() + extraKey + var ek [8]byte + binary.LittleEndian.PutUint64(ek[:], extraKey) + _, _ = h.Write(ek[:]) + return h.Sum64() }Also applies to: 85-96
v2/pkg/engine/datasource/introspection_datasource/source_test.go (1)
31-41: Return-based Load usage looks good; simplify newline trim.
The switch to returning responseData is correct. You can simplify the newline removal from json.Indent with TrimSuffix for clarity.-// Trim the trailing newline that json.Indent adds -responseBytes := actualResponse.Bytes() -if len(responseBytes) > 0 && responseBytes[len(responseBytes)-1] == '\n' { - responseBytes = responseBytes[:len(responseBytes)-1] -} +// Trim the trailing newline that json.Indent adds +responseBytes := bytes.TrimSuffix(actualResponse.Bytes(), []byte{'\n'})v2/pkg/variablesvalidation/variablesvalidation.go (1)
101-101: Normalize empty/null variables to "{}" before parsing.
To match GraphQL semantics and mirror UploadFinder, guard nil/empty/"null" variables so ParseBytes never errors on absent variables.func (v *VariablesValidator) Validate(operation, definition *ast.Document, variables []byte) error { v.visitor.definition = definition v.visitor.operation = operation - v.visitor.variables, v.visitor.err = astjson.ParseBytes(variables) + // Normalize absent variables to {} + if variables == nil || bytes.Equal(variables, []byte("null")) || bytes.Equal(variables, []byte("")) { + variables = []byte("{}") + } + v.visitor.variables, v.visitor.err = astjson.ParseBytes(variables) if v.visitor.err != nil { return v.visitor.err }Please confirm callers never pass nil/empty variables inadvertently. If they do, add/keep this guard to prevent regressions.
v2/pkg/engine/resolve/resolvable_test.go (1)
15-15: Constructor updates LGTM; consider a tiny test helper to DRY.
All call sites now pass the arena argument (nil) correctly. Optionally add a newTestResolvable(opts) helper to cut duplication.func newTestResolvable(opts ResolvableOptions) *Resolvable { return NewResolvable(nil, opts) }Also applies to: 87-87, 160-160, 234-234, 262-262, 337-337, 373-373, 443-443, 473-473, 504-504, 552-552, 626-626, 656-656, 690-690, 722-722, 758-758, 841-841, 926-926, 953-953, 976-976, 998-998, 1021-1021, 1049-1049, 1147-1147, 1245-1245, 1345-1345
v2/pkg/engine/resolve/variables_renderer.go (1)
350-356: Use sync.Pool.New to remove the nil-branch; ensure Reset clears arena.
Minor micro-optimization: initialize the pool with New to simplify getResolvable.-var ( - _graphQLVariableResolveRendererPool = &sync.Pool{} -) +var ( + _graphQLVariableResolveRendererPool = &sync.Pool{ + New: func() any { return NewResolvable(nil, ResolvableOptions{}) }, + } +) func (g *GraphQLVariableResolveRenderer) getResolvable() *Resolvable { - v := _graphQLVariableResolveRendererPool.Get() - if v == nil { - return NewResolvable(nil, ResolvableOptions{}) - } - return v.(*Resolvable) + return _graphQLVariableResolveRendererPool.Get().(*Resolvable) }Please confirm Resolvable.Reset releases any arena-backed state (if ever set on this path), so pooled instances don’t retain large allocations between uses.
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (1)
78-80: Consider returning an error instead of panicking.The panic for unimplemented functionality could cause runtime crashes if this method is accidentally called. Consider returning a descriptive error instead:
-func (Source) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("not implemented") -} +func (Source) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + return nil, fmt.Errorf("static data source does not support file uploads") +}Note: You'll need to add
"fmt"to the imports if you adopt this change.v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)
8455-8455: Updated Start signature: ensure nil-safe header handlingCalls now use Start(ctx, headers, options, updater) with headers=nil. Verify the implementation treats nil as empty and does not mutate the provided map. Consider adding one test that passes non-empty headers and asserts they reach the upstream.
Also applies to: 8461-8461, 8474-8474, 8486-8486, 8504-8504, 8527-8527, 8591-8591, 8611-8611, 8635-8635
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (2)
1957-1964: Avoid clobbering configured headers; prefer merge with precedence.Overwriting options.Header loses statically configured subscription headers. Merge instead, letting passed headers override duplicates.
Apply:
err := json.Unmarshal(input, &options) if err != nil { return err } - options.Header = headers + // merge configured headers with passed headers; passed headers take precedence + if options.Header == nil { + options.Header = make(http.Header) + } + for k, v := range headers { + options.Header[k] = v + }
1977-1984: Same header-merge concern as AsyncStart.Use the same merge strategy here to preserve configured headers.
- options.Header = headers + if options.Header == nil { + options.Header = make(http.Header) + } + for k, v := range headers { + options.Header[k] = v + }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
154-156: Do not panic; return a proper error for unsupported file uploads.Panics in datasource paths can crash the process. Return an error or GraphQL error JSON.
-func (d *DataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("unimplemented") -} +func (d *DataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + // gRPC transport does not support multipart uploads + return nil, fmt.Errorf("gRPC datasource: file uploads are not supported") +}v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
237-239: Remove leftover debug printing.Avoid noisy stdout in tests.
- bytes := output - fmt.Println(string(bytes)) + bytes := outputv2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go (2)
59-61: Avoid panic; return unsupported operation error.Never panic in datasource code paths.
-func (s *KafkaPublishDataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("not implemented") -} +func (s *KafkaPublishDataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { + return nil, fmt.Errorf("kafka publish: file uploads are not supported") +}
46-57: Choose one error strategy: return error or JSON status, not both.Returning non-nil error and a JSON body can cause double-handling upstream. Prefer nil error with explicit JSON or return only error.
Option A (error-only):
- if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { - return []byte(`{"success": false}`), err - } - return []byte(`{"success": true}`), nil + if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { + return nil, err + } + return []byte(`{"success": true}`), nilOption B (JSON-only, nil error):
- if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { - return []byte(`{"success": false}`), err - } + if err := s.pubSub.Publish(ctx, publishConfiguration); err != nil { + return []byte(`{"success": false","error":"publish failed"}`), nil + }Pick consistently across datasources.
Please confirm the engine’s expectation for datasource error handling so we align consistently.
v2/pkg/engine/resolve/resolvable.go (1)
81-100: Reset clears the arena; ensure it is set before next Init/ResolveReset sets r.astjsonArena = nil. Any subsequent Init/Resolve must set the arena first, or calls like astjson.ObjectValue/ParseBytesWithArena will likely panic or mis-allocate. Add a comment or a guard, and ensure all call sites set the arena before use.
Would you like me to add a SetArena(a arena.Arena) helper and guard Init when arena is nil?v2/pkg/engine/resolve/resolve.go (3)
1089-1114: Set arena in subscription SkipLoader path for consistencySkipLoader still constructs Resolvable with nil arena. It often works, but error/value-completion paths would allocate via arena. Acquire/assign an arena as in ResolveGraphQLResponse to avoid surprises.
Example:
- t := newTools(r.options, r.allowedErrorExtensionFields, r.allowedErrorFields, r.sf) + t := newTools(r.options, r.allowedErrorExtensionFields, r.allowedErrorFields, r.sf) + resolveArena := r.resolveArenaPool.Acquire(ctx.Request.ID) + t.loader.jsonArena = resolveArena.Arena + t.resolvable.astjsonArena = resolveArena.Arena + defer r.resolveArenaPool.Release(ctx.Request.ID, resolveArena)
1199-1224: Also set arena in Async subscription SkipLoader pathSame rationale as above; apply the same acquire/assign/release here.
1168-1174: Improve uniqueID mixing: write headersHash into hasher instead of addingAdding 64-bit hashes can increase collisions. Prefer feeding headersHash bytes into xxhash and use Sum64().
Suggested change (add import "encoding/binary"):
+ import "encoding/binary" @@ - _, _ = xxh.Write(input) - // the hash for subgraph headers is pre-computed - // we can just add it to the input hash to get a unique id - uniqueID := xxh.Sum64() + headersHash + _, _ = xxh.Write(input) + var hb [8]byte + binary.LittleEndian.PutUint64(hb[:], headersHash) + _, _ = xxh.Write(hb[:]) + uniqueID := xxh.Sum64()Also applies to: 1226-1233
v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go (2)
43-51: Headers currently unusedStart receives headers but the underlying NatsPubSub.Subscribe doesn’t. If headers are intentionally ignored for NATS, add a short comment; otherwise consider plumbing usage or dropping the param in this source.
79-93: LGTM: request path returns data slice and propagates errorReadability nit: rename subscriptionConfiguration -> requestConfiguration. Optional.
v2/pkg/engine/resolve/resolve_test.go (1)
112-136: Remove or use unused parameter enableSingleFlight.The enableSingleFlight bool is unused in testFn; either wire it to options or remove it to avoid confusion.
v2/pkg/engine/datasource/httpclient/nethttpclient.go (4)
315-325: Confirm FileUpload lifetime; avoid deleting user files.The defer removes every opened file (os.Remove). If FileUpload.Path() can point to non-temporary user files, this deletes user data.
Option: only delete when FileUpload is marked temporary, or move cleanup behind an explicit flag.
- if err = os.Remove(file.Name()); err != nil { + if fileShouldBeDeleted(file) { // e.g., a field or method on FileUpload + if err = os.Remove(file.Name()); err != nil { return - } + } + }Please confirm the contract of FileUpload.Path().
232-241: Close decompressor readers after use to free resources.gzip/deflate readers implement io.ReadCloser. Close them after ReadFrom to release resources early.
out := buffer(ctx) -_, err = out.ReadFrom(respReader) +_, err = out.ReadFrom(respReader) if err != nil { return nil, err } +if rc, ok := respReader.(io.ReadCloser); ok { + _ = rc.Close() +}
209-216: Prefer a single Accept-Encoding value.Set “gzip, deflate” once instead of two separate headers; clearer and avoids duplicate values.
-request.Header.Set(AcceptEncodingHeader, EncodingGzip) -request.Header.Add(AcceptEncodingHeader, EncodingDeflate) +request.Header.Set(AcceptEncodingHeader, EncodingGzip+", "+EncodingDeflate)
238-265: Trace injection assumes JSON object; guard or fallback for non-objects.jsonparser.Set will fail for non-object bodies (arrays, scalars). Consider detecting object first; otherwise return original data and put trace into ResponseContext only.
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (2)
406-415: Clarify nesting bound check.if level > md.NestingLevel likely intends >=; using >= avoids an unnecessary extra frame and is easier to reason about.
- if level > md.NestingLevel { + if level >= md.NestingLevel { return current, nil }
114-121: Consider exposing a Reset to reuse jsonBuilder arenas safely.If jsonBuilder instances are reused, add a Reset method to re-init jsonArena (or allocate per call) to avoid unbounded growth.
v2/pkg/engine/resolve/loader.go (1)
432-446: Consider using arena for array creation.The function now accepts an arena parameter and uses it for
SetArrayItem, but the array itself is created withMustParseBytes(line 441) without the arena. For consistency and to fully leverage arena benefits, consider:- arr := astjson.MustParseBytes([]byte(`[]`)) + arr := astjson.ArrayValue(a) for i, item := range items { arr.SetArrayItem(a, i, item) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumv2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
v2/go.mod(1 hunks)v2/pkg/astnormalization/uploads/upload_finder.go(1 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(3 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(14 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go(0 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(0 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(5 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(25 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(15 hunks)v2/pkg/engine/datasource/httpclient/httpclient_test.go(2 hunks)v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/fixtures/type_introspection.golden(1 hunks)v2/pkg/engine/datasource/introspection_datasource/source.go(3 hunks)v2/pkg/engine/datasource/introspection_datasource/source_test.go(1 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go(4 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go(3 hunks)v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go(3 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource.go(2 hunks)v2/pkg/engine/plan/planner_test.go(2 hunks)v2/pkg/engine/plan/visitor.go(1 hunks)v2/pkg/engine/resolve/arena.go(1 hunks)v2/pkg/engine/resolve/authorization_test.go(3 hunks)v2/pkg/engine/resolve/context.go(2 hunks)v2/pkg/engine/resolve/datasource.go(1 hunks)v2/pkg/engine/resolve/event_loop_test.go(2 hunks)v2/pkg/engine/resolve/inputtemplate.go(7 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/loader_hooks_test.go(14 hunks)v2/pkg/engine/resolve/loader_test.go(14 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go(2 hunks)v2/pkg/engine/resolve/resolvable_test.go(26 hunks)v2/pkg/engine/resolve/resolve.go(20 hunks)v2/pkg/engine/resolve/resolve_federation_test.go(21 hunks)v2/pkg/engine/resolve/resolve_mock_test.go(3 hunks)v2/pkg/engine/resolve/resolve_test.go(29 hunks)v2/pkg/engine/resolve/response.go(1 hunks)v2/pkg/engine/resolve/singleflight.go(1 hunks)v2/pkg/engine/resolve/tainted_objects_test.go(3 hunks)v2/pkg/engine/resolve/variables_renderer.go(1 hunks)v2/pkg/fastjsonext/fastjsonext.go(1 hunks)v2/pkg/fastjsonext/fastjsonext_test.go(1 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(1 hunks)
💤 Files with no reviewable changes (2)
- v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go
🧰 Additional context used
🧬 Code graph analysis (30)
v2/pkg/engine/resolve/variables_renderer.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/plan/planner.go (1)
IncludeQueryPlanInResponse(92-96)
v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/resolve/loader_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/datasource/introspection_datasource/source.go (2)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolve_federation_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/resolve/response.go (4)
v2/pkg/engine/resolve/inputtemplate.go (1)
InputTemplate(31-37)v2/pkg/engine/resolve/variables.go (1)
Variables(27-27)v2/pkg/engine/resolve/datasource.go (1)
SubscriptionDataSource(15-19)v2/pkg/engine/resolve/fetch.go (2)
PostProcessingConfiguration(116-132)QueryPlan(251-254)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (3)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
DoMultipartForm(272-328)Do(267-270)
v2/pkg/engine/resolve/resolve_mock_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolvable_test.go (1)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (3)
v2/pkg/engine/resolve/context.go (1)
NewContext(168-175)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
SubscriptionSource(1953-1955)v2/pkg/engine/datasource/httpclient/file.go (2)
FileUpload(3-7)NewFileUpload(9-15)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(181-245)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/plan/planner_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/resolve_test.go (10)
v2/pkg/engine/resolve/context.go (2)
Context(16-37)ExecutionOptions(50-56)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
Do(267-270)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)v2/pkg/engine/resolve/response.go (2)
GraphQLResponse(35-43)GraphQLResponseInfo(45-47)v2/pkg/engine/resolve/fetchtree.go (6)
Single(59-67)SingleWithPath(69-82)Sequence(26-31)Parallel(33-38)ArrayPath(52-57)ObjectPath(40-45)v2/pkg/engine/resolve/fetch.go (4)
SingleFetch(91-99)SingleFetch(153-155)FetchConfiguration(270-302)PostProcessingConfiguration(116-132)v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/resolve/inputtemplate.go (5)
InputTemplate(31-37)TemplateSegment(22-29)SegmentType(15-15)StaticSegmentType(18-18)VariableSegmentType(19-19)v2/pkg/engine/resolve/variables.go (4)
VariableKind(7-7)ContextVariableKind(10-10)Variables(27-27)ResolvableObjectVariableKind(13-13)v2/pkg/engine/resolve/subscription_filter.go (2)
SubscriptionFilter(16-21)SubscriptionFieldFilter(23-26)
v2/pkg/engine/resolve/singleflight.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(181-245)v2/pkg/engine/resolve/fetch.go (3)
FetchItem(29-34)Fetch(20-27)FetchInfo(376-397)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_kafka.go (3)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)v2/pkg/engine/datasource/pubsub_datasource/kafka_event_manager.go (1)
KafkaPublishEventConfiguration(17-21)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(16-22)v2/pkg/engine/resolve/fetch.go (5)
FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(139-141)
v2/pkg/engine/resolve/inputtemplate.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)
v2/pkg/engine/resolve/datasource.go (3)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)
v2/pkg/engine/resolve/event_loop_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (4)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
Source(1829-1831)v2/pkg/engine/datasource/introspection_datasource/source.go (1)
Source(18-20)v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/fastjsonext/fastjsonext_test.go (1)
v2/pkg/fastjsonext/fastjsonext.go (3)
AppendErrorToArray(8-15)PathElement(29-32)CreateErrorObjectWithPath(34-50)
v2/pkg/engine/resolve/resolve.go (6)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(13-17)NewArenaPool(30-34)v2/pkg/engine/resolve/singleflight.go (2)
SingleFlight(16-22)NewSingleFlight(29-41)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(71-79)ResolvableOptions(64-69)v2/pkg/engine/resolve/context.go (4)
Context(16-37)Request(163-166)ExecutionOptions(50-56)SubgraphHeadersBuilder(39-41)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(35-43)GraphQLSubscription(12-16)SubscriptionResponseWriter(68-74)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
RPCMessage(71-85)ListMetadata(190-195)LevelInfo(198-201)
v2/pkg/engine/resolve/loader_hooks_test.go (1)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)
v2/pkg/engine/datasource/pubsub_datasource/pubsub_nats.go (4)
v2/pkg/engine/resolve/context.go (2)
Context(16-37)Request(163-166)v2/pkg/engine/resolve/resolve.go (1)
SubscriptionUpdater(1383-1390)v2/pkg/engine/datasource/pubsub_datasource/nats_event_manager.go (1)
NatsPublishAndRequestEventConfiguration(31-35)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/resolve/authorization_test.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/resolve/resolve_mock_test.go (1)
NewMockDataSource(28-32)
v2/pkg/engine/datasource/httpclient/httpclient_test.go (1)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
Do(267-270)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/loader_test.go
[error] 1046-1046: BenchmarkLoader_LoadGraphQLResponseData failed: expected a response containing an errors field, but got a response without 'errors' field. Deep comparison mismatch detected in test.
v2/pkg/engine/resolve/inputtemplate.go
[error] 161-161: golangci-lint: Error return value of preparedInput.WriteString is not checked (errcheck).
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolvable.go
[failure] 14-14:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 20-20:
File is not properly formatted (gci)
v2/pkg/engine/resolve/resolve.go
[failure] 15-15:
File is not properly formatted (gci)
🔇 Additional comments (52)
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection_with_custom_root_operation_types.golden (1)
514-514: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/datasource/introspection_datasource/fixtures/schema_introspection.golden (1)
366-366: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/datasource/introspection_datasource/fixtures/type_introspection.golden (1)
59-59: LGTM - Formatting-only change.The trailing newline adjustment has no functional impact on the fixture data.
v2/pkg/engine/resolve/inputtemplate.go (3)
7-7: LGTM!The
ioimport is necessary for theInputTemplateWriterinterface definition.
58-63: LGTM! Clean abstraction for arena-based memory management.The interface is well-designed and appropriately scoped, including only the methods needed for the rendering flow. This abstraction enables the arena-based memory management improvements mentioned in the PR objectives while maintaining compatibility with
*bytes.Buffer.
65-65: All callers are compatible with the interface change.The exported methods
RenderandRenderAndCollectUndefinedVariableshave been correctly updated to acceptInputTemplateWriter. Sincebytes.Bufferimplements the required interface (io.Writer, io.StringWriter, Reset(), Bytes()), all existing callers—in loader.go, resolve.go, subscription_filter.go, and tests—pass compatible types without requiring updates.v2/pkg/engine/resolve/tainted_objects_test.go (1)
73-75: LGTM: switched to astjson.ParseBytesThe change is straightforward; parsing behavior in tests remains correct.
Also applies to: 97-99, 323-327
v2/pkg/engine/plan/visitor.go (1)
1293-1296: Populate SourceName/ID on subscription triggerGood addition; aligns tests and surfaces subgraph identity on triggers.
Confirm objectFetchConfiguration always sets both sourceName and sourceID for subscription fetches; if either can be empty, consider defaulting SourceName to sourceID (and vice versa) for consistency.
v2/pkg/engine/resolve/response.go (1)
19-26: LGTM: GraphQLSubscriptionTrigger includes SourceName/SourceIDPublic surface extended as intended; no behavioral changes.
v2/pkg/engine/datasource/pubsub_datasource/pubsub_datasource_test.go (1)
427-429: Tests updated for SourceName/SourceIDExpectations match the new trigger fields and datasource id/name.
Also applies to: 492-494, 539-541, 592-594
v2/pkg/engine/resolve/resolve_federation_test.go (1)
19-28: Mocks updated for new Load signature and response []byteGood updates: include headers param and return []byte. Assertions validate inputs properly.
Also applies to: 23-27
v2/pkg/engine/resolve/context.go (1)
164-166: Request.ID type change verified — all consumers compatibleThe type change from prior type to
uint64is verified as safe:
- Pooling/dedup: Both
ArenaPool.Acquire()andArenaPool.Release()have signatures expectinguint64; all call sites passctx.Request.IDdirectly—fully compatible.- Logging: No code logs
Request.ID.- Tracing:
RequestData(the struct included in trace output) contains noIDfield;Request.IDis not part of trace output.- JSON marshaling: No JSON marshaling of
Requestor itsIDfield.v2/go.mod (1)
31-33: All concerns verified—no issues found.The repository correctly uses Go 1.25 across both
go.moddirective and CI workflows (matrix values in v2.yml and execution.yml both specifygo: [ "1.25" ]). Thesetup-go@v5action is compatible with this version. The astjson v1.0.0 and go-arena v1.0.0 dependencies are properly listed in the require section, go.sum is present and synchronized (226 entries), and no deprecated astjson APIs are in use. The module is tidy.v2/pkg/astnormalization/uploads/upload_finder.go (1)
77-77: ParseBytes migration verified—behavior parity and concurrency characteristics confirmed.Edge case handling (nil, "", "null" → "{}") is explicit at lines 74–76, ensuring normalized input regardless of ParseBytes implementation. Variables are accessed only within the same
FindUploadsinvocation, with no shared Parser instance—each call is independent. The migration is safe and complete (noParseBytesWithoutCachecalls remain in the codebase).v2/pkg/engine/resolve/authorization_test.go (2)
512-519: LGTM! Mock signature correctly updated.The mock expectation properly reflects the new DataSource Load signature with HTTP headers and direct byte slice return. The DoAndReturn handler is correctly structured to return
([]byte, error).
817-824: LGTM! Consistent mock pattern.All mock data source setups in this test file follow the same pattern with the updated Load signature. The test data continues to return valid GraphQL responses wrapped in the expected format.
v2/pkg/engine/resolve/resolvable_custom_field_renderer_test.go (1)
443-443: LGTM! Constructor signature updated correctly.The NewResolvable call now includes the arena.Arena parameter (nil) as the first argument, aligning with the updated constructor signature in the broader PR.
v2/pkg/fastjsonext/fastjsonext_test.go (1)
24-29: LGTM! Arena parameter migration applied consistently.The test correctly passes
nilfor the arena parameter toAppendErrorToArray, aligning with the PR's migration from*astjson.Arenatoarena.Arenainterface.v2/pkg/engine/resolve/event_loop_test.go (1)
74-85: LGTM! Subscription Start signature updated.The Start method now correctly accepts HTTP headers as the second parameter, consistent with the PR's objective to propagate headers through the data flow. The method implementation remains functionally correct.
v2/pkg/engine/resolve/loader_test.go (3)
20-34: LGTM! Mock responses correctly structured.The mock data sources now return responses wrapped under a top-level
"data"field, consistent with the GraphQL response format and the broader changes in this PR.
290-300: LGTM! Test setup correctly updated.Both the NewResolvable constructor call and the expected output assertion have been updated to match the new patterns introduced in the PR.
1524-1530: LGTM! Error path rewriting correctly updated.The rewriteErrorPaths function calls now pass
nilas the first parameter (arena), consistent with the arena-based memory management changes in this PR.v2/pkg/engine/datasource/httpclient/httpclient_test.go (1)
82-85: LGTM! Test correctly updated for new Do signature.The test now properly handles the new Do signature that returns
([]byte, error)directly instead of writing to a buffer. The assertion compares the returned bytes with the expected output.v2/pkg/engine/resolve/loader_hooks_test.go (1)
52-56: LGTM! Mock expectations correctly updated.The mock Load method now expects three parameters (context, headers, input) and the DoAndReturn handler correctly returns
([]byte, error), aligning with the updated DataSource interface.v2/pkg/engine/datasource/introspection_datasource/source.go (2)
22-33: LGTM! Load method signature correctly updated.The Load method now:
- Accepts
headers http.Headeras the second parameter- Returns
(data []byte, err error)instead of writing to a buffer- Uses the new
singleTypeByteshelper for type-specific responsesAll error paths correctly return
(nil, err)on failure.
61-68: LGTM! Helper method properly implements byte-returning pattern.The new
singleTypeByteshelper correctly mirrors the previoussingleTypelogic but returns bytes directly instead of writing to an io.Writer. The nil type handling properly returns thenullbyte slice.v2/pkg/engine/plan/planner_test.go (3)
7-7: LGTM - Import added for new API.The
net/httpimport is correctly added to support the newhttp.Headerparameter in the updated DataSource method signatures.
1078-1080: LGTM - Test mock updated to new API.The
Loadmethod signature correctly reflects the new DataSource API that accepts HTTP headers and returns data directly instead of writing to a buffer. Returningnil, nilis appropriate for a test fake.
1082-1084: LGTM - Test mock updated consistently.The
LoadWithFilesmethod signature correctly mirrors theLoadmethod changes with HTTP headers and direct return values. The test fake implementation is appropriate.v2/pkg/engine/datasource/staticdatasource/static_datasource.go (2)
5-5: LGTM - Import added for new API.The
net/httpimport correctly supports the updated method signatures.
74-76: LGTM - Static source correctly returns input.The implementation appropriately returns the input bytes directly, which is the expected behavior for a static data source.
v2/pkg/engine/resolve/resolve_mock_test.go (1)
1-67: LGTM - Auto-generated mock updated correctly.This is an auto-generated mock file (by MockGen) that has been correctly regenerated to match the new DataSource interface signatures with HTTP headers and direct return values. No manual review concerns.
v2/pkg/fastjsonext/fastjsonext.go (5)
5-5: LGTM - Arena dependency added for memory pooling.The
go-arenaimport supports the PR's objective of improving memory management through arena-based pooling.
8-15: LGTM - Arena-based error appending refactored correctly.The function signature and implementation correctly adopt the new arena-centric API, passing the arena context through value creation and mutation calls.
17-27: LGTM - Consistent arena usage for error with extensions.The function correctly uses arena-based value construction throughout, maintaining consistency with the new API pattern.
34-50: LGTM - Error object creation properly arena-aware.The function correctly constructs error objects using arena-based value creation, ensuring memory efficiency and proper lifecycle management.
52-59: No issues found - nil arena usage is intentional and correct.After thorough verification:
- The
PrintGraphQLResponsefunction usesout.Set(nil, ...)to set fields on a value created byastjson.MustParse- This nil arena pattern is consistent with the codebase: the codebase explicitly uses
ParseBytesWithArena(nil, data)in loader_test.go, demonstrating nil arena is an established, intentional pattern- Tests pass successfully across multiple scenarios (loader_test.go lines 297, 759, 1044, 1431), confirming the function works correctly
- The function serves as a terminal operation that composes pre-constructed values and serializes them—it does not manage arena lifetimes like the building functions (
AppendErrorToArray,CreateErrorObjectWithPath)- The design is appropriate: building functions receive and manage explicit arena parameters, while terminal functions like
PrintGraphQLResponseuse nil arena for simpler composition and serializationv2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (2)
4023-4025: Include of SourceName/SourceID on subscription trigger looks rightGood addition for tracing/observability and source attribution. Please just confirm the values match the configured DataSource ID in the surrounding plan to avoid misleading logs/metrics.
Also applies to: 4066-4068
8773-8776: Load now returns bytes — assertions look goodThe switch to capturing data from Load and asserting on the exact JSON payload is correct and matches the new API.
Also applies to: 8795-8798
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1909-1917: Good: headers plumbed and return-bytes API adhered to.The methods correctly compact variables, forward headers to httpclient, and return data bytes.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
404-409: Assertion aligns with error-as-data policy; keep consistent with Load behavior.No change required; just ensure datasource consistently returns nil error in all error paths (see datasource comment).
If you adopt the merge-error change, re-run this test to confirm it still passes.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
101-129: ****The repository declares Go 1.25 in its go.mod files. Since Go 1.22+, loop variables are scoped per-iteration by default, automatically preventing the closure capture issue the review comment warns about. The suggested rebinding refactor is unnecessary—the original code is correct and does not have a closure capture bug. No changes are required.
Likely an incorrect or invalid review comment.
v2/pkg/engine/resolve/resolve.go (2)
5-20: Fix import ordering to satisfy gciLinters report “File is not properly formatted (gci)”. Run gci/fmt over this file (and module-wide) to fix grouping/order.
Example:
- standard lib
- third-party
- project-local
306-349: ArenaResolveGraphQLResponse looks solidAcquire/assign/release of arenas and buffered write via arena buffer are correct.
v2/pkg/engine/resolve/loader.go (8)
360-373: Good use of arena allocation for items.The arena-based allocation in
selectItemsForPathproperly uses the loader's arena for memory management, which aligns with the PR's memory management improvements.
547-549: Proper arena allocation before parsing.The code correctly allocates a slice copy in the arena before parsing, ensuring the parsed data is arena-managed throughout its lifecycle.
1576-1592: Operation type context propagation looks good.The new context key pattern for operation type propagation is implemented correctly with a safe default fallback.
1594-1603: Headers extraction method looks correct.The
headersForSubgraphRequestmethod properly handles nil checks and extracts headers for subgraph requests, with the extraKey used for request deduplication.
1656-1666: Direct load implementation is correct.The
loadByContextDirectmethod properly calls the updatedDataSourceinterface methods with headers, aligning with the PR's API changes.
1806-1814: Execution now properly uses loadByContext.The refactored execution flow correctly delegates to
loadByContext, which encapsulates SingleFlight deduplication logic while respecting loader hooks.
791-791: Consistent arena usage for error object creation.All error object creation now uses
ParseWithArena, ensuring arena-based memory management is applied consistently throughout the error handling paths.Also applies to: 1018-1028, 1065-1065, 1079-1079, 1091-1091, 1109-1109, 1144-1144, 1150-1150, 1160-1160, 1166-1166, 1186-1186, 1191-1191, 1198-1198, 1203-1203, 1210-1210, 1214-1214
185-186: ****The initialization concern is unfounded. Both
jsonArenaandsfare properly initialized before use:
sfis assigned innewTools()(line 265 in resolve.go) from the Resolver'ssffield, which is created withNewSingleFlight()at Resolver initialization.jsonArenais assigned from the arena pool inresolve.go(lines 319, 485) immediately before the loader is used, after whichLoadGraphQLResponseDatais called.The fields don't need cleanup in
Free()because they're lifecycle-managed by their respective owners (Resolver forsf, arena pool forjsonArena), not by the Loader.Likely an incorrect or invalid review comment.
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (3)
163-165: Avoid mutating/capturing caller's headers; clone before use.Direct assignment
request.Header = baseHeadersaliases the map, allowing subsequentAdd/Setcalls to mutate the caller's map, which can cause race conditions across concurrent requests.Apply this diff:
if baseHeaders != nil { - request.Header = baseHeaders + request.Header = baseHeaders.Clone() }
219-223: Set Request.ContentLength field instead of the header.Manually setting the
Content-Lengthheader is ignored or overwritten by net/http. Therequest.ContentLengthfield should be set instead.Apply this diff:
if contentLength > 0 { - // always set the Content-Length Header so that chunking can be avoided - // and other parties can more efficiently parse - request.Header.Set(ContentLengthHeader, fmt.Sprintf("%d", contentLength)) + request.ContentLength = int64(contentLength) }Note: Remove the header manipulation; net/http will populate the header from the field. For streaming bodies with unknown length, leave
ContentLengthat its default (-1).
314-319: Check os.Open error before appending; prevent nil deref in cleanup.
temporaryFileis appended totempFilesbefore checking the error fromos.Open. If the open fails,tempFileswill contain a nil entry, causing the defer cleanup at lines 329-339 to panic when attempting to close or remove the file.Apply this diff:
temporaryFile, err := os.Open(file.Path()) -tempFiles = append(tempFiles, temporaryFile) if err != nil { return nil, err } +tempFiles = append(tempFiles, temporaryFile) formValues[key] = bufio.NewReader(temporaryFile)v2/pkg/engine/resolve/loader.go (1)
20-20: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolvable.go (1)
14-14: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/resolvable.gov2/pkg/engine/resolve/resolve.go (1)
15-15: Fix import ordering per static analysis.The
gcilinter indicates this file is not properly formatted. Ensure imports are grouped correctly (standard library, external, internal).Run the following to fix:
#!/bin/bash # Format the file with gci gci write --skip-generated -s standard -s default -s "prefix(github.com/wundergraph/graphql-go-tools)" v2/pkg/engine/resolve/resolve.go
🧹 Nitpick comments (3)
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go (1)
2440-2440: LGTM - discarding return values is acceptable here.Explicitly discarding both return values from
fmt.Fprintfis a common pattern in test mock handlers. Since this test focuses on WebSocket upgrade failure handling rather than the mock server's write success, this is acceptable.If you want to be more defensive, consider checking the error:
- _, _ = fmt.Fprintf(w, `{"error": "WebSocket upgrade failed", "status": %d}`, tc.statusCode) + _, err := fmt.Fprintf(w, `{"error": "WebSocket upgrade failed", "status": %d}`, tc.statusCode) + require.NoError(t, err)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
329-339: Consider completing cleanup even on error.The early
returnonClose()orRemove()errors prevents cleanup of remaining files, potentially leaking file descriptors. While minor compared to the nil entry issue at lines 314-319, consider logging errors and continuing the cleanup loop.Example approach:
defer func() { multipartBody.Close() for _, file := range tempFiles { - if err := file.Close(); err != nil { - return - } - if err = os.Remove(file.Name()); err != nil { - return - } + if closeErr := file.Close(); closeErr != nil { + // log closeErr but continue cleanup + } + if removeErr := os.Remove(file.Name()); removeErr != nil { + // log removeErr but continue cleanup + } } }()v2/pkg/engine/resolve/loader.go (1)
1636-1686: Consider defensive copy when caching single-flight responses.Line 1684 stores
res.outdirectly into the shared cache without copying. WhilemergeResult(lines 556-558) makes a defensive copy before parsing, the cacheditem.responseslice could be problematic if:
- The
DataSource.Load()implementation reuses the backing array across calls, or- Concurrent readers access the cached response while the backing array is modified elsewhere.
A defensive copy at line 1684 would eliminate this risk:
- item.response = res.out + item.response = append([]byte(nil), res.out...)This aligns with the defensive copy pattern already used at line 547-548 (in earlier code) and lines 556-558.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/engine/datasource/graphql_datasource/graphql_subscription_client_test.go(1 hunks)v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/resolve/context.go(2 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)v2/pkg/engine/resolve/resolve.go(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/context.go
🧰 Additional context used
🧬 Code graph analysis (4)
v2/pkg/engine/resolve/resolve.go (7)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(13-17)NewArenaPool(30-34)v2/pkg/engine/resolve/singleflight.go (2)
SingleFlight(16-22)NewSingleFlight(29-41)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/context.go (4)
Context(16-37)Request(170-173)ExecutionOptions(57-63)SubgraphHeadersBuilder(43-47)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(35-43)GraphQLSubscription(12-16)SubscriptionResponseWriter(68-74)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(16-22)v2/pkg/engine/resolve/fetch.go (5)
FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/context.go (3)
RateLimitOptions(129-142)Context(16-37)ExecutionOptions(57-63)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(187-251)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolve.go
[failure] 15-15:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 20-20:
File is not properly formatted (gci)
v2/pkg/engine/resolve/resolvable.go
[failure] 14-14:
File is not properly formatted (gci)
🔇 Additional comments (19)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (6)
30-30: LGTM: constant addition.The
ContentLengthHeaderconstant is properly defined and aligns with other header constants in the file.
133-154: LGTM: well-designed size hint mechanism.The context-based size hint with a custom key type prevents collisions, and the buffer sizing logic balances memory efficiency with allocation overhead.
240-250: LGTM: thoughtful buffer lifecycle management.The comment explaining the GC-based approach over pooling is valuable documentation. The buffer creation leverages the size hint mechanism to reduce allocations efficiently.
252-278: LGTM: clean trace extension handling.The conditional trace embedding is well-structured, and error propagation follows the consistent
(nil, err)pattern throughout.
281-284: LGTM: signature change aligns with PR objectives.The updated signature to return
[]bytedirectly and acceptbaseHeadersmatches the PR's goal of improving memory management and moving request deduplication into the engine.
341-341: LGTM: appropriate contentLength for multipart.Passing
contentLength=0for the multipart body is correct since the body is a pipe reader with unknown length at request creation time.v2/pkg/engine/resolve/loader.go (5)
186-196: Arena and single-flight integration looks good.The documentation clearly explains the arena's thread-safety constraints and lifecycle. The warning about tying parsed JSON's underlying bytes to the arena lifecycle is crucial for avoiding segfaults.
369-383: Arena-aware helper functions are well-structured.The consistent pattern of accepting
arena.Arenaas the first parameter and using arena-backed allocations throughoutselectItemsForPath,selectItems, anditemsDataensures memory is managed correctly.Also applies to: 401-439, 441-455
556-558: Defensive copy before parsing is essential.This pattern correctly ties the lifecycle of the parsed JSON to the arena-allocated slice, preventing segfaults as noted in the comments at lines 190-193.
752-831: Arena-aware error handling is consistently implemented.The lazy initialization via
l.resolvable.ensureErrorsInitialized()(lines 788, 828, etc.) is a good optimization, and arena-backed object creation throughout error rendering paths is correct.Also applies to: 873-1044
1606-1624: New context helper looks good.
GetOperationTypeFromContextprovides a clean way to access the operation type from context, useful for the transport layer to make decisions (e.g., disabling single-flight for mutations).v2/pkg/engine/resolve/resolvable.go (3)
72-80: Constructor signature updated for arena support.Accepting
arena.Arenaas a parameter is correct. Callers must ensure they provide a valid arena before calling methods that use it (likeInit), or rely on the newArenaResolveGraphQLResponsepath in resolve.go.
240-245: Lazy error initialization is an excellent optimization.This pattern avoids allocating the errors array upfront, which can significantly reduce memory usage when most operations complete without errors. The consistent use of
ensureErrorsInitialized()throughout the file (lines 773, 1215, 1286, 1292, 1298) ensures correctness.
110-128: Arena integration in Init methods looks correct.Both
InitandInitSubscriptionproperly use arena-backed object creation and parsing. The explicit comments about not initializing errors (lines 115-116, 134-135) helpfully document the lazy initialization strategy.Also applies to: 130-166
v2/pkg/engine/resolve/resolve.go (5)
75-85: Arena pools and single-flight initialization look excellent.The separation of
resolveArenaPoolandresponseBufferPoolis well-reasoned (as explained in the comments), and both pools along with the single-flight cache are properly initialized.Also applies to: 239-241
313-356: ArenaResolveGraphQLResponse has excellent arena lifecycle management.This new method demonstrates proper arena handling:
- Acquires arenas before use (lines 325, 343)
- Sets arenas on both loader and resolvable (lines 326-327)
- Releases arenas on all error paths (lines 331, 338, 347-349)
- Releases arenas after successful completion (lines 352, 354)
The use of arena-backed buffers (line 344) for response writing is also correct.
488-530: Subscription update arena handling is correct.The pattern of acquiring the arena (line 490), setting it on tools (lines 491-492), and releasing on all exit paths (lines 495, 507, 519, 530) is consistent and safe.
1079-1084: Header propagation helper is clean and straightforward.The
triggerHeadersmethod correctly delegates toSubgraphHeadersBuilderwhen available and provides sensible defaults.
253-275: Clarify the misleading "we set the arena manually" comment and verify nil arena handling in ResolveGraphQLResponse.The code shows both ResolveGraphQLResponse and ArenaResolveGraphQLResponse exist in parallel, but the comment at line 255 is inaccurate. ResolveGraphQLResponse never sets the arena before calling Init (line 293)—only ArenaResolveGraphQLResponse does. The data parameter difference between them (ResolveGraphQLResponse passes data bytes, ArenaResolveGraphQLResponse passes nil) may influence arena behavior, but without examining the astjson library's nil-arena handling, it's unclear if this is an intentional design or a latent issue.
Either:
- Update the comment to clarify that newTools intentionally creates Resolvable with nil arena for the non-arena code path, and verify tests cover this path
- Or verify that astjson functions (ObjectValue, ParseBytesWithArena, MergeValues) safely handle nil arenas
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
314-318: Critical: os.Open error still checked after append - can cause nil dereference.The past review comment on this issue was not addressed. When
os.Openfails,temporaryFileis nil but still appended totempFiles. The defer cleanup at line 332 will then panic when callingfile.Close()on the nil entry.Apply this diff to fix:
temporaryFile, err := os.Open(file.Path()) - tempFiles = append(tempFiles, temporaryFile) if err != nil { return nil, err } + tempFiles = append(tempFiles, temporaryFile)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/pkg/engine/datasource/httpclient/nethttpclient.go(9 hunks)v2/pkg/engine/resolve/inputtemplate.go(8 hunks)v2/pkg/engine/resolve/loader.go(50 hunks)v2/pkg/engine/resolve/loader_test.go(14 hunks)v2/pkg/engine/resolve/resolvable.go(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/resolvable.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(187-251)v2/pkg/fastjsonext/fastjsonext.go (2)
AppendErrorToArray(8-15)AppendErrorWithExtensionsCodeToArray(17-27)
v2/pkg/engine/resolve/inputtemplate.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/lexer/literal/literal.go (1)
COMMA(23-23)
v2/pkg/engine/resolve/loader_test.go (2)
v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/loader.go (1)
Loader(159-196)
v2/pkg/engine/resolve/loader.go (5)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
ResponseContext(72-76)WithHTTPClientSizeHint(142-144)v2/pkg/engine/resolve/fetch.go (11)
SingleFetch(91-99)SingleFetch(153-155)BatchEntityFetch(160-169)BatchEntityFetch(194-196)EntityFetch(200-209)EntityFetch(226-228)FetchItemPathElement(78-82)FetchItem(29-34)DataSourceLoadTrace(405-419)Fetch(20-27)FetchInfo(376-397)v2/pkg/engine/resolve/context.go (3)
RateLimitOptions(129-142)Context(16-37)ExecutionOptions(57-63)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
v2/pkg/engine/resolve/context.go (1)
Context(16-37)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
🪛 GitHub Actions: v2-ci
v2/pkg/engine/resolve/loader.go
[error] 23-23: golangci-lint: File is not properly formatted (gci).
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/resolvable.go
[failure] 16-16:
File is not properly formatted (gci)
v2/pkg/engine/resolve/loader.go
[failure] 23-23:
File is not properly formatted (gci)
🔇 Additional comments (12)
v2/pkg/engine/resolve/resolvable.go (3)
239-244: LGTM: Well-designed lazy initialization helper.The
ensureErrorsInitialized()helper cleanly implements the lazy initialization pattern for the errors array, supporting the memory optimization goal mentioned in the PR objectives. The consistent usage throughout the file (lines 772, 1214, 1285, 1291, 1297) ensures errors are only allocated when needed.
113-115: Effective memory optimization through lazy initialization.Setting
r.errors = nilinstead of eager initialization is a good optimization that defers allocation until errors actually occur. The inline comments clearly document this design decision, and theensureErrorsInitialized()helper ensures safe usage throughout the codebase.Also applies to: 133-134, 172-173
71-79: Intentional API change for arena integration.The constructor signature change from
NewResolvable(context.Context, ResolvableOptions)toNewResolvable(arena.Arena, ResolvableOptions)is a breaking change that aligns with the PR's arena-based memory management objectives. All test call sites have been correctly updated to passnilor an appropriate arena instance.v2/pkg/engine/resolve/loader_test.go (1)
290-290: Test updates correctly reflect API changes.All test instantiations of
NewResolvablehave been properly updated to passnilas the first arena parameter, consistent with the new constructor signature. This approach is appropriate for tests that don't require specific arena lifecycle management.Also applies to: 379-379, 470-470, 752-752, 1027-1027, 1128-1128, 1424-1424
v2/pkg/engine/resolve/inputtemplate.go (2)
58-63: Well-designed abstraction with InputTemplateWriter interface.The new
InputTemplateWriterinterface cleanly abstracts buffer operations (io.Writer,io.StringWriter,Reset(),Bytes()) and enables controlled reuse across the template rendering pipeline. This design improves testability and flexibility.
161-174: Error handling improvement for write operations.Previously, write errors in
renderHeaderVariablewere silently ignored using_, _ = preparedInput.Write(...). The updated code now properly checks and propagates these errors (lines 161-163, 168-174), which improves robustness and makes potential issues visible to callers.v2/pkg/engine/resolve/loader.go (3)
185-195: Excellent documentation of arena thread-safety constraints.The detailed comments on
jsonArenaclearly explain critical constraints:
- Not thread-safe (use only from main goroutine)
- Lifecycle managed by Resolver (don't Reset or Release)
- Critical requirement: underlying bytes must be arena-allocated to tie lifecycles
This documentation will help prevent common arena misuse patterns that lead to segfaults.
552-557: Critical arena lifecycle management pattern.Lines 555-557 implement a crucial pattern: allocating a new slice on the arena and copying
res.outbefore parsing withParseBytesWithArena. This ensures the parsed AST and the underlying bytes share the same arena lifecycle, preventing segfaults when the arena is released. As noted in the arena comments (line 191), this pattern is essential for correct arena usage.
1635-1685: SingleFlight integration properly handles mutations and context.The
loadByContextfunction correctly:
- Disables deduplication for mutations (line 1644) to avoid side-effect issues
- Respects the
DisableRequestDeduplicationflag (line 1645)- Handles context cancellation via select with
item.loadedandctx.Done()(lines 1657-1661)- Provides size hints to the HTTP client (line 1672)
- Properly stores shared responses (line 1667, 1683)
The integration aligns with the PR's request deduplication objectives.
v2/pkg/engine/datasource/httpclient/nethttpclient.go (3)
133-154: LGTM: Buffer size hint mechanism is well-designed.The context-based size hint and the
bufferfunction provide an efficient way to pre-allocate buffers based on expected response sizes, reducing allocations and copies. The 64-byte default for cases without a hint is a reasonable middle ground.
240-278: LGTM: Response buffering and trace handling are well-implemented.The buffer allocation strategy is clearly documented, and the trace injection using
jsonparser.Setis correct. Error handling is consistent throughout.
281-283: LGTM: Do function correctly updated to new signature.The function now properly accepts
baseHeadersand returns data directly, aligning with the PR objectives. Passinglen(body)ascontentLengthis correct for regular HTTP requests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader.go (1)
1415-1534: LGTM: Sophisticated batch deduplication with arena optimization.The batch entity loading correctly:
- Uses arena-backed buffers for temporary allocations (lines 1429-1431)
- Implements hash-based deduplication (lines 1465-1470)
- Critically copies
*astjson.Valuepointers off the arena into persistent storage (lines 1510-1516) before the arena is resetOptional: Remove ineffectual assignment at line 1517.
The static analysis correctly identifies that
batchStats = nilat line 1517 has no effect sincebatchStatsis not used afterward. While harmless, you can remove it for cleaner code:for i := range batchStats { res.batchStats[i] = make([]*astjson.Value, len(batchStats[i])) copy(res.batchStats[i], batchStats[i]) - batchStats[i] = nil } - batchStats = nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(48 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (6)
v2/pkg/engine/resolve/fetch.go (8)
PostProcessingConfiguration(115-131)FetchItemPathElement(77-81)FetchItem(28-33)DataSourceLoadTrace(374-388)BatchEntityFetch(159-168)BatchEntityFetch(193-195)Fetch(19-26)FetchInfo(345-366)v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(9-15)v2/pkg/engine/resolve/context.go (3)
RateLimitOptions(135-148)Context(16-37)ExecutionOptions(57-69)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/engine/resolve/resolve.go (1)
New(188-252)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(142-144)
🪛 GitHub Check: Linters (1.25)
v2/pkg/engine/resolve/loader.go
[failure] 1517-1517:
ineffectual assignment to batchStats (ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (23)
v2/pkg/engine/resolve/loader.go (23)
60-65: LGTM: Clean API boundary for response body access.The transition from
*bytes.Bufferto[]bytestorage with a public getter method provides a clear API boundary and aligns with the arena-based memory management approach.
95-132: LGTM: Well-documented struct enhancements.The
batchStatsdocumentation clearly explains the deduplication mapping, and the new fields (singleFlightStats,tools) are appropriately added to support the single-flight and arena-based batch processing features.
175-186: LGTM: Excellent documentation on arena usage constraints.The detailed comments on
jsonArenaclearly communicate critical safety constraints (thread safety, lifecycle management, byte allocation requirements). This documentation will help prevent segfaults and misuse.
225-230: LGTM: Proper pool resource cleanup.The deferred cleanup ensures
batchEntityToolsare returned to the pool, with graceful nil handling.
293-329: LGTM: Consistent resource initialization and cleanup.The pattern of result initialization with deferred pool cleanup (line 305) is applied consistently across all fetch types.
331-345: LGTM: Arena-based allocation for path selection.The initial items slice is properly allocated on the arena (line 333) and the arena is consistently propagated to
selectItems(line 342).
363-401: LGTM: Complete arena integration in item selection.All slice allocations within
selectItemsnow use arena-aware methods (AllocateSlice,SliceAppend), ensuring memory is tied to the arena lifecycle.
403-417: LGTM: Thread-safe array creation without arena.The comment (lines 410-411) correctly explains why arena allocation isn't used here—
itemsDatacan be called concurrently. The freshMustParseBytesallocation avoids race conditions at the cost of efficiency.
488-503: LGTM: Critical arena lifecycle pattern correctly implemented.Lines 491-495 demonstrate the essential pattern: allocating bytes on the arena before parsing them with
ParseBytesWithArena. This ties lifecycles together and prevents segfaults, as warned in thejsonArenafield documentation.
577-636: LGTM: Arena-aware merge operations.All
MergeValuesWithPathcalls now correctly pass the arena as the first parameter (lines 577, 603, 624), and the batchStats iteration (lines 596-617) properly handles the new per-unique-item target structure.
644-664: LGTM: Clean refactor to return []byte.The method now returns
[]byte(line 663) instead of modifying a buffer, providing a cleaner API that aligns with the arena-based architecture.
720-726: LGTM: Lazy initialization pattern with clear documentation.The
ensureErrorsInitializedcall before appending (lines 720-723) is well-documented as a performance optimization. Similar comments throughout (lines 760-763, 1017-1020, etc.) maintain consistency.
737-767: LGTM: Comprehensive arena integration in error wrapping.All JSON parsing and value manipulation in the wrap-mode error handling uses arena-aware methods (
ParseWithArena,SetValue), maintaining memory safety.
809-821: LGTM: Arena-aware extension code injection.The extension code setting correctly uses arena methods for creating string values (line 809) and objects (lines 812-814, 817-819).
899-949: LGTM: Arena-aware path rewriting.The
rewriteErrorPathsfunction signature correctly accepts an arena (line 899) and uses it for parsing the new path (line 941) and setting it (line 945).
1253-1292: LGTM: Single fetch loading with proper error handling.The input rendering uses a regular buffer (line 1255) which is appropriate for temporary input construction. The error path (lines 1279-1281) correctly sets the error response and returns nil since the error is captured in the result.
1294-1371: LGTM: Entity fetch input preparation.The entity fetch correctly uses temporary buffers (lines 1304-1305) for input construction, which is appropriate since the input is passed to the DataSource and not stored in the arena.
1373-1413: LGTM: Efficient batch processing tool pooling.The
batchEntityToolspooling mechanism properly encapsulates the xxhash digest, deduplication map, and arena (lines 1373-1377), with correct reset logic (lines 1379-1385) to prepare for reuse.
1590-1607: LGTM: Well-designed context key pattern.The
GetOperationTypeFromContexthelper provides a clean way for the transport layer to check operation types, with sensible defaults (Query) and proper nil handling.
1609-1618: LGTM: Clean header extraction helper.The helper properly handles nil checks and encapsulates the logic for retrieving headers and their hash for single-flight deduplication.
1620-1682: LGTM: Single-flight deduplication correctly implemented.The implementation properly:
- Excludes mutations to avoid side-effect issues (line 1629)
- Handles context cancellation (lines 1642-1646)
- Manages shared response lifecycle (lines 1641-1669)
- Delegates to
loadByContextDirectfor actual I/O (lines 1672-1682)The header hash (
extraKey) at line 1626 ensures requests with different headers are not deduplicated.
1709-1861: LGTM: Tracing properly integrated with single-flight.The execution flow consistently routes through
loadByContext(lines 1822, 1824, 1829), and tracing correctly captures single-flight statistics (lines 1836-1839) and output from the[]byteresponse (lines 1840-1841).
1863-1879: LGTM: Appropriate non-arena usage with clear rationale.The comments (lines 1870-1872) correctly explain why arena allocation isn't used in
compactJSON—it's not a hot path and would require the parsed content to also be on the arena. The trade-off is reasonable for this tracing helper.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/loader.go (2)
1278-1281: Clarify error handling intent.When input rendering fails, this assigns an error response to
res.outand returnsnilrather than propagating the error. While this appears intentional (encoding errors as GraphQL error responses rather than Go errors), the pattern is subtle.Consider whether callers expect to see an error return value for this failure case, or if encoding it in the response body is the desired behavior for all scenarios.
720-726: Document ensureErrorsInitialized pattern.The
ensureErrorsInitialized()calls before every error append are critical (as the comments note, it's measurable for performance). However, the pattern is repeated ~8 times across error handling paths.Consider extracting a helper like
appendError(value *astjson.Value)that handles initialization and append, reducing duplication and making the pattern more maintainable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(48 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (6)
v2/pkg/engine/resolve/fetch.go (8)
PostProcessingConfiguration(115-131)FetchItemPathElement(77-81)FetchItem(28-33)DataSourceLoadTrace(374-388)BatchEntityFetch(159-168)BatchEntityFetch(193-195)Fetch(19-26)FetchInfo(345-366)v2/pkg/engine/resolve/singleflight.go (1)
SingleFlight(9-15)v2/pkg/engine/resolve/context.go (2)
Context(16-37)ExecutionOptions(57-69)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/engine/resolve/resolve.go (1)
New(188-252)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(142-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (11)
v2/pkg/engine/resolve/loader.go (11)
175-186: LGTM – Excellent arena lifecycle documentation.The warnings about thread-safety, lifecycle management, and the requirement to allocate parsed JSON on the same arena are critical for preventing segfaults. This documentation will help future maintainers avoid subtle memory bugs.
491-496: LGTM – Correct arena lifecycle pattern.The explicit allocation and copy before parsing ensures the byte slice and parsed JSON share the same lifecycle, preventing the segfaults mentioned in the comments.
403-417: Verify nil arena usage in concurrent context.Line 414 calls
SetArrayItem(nil, i, item)without an arena. While this appears intentional (the comment notes concurrent calls, andarris created viaMustParseBytesrather than on the loader's arena), it differs from the arena-first pattern used elsewhere.Confirm this exception is safe and consider adding a comment explaining why this particular allocation doesn't use the arena.
1373-1413: LGTM – Well-structured pool with proper lifecycle management.The
batchEntityToolspool correctly manages arena lifecycle:reset()clears both the arena and map, and the pool pattern ensures resources are reused efficiently. The arena scoping to batch operations is appropriate.
1428-1441: LGTM – Defensive cleanup prevents use-after-free.Explicitly clearing
batchStatspointers in the defer prevents memory corruption when the arena is reset after the function returns. This is a critical safety measure given the comment on line 1434.
1604-1615: LGTM – Well-designed public helper for operation type.
GetOperationTypeFromContextprovides a clean API for the transport layer to check operation types, with sensible defaults (returningQuerywhen context is nil or value is missing).
1636-1641: LGTM – Correct single-flight exclusion logic.Properly excludes mutations from deduplication (avoiding side-effect issues) and respects the
DisableRequestDeduplicationflag. This addresses the critical concerns about request deduplication safety.
1878-1881: LGTM – Appropriate arena exclusion with clear rationale.The explicit comment about not using arena here is valuable. Since
compactJSONoperates on trace/debug data (not hot path) and the input bytes aren't arena-allocated, this exclusion prevents segfaults while keeping the code simple.
1475-1495: LGTM – Efficient batch deduplication with correct arena usage.The hash-based deduplication correctly identifies unique representations and accumulates merge targets. Using arena allocations during processing, then copying off the arena (lines 1521-1524) prevents memory corruption while maintaining efficiency.
577-624: LGTM – Consistent arena-aware merge operations.All
MergeValuesWithPathcalls properly passl.jsonArena, ensuring merged values are allocated on the same arena. The three merge paths (single item, batch stats, items) follow the same pattern.
1429-1430: LGTM – Arena buffers appropriate for batch processing.Using
arena.NewArenaBufferfor batch entity processing is appropriate given the complex deduplication logic and multiple passes over the data. The simpler single and entity fetches correctly use standardbytes.Bufferfor transient rendering.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
v2/pkg/engine/resolve/loader.go (1)
1676-1677: Optional: defensive copy when caching shared response.Storing res.out directly in item.response assumes immutability and unique backing arrays from DataSource. If that ever changes, followers may observe mutated data. Consider:
item.response = append([]byte(nil), res.out...)Tagging as already discussed previously.
🧹 Nitpick comments (5)
v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
17-20: Consider RWMutex for read-mostly path.Followers only read the shard map before waiting. Switching to sync.RWMutex and using RLock for the lookup can reduce contention under load (mirrors subgraph single-flight). Optional.
Also applies to: 77-92
121-131: Minor: set Err before unlocking (clarity).Setting req.Err under the lock (or immediately before close) avoids any doubt about visibility; channel close already provides the happens‑before. Optional.
v2/pkg/engine/resolve/subgraph_request_singleflight.go (1)
12-16: Remove unused cleanup channel.cleanup is unused; drop the field and its initialization to reduce noise.
@@ type SubgraphRequestSingleFlight struct { shards []singleFlightShard xxPool *sync.Pool - cleanup chan func() } @@ s := &SubgraphRequestSingleFlight{ shards: make([]singleFlightShard, shardCount), xxPool: &sync.Pool{ New: func() any { return xxhash.New() }, }, - cleanup: make(chan func()), }Also applies to: 53-61
v2/pkg/engine/resolve/resolve.go (1)
85-89: Comment nit: fix description of inboundRequestSingleFlight.It deduplicates inbound GraphQL requests (not subgraph requests).
v2/pkg/engine/resolve/loader.go (1)
1848-1853: Consider redacting sensitive headers in trace output (if present).Input redaction is handled; Output may still include upstream headers under extensions.trace.response.headers. Consider stripping or masking standard sensitive headers before exposing traces.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/engine/resolve/const.go(1 hunks)v2/pkg/engine/resolve/context.go(3 hunks)v2/pkg/engine/resolve/inbound_request_singleflight.go(1 hunks)v2/pkg/engine/resolve/loader.go(48 hunks)v2/pkg/engine/resolve/resolve.go(21 hunks)v2/pkg/engine/resolve/subgraph_request_singleflight.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/context.go (1)
v2/pkg/engine/plan/planner.go (1)
IncludeQueryPlanInResponse(92-96)
v2/pkg/engine/resolve/loader.go (6)
v2/pkg/engine/resolve/fetch.go (8)
PostProcessingConfiguration(115-131)FetchItemPathElement(77-81)FetchItem(28-33)DataSourceLoadTrace(374-388)BatchEntityFetch(159-168)BatchEntityFetch(193-195)Fetch(19-26)FetchInfo(345-366)v2/pkg/engine/resolve/context.go (2)
Context(16-38)ExecutionOptions(60-72)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/engine/resolve/resolve.go (1)
New(190-255)v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(142-144)
v2/pkg/engine/resolve/resolve.go (8)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(19-24)NewArenaPool(38-42)v2/pkg/engine/resolve/subgraph_request_singleflight.go (2)
SubgraphRequestSingleFlight(12-16)NewSingleFlight(49-69)v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
InboundRequestSingleFlight(13-15)NewRequestSingleFlight(26-39)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/context.go (4)
Context(16-38)Request(179-182)ExecutionOptions(60-72)SubgraphHeadersBuilder(44-50)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(36-44)GraphQLSubscription(12-16)SubscriptionResponseWriter(69-75)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/resolve/subgraph_request_singleflight.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(190-255)v2/pkg/engine/resolve/fetch.go (3)
FetchItem(28-33)Fetch(19-26)FetchInfo(345-366)
v2/pkg/engine/resolve/inbound_request_singleflight.go (3)
v2/pkg/engine/resolve/context.go (4)
Context(16-38)ExecutionOptions(60-72)Request(179-182)SubgraphHeadersBuilder(44-50)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(36-44)v2/pkg/ast/ast_operation_definition.go (2)
OperationType(11-11)OperationTypeMutation(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/const.go (1)
11-12: LGTM! Constants are properly used.Both
pipeanddotconstants are actively used inv2/pkg/engine/resolve/subgraph_request_singleflight.go(lines 152 and 158 respectively) for hash computation during request deduplication. The additions follow the established pattern and are correctly integrated.v2/pkg/engine/resolve/context.go (1)
37-50: LGTM: header hashing API + VariablesHash + Request.ID(uint64).Interfaces and fields align with new dedup paths; helper HeadersForSubgraphRequest is clear.
Ensure all call sites updated for Request.ID type switch to uint64.
Also applies to: 52-58, 60-72, 179-182
…eration type is Mutation or Subscription
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
v2/pkg/engine/resolve/arena_test.go (3)
71-125: Document the specific bug being fixed.The test name and comments reference a bug fix, but the actual bug and its root cause aren't documented. Consider adding a comment explaining:
- What the original bug was (e.g., "items were skipped during pool traversal when weak pointers were collected")
- Why the safeguard
processed < numItems*2is set to 2x- What symptoms users would have seen
This will help future maintainers understand why this test exists and what regression it prevents.
127-157: Consider making peak value assertions more flexible.Line 138 asserts
peak1 == 5, which assumes the arena allocates exactly the string length with no overhead. This assertion is fragile—if the underlyinggo-arenaimplementation changes its allocation strategy or adds padding, the test will break even though the ArenaPool logic is correct.Consider either:
- Removing the exact peak assertion (the test already validates size tracking)
- Using a range assertion like
assert.GreaterOrEqual(t, peak1, 5)to allow for allocation overhead
214-261: Consider reducing coupling to implementation details.This test validates very specific internal behavior (window size of 50, exact division logic, count reset). While thorough, it's tightly coupled to the implementation, making it fragile:
- Changes to the window size constant would require test updates
- The exact arithmetic (lines 244-248) could break with algorithm tweaks
Consider:
- Testing observable behavior instead (e.g., "size estimates converge after many operations")
- Making magic values like
50discoverable from the ArenaPool API if they're part of the contract- Adding a comment explaining why this specific window behavior is critical to test
That said, if the window size and reset logic are part of the performance contract, this level of detail may be justified.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/astvisitor/visitor.go(5 hunks)v2/pkg/engine/resolve/arena.go(1 hunks)v2/pkg/engine/resolve/arena_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/arena.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/arena_test.go (1)
v2/pkg/engine/resolve/arena.go (2)
NewArenaPool(38-42)ArenaPoolItem(33-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (10)
v2/pkg/engine/resolve/arena_test.go (5)
13-19: LGTM!The initialization test correctly verifies that a new ArenaPool is created with empty internal structures.
21-35: LGTM!The test correctly verifies that acquiring from an empty pool creates a new usable arena and doesn't leave artifacts in the pool.
37-69: LGTM!The test effectively validates the release-acquire cycle, including verification that arenas are properly reset between uses.
159-177: LGTM!The test appropriately validates both default and usage-based size calculations with reasonable flexibility on line 176.
179-212: LGTM!The test correctly validates that multiple items can be pooled and subsequently re-acquired.
v2/pkg/astvisitor/visitor.go (5)
8-9: LGTM: Arena package import added.The import is properly positioned and necessary for the arena-based memory management integration.
99-100: LGTM: Arena field added to Walker struct.The arena field is properly defined as an unexported field for internal memory management. The placement at the end of the struct is appropriate.
131-134: LGTM: Arena properly reset in Release.The nil check is defensive and correct since the arena is lazily initialized. Resetting the arena before returning the Walker to the pool ensures proper memory management across reuses.
1837-1837: LGTM: Arena-based slice allocation in walkSelectionSet.The use of
arena.SliceAppendto create a snapshot of selection references is an effective optimization. This reduces heap allocations in the hot path of AST traversal while maintaining correct semantics for detecting selection set changes during iteration.
1380-1384: No changes needed — buffer sizing is appropriate for the AST visitor workload.The 64-byte minimum buffer size is a deliberate design choice for the visitor's allocation patterns. Unlike the larger 1024-byte buffers used for heavier workloads (like batch loading), the AST visitor handles many small allocations (slices of selection references). The monotonic arena grows on demand and retains the larger size across subsequent
Reset()calls, making the small initial size efficient without incurring repeated resizing overhead.
# Conflicts: # v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go # v2/pkg/engine/datasource/grpc_datasource/json_builder.go
| // Double-check if the item was created while acquiring the write lock | ||
| item, exists = shard.items[sfKey] | ||
| if exists { | ||
| shard.mu.Unlock() | ||
| return sfKey, fetchKey, item, true | ||
| } |
There was a problem hiding this comment.
Is this second check worth having? these checks would be microseconds apart, the possible mutex contention and channel delay probably outweighs the performance benefit
if we wanted to wait, we might as well implement some kind of "waiting" function that has a more sensible interval to actually catch one, but wouldn't it be faster just to use this one to create the item and hope the later one picks up this key?
| items map[uint64]*SingleFlightItem | ||
| sizes map[uint64]*fetchSize |
There was a problem hiding this comment.
Nit: If these are read-heavy in usage which is what it seems to be under load, sync.Map is probably more efficient
There was a problem hiding this comment.
that's worth a benchmark test
| @@ -0,0 +1,192 @@ | |||
| package resolve | |||
There was a problem hiding this comment.
Things in this file seem self contained enough to possibly be moved into their own module, which I think would improve readability since the reader wouldn't have to scan so hard for anything being used in or from the rest of the "resolve" module.
| @@ -0,0 +1,138 @@ | |||
| package resolve | |||
There was a problem hiding this comment.
Maybe this can go with subgraph_request_singleflight.go into another module
| assert.Equal(t, 0, len(pool.pool), "expected empty pool") | ||
| assert.Equal(t, 0, len(pool.sizes), "expected empty sizes map") |
There was a problem hiding this comment.
Nit: assert.Len() is recommended for this case by testifylint.
| w.Report = report | ||
| } | ||
| if w.arena == nil { | ||
| w.arena = arena.NewMonotonicArena(arena.WithMinBufferSize(64)) |
There was a problem hiding this comment.
Where does the magic number 64 come from?
Does the arena being initilazied here and not in the NewWalker because of cases when we create a walker but never "walk" it? If it is not the case we could eliminate branching by moving initialization to the NewWalker
There was a problem hiding this comment.
We very rarely have situations where subgraph responses are smaller, so it's less buffer grow operations until we find the right buffer size, at the same time it's still small enough so that we're not over-allocating
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Outdated
Show resolved
Hide resolved
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
Outdated
Show resolved
Hide resolved
| tempFiles = append(tempFiles, temporaryFile) | ||
| if err != nil { |
There was a problem hiding this comment.
defer below only happens when there was no early return before. If for some file we return early, and some fiels were previsouly added to tempFiles then we will not remove close/remove those files. Also the if check should happen before adding the temporaryFile to the slice.
| return nil, err | ||
| } | ||
|
|
||
| defer func() { |
There was a problem hiding this comment.
I guess this defer should be moved before the loop.
| if len(responseBytes) > 0 && responseBytes[len(responseBytes)-1] == '\n' { | ||
| responseBytes = responseBytes[:len(responseBytes)-1] | ||
| } |
| @@ -0,0 +1,107 @@ | |||
| package resolve | |||
There was a problem hiding this comment.
What about moving this into separate module at the v2/pkg/engine/ path, so that other parts of the engine might benefit from it? (Although I do not see immediate condidate to use it yet.) This pool is a good abstration that is not connected tightly to the "resolve".
Or maybe even it could live in go-arena...
| return FetchKindEntity | ||
| } | ||
|
|
||
| // The ParallelListItemFetch can be used to make nested parallel fetches within a list |
There was a problem hiding this comment.
I kind of do not get why you removed it. It was not used or was performing poorly?
There was a problem hiding this comment.
it's in fact not used at all, it's legacy and everyone else was afraid of deleting xD
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
I am done and it looks good. I have mostly minor findings.
| } | ||
| out := dst.Bytes() | ||
| v, err := astjson.ParseBytesWithoutCache(out) | ||
| // don't use arena here or segfault |
There was a problem hiding this comment.
| // don't use arena here or segfault | |
| // Don't use arena here to avoid segfaults. |
| // resolveArenaPool is the arena pool dedicated for Loader & Resolvable | ||
| // ArenaPool automatically adjusts arena buffer sizes per workload | ||
| // resolving & response buffering are very different tasks | ||
| // as such, it was best to have two arena pools in terms of memory usage | ||
| // A single pool for both was much less efficient |
There was a problem hiding this comment.
Personal nit. I would love to see punctuation here. Without dots and commas it is harder to read this text fast. I have to stop multiple times and re-read lines to identify sentences. It applies not only to this place but to other places with multi-line comments.
| // resolveArenaPool is the arena pool dedicated for Loader & Resolvable | |
| // ArenaPool automatically adjusts arena buffer sizes per workload | |
| // resolving & response buffering are very different tasks | |
| // as such, it was best to have two arena pools in terms of memory usage | |
| // A single pool for both was much less efficient | |
| // resolveArenaPool is the arena pool dedicated for Loader & Resolvable. | |
| // ArenaPool automatically adjusts arena buffer sizes per workload. | |
| // Resolving & response buffering are very different tasks; | |
| // as such, it was best to have two arena pools in terms of memory usage. | |
| // A single pool for both was much less efficient. |
| jsonArena arena.Arena | ||
| // sf is the SubgraphRequestSingleFlight object shared across all client requests | ||
| // it's thread safe and can be used to de-duplicate subgraph requests | ||
| sf *SubgraphRequestSingleFlight |
There was a problem hiding this comment.
How about this name?
| jsonArena arena.Arena | |
| // sf is the SubgraphRequestSingleFlight object shared across all client requests | |
| // it's thread safe and can be used to de-duplicate subgraph requests | |
| sf *SubgraphRequestSingleFlight | |
| jsonArena arena.Arena | |
| // singleFlight is the SubgraphRequestSingleFlight object shared across all client requests. | |
| // It's thread safe and can be used to de-duplicate subgraph requests. | |
| singleFlight *SubgraphRequestSingleFlight |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (2)
1848-1855: Bug: return value and compaction condition in compactAndUnNullVariables.
- On json.Compact error you return variables (the inner blob), not the full input, corrupting the request.
- The condition is inverted; compact only when whitespace is present.
- // compact - if !bytes.ContainsAny(variables, " \t\n\r") { - buf := bytes.NewBuffer(make([]byte, 0, len(variables))) - if err := json.Compact(buf, variables); err != nil { - return variables - } - variables = buf.Bytes() - } + // compact only if there is whitespace + if bytes.ContainsAny(variables, " \t\n\r") { + var buf bytes.Buffer + buf.Grow(len(variables)) + if err := json.Compact(&buf, variables); err != nil { + return input + } + variables = buf.Bytes() + }
1891-1907: Guard against out-of-bounds in replaceEmptyObject.Accessing variables[end] can panic when the empty object is the last entry.
- end := i + 3 - hasTrailingComma := false - if variables[end] == ',' { + end := i + 3 // position after '}' + hasTrailingComma := false + if end < len(variables) && variables[end] == ',' { end++ hasTrailingComma = true } startQuote := bytes.LastIndex(variables[:i-2], []byte("\"")) - if !hasTrailingComma && variables[startQuote-1] == ',' { + if !hasTrailingComma && startQuote-1 >= 0 && variables[startQuote-1] == ',' { startQuote-- }
♻️ Duplicate comments (4)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
222-226: Fix input shape; place variables under body and drop debug print.This test still uses top-level "variables" and prints output. Align with the new API.
- output, err := ds.Load(context.Background(), nil, []byte(`{"query":"`+query+`","variables":`+variables+`}`)) + output, err := ds.Load(context.Background(), nil, []byte(`{"query":"`+query+`","body":`+variables+`}`)) require.NoError(t, err) - - fmt.Println(string(output))v2/pkg/engine/resolve/resolve.go (2)
77-89: Add punctuation to multi-line comments for readability.As noted in a previous review, multi-line comments are harder to parse without proper punctuation. Please add periods and commas to improve readability.
Apply this diff:
- // resolveArenaPool is the arena pool dedicated for Loader & Resolvable - // ArenaPool automatically adjusts arena buffer sizes per workload - // resolving & response buffering are very different tasks - // as such, it was best to have two arena pools in terms of memory usage - // A single pool for both was much less efficient + // resolveArenaPool is the arena pool dedicated for Loader & Resolvable. + // ArenaPool automatically adjusts arena buffer sizes per workload. + // Resolving & response buffering are very different tasks; + // as such, it was best to have two arena pools in terms of memory usage. + // A single pool for both was much less efficient. resolveArenaPool *ArenaPool - // responseBufferPool is the arena pool dedicated for response buffering before sending to the client + // responseBufferPool is the arena pool dedicated for response buffering before sending to the client. responseBufferPool *ArenaPool
1100-1116: Critical: prepareTrigger still returns triggerID=0 when no SubgraphHeadersBuilder.Despite a previous review comment being marked as addressed, the current code still returns
triggerID=0whenctx.SubgraphHeadersBuilderis nil. This causes all subscriptions (regardless of input) to collide and share the same trigger when no header builder is present.Apply this diff to hash the input regardless of whether a SubgraphHeadersBuilder exists:
func (r *Resolver) prepareTrigger(ctx *Context, sourceName string, input []byte) (headers http.Header, triggerID uint64) { + var header http.Header + var headerHash uint64 if ctx.SubgraphHeadersBuilder != nil { - header, headerHash := ctx.SubgraphHeadersBuilder.HeadersForSubgraph(sourceName) - keyGen := pool.Hash64.Get() - _, _ = keyGen.Write(input) - var b [8]byte - binary.LittleEndian.PutUint64(b[:], headerHash) - _, _ = keyGen.Write(b[:]) - triggerID = keyGen.Sum64() - pool.Hash64.Put(keyGen) - return header, triggerID + header, headerHash = ctx.SubgraphHeadersBuilder.HeadersForSubgraph(sourceName) } - return nil, 0 + keyGen := pool.Hash64.Get() + _, _ = keyGen.Write(input) + var b [8]byte + binary.LittleEndian.PutUint64(b[:], headerHash) + _, _ = keyGen.Write(b[:]) + triggerID = keyGen.Sum64() + pool.Hash64.Put(keyGen) + return header, triggerID }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
179-180: Inconsistent error semantics: return nil error with GraphQL error JSON.Line 166 correctly returns
nilafter settingdatato error bytes, but line 179 returnserrafter settingdata. This inconsistency was noted in a previous review.Apply this diff:
if err != nil { data = builder.writeErrorBytes(err) - return err + return nil }
🧹 Nitpick comments (6)
v2/pkg/engine/resolve/arena.go (1)
48-72: Reduce lock hold time in Acquire to avoid blocking during arena allocation.NewMonotonicArena can allocate sizable buffers; do the heavy work outside the critical section.
func (p *ArenaPool) Acquire(key uint64) *ArenaPoolItem { - p.mu.Lock() - defer p.mu.Unlock() - - // Try to find an available arena in the pool - for len(p.pool) > 0 { + p.mu.Lock() + // Try to find an available arena in the pool + for len(p.pool) > 0 { // Pop the last item lastIdx := len(p.pool) - 1 wp := p.pool[lastIdx] p.pool = p.pool[:lastIdx] v := wp.Value() if v != nil { v.Key = key - return v + p.mu.Unlock() + return v } // If weak pointer was nil (GC collected), continue to next item } - // No arena available, create a new one - size := arena.WithMinBufferSize(p.getArenaSize(key)) - return &ArenaPoolItem{ + // No arena available, create a new one (outside lock) + sizeHint := p.getArenaSize(key) + p.mu.Unlock() + size := arena.WithMinBufferSize(sizeHint) + return &ArenaPoolItem{ Arena: arena.NewMonotonicArena(size), Key: key, } }v2/pkg/engine/resolve/arena_test.go (1)
13-19: Add a race test to exercise thread-safety under -race.Pool is guarded by a mutex; add a Parallel/WaitGroup test to concurrently Acquire/Release and run with -race to validate.
I can draft a test skeleton if helpful.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
310-314: Remove leftover debug and avoid temp var.Use output directly; no printlns in tests.
- bytes := output - fmt.Println(string(bytes)) - - err = json.Unmarshal(bytes, &resp) + err = json.Unmarshal(output, &resp)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
1990-1992: Nit: variable name typo.dataSouceName → dataSourceName.
-var ( - dataSouceName = []byte("graphql") -) +var ( + dataSourceName = []byte("graphql") +)v2/pkg/engine/resolve/resolve.go (1)
243-246: Consider extracting shard count to a named constant.The shard count of 8 is hardcoded for both single-flight instances. Extracting this to a named constant (e.g.,
defaultSingleFlightShardCount) would make the choice more explicit and easier to tune in the future.Example:
+const ( + defaultSingleFlightShardCount = 8 +) + resolver := &Resolver{ ... - subgraphRequestSingleFlight: NewSingleFlight(8), - inboundRequestSingleFlight: NewRequestSingleFlight(8), + subgraphRequestSingleFlight: NewSingleFlight(defaultSingleFlightShardCount), + inboundRequestSingleFlight: NewRequestSingleFlight(defaultSingleFlightShardCount), }v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
211-212: Consider returning an error instead of panicking.Panicking in library code can crash the host application unexpectedly. Consider returning a descriptive error instead.
func (d *DataSource) LoadWithFiles(ctx context.Context, headers http.Header, input []byte, files []*httpclient.FileUpload) (data []byte, err error) { - panic("unimplemented") + return nil, fmt.Errorf("LoadWithFiles is not implemented for gRPC datasource") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(7 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(28 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(15 hunks)v2/pkg/engine/resolve/arena.go(1 hunks)v2/pkg/engine/resolve/arena_test.go(1 hunks)v2/pkg/engine/resolve/resolve.go(21 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-07-28T12:44:56.405Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1246
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:457-457
Timestamp: 2025-07-28T12:44:56.405Z
Learning: In the graphql-go-tools gRPC datasource, only non-null lists use protobuf's repeated field syntax directly. For nullable or nested lists, wrapper types are used because protobuf repeated fields cannot be nullable. The TypeIsNonNullList check ensures only appropriate list types are marked as repeated in the protobuf message structure.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/json_builder.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-09-19T14:51:33.724Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:339-347
Timestamp: 2025-09-19T14:51:33.724Z
Learning: In the graphql-go-tools resolver, the event loop follows a strict single-threaded design where all subscription lifecycle management (add/remove from triggers) must happen through the events channel to the main processEvents() goroutine. Worker goroutines should not directly modify subscription state or call unsubscribe methods, as this would break the single-threaded event loop architecture and create race conditions.
Applied to files:
v2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-16T13:05:19.838Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1323
File: v2/pkg/engine/datasource/grpc_datasource/compiler.go:683-702
Timestamp: 2025-10-16T13:05:19.838Z
Learning: In GraphQL field resolver context resolution (v2/pkg/engine/datasource/grpc_datasource/compiler.go), when traversing paths in resolveContextDataForPath, the code can safely assume that intermediate path segments will only be messages or lists, never scalars. This is because field resolvers are only defined on GraphQL object types, not scalar types, so the parent function must return either a message or a list. This invariant is enforced by the GraphQL type system design.
Applied to files:
v2/pkg/engine/resolve/resolve.go
📚 Learning: 2025-10-15T13:34:15.892Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1322
File: v2/pkg/astvalidation/operation_rule_defer_stream_on_root_fields.go:92-127
Timestamp: 2025-10-15T13:34:15.892Z
Learning: In the graphql-go-tools repository, validation for defer and stream directives runs after normalization, which performs fragment inlining. Therefore, fragment spreads don't exist in the AST when these validation rules execute—they're already expanded into inline fragments or fields.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/arena_test.go (1)
v2/pkg/engine/resolve/arena.go (2)
NewArenaPool(39-43)ArenaPoolItem(33-36)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (2)
v2/pkg/engine/datasource/grpc_datasource/configuration.go (1)
GRPCMapping(26-46)v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
RPCMessage(96-110)ListMetadata(217-222)LevelInfo(225-228)
v2/pkg/engine/resolve/resolve.go (8)
v2/pkg/engine/resolve/arena.go (2)
ArenaPool(19-24)NewArenaPool(39-43)v2/pkg/engine/resolve/subgraph_request_singleflight.go (2)
SubgraphRequestSingleFlight(12-16)NewSingleFlight(49-69)v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
InboundRequestSingleFlight(14-16)NewRequestSingleFlight(27-40)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(72-80)ResolvableOptions(65-70)v2/pkg/engine/resolve/context.go (4)
Context(16-38)Request(185-188)ExecutionOptions(60-78)SubgraphHeadersBuilder(44-50)v2/pkg/engine/resolve/response.go (3)
GraphQLResponse(36-44)GraphQLSubscription(12-16)SubscriptionResponseWriter(82-88)v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/pool/hash64.go (1)
Hash64(10-16)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (4)
v2/pkg/engine/resolve/arena.go (3)
ArenaPool(19-24)NewArenaPool(39-43)ArenaPoolItem(33-36)v2/pkg/engine/resolve/datasource.go (1)
DataSource(10-13)v2/pkg/engine/resolve/resolve.go (1)
New(191-256)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (4)
v2/pkg/engine/datasource/staticdatasource/static_datasource.go (3)
Source(72-72)Source(74-76)Source(78-80)v2/pkg/engine/resolve/context.go (1)
Context(16-38)v2/pkg/engine/datasource/httpclient/file.go (1)
FileUpload(3-7)v2/pkg/engine/datasource/httpclient/nethttpclient.go (2)
DoMultipartForm(286-342)Do(281-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (12)
v2/pkg/engine/resolve/arena.go (1)
10-24: LGTM: pop-from-end retrieval, weak reuse, and per-key sizing history.The pool semantics look correct; Acquire now pops from the tail and skips GC’ed entries; Release/ReleaseMany record peaks and reinsert as weak refs; per-key moving average avoids skew. Nice.
Please confirm go-arena Peak()/Reset() semantics are safe across reuse with the chosen allocator.
Also applies to: 26-37, 45-47, 66-72, 74-104, 105-135, 137-144
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (3)
111-124: Confirm nil-arena behavior for astjson allocations.newJSONBuilder accepts a nil arena in tests. Ensure astjson.ObjectValue/ArrayValue/... tolerate a nil arena (i.e., allocate from a fallback) and won’t panic.
If a non-nil arena is required, consider defaulting to a small monotonic arena in newJSONBuilder when a is nil.
165-189: LGTM: federation merge preserves representation order.Using indexMap with per-type ordinals and placing via SetArrayItem with the original representation positions is correct for _entities ordering. Based on learnings.
Also applies to: 195-223
320-467: LGTM: arena-plumbed JSON construction.All astjson constructors and Set operations consistently use j.jsonArena; optional wrappers, lists (including nested), enums, and error envelopes are handled properly.
Also applies to: 474-575, 580-639, 644-688, 690-725
v2/pkg/engine/resolve/arena_test.go (1)
13-69: Solid coverage of pool lifecycle and accounting.Good lifecycle tests (acquire/release, moving window, GC-ed entries processed) and size tracking validation.
Also applies to: 71-125, 127-157, 159-177, 179-212, 214-261
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
57-59: LGTM: tests updated to body+Load returning bytes.Benchmarks/tests consistently call ds.Load(ctx, nil, []byte(payload)) and marshal from returned []byte; “body” placement is correct across these cases.
Also applies to: 95-97, 293-295, 385-387, 477-479, 803-805, 1073-1075, 1209-1211, 1379-1381, 1772-1775, 1848-1850, 2266-2268, 3527-3529, 3726-3728, 3912-3914
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (2)
1909-1912: LGTM: Source now returns data and accepts headers.Load/LoadWithFiles now return []byte and propagate headers to httpclient; looks good.
Also applies to: 1914-1917
1957-1964: LGTM: subscription headers propagation.AsyncStart/Start accept headers and set options.Header accordingly; aligns with planner changes.
Also applies to: 1978-1984
v2/pkg/engine/resolve/resolve.go (2)
318-382: Well-designed arena lifecycle and single-flight integration.The two-stage arena approach (separate pools for resolution and response buffering) is well-reasoned, and the lifecycle management ensures arenas are released on all paths including errors. The single-flight deduplication correctly handles both leader and follower cases.
514-553: Correct arena lifecycle in subscription updates.The arena is properly acquired and released on all paths (error and success), preventing memory leaks.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
50-86: Arena pool integration looks good.The pool field is properly initialized and follows the same pattern used in the resolver.
193-202: Pool key generation is well-designed.The use of xxhash combined with binary-encoded index ensures stable, collision-resistant keys for arena pool items.
| import ( | ||
| "sync" | ||
| "weak" | ||
|
|
||
| "github.com/wundergraph/go-arena" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Pin and validate the weak-pointer dependency.
The import path "weak" is non-standard and easy to break in module mode. Please ensure the intended module is pinned in go.mod (or replaced via replace) and the import path is fully qualified.
Run:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== go.mod roots =="
fd -a go.mod | xargs -I{} bash -lc 'echo :: {}; rg -n "require|replace" "{}" | sed "s/^/ /"'
echo
echo "== search imports =="
rg -nP '^\s*"weak"\s*$' -n v2/pkg/engine/resolve/arena.go
echo
echo "== resolve module providing weak =="
rg -n "module\\s+weak\\b" -S || true
rg -n "module\\s+.*weak" -S || trueLength of output: 278118
The "weak" import path is unqualified and unpinned—add it to go.mod or replace it.
The verification confirms your concern: the bare import "weak" at line 5 has no corresponding module definition in any go.mod file. This will fail at build time.
Either add a require directive to v2/go.mod pinning the correct module (e.g., require github.com/your-org/weak vX.Y.Z), or replace it with a fully qualified import path and add a replace directive. Do not commit code with unresolved imports.
🤖 Prompt for AI Agents
In v2/pkg/engine/resolve/arena.go around lines 3 to 8 the import "weak" is
unqualified and not present in any go.mod, which will break builds; replace the
bare import with the package's full module path or add a require in v2/go.mod
for the correct module (e.g., require github.com/<owner>/weak vX.Y.Z) and update
any replace directives if using a local fork, then run go mod tidy to resolve
and vendor if needed; ensure the import statement and go.mod entry match exactly
and re-run tests/build to confirm resolution.
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
I agree with the approach taken. The only big wish it to improbethe organization of the code mentioned in the comments.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)
8857-8862: Close temp files to avoid FD leaks in testsDefer closing files created with os.CreateTemp to prevent descriptor leaks and flaky CI.
Apply in both subtests:
@@ - f, err := os.CreateTemp(dir, fileName) + f, err := os.CreateTemp(dir, fileName) assert.NoError(t, err) + defer func() { _ = f.Close() }() @@ - f1, err := os.CreateTemp(dir, file1Name) + f1, err := os.CreateTemp(dir, file1Name) assert.NoError(t, err) + defer func() { _ = f1.Close() }() @@ - f2, err := os.CreateTemp(dir, file2Name) + f2, err := os.CreateTemp(dir, file2Name) assert.NoError(t, err) + defer func() { _ = f2.Close() }()Also applies to: 8915-8924
v2/pkg/engine/resolve/subgraph_request_singleflight.go (1)
156-169: MixextraKeyinto the SF hash via bytes, not arithmetic addition.
computeSFKeystill does:_, _ = h.Write(input) return h.Sum64() + extraKey // extraKey is the pre-generated hash for the headersAdding
extraKeyarithmetically can fold distinct(bodyHash, extraKey)pairs into the same final value and weakens the distribution guarantees you get fromxxhash.As previously suggested in an earlier review, encode
extraKeyinto 8 bytes and feed it into the digest so it participates in the hash properly:func (s *SubgraphRequestSingleFlight) computeSFKeyWithDigest(h *xxhash.Digest, fetchItem *FetchItem, input []byte, extraKey uint64) uint64 { if fetchItem != nil && fetchItem.Fetch != nil { if info := fetchItem.Fetch.FetchInfo(); info != nil { _, _ = h.WriteString(info.DataSourceID) _, _ = h.WriteString(":") } } _, _ = h.Write(input) if extraKey != 0 { var b [8]byte binary.LittleEndian.PutUint64(b[:], extraKey) _, _ = h.Write(b[:]) } return h.Sum64() }This keeps the strong SF key semantics even when header hashes are mixed in.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)
8456-8456: Remove no-op defers on ctx.Context().Done()Done() returns a channel; deferring it has no effect. Remove or use context.WithCancel and defer cancel() when you need lifecycle control.
- defer ctx.Context().Done() + // no-op removed; use a cancelable context if needed: + // c, cancel := context.WithCancel(ctx.Context()); defer cancel()Also applies to: 8468-8468, 8508-8508, 8574-8574, 8616-8616
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(14 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(28 hunks)v2/pkg/engine/resolve/inbound_request_singleflight.go(1 hunks)v2/pkg/engine/resolve/loader.go(48 hunks)v2/pkg/engine/resolve/subgraph_request_singleflight.go(1 hunks)v2/pkg/engine/resolve/subgraph_request_singleflight_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-19T14:50:19.528Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:999-1011
Timestamp: 2025-09-19T14:50:19.528Z
Learning: In the graphql-go-tools resolver, when handling backpressure for async unsubscribe operations, the team prefers using individual goroutines over bounded deferrer implementations. The rationale is that bounded implementations can also overflow, and the preferred solution for overload is scaling the deployment rather than internal buffering. The current approach prevents deadlocks at the cost of goroutine creation, which is an acceptable tradeoff.
Applied to files:
v2/pkg/engine/resolve/inbound_request_singleflight.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.gov2/pkg/engine/resolve/loader.go
📚 Learning: 2025-09-19T14:51:33.724Z
Learnt from: endigma
Repo: wundergraph/graphql-go-tools PR: 1298
File: v2/pkg/engine/resolve/resolve.go:339-347
Timestamp: 2025-09-19T14:51:33.724Z
Learning: In the graphql-go-tools resolver, the event loop follows a strict single-threaded design where all subscription lifecycle management (add/remove from triggers) must happen through the events channel to the main processEvents() goroutine. Worker goroutines should not directly modify subscription state or call unsubscribe methods, as this would break the single-threaded event loop architecture and create race conditions.
Applied to files:
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
📚 Learning: 2025-08-29T09:35:47.969Z
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1282
File: v2/pkg/engine/plan/visitor.go:5-5
Timestamp: 2025-08-29T09:35:47.969Z
Learning: The wundergraph/graphql-go-tools project does not support Go versions < 1.23, so compatibility concerns for features available in Go 1.21+ (like cmp.Or) should not be raised.
Applied to files:
v2/pkg/engine/resolve/loader.go
📚 Learning: 2025-07-02T15:28:02.122Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.122Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
Applied to files:
v2/pkg/engine/resolve/loader.go
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (5)
v2/pkg/engine/resolve/subgraph_request_singleflight.go (2)
v2/pkg/engine/resolve/resolve.go (1)
New(191-256)v2/pkg/engine/resolve/fetch.go (2)
Fetch(19-26)FetchInfo(345-366)
v2/pkg/engine/resolve/inbound_request_singleflight.go (2)
v2/pkg/engine/resolve/context.go (4)
Context(16-38)ExecutionOptions(60-78)Request(185-188)SubgraphHeadersBuilder(44-50)v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(36-44)
v2/pkg/engine/resolve/subgraph_request_singleflight_test.go (2)
v2/pkg/engine/resolve/fetch.go (6)
FetchInfo(345-366)FetchKind(11-11)FetchKindSingle(14-14)FetchDependencies(110-113)Fetch(19-26)GraphCoordinate(368-372)v2/pkg/engine/resolve/subgraph_request_singleflight.go (1)
NewSingleFlight(52-65)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (3)
v2/pkg/engine/resolve/context.go (1)
NewContext(190-197)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go (1)
SubscriptionSource(1953-1955)v2/pkg/engine/datasource/httpclient/file.go (2)
FileUpload(3-7)NewFileUpload(9-15)
v2/pkg/engine/resolve/loader.go (6)
v2/pkg/engine/resolve/fetch.go (8)
PostProcessingConfiguration(115-131)FetchItemPathElement(77-81)FetchItem(28-33)DataSourceLoadTrace(374-388)BatchEntityFetch(159-168)BatchEntityFetch(193-195)Fetch(19-26)FetchInfo(345-366)v2/pkg/engine/resolve/subgraph_request_singleflight.go (1)
SubgraphRequestSingleFlight(12-15)v2/pkg/engine/resolve/context.go (2)
Context(16-38)ExecutionOptions(60-78)v2/pkg/engine/resolve/inputtemplate.go (1)
SetInputUndefinedVariables(39-51)v2/pkg/engine/resolve/resolve.go (1)
New(191-256)v2/pkg/engine/datasource/httpclient/nethttpclient.go (1)
WithHTTPClientSizeHint(142-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (7)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (4)
4011-4013: LGTM: populate SourceName/SourceID on subscription triggersConsistently tying triggers to the DS id improves traceability. No issues spotted.
Also applies to: 4054-4056
8761-8764: LGTM: Load now returns []byte; assertions updatedReturn contract and assertions look correct for null/undefined variables handling.
Also applies to: 8783-8786
8870-8872: LGTM: LoadWithFiles return value and assertions updatedSwitch to returning []byte and asserting on empty body is correct for these handlers.
Also applies to: 8927-8931
8443-8443: Nil headers passed to Start: verified as nil-safe ✓The Start() method implementation shows that nil headers are handled safely:
Direct assignment (safe): Line
options.Header = headerssimply assigns the nil value to a struct field—no dereference occurs here.Defensive nil-guard downstream: In
nethttpclient.go:167, the code checksif headers != nilbefore callingjsonparser.ObjectEach(). This prevents any unsafe access of nil headers in the transport stack.No unsafe dereferences found: Throughout the verified call path, nil headers are never dereferenced without a preceding nil check.
The code follows Go best practices for nil-safety. The concern about nil dereference is not substantiated by the codebase—the implementation properly guards against nil headers through defensive nil-checks in downstream consumers.
v2/pkg/engine/resolve/subgraph_request_singleflight_test.go (1)
9-209: Solid coverage of subgraph single‑flight semantics.The stubs plus the four tests exercise all the important behaviors: leader/follower distinction, size‑hint propagation, FetchKey vs SFKey separation, and nil/missing
FetchInfocases. The rolling‑window test also closely matches the logic inFinishand guards against regressions in the size‑hint algorithm.v2/pkg/engine/resolve/loader.go (2)
51-73: ResponseInforesponseBodyconversion looks correct.Storing the raw subgraph body as
[]byteand exposing it viaGetResponseBody()keeps the internal field private while still allowing hooks/transport code to inspect the payload. SincenewResponseInfoonly assignsres.outand consumers get a freshstringon each call, there’s no mutation risk here.
331-345: Subgraph single-flight integration in Loader is well‑threaded.
selectItemsForPathandselectItemsuse arena‑allocated slices without sharingjsonArenaacross goroutines, respecting the “not thread safe” comment.loadByContextcleanly wiresSubgraphRequestSingleFlightin: it derives headers + header hash, checkssingleFlightAllowed, uses size hints viaWithHTTPClientSizeHint, and correctly distinguishes leader vs follower.- Leader/follower stats in
singleFlightStatsare set once inloadByContextand exported viaexecuteSourceLoadtoDataSourceLoadTraceasSingleFlightUsed/SingleFlightSharedResponse, which matches the intended tracing semantics.Given downstream code only reads from
res.outanditem.responseis treated as immutable, this integration looks safe and matches the broader design changes in the PR.Also applies to: 363-401, 419-427, 115-187, 1652-1700
| func (r *InboundRequestSingleFlight) GetOrCreate(ctx *Context, response *GraphQLResponse) (*InflightRequest, error) { | ||
|
|
||
| if ctx.ExecutionOptions.DisableInboundRequestDeduplication { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if !response.SingleFlightAllowed() { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // Derive a robust key from request ID, variables hash and (optional) headers hash | ||
| var b [24]byte | ||
| binary.LittleEndian.PutUint64(b[0:8], ctx.Request.ID) | ||
| binary.LittleEndian.PutUint64(b[8:16], ctx.VariablesHash) | ||
| hh := uint64(0) | ||
| if ctx.SubgraphHeadersBuilder != nil { | ||
| hh = ctx.SubgraphHeadersBuilder.HashAll() | ||
| } | ||
| binary.LittleEndian.PutUint64(b[16:24], hh) | ||
| key := xxhash.Sum64(b[:]) | ||
|
|
||
| shard := r.shardFor(key) | ||
| req, shared := shard.m.Load(key) | ||
| if shared { | ||
| req := req.(*InflightRequest) | ||
| req.Mu.Lock() | ||
| req.HasFollowers = true | ||
| req.Mu.Unlock() | ||
| select { | ||
| case <-req.Done: | ||
| if req.Err != nil { | ||
| return nil, req.Err | ||
| } | ||
| return req, nil | ||
| case <-ctx.ctx.Done(): | ||
| return nil, ctx.ctx.Err() | ||
| } | ||
| } | ||
|
|
||
| value := &InflightRequest{ | ||
| Done: make(chan struct{}), | ||
| ID: key, | ||
| } | ||
|
|
||
| shard.m.Store(key, value) | ||
| return value, nil | ||
| } | ||
|
|
||
| func (r *InboundRequestSingleFlight) FinishOk(req *InflightRequest, data []byte) { | ||
| if req == nil { | ||
| return | ||
| } | ||
| shard := r.shardFor(req.ID) | ||
| shard.m.Delete(req.ID) | ||
| req.Mu.Lock() | ||
| hasFollowers := req.HasFollowers | ||
| req.Mu.Unlock() | ||
| if hasFollowers { | ||
| // optimization to only copy when we actually have to | ||
| req.Data = make([]byte, len(data)) | ||
| copy(req.Data, data) | ||
| } | ||
| close(req.Done) | ||
| } |
There was a problem hiding this comment.
Fix race between HasFollowers and FinishOk to avoid followers seeing empty data.
The current protocol:
- Followers:
Load→ markHasFollowers = trueunderMu→ wait onreq.Done. - Leader: on success
FinishOkdoesDelete, then readsHasFollowersunderMuto decide whether to copydataintoreq.Data, then closesreq.Done.
There is a window where a follower has already obtained req from Load but the leader wins the Mu race in FinishOk, reads HasFollowers == false, skips the copy, closes Done, and only afterwards the follower manages to set HasFollowers = true. In that case followers wake up with req.Err == nil but req.Data == nil, which breaks the contract for deduplicated requests.
Given this is correctness‑sensitive and the copy happens only once per in‑flight request, it’s safer to always copy data before closing Done and drop the HasFollowers optimization.
Example fix:
type InflightRequest struct {
- Done chan struct{}
- Data []byte
- Err error
- ID uint64
-
- HasFollowers bool
- Mu sync.Mutex
+ Done chan struct{}
+ Data []byte
+ Err error
+ ID uint64
}
func (r *InboundRequestSingleFlight) FinishOk(req *InflightRequest, data []byte) {
if req == nil {
return
}
shard := r.shardFor(req.ID)
shard.m.Delete(req.ID)
- req.Mu.Lock()
- hasFollowers := req.HasFollowers
- req.Mu.Unlock()
- if hasFollowers {
- // optimization to only copy when we actually have to
- req.Data = make([]byte, len(data))
- copy(req.Data, data)
- }
+ // Always copy to ensure any followers see a valid immutable slice.
+ if data != nil {
+ buf := make([]byte, len(data))
+ copy(buf, data)
+ req.Data = buf
+ }
close(req.Done)
}If Mu/HasFollowers aren’t used elsewhere, you can remove them entirely; otherwise, they can be kept for future use but should not gate the copy.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In v2/pkg/engine/resolve/inbound_request_singleflight.go around lines 52 to 115,
followers can race with FinishOk so they may set HasFollowers after the leader
checked it and skipped copying data, causing followers to see nil Data; remove
the HasFollowers gating and always copy the response bytes into req.Data on
FinishOk before closing req.Done (i.e., drop the conditional that checks
HasFollowers and unconditionally allocate, copy and assign req.Data while
holding the lock as needed), and remove or ignore setting HasFollowers in the
Load/follower path (or delete the field if unused).
| // jsonArena is the arena to allocation json, supplied by the Resolver | ||
| // Disclaimer: this arena is NOT thread safe! | ||
| // Only use from main goroutine | ||
| // Don't Reset or Release, the Resolver handles this | ||
| // Disclaimer: When parsing json into the arena, the underlying bytes must also be allocated on the arena! | ||
| // This is very important to "tie" their lifecycles together | ||
| // If you're not doing this, you will see segfaults | ||
| // Example of correct usage in func "mergeResult" | ||
| jsonArena arena.Arena |
There was a problem hiding this comment.
Align rewriteErrorPaths with arena parsing rules to avoid segfaults.
The comments around jsonArena and the implementation of mergeResult/compactJSON are very explicit:
- Before calling
ParseBytesWithArena, the underlying byte slice should be allocated on the samearena.Arenato tie lifetimes correctly. compactJSONdeliberately avoids arena parsing for this reason.
In rewriteErrorPaths, however, the code does:
newPathJSON, _ := json.Marshal(newPath)
pathBytes, err := astjson.ParseBytesWithArena(a, newPathJSON)
...
value.Set(a, "path", pathBytes)Here newPathJSON is heap‑allocated and not tied to a, which contradicts the earlier guidance and could lead to subtle memory issues when the arena is reset or reused.
To make this consistent with mergeResult and the comments above jsonArena, allocate the bytes on a before parsing:
- newPathJSON, _ := json.Marshal(newPath)
- pathBytes, err := astjson.ParseBytesWithArena(a, newPathJSON)
+ newPathJSON, _ := json.Marshal(newPath)
+ // Allocate bytes on the same arena before parsing to tie lifecycles together.
+ buf := arena.AllocateSlice[byte](a, len(newPathJSON), len(newPathJSON))
+ copy(buf, newPathJSON)
+ pathBytes, err := astjson.ParseBytesWithArena(a, buf)
if err != nil {
continue
}
- value.Set(a, "path", pathBytes)
+ value.Set(a, "path", pathBytes)This mirrors the pattern used in mergeResult and keeps arena usage consistent.
Also applies to: 488-497, 899-949, 1893-1903
🤖 Prompt for AI Agents
v2/pkg/engine/resolve/loader.go around lines 175-183 (and also at roughly
488-497, 899-949, 1893-1903): rewriteErrorPaths currently marshals into a
heap-allocated []byte then calls ParseBytesWithArena(a, ...), which violates the
arena lifetime rules and can cause segfaults; instead, allocate/copy the
marshaled bytes into the same arena before parsing (e.g. allocate a byte slice
on a or use the arena's copy/append helper to place the bytes into a) and then
call ParseBytesWithArena with that arena-backed slice, and finally call
value.Set(a, "path", parsedBytes); update all mentioned locations to follow this
pattern so the parsed JSON and the underlying bytes share the arena lifetime.
| func NewSingleFlight(shardCount int) *SubgraphRequestSingleFlight { | ||
| if shardCount <= 0 { | ||
| shardCount = defaultSingleFlightShardCount | ||
| } | ||
| s := &SubgraphRequestSingleFlight{ | ||
| shards: make([]singleFlightShard, shardCount), | ||
| xxPool: &sync.Pool{ | ||
| New: func() any { | ||
| return xxhash.New() | ||
| }, | ||
| }, | ||
| } | ||
| return s | ||
| } |
There was a problem hiding this comment.
Reset and reuse xxhash.Digest correctly; current fetchKey is unstable and leaks entropy into sizes.
Two related issues around the xxPool usage and key computation:
computeFetchKeynever resets the digest:
func (s *SubgraphRequestSingleFlight) computeFetchKey(fetchItem *FetchItem) uint64 {
h := s.xxPool.Get().(*xxhash.Digest)
defer s.xxPool.Put(h)
...
_, _ = h.WriteString(info.DataSourceID)
...
sum := h.Sum64()
return sum
}Because h.Reset() is never called, each call appends to the previous state. The hash for a given (DataSourceID, RootFields) pair therefore changes over time and depends on prior calls. That means:
- Identical fetches do not share a stable
fetchKey. shard.sizesfills with one entry per call instead of per logical fetch, breaking grouping and causing unbounded growth of thesizesmap.sizeHintalmost always stays at zero for leaders, defeating the buffer‑sizing optimization.
computeSFKeyandcomputeKeysmisuse the pool:
computeKeysgets a digest (h := s.xxPool.Get()) but never passes it to the helpers; it justReset()s andPut()s it.computeSFKeyindependently gets a digest from the pool but neverReset()s orPut()s it, so those digests are never reused and the pool is effectively bypassed.
A minimal and safe pattern is to allocate one digest, reset it, and reuse it for both keys per call:
func (s *SubgraphRequestSingleFlight) computeKeys(fetchItem *FetchItem, input []byte, extraKey uint64) (sfKey, fetchKey uint64) {
- h := s.xxPool.Get().(*xxhash.Digest)
- sfKey = s.computeSFKey(fetchItem, input, extraKey)
- h.Reset()
- fetchKey = s.computeFetchKey(fetchItem)
- h.Reset()
- s.xxPool.Put(h)
- return sfKey, fetchKey
+ h := s.xxPool.Get().(*xxhash.Digest)
+ defer s.xxPool.Put(h)
+
+ h.Reset()
+ sfKey = s.computeSFKeyWithDigest(h, fetchItem, input, extraKey)
+
+ h.Reset()
+ fetchKey = s.computeFetchKeyWithDigest(h, fetchItem)
+
+ return sfKey, fetchKey
}
-func (s *SubgraphRequestSingleFlight) computeFetchKey(fetchItem *FetchItem) uint64 {
- h := s.xxPool.Get().(*xxhash.Digest)
- defer s.xxPool.Put(h)
+func (s *SubgraphRequestSingleFlight) computeFetchKeyWithDigest(h *xxhash.Digest, fetchItem *FetchItem) uint64 {
if fetchItem == nil || fetchItem.Fetch == nil {
return 0
}
...
_, _ = h.WriteString(info.DataSourceID)
...
return h.Sum64()
}and similarly introduce computeSFKeyWithDigest(h *xxhash.Digest, ...) that writes datasource ID + body + header hash into h and returns h.Sum64().
This gives you:
- Stable
fetchKeyfor a given(DataSourceID, RootFields)tuple. - Bounded
sizesmap keyed by logical fetches, as intended. - Actual reuse of the pooled digest without per‑call allocations.
Also applies to: 84-105, 141-153, 171-195
Summary by CodeRabbit
Performance Improvements
Subscription Enhancements
API Refinements
Behavioral Change