Skip to content

chipingress: fix retry policy service-config, make it configurable - #2373

Draft
pkcll wants to merge 1 commit into
mainfrom
fix/chipingress-retry-config
Draft

chipingress: fix retry policy service-config, make it configurable#2373
pkcll wants to merge 1 commit into
mainfrom
fix/chipingress-retry-config

Conversation

@pkcll

@pkcll pkcll commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

The retry policy passed to grpc.WithDefaultServiceConfig in pkg/chipingress/client.go was a bare
retry-policy object, missing the methodConfig[].retryPolicy wrapper 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, not 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.

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.go drives the constructed JSON through gRPC's real
service-config parser (manual resolver + ClientConn.ParseServiceConfig) and asserts MethodConfig
is actually populated. Also includes a test proving the old malformed JSON parses with zero
MethodConfig entries, and a test proving "100ms" is rejected.

Non-goal

This does not address DEADLINE_EXCEEDED failures and would not have changed the root cause of
a separate production incident found while investigating this — 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.

Test plan

  • go build ./..., go vet ./pkg/chipingress/..., gofmt -l clean
  • New tests in retry_policy_test.go pass
  • pkg/beholder/... full suite passes

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.
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ API Diff Results - github.com/smartcontractkit/chainlink-common/pkg/chipingress

⚠️ Breaking Changes (1)

./ (1)
  • clientConfig — Old is comparable, new is not

✅ Compatible Changes (3)

./ (3)
  • RetryPolicy — ➕ Added

  • RetryThrottlingPolicy — ➕ Added

  • WithRetryPolicy — ➕ Added


📄 View full apidiff report

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant