Repository navigation
Conversation
PublishBatch always returned an empty PublishResponse regardless of the
batch's outcome. The node-side batch client
(chainlink-common/pkg/chipingress/batch) requires one PublishResult per
event to resolve delivery: since chainlink-common#2326 an empty results
array is reported to every event's callback as ErrCodeResultsMismatch,
so a chainlink v2.67 node's durable emitter against this router treated
every event as undelivered and retransmitted the whole stream every 60s,
forever — measured at 301k 'failed to deliver event. Relying on
retransmit.' warnings on a single node within minutes of startup, a ~20x
duplicated Kafka topic, and metric delivery-latency gaps long enough to
break downstream freshness gates.
- Ack each event (PublishResult{EventId}, nil error) when the batch was
handed to at least one subscriber; the events were forwarded in that
case, so the caller can resolve delivery and stop retransmitting.
- Report a per-event PublishError when no subscriber accepted the batch,
so the caller retains and retries (at-least-once preserved).
- Return Unavailable when no subscribers are registered instead of an
empty success — the previous behavior acked events that went nowhere,
letting a durable store delete them undelivered (the data-loss shape
flagged by the audit behind chainlink-common#2326).
Bumps the module's chainlink-common/pkg/chipingress pin (Dec 2025 ->
Sep 2026) — the old pin's pb predates PublishResult.Error.
|
👋 cawthorne, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
📊 API Diff Results
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Downstream per-event rejections and transactional forwarding failures can still be acknowledged as successful, risking data loss.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Updates the chip router’s batch acknowledgements to stop unnecessary durable-emitter retransmissions while reporting forwarding failures.
Changes:
- Returns per-event results and rejects batches when no subscribers exist.
- Adds tests for successful forwarding and failure responses.
- Updates ChipIngress, gRPC, and Go versions.
| File | Description |
|---|---|
| framework/components/chiprouter/go.sum | Updates dependency checksums. |
| framework/components/chiprouter/go.mod | Updates protocol dependencies and Go version. |
| framework/components/chiprouter/cmd/chip-router/main.go | Adds batch delivery results and failure handling. |
| framework/components/chiprouter/cmd/chip-router/main_test.go | Tests batch acknowledgements and forwarding failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…PC when nothing accepts Addresses the review findings: - A nil downstream RPC error does not mean every event was accepted — the protocol supports per-event rejections in a successful response. The router now aggregates the subscribers' per-event results and passes a rejection through only while no subscriber has accepted the event. Subscribers that accept without per-event detail (nil-error response, empty results array) count as accepting the whole batch — the router trusts a subscriber's RPC success at the forwarding boundary, the same as Publish does. - When no subscriber accepts the batch, return Unavailable instead of a nil-error response with per-event errors: transactional callers resolve the whole batch from the RPC outcome alone, so per-event errors would still acknowledge everything. An RPC error makes every caller mode retain and retry.
📊 API Diff Results
|
cawthorne
marked this pull request as draft
October 7, 2026 10:29
Tofel
reviewed
Oct 7, 2026
| framework.L.Debug().Msgf("chip router forwarded batch to subscriber id=%s", sub.id) | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| anyAccepted = true |
Contributor
There was a problem hiding this comment.
can we be sure that it was accepted at this point? we don't need to check results first?
…on fails the RPC A subscriber's nil RPC error does not mean its events were accepted: the pinned protocol reports per-event rejections in a successful response. Renamed the flag to anyForwarded (what an RPC success actually tells the router) and gated the response on the batch's own PublishOptions: - transaction_enabled: any unaccepted event fails the RPC (Unavailable) — transactional callers resolve the whole batch from the RPC outcome alone, so per-event errors would still acknowledge everything, including events no subscriber produced. - otherwise: per-event results as before (rejections passed through, acceptance wins over rejection across subscribers).
This branch has not been deployed
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.
A previous attempt to fix think in chainlink-common by reverting smartcontractkit/chainlink-common@dc8a1dc was here:
smartcontractkit/chainlink-common#2446 (comment)
But that PR has been closed for now, since it was intentional and implemented due to an audit finding:
https://ainv.smartcontract.com/findings/79362753-0fef-4a50-934c-572e8f7c504d
Summary
PublishBatchalways returned an emptyPublishResponseregardless of the batch's outcome. The node-side batch client (chainlink-common/pkg/chipingress/batch) requires onePublishResultper event to resolve delivery: since chainlink-common#2326 (an audit remediation for a data-loss finding) an empty results array is reported to every event's callback asErrCodeResultsMismatch.Against this router, a chainlink v2.67.2-rc.0 node's durable emitter therefore treated every event as undelivered and retransmitted the whole stream every 60s, forever:
failed to deliver event. Relying on retransmit.warnings on a single node, first at 12s after boot, zero successful acks;time() - <metric>_ts < 120gates).The events were forwarded to Kafka on every attempt — only the acks were missing — so the storm duplicated the stream rather than losing data.
The fix
PublishResult{EventId}, nil error) when the batch was handed to at least one subscriber — the events were forwarded in that case, so the caller can resolve delivery and stop retransmitting.PublishErrorwhen no subscriber accepted the batch, so the caller retains and retries (at-least-once preserved).Unavailablewhen no subscribers are registered instead of an empty success — the previous behavior acked events that went nowhere, letting a durable store delete them undelivered (the exact data-loss shape flagged by the audit behind chainlink-common#2326).Also bumps the module's
chainlink-common/pkg/chipingresspin (Dec 2025 → Sep 2026): the old pin's pb predatesPublishResult.Error.Tests
New
cmd/chip-routertests: per-event results with matching ids on a successful forward;Unavailablewith no subscribers; per-event errors when every forward fails.Unrelated check failure: TestLocalS3
TestLocalS3fails on this branch withcreate container: ... 401 UNAUTHORIZEDfrom quay.io. Reproduced locally:quay.io/minio/minionow 401s anonymous pulls on every tag (s3provider.DefaultImage=quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z), andminio/minioon Docker Hub is denied as well, while other quay images (e.g. prometheus) pull fine anonymously. I.e. MinIO's registry purge broke this test repo-wide — it will fail onmaintoo. Fixing it (a new image source fors3provider) looks like a supply-chain decision for the maintainers, so it is left out of this PR.