fix(gateway): stop benching domains for our own cancellations; dedupe websocket health checks - #529
Open
oten91 wants to merge 3 commits into
Open
fix(gateway): stop benching domains for our own cancellations; dedupe websocket health checks#529oten91 wants to merge 3 commits into
oten91 wants to merge 3 commits into
Conversation
HTTP health checks were deduplicated per backend URL, but websocket checks were explicitly not: the sibling loop probed each supplier individually. A backend carrying N supplier registrations therefore took N full TCP+TLS+WS handshakes per cycle where HTTP took one, so the cost tracked how a provider spread its registrations rather than how much traffic it served. Stacked registrations are common, and one operator was receiving roughly 3 handshakes per second per gateway egress IP as a result. Fire one handshake per distinct backend websocket URL and record its outcome for every supplier sharing that URL. Recording rather than skipping is load-bearing: a websocket probe is a property of the backend, so a broken backend must move every sharing supplier's websocket reputation key. Skipping the siblings would penalize only the probed supplier while leaving the others on the same backend clean and selectable, and the endpoint would be reached anyway through a different supplier address. The probing supplier rotates with the cycle counter so each one's own websocket path is still validated directly. Also drop the text from the health-check probe's close frame. The close code is what keeps these probes out of an endpoint's error budget; the reason string added nothing an operator could act on. The new test asserts two backend URLs across five endpoints cost two handshakes with all four sharing suppliers still recorded. Its first version failed because both test URLs sat under one registrable domain and the metric label is eTLD+1 — the same collapse that hides per-hostname state behind a per-operator series.
…sued An endpoint cannot cancel our context — only we can — so context.Canceled is never evidence about the endpoint. A genuinely slow endpoint surfaces as context.DeadlineExceeded, which is a real signal and still breaks. The two were being conflated, and healthy operators were locked out for hours at gate failure rates under one percent. The existing guard only covered a cancel of the CLIENT's context. A second source went straight past it. The hedge racer repoints the primary's protocol context at a detached parent (detachedHedgeCtx + SetParentContext) and cancels that parent on every one of its exit paths. When the race produced no usable response, the batch item fell through to the normal request path and reused the same, now-dead, context: the relay failed immediately with "context canceled" without ever reaching the endpoint, and that was stamped on whichever endpoint happened to be primary. A fifteen-minute fleet capture recorded 158 breaks, 22 of them cancellations. On one batch-heavy service the split falls cleanly along operators: the operator serving it from two hostnames took 18 of its 19 breaks as cancellations, while the two other operators took 10 of 12 as deadlines. That is the shape of a fault of ours landing on one participant, not of that participant degrading. Two changes. shouldCircuitBreak now returns false for a wrapped context.Canceled; it lives in the shared decision function because MarkBroken has four callers and the invariant belongs to all of them. And the hedge fallthrough builds a fresh protocol context instead of reusing the cancelled one, so the fallthrough actually sends. The control case in the existing test used context.Canceled with a live parent and asserted that it should break — the exact production shape, asserted as correct — so it could not separate a cancel from a real fault. It now uses connection refused, and a case covering cancel-with-live-parent is added alongside it. Both fixes were revert-checked; the rebuild assertion is on call ORDER, since a count alone passes on the buggy code because the race also builds a context for the hedge endpoint.
The misspell check runs with locale US. Comments added in this branch used cancelled, behaviour and labelled.
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
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.
Two independent gateway fixes found while diagnosing why one operator was receiving
no traffic on a batch-heavy service.
1. Circuit breaker benched domains for cancellations we issued
An endpoint cannot cancel our context — only we can — so
context.Canceledis neverevidence about the endpoint. A genuinely slow endpoint surfaces as
context.DeadlineExceeded, which is a real signal and still breaks. The two were beingconflated.
An existing guard covered a cancel of the client's context. A second source went
straight past it: the hedge racer repoints the primary's protocol context at a detached
parent (
detachedHedgeCtx+SetParentContext) and cancels that parent on every one ofits exit paths. When the race produced no usable response, the batch item fell through to
the normal request path and reused the same, now-dead, context — the relay failed
instantly with
context canceledwithout ever reaching the endpoint, and that was stampedon whichever endpoint happened to be primary.
Impact measured before the fix: one operator's endpoints were circuit-broken on 12–18 pods
near-continuously for six hours at a gate failure rate of 0.75%, while its traffic was
carried entirely by its second hostname.
Two changes:
shouldCircuitBreakreturns false for a wrappedcontext.Canceled. It lives in theshared decision function because
MarkBrokenhas four callers and the invariant belongsto all of them.
one, so the fallthrough actually sends.
2. Websocket health checks were not deduplicated by backend URL
HTTP checks were deduplicated per backend URL; websocket checks were explicitly not. A
backend carrying N supplier registrations took N full TCP+TLS+WS handshakes per cycle
where HTTP took one, so the cost tracked how a provider spread its registrations rather
than how much traffic it served. One operator was receiving roughly 3 handshakes per
second per gateway egress IP.
Now one handshake per distinct backend websocket URL, with the outcome recorded for every
supplier sharing that URL. Recording rather than skipping is load-bearing: a websocket
probe is a property of the backend, so a broken backend must move every sharing supplier's
websocket reputation key — otherwise only the probed supplier is penalized and the others
stay clean and selectable while fronting the same machine. The probing supplier rotates
with the cycle counter so each one's own path is still validated directly.
Also drops the text from the probe's close frame; the close code is what keeps these out
of an endpoint's error budget.
Testing
Both fixes revert-checked — the fix removed, the test confirmed failing, the fix restored.
TestProcessSinglePayloadWithRetry_ClientCancelDoesNotFeedCircuitBreaker— the controlcase previously used
context.Canceledwith a live parent and asserted it shouldbreak, which is the exact production shape asserted as correct, so it could not separate
a cancel from a real fault. Control now uses
connection refused, with a new casecovering cancel-with-live-parent.
TestProcessSinglePayloadWithRetry_HedgeFallthroughRebuildsProtocolContext— asserts oncall ORDER (
build relay build relay), because a count alone passes on the buggy code:the race also builds a context for the hedge endpoint.
Test_WebsocketCheck_OneHandshakePerBackendURL— five endpoints across two backend URLscost two handshakes, with all four sharing suppliers still recorded.
Canary validation
Canary on this build, mainnet as control. Roughly ten minutes in:
context canceledbreaks on canary across all services, in 25 captured breakevents. Before the fix, one operator alone produced 18 in fifteen minutes.
same window, and mainnet is still breaking the operator that canary no longer breaks.
deadlinebreaks, confirming the guard is not over-matching.Note the websocket half is not observable in existing metrics:
path_health_check_status_totalcounts recorded results, and the fan deliberately keepsone result per supplier, so the handshake reduction is real but invisible there. It is
covered by the unit test rather than by a production counter.