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.
Comment on lines
+186
to
+187
| } else { | ||
| forwarded.Store(true) |
| } | ||
| results = append(results, result) | ||
| } | ||
| return &chippb.PublishResponse{Results: results}, nil |
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.
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.