Skip to content

fix/refactor: deep copy status in translator for all resources#9458

Open
rudrakhp wants to merge 1 commit into
envoyproxy:mainfrom
rudrakhp:translator_coalesce_race_gw
Open

fix/refactor: deep copy status in translator for all resources#9458
rudrakhp wants to merge 1 commit into
envoyproxy:mainfrom
rudrakhp:translator_coalesce_race_gw

Conversation

@rudrakhp

@rudrakhp rudrakhp commented Jul 9, 2026

Copy link
Copy Markdown
Member

@rudrakhp rudrakhp requested a review from a team as a code owner July 9, 2026 13:24
@netlify

netlify Bot commented Jul 9, 2026

Copy link
Copy Markdown

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit 3247d7d
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/6a54fa5960cc650008e1250f
😎 Deploy Preview https://deploy-preview-9458--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@rudrakhp rudrakhp force-pushed the translator_coalesce_race_gw branch from 0c169cb to a9bef03 Compare July 9, 2026 13:57
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.10526% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.38%. Comparing base (2df4a96) to head (3247d7d).

Files with missing lines Patch % Lines
internal/gatewayapi/resource/resource.go 91.30% 2 Missing and 2 partials ⚠️
internal/gatewayapi/securitypolicy.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9458      +/-   ##
==========================================
- Coverage   75.41%   75.38%   -0.04%     
==========================================
  Files         252      252              
  Lines       41660    41608      -52     
==========================================
- Hits        31417    31365      -52     
+ Misses       8113     8111       -2     
- Partials     2130     2132       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@rudrakhp rudrakhp force-pushed the translator_coalesce_race_gw branch 3 times, most recently from 864d3a4 to be6f0dd Compare July 9, 2026 17:16
@rudrakhp rudrakhp added this to the v1.9.0-rc.1 Release milestone Jul 9, 2026

@arkodg arkodg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team July 12, 2026 22:33
@arkodg

arkodg commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

hey @rudrakhp gen-check is failng

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
@rudrakhp rudrakhp force-pushed the translator_coalesce_race_gw branch from 2b66ec8 to 3247d7d Compare July 13, 2026 14:46
@anrs

anrs commented Jul 14, 2026

Copy link
Copy Markdown

StatusDeepCopy() isolates status mutations correctly, but resolveLocalObjectRefsInPolicy mutates Spec.ResponseOverride[*].Response.Body (Type/Inline/ValueRef) in place on the same *CustomResponseBody that the original holds. Since StatusDeepCopy only deep-copies Status and shallow-copies Spec, the Spec pointer graph is still shared with watchable's coalesce goroutine. This leaves the same class of race open for any BTP with a ResponseOverride.Response.Body.ValueRef — it just needs a different trigger than the original Gateway/GatewayClass panic probably.

@rudrakhp

rudrakhp commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

Good catch, and you're right — resolveLocalObjectRefsInPolicy mutates Spec.ResponseOverride[*].Response.Body in place on the same *CustomResponseBody the shared tree holds, and since StatusDeepCopy() only deep-copies Status, that Spec pointer graph is still shared with the coalesce goroutine's reflect.DeepEqual walk. So this is a genuine instance of the same class of race.

Two clarifications on the scope:

  • It's gated on the merge path (mergeBackendTrafficPolicy returns early when routePolicy.Spec.MergeType == nil), so the trigger is a BTP that has both spec.mergeType set and a responseOverride[...].response.body.valueRef — narrower than any BTP with a ValueRef, but still real and reachable.
  • As you note, it won't reproduce the exact slice index out of range (that needs a slice resize mid-walk); reassigning the three body fields would more likely surface as a torn read / different panic under the race detector. Same class, different symptom.

On the fix: I'd like to keep this PR's invariant that only Status is ever deep-copied — broadening the pre-translation copy to cover Spec sub-trees would reintroduce the memory cost we're explicitly avoiding. The right fix here is to stop mutating Spec at all, and instead adopt the field-owner pattern introduced for SecurityPolicy merge in #8538.

I will open a follow up for that fix.

cc @arkodg @kkk777-7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants