Skip to content

fix: propagate actualListSizes via resolver context#1404

Merged
ysmolski merged 3 commits intomasterfrom
yury/eng-8986-expose-costs-in-response-headers
Feb 23, 2026
Merged

fix: propagate actualListSizes via resolver context#1404
ysmolski merged 3 commits intomasterfrom
yury/eng-8986-expose-costs-in-response-headers

Conversation

@ysmolski
Copy link
Copy Markdown
Contributor

@ysmolski ysmolski commented Feb 23, 2026

It is needed to make router work with setting the header for actual cost via special hook that is called before writing to the client happens.

Summary by CodeRabbit

  • Refactor
    • Improved how actual list sizes are tracked for cost computation: size data is now stored per-request execution context rather than response objects, and cleanup now clears this data to avoid lingering references. This streamlines internal data flow and reduces risk of stale data affecting cost calculations.

It is needed to make router work with setting the header
for actual cost via special hook that is called before writing to the
client happens.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b65946 and 198f90c.

📒 Files selected for processing (2)
  • execution/engine/execution_engine.go
  • v2/pkg/engine/resolve/context.go

📝 Walkthrough

Walkthrough

Actual list size tracking moved from response-level GraphQLResolveInfo to the per-request resolve Context. Resolution now populates Context.ActualListSizes, and the execution engine reads actual-cost data from execContext.resolveContext instead of the response.

Changes

Cohort / File(s) Summary
Execution Engine
execution/engine/execution_engine.go
Execute now reads ActualListSizes from execContext.resolveContext (removed use of response-held data and the internalExecutionContext.reset() call).
Resolve Context API
v2/pkg/engine/resolve/context.go
Added public field ActualListSizes map[string]int to Context; Free() clears ActualListSizes.
Resolver Flow Updates
v2/pkg/engine/resolve/resolve.go
Removed ActualListSizes from GraphQLResolveInfo; resolve functions assign actual list sizes to ctx.ActualListSizes instead of resp.
Module Manifest
go.mod
Minor edits (lines changed +2/-8).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: propagate actualListSizes via resolver context' accurately and concisely summarizes the main change—moving the ActualListSizes field from the response to the resolver context for cost header propagation.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch yury/eng-8986-expose-costs-in-response-headers

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@v2/pkg/engine/resolve/context.go`:
- Around line 48-51: Context.Free currently doesn't clear the per-request map
ActualListSizes, so add ActualListSizes to the cleanup logic inside the
Context.Free method: ensure Context.Free resets or nils out c.ActualListSizes
(in the same place other per-request fields are cleared) to avoid retaining
stale data or memory when contexts are reused; locate the Context.Free function
and update it to clear c.ActualListSizes alongside the other cleanup steps.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2657bf and 0b65946.

📒 Files selected for processing (3)
  • execution/engine/execution_engine.go
  • v2/pkg/engine/resolve/context.go
  • v2/pkg/engine/resolve/resolve.go

Comment thread v2/pkg/engine/resolve/context.go
@ysmolski ysmolski merged commit 5433fcb into master Feb 23, 2026
11 checks passed
@ysmolski ysmolski deleted the yury/eng-8986-expose-costs-in-response-headers branch February 23, 2026 15:19
ysmolski pushed a commit that referenced this pull request Feb 23, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.0.0-rc.256](v2.0.0-rc.255...v2.0.0-rc.256)
(2026-02-23)


### Bug Fixes

* propagate actualListSizes via resolver context
([#1404](#1404))
([5433fcb](5433fcb))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
ysmolski pushed a commit that referenced this pull request Mar 20, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.10.0](execution/v1.9.0...execution/v1.10.0)
(2026-03-19)


### Features

* add initial support for [@requires](https://github.com/requires) in
gRPC
([#1362](#1362))
([07ae75e](07ae75e))
* support sizedFields in cost computations
([#1410](#1410))
([48f7582](48f7582))


### Bug Fixes

* propagate actualListSizes via resolver context
([#1404](#1404))
([5433fcb](5433fcb))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants