fix(gateway): prevent API routes from randomly disappearing after concurrent deploys causing 404s#2747
Open
tricktron wants to merge 4 commits into
Open
fix(gateway): prevent API routes from randomly disappearing after concurrent deploys causing 404s#2747tricktron wants to merge 4 commits into
tricktron wants to merge 4 commits into
Conversation
tricktron
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Induwara04,
Krishanx92,
PasanT9,
Piumal1999,
RakhithaRR,
Tharsanan1,
Thushani-Jayasekera,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
lasanthaS,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub,
tharindu1st and
thivindu
as code owners
July 18, 2026 10:04
Contributor
📝 WalkthroughWalkthroughChangesThe event listeners now perform bounded synchronous xDS snapshot updates. xDS snapshot race fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add a deterministic test that reproduces the TOCTOU race in UpdateSnapshot. A test hook forces goroutine A to read a stale store while goroutine B writes the correct snapshot — then A overwrites it with a higher version number. The test is RED: api-two is missing from the final snapshot.
Protect the GetAll → Translate → IncrementVersion → SetSnapshot sequence with sync.Mutex, matching the pattern used by the other four snapshot managers (subscription, apikey, lazyresource, policy).
Rename updateSnapshotAsync to updateSnapshot and remove the go func() wrapper. The event loop is already serial, and the mutex on SnapshotManager now protects concurrent callers from other paths (certificates, control plane sync).
tricktron
force-pushed
the
api-publish-race-bug
branch
from
July 18, 2026 14:54
1326ac4 to
50f94bf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/gateway-controller/pkg/xds/snapshot.go`:
- Around line 66-74: Replace the blocking sync.Mutex update gate in the gateway
controller with a context-aware semaphore, such as a buffered channel, and
update the serialization logic around UpdateSnapshot to acquire it using the
caller’s context. If acquisition is canceled, return ctx.Err() immediately
without translation or cache work; release the gate after the update completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04d2da14-19bf-434c-9361-c771a1d4da3d
📒 Files selected for processing (5)
gateway/gateway-controller/pkg/eventlistener/api_processor.gogateway/gateway-controller/pkg/eventlistener/llm_provider_processor.gogateway/gateway-controller/pkg/eventlistener/mcp_processor.gogateway/gateway-controller/pkg/xds/snapshot.gogateway/gateway-controller/pkg/xds/snapshot_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- gateway/gateway-controller/pkg/eventlistener/mcp_processor.go
- gateway/gateway-controller/pkg/eventlistener/llm_provider_processor.go
- gateway/gateway-controller/pkg/xds/snapshot_test.go
- gateway/gateway-controller/pkg/eventlistener/api_processor.go
Comment on lines
+66
to
+74
| mu sync.Mutex | ||
| cache cache.SnapshotCache | ||
| translator *Translator | ||
| store *storage.ConfigStore | ||
| logger *slog.Logger | ||
| nodeID string // Node ID for Envoy (default: "router-node") | ||
| statusCallback StatusUpdateCallback | ||
| sdsSecretManager *SDSSecretManager | ||
| afterGetAll func() // nil in production; test hook for deterministic race testing |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make update serialization honor the caller’s context deadline.
The new sync.Mutex blocks before UpdateSnapshot can observe ctx.Done(). Under concurrent updates, a synchronous event refresh can wait past its bounded timeout and then still perform translation and cache work. Use a context-aware gate (for example, a buffered channel semaphore) and return the context error when acquisition is canceled.
Also applies to: 106-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gateway/gateway-controller/pkg/xds/snapshot.go` around lines 66 - 74, Replace
the blocking sync.Mutex update gate in the gateway controller with a
context-aware semaphore, such as a buffered channel, and update the
serialization logic around UpdateSnapshot to acquire it using the caller’s
context. If acquisition is canceled, return ctx.Err() immediately without
translation or cache work; release the gate after the update completes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
updateSnapshotAsyncspawns a goroutine per event. Multiple goroutines callUpdateSnapshotconcurrently. Each reads the config store, translates, increments the version, and writes the snapshot without any synchronization. A slow goroutine can overwrite a newer snapshot with stale data, dropping API routes. Gateway pods then 404 requests depending on which snapshot they received.Resolves #2739
Goals
UpdateSnapshotwith a mutex so concurrent callers can't interleave the read-write sequenceupdateSnapshotAsyncto make it synchronous. Also rename it toupdateSnapshot.Approach
Two changes, matching patterns already in the codebase:
Add
sync.Mutextoxds.SnapshotManager:Lock/Unlockwraps the entireUpdateSnapshotbody. The other four snapshot managers (subscription, apikey, lazyresource, policy) already do this.xds.SnapshotManagerwas the only one without it.Rename
updateSnapshotAsync->updateSnapshot, remove thego func()wrapper. The event loop processes events serially, and the subscription manager'sUpdateSnapshotis already called synchronously in the same path.The test uses Go's
testHookpattern (same as net/http) to force the exact goroutine interleaving that causes the stale overwrite. It fails without the mutex and passes with it. Check out the b494ad5 to see it fail or remove the mutex manually.User stories
As a gateway operator, I want all deployed API routes to be consistently available across all gateway pods so that requests aren't randomly 404'd depending on which pod handles them.
Documentation
N/A. Bug fix with no API changes, no new user-facing behavior.
Automation tests
TestConcurrentUpdateSnapshotinpkg/xds/snapshot_test.go: deterministic TOCTOU race reproduction using test hook. Forces stale interleaving, asserts the snapshot contains routes for all APIs.Security checks
Samples
N/A. Internal concurrency fix, no sample changes.
Related PRs
None.
Test environment