Repository navigation
chipingress: fix retry policy service-config, make it configurable - #2373
Conversation
✅ API Diff Results -
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Default retries may replay side-effecting RPCs, with additional policy validation and test coverage gaps.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
Fixes malformed ChipIngress gRPC retry configuration and adds typed, configurable retry policies.
Changes:
- Corrects service-config nesting and duration encoding.
- Adds
WithRetryPolicyand retry throttling. - Adds parser-backed regression tests.
| File | Summary |
|---|---|
pkg/chipingress/retry_policy.go |
Defines retry policies and generates service configuration JSON. |
pkg/chipingress/retry_policy_test.go |
Tests generated configuration and parser behavior. |
pkg/chipingress/client.go |
Applies default and custom retry policies to clients. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build retry policy service config: %w", err) | ||
| } | ||
| grpcOpts = append(grpcOpts, grpc.WithDefaultServiceConfig(retryServiceConfig)) |
There was a problem hiding this comment.
Agreed. Retries are now opt-in: NewClient installs no retry service config unless the caller passes WithRetryPolicy, so this ships disabled and can be rolled out per-client. DefaultRetryPolicy() is exported for callers who opt in, and the WithRetryPolicy doc calls out that the policy also replays Publish/PublishBatch/RegisterSchema and that callers should set idempotencykey when enabling it.
| MethodConfig: []methodConfigJSON{ | ||
| { | ||
| Name: []methodNameJSON{{Service: pb.ChipIngress_ServiceDesc.ServiceName}}, |
There was a problem hiding this comment.
Same resolution as the thread on client.go: the policy is service-wide, so it is no longer applied by default. It only takes effect when a client opts in with WithRetryPolicy, and the doc now documents the duplicate-delivery caveat. Per-method scoping (e.g. only Ping) is a possible follow-up if we want to enable it more broadly later.
| throttling := RetryThrottlingPolicy{MaxTokens: 20, TokenRatio: 0.2} | ||
| scJSON, err := buildRetryServiceConfigJSON(defaultRetryPolicy(), &throttling) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Fixed. The test now decodes the generated JSON and asserts retryThrottling is present with maxTokens: 20 / tokenRatio: 0.2, asserts the block is omitted when throttling is nil, and proves gRPC actually parses it by checking that maxTokens: 0 (out of range) is rejected.
| // hand-writing a gRPC "retryPolicy" service-config JSON block, which is easy to get wrong: the | ||
| // nesting (methodConfig[].retryPolicy, not top-level), the duration encoding (protobuf JSON | ||
| // duration, e.g. "0.1s", not Go's "100ms"), and the status-code names are all silently ignored | ||
| // by gRPC's parser if malformed, rather than rejected - see the client_test.go regression test |
There was a problem hiding this comment.
Fixed, now points to retry_policy_test.go.
The retry policy passed to grpc.WithDefaultServiceConfig was a bare
retry-policy object, missing the required methodConfig[].retryPolicy
wrapper that gRPC service config requires. gRPC's parser silently
discards unknown top-level fields via plain json.Unmarshal, so the
config parsed to a valid-but-empty result (MethodConfig == nil) with
no error. grpc.NewClient succeeded silently. This client has never
performed a single gRPC-level retry.
Fixes the JSON shape via a typed RetryPolicy struct and a JSON builder
(buildRetryServiceConfigJSON) instead of a hand-written literal, so this
bug class can't recur silently. Adds WithRetryPolicy so callers can
override without hand-writing service-config JSON themselves. Pairs the
retry policy with retryThrottling (10 tokens, 0.1 ratio) so a genuinely
degraded server cannot be amplified by a newly-functioning retry policy.
Verified from source rather than assumed:
- Duration format: grpc-go's internal serviceconfig.Duration requires
protobuf-JSON seconds strings ("0.1s"); the original "100ms" does not
parse. Once methodConfig is correctly nested, an invalid duration now
makes grpc.NewClient return a hard error instead of silently doing
nothing -- confirmed with a test.
- Service name: read from pb.ChipIngress_ServiceDesc.ServiceName at
runtime rather than hardcoding a copy of the string.
- Call sites: only pkg/beholder/client.go and pkg/durableemitter/setup.go
call chipingress.NewClient, both via variadic Opt, so this is
non-breaking; existing callers get the corrected default policy.
The regression test drives the constructed JSON through gRPC's real
service-config parser (via a manual resolver + ClientConn.ParseServiceConfig)
and asserts MethodConfig is actually populated -- a bare "no error"
assertion would not have caught the original bug, since the original bug
never produced an error. Also includes a test proving the old malformed
JSON parses with zero MethodConfig entries, and a test proving "100ms"
is rejected.
Does not address DEADLINE_EXCEEDED failures, and would not have changed
the root cause of a separate incident this fix was found while
investigating -- retries share the caller's deadline, and the dominant
failure mode there had already exhausted it. This closes a chronic,
unrelated data-loss gap: every UNAVAILABLE/RESOURCE_EXHAUSTED failure
that should have retried was instead an immediate permanent drop, on a
path with no persistence.
- clientConfig gains a *RetryPolicy pointer instead of a value so the struct stays comparable; api-diff CI reports a comparability break as a breaking change via the exported Opt type. A compile-time map-key guard now enforces comparability. - Silence SA1019 (staticcheck) in retry_policy_test.go: grpc's ParseServiceConfig only returns the deprecated ServiceConfig concrete type, and inspecting it is the point of the regression tests.
Replace the grpcStatusCodeName switch with a canonical-names table whose
doc records why codes.Code.String() cannot be used: String() is CamelCase
("Unavailable") and even spells Canceled differently ("Canceled" vs
canonical "CANCELLED" [sic]), while the service-config parser
(codes.Code.UnmarshalJSON -> strToCode) accepts only canonical uppercase
names. Unknown codes now fail loudly from buildRetryServiceConfigJSON
with a clear message; the old numeric fallback emitted a quoted numeric
string, which the parser rejects (only bare unquoted numerics parse).
Drop the mapKeys test helper in favor of slices.Collect(maps.Keys(...)).
Add TestGRPCStatusCodeNames_RoundTripThroughParser: all 17 well-known
names round-trip through codes.Code.UnmarshalJSON back to the same code,
and String() output is rejected.
b25bbc1 to
417fc14
Compare
|
Looks good to me. The previous review concerns have been addressed: retries are now opt-in, the duplicate-delivery risk of service-wide retries is clearly documented, and the parser-backed tests verify the generated retry and throttling configuration. The status-code handling and follow-up test coverage also look solid. No further blocking comments from me—approved. |
Summary
The retry policy passed to
grpc.WithDefaultServiceConfiginpkg/chipingress/client.gowas a bareretry-policy object, missing the
methodConfig[].retryPolicywrapper gRPC service config requires.The gRPC parser silently discards unknown top-level fields via plain
json.Unmarshal, so the configparsed to a valid-but-empty result (
MethodConfig == nil) with no error.grpc.NewClientsucceeded silently. This client has never performed a single gRPC-level retry.
RetryPolicystruct and a JSON builder(
buildRetryServiceConfigJSON) instead of a hand-written literal, so this bug class cannot recursilently.
WithRetryPolicyso callers can enable and tune retries without hand-writing service-config JSON.retryThrottling(10 tokens, 0.1 ratio) so a degraded servercannot be amplified by retries.
Why opt-in.
Jira: INFOPLAT-19422
Usage
Why opt-in
The policy is scoped to the whole
ChipIngressservice, so it also replaysPublish,PublishBatchandRegisterSchema. AnUNAVAILABLEcan be returned after the server has alreadydone the work, so a retried batch can produce duplicate events unless the caller sets the
idempotencykeyextension (IdempotencyKeyAttr). Because the previous config never took effect,turning retries on by default would have been a first-ever behavior change for every caller.
Verified from source, not assumed
serviceconfig.Durationrequires protobuf-JSON secondsstrings (
"0.1s"); the original"100ms"does not parse. OncemethodConfigis correctlynested, an invalid duration now makes
grpc.NewClientreturn a hard error instead of silentlydoing nothing - confirmed with a test.
codes.Codehas no JSON marshaler, so codes are emitted as bare numbers, whichgRPC's
codes.Code.UnmarshalJSONaccepts.codes.Code.String()("Unavailable") is not validparser input (only upper-case names or bare numbers are), so config-by-name goes through
ParseStatusCodes, built fromString()so it can't drift. Codes aboveUnauthenticatedarerejected up front.
pb.ChipIngress_ServiceDesc.ServiceNameat runtime rather thanhardcoding a copy of the string.
pkg/beholder/client.goandpkg/durableemitter/setup.gocallchipingress.NewClient, both via variadicOpt. With retries opt-in there is no behavior changefor existing callers; the change is additive API (
RetryPolicy,RetryThrottlingPolicy,WithRetryPolicy,DefaultRetryPolicy,ParseStatusCodes).Regression test
The defining property of this bug is that it produces no error, so a bare no error assertion
would not have caught it.
retry_policy_test.godrives the constructed JSON through the real gRPCservice-config parser (manual resolver +
ClientConn.ParseServiceConfig) and assertsMethodConfigis actually populated. Also includes a test proving the old malformed JSON parses with zero
MethodConfigentries, a test proving"100ms"is rejected, throttling value checks, a round-tripof every well-known status code through the parser, and a guard that retries are off by default.
Non-goals / follow-ups
DEADLINE_EXCEEDEDfailures: retries share the caller deadline, and thedominant failure mode in a separate production incident had already exhausted it.
tracked separately: INFOPLAT-19419, INFOPLAT-19420, INFOPLAT-19421.
idempotency story for
Publish/PublishBatchis confirmed.Test plan
go build ./...,go vet ./pkg/chipingress/...,gofmt -lcleanretry_policy_test.gopasspkg/beholder/...full suite passes