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
|
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
|
…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).
pkcll
left a comment
pkcll
left a comment
There was a problem hiding this comment.
Review vs the atlas/chip-ingress write-service reference (internal/server/server.go, internal/service/write_service.go).
What matches the write-service ✓
- One positionally-ordered
PublishResultper event — satisfies the proto contract (chip_ingress.proto: "results[i] corresponds to events[i]") and the batch client's by-index dispatch (chainlink-common/pkg/chipingress/batch/client.go:390); this kills theErrCodeResultsMismatchretransmit storm. - Partial-delivery mode: per-event errors passed through in a nil-error RPC response — same as the write-service's all-events-skipped path (write_service.go:205-212).
transaction_enabled: any unaccepted event fails the RPC — matches the write-service's all-or-nothing pre-publish behavior, and is required because transactional batch clients resolve the whole batch from the RPC outcome alone (batch/client.go:361-362).- No subscribers → RPC error instead of an empty success — preserves the fail-don't-ack shape behind the chainlink-common#2326 audit fix.
- Acceptance-wins-over-rejection merge, and detail-less nil-error responses counting as full acceptance, are sound for a fan-out router and consistent with trusting RPC success at the forwarding boundary.
Deviations from the write-service
-
Publish(single event) is untouched —main.goPublishstill returns&PublishResponse{}, nilunconditionally: no subscribers → silent ack; every forward failed → silent ack (the same data-loss shape this PR condemns forPublishBatch); success → 0 results. The write-service'sPublishis always transactional and returns exactly one result (internal/server/server.go:195-200 hardcodesTransactionEnabled: true; failure → gRPC error; success → 1 result). If the mandate is to match the write-service,Publishneeds the same treatment. -
gRPC code choice — the router returns
Unavailablein all three failure paths; the write-service returnsInternal("failed to publish events", write_service.go:54) for pre-publish/produce failures. No node-side impact today (the durable emitter retransmits on any error), but if parity is the goal,Internalis the reference — or worth a comment explaining the deviation. -
Empty batch edge — the write-service short-circuits an empty batch to success with an empty results array (write_service.go:189-194); the router returns
Unavailablewhen there are no subscribers / all forwards fail. Unreachable via the batch client; cosmetic. -
Merge edge (contract-violating subscribers only) — if the first detail-providing subscriber returns fewer results than events, later subscribers' results cannot extend
detail(detail = resultsis fixed-length after the first assignment): trailing events are acked with no detail, and a later subscriber's rejection beyond the first provider's length is dropped.
Nits: stale "Transactional callers resolve the whole batch from the RPC outcome alone." comment in TestPublishBatchAllForwardsFailedIsUnavailable (belongs to the transactional tests); stubPublishClient.batches/batchCount() are unused.
…ntics (#2873) * fix(chip-router): match atlas write-service Publish/PublishBatch semantics Aligns the router with the atlas chip-ingress write-service, addressing the review findings on #2861: - Publish (single event) now follows the write-service's all-or-nothing contract (atlas chip-ingress internal/server: Publish delegates to PublishCloudEvents with transaction_enabled=true): it fails the RPC when no subscribers are registered, when every forward fails, or when no subscriber accepts the event, and returns exactly one PublishResult on success. Previously it returned an empty PublishResponse with a nil error unconditionally, acking events that went nowhere. - Failure RPCs use codes.Internal, the write-service's failure code, in place of codes.Unavailable. - An empty batch short-circuits to a no-op success before the subscriber check, matching the write-service's empty-batch path. - The per-event detail merge is now order-independent and sized to the batch: a later subscriber's results can extend detail beyond the first provider's length (previously trailing events were acked with no detail), and a detail-less success (empty results, nil error) now accepts the whole batch, clearing other subscribers' rejections — the intent documented in #2861 but not enforced by its merge. Tests: single-event success/failure paths, empty batch, detail-less acceptance, merge extension past the first provider's detail, rejection surviving another subscriber's RPC failure; stub simplified to an immutable struct. * fix(chip-router): bump builder image to golang:1.26.6 to match go.mod go.mod requires go >= 1.26.6 (bumped alongside the pkg/chipingress pin), so the pinned golang:1.25.3 builder fails at 'go mod download' with 'go: go.mod requires go >= 1.26.6 (running go 1.25.3; GOTOOLCHAIN=local)'. * fix(chip-router): implement Ping for write-service surface parity The write-service answers Ping with a static pong; the router inherited UnimplementedChipIngressServer's Unimplemented for it, which the new atlas parity suite's P0 surface probe flags as drift. Ping never needs to reach subscribers, so answer directly like the write-service. * fix(chip-router): make the image build self-contained (no framework dep in the binary) The image build recipe (chainlink cre environment: docker_ctx = framework/components/chiprouter) passes the module dir as the build context, so go.mod's `replace framework => ../../` (a monorepo-dev convenience inherited from main) cannot resolve inside the container and 'go mod download' fails. Published framework releases also do not compile against this module's moby pins, so the replace cannot simply be dropped for the module as a whole. The router binary only used framework for its global logger, so: - cmd/chip-router now carries its own zerolog logger, mirroring framework.L's setup (CTF_LOG_LEVEL env var, console writer to stderr); output format is unchanged. - the Dockerfile drops the replace for the isolated build and runs 'go mod tidy' so the published framework — needed only by the deploy-side library, which is not compiled into the image — resolves from the proxy for module-graph completeness. The deploy-side library (chiprouter.go) keeps its framework dependency and the monorepo replace. * fix(chip-router): reject mismatched batch results and tighten Publish failure path PublishBatch now treats a non-empty subscriber response whose result count differs from the batch size as a contract violation (the write-service returns exactly one result per event in input order): it is logged at Warn and counted as a failed forward instead of silently acking the events it does not cover. Empty responses stay a detail-less success, which chainlink's chip-testsink subscriber relies on. Publish drops the redundant anyForwarded flag and surfaces the subscriber's rejection reason in the Internal error when no subscriber accepted the event. Tests: rewrite the merge test into mismatch cases, make the detail-less acceptance test genuinely detail-less, and cover the single-subscriber empty-response Publish path. Also fix gofmt import order and make the package logger unexported.
…eset chiprouter is its own Go module (tags framework/components/chiprouter/vX.Y.Z) and consumers pin it separately. release-go-module.yml reads .changeset/<version>.md from the module directory and fails when it is missing, so the notes in framework/.changeset/v0.16.10.md could not release the router, while a framework release would have carried no framework change. Move them to chiprouter v1.0.5.
…ssage
A subscriber may reject an event without a PublishError.Reason, which made the
router return a Publish error with an empty gRPC message. Prefix the message
('event rejected by subscribers: <reason>') so it is never empty.
pkcll
changed the title
|
Superseded by #2877 (same branch and commits, recreated and closed here). Earlier review threads stay on this PR. |
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
The router now follows the atlas chip-ingress write-service (
internal/server/server.go,internal/service/write_service.go). This includes the follow-up #2873 that was merged into this branch.PublishBatchPublishResultper event, index-aligned with the batch (EventIdechoed), merged across subscribers. An acceptance always wins over a rejection and a rejection only fills an empty slot, so the outcome does not depend on subscriber completion order. A nil-error response with no results (a detail-less success) accepts the whole batch.transaction_enabled=true, any unaccepted event fails the RPC, because transactional callers resolve the whole batch from the RPC outcome alone.codes.Internalwhen no subscribers are registered or no forward succeeded, so durable callers retain and retry (at-least-once) instead of acking events nothing carried.Publish(single event) is all-or-nothing like the write-service: no subscribers, or no subscriber accepted the event →codes.Internal(the message carries the subscriber's rejection reason); success → exactly onePublishResult.Also
Pingis implemented (staticpong) for write-service surface parity.codes.Internal, the write-service's failure code, instead ofcodes.Unavailable.golang:1.26.6; the binary no longer imports theframeworkmodule (own zerolog logger mirroringframework.L), and the Dockerfile drops the monorepo-localreplacethat cannot resolve in the module-dir build context.chainlink-common/pkg/chipingresspin (Dec 2025 → Sep 2026): the old pin's pb predatesPublishResult.Error.framework/components/chiprouter/.changeset/v1.0.5.md.Tests
go test -race -count=20 ./cmd/chip-router/— 21 tests covering: per-event results with matching ids; empty batch; no-subscribers and all-forwards-failed (Internal); downstream rejection pass-through and survival of another subscriber's RPC failure; acceptance-wins / order independence; detail-less acceptance; length-mismatch handling; transactional partial / all-rejected / all-accepted; single-eventPublish(one result, no subscribers, all forwards failed, all rejected, detail-less success, empty rejection reason);Ping.golangci-lint(repo config): 0 issues.