Grpc-Tunnel: Fix pool var rotation - #134
Merged
Merged
Conversation
This was referenced Aug 28, 2026
shawnburke
changed the title
aszarama
previously approved these changes
Aug 28, 2026
shawnburke
dismissed
aszarama’s stale review
August 28, 2026 06:33
The merge-base changed after approval.
`Origin()` expanded ${VAR} against the environment before the pool was
consulted, so a variable that exists only as `VAR_POOL` became the empty
string and every request against a pooled origin failed — while
`pool_test.go`, which exercises `PoolManager` directly, stayed green.
The Router now resolves the origin from `RawOrigin()`, with the ${VAR}
reference intact, so `ResolvePoolVars` has something left to rotate.
`defaultScheme` comes out of `Origin()` so both paths fill in https the
same way.
`router_pool_test.go` covers this at the Router level, where the bug
was; it fails against the old resolution.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shawnburke
force-pushed
the
grpc/pool-rotation
branch
from
August 28, 2026 06:33
fd239c5 to
3392fe0
Compare
aszarama
approved these changes
Aug 28, 2026
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
2 participants
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.
Stack 2/3 — base #133. Review the two-commit range or just the top commit.
The bug
Every request against a pooled origin failed.
Origin()runsos.ExpandEnvon the raw origin before the pool is consulted. A variable that exists only asVAR_POOL— which is the whole point of a pool — has no plainVARin the environment, so${API}expanded to the empty string.ResolvePoolVarsthen had nothing left to rotate: the${API}reference it matches on was already gone.Two things kept this hidden:
pool_test.goexercisesPoolManagerdirectly, so it never went throughOrigin()and stayed green.varIsSetacceptsVAR_POOLas satisfying a required variable, so an accept file declaring a pool loaded cleanly and only failed at request time.The fix
The Router resolves from
RawOrigin()— the origin exactly as written,${VAR}intact — soResolvePoolVarshas a reference to rotate on.defaultSchemecomes out ofOrigin()so both paths fill inhttpsidentically;Origin()now delegates to it and is otherwise unchanged.Origin()is the one accessor this package shares with the snyk-broker reflector (relay_instance_manager.go).TestOriginContractpins its behaviour and is untouched by this change.Test plan
router_pool_test.gocovers rotation at the Router level, where the bug was — round-robin across requests, whitespace/empty-entry trimming, and a pool of one.resolveOrigincall makes all three fail with a routing error, and restoring it makes them pass. The tests catch the regression rather than merely describing the fix.go test ./server/snykbroker/... ./common/...green.🤖 Generated with Claude Code