Repository navigation
Conversation
…ntics 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.
📊 API Diff Results
|
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)'.
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.
…ep 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.
| L.Debug().Msgf("chip router forwarded event to subscriber id=%s", sub.id) | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| anyForwarded = true |
There was a problem hiding this comment.
is it always true if err == nil? even if len(results) == 0?
There was a problem hiding this comment.
Yes, by design. anyForwarded only meant "the subscriber's RPC succeeded" — it was never the acceptance flag. A nil error with no results is a detail-less success and also sets accepted. That is the shape chainlink's chip-testsink subscriber returns on success (&PublishResponse{}, nil), while the write-service returns exactly one result. Requiring a result would fail every Publish through the testsink.
You were right that it was confusing, though: anyForwarded only chose between two codes.Internal messages, and rejection was never read after Wait(). Fixed in 0748e96: dropped anyForwarded, and when nothing accepted the event the error now carries the subscriber's rejection reason (otherwise "all N subscribers failed to accept the event"). Added a test for the empty-results case (TestPublishDetaillessSuccessAcceptsEvent) and made the detail-less acceptance test actually use an empty response.
| return nil | ||
| } | ||
| for i, res := range results { | ||
| if i >= len(detail) { |
There was a problem hiding this comment.
can that really happen that we have details only for some results?
There was a problem hiding this comment.
Not with any subscriber we have: the write-service returns exactly one result per event in input order, and the testsink returns none. A partial list is a contract violation, and the i >= len(detail) check only guarded an index panic on an over-long list. Worse, a short list silently acked the uncovered events, which is more lenient than the node-side client (it treats a length mismatch as ErrCodeResultsMismatch).
Fixed in 0748e96: a non-empty response whose length differs from the batch is now logged at Warn and counted as a failed forward (not merged, doesn't set anyForwarded). That removes this guard and the partial-merge case. The old merge test became two mismatch tests: a short list from the only subscriber fails the RPC with Internal, and a short list next to another subscriber's full list is ignored in favor of the full one.
… 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.
This branch has not been deployed
2 participants
Stacked on #2861 — implements the review findings so the chip-router matches the atlas chip-ingress write-service's behavior. Only the delta on top of #2861's head (
c69acae) is in this PR's diff.Changes
Reference: atlas
chip-ingressinternal/server/server.go/internal/service/write_service.go.Write-service parity (
cmd/chip-router/main.go)Publish(single event) now matches the write-service's all-or-nothing contract. The write-service's single-event RPC delegates toPublishCloudEventswithtransaction_enabled=true: any failure returns a gRPC error, success returns exactly onePublishResult. The router previously returned&PublishResponse{}, nilunconditionally — acking events that went nowhere (the data-loss shape fix(chip-router): populate per-event PublishBatch results #2861 fixes forPublishBatch) and returning zero results on success. It now fails when no subscribers are registered, when every forward fails, or when no subscriber accepts the event, and returns one result on success. When subscribers rejected the event, the error carries the rejection's reason.codes.Internal— the write-service's failure code (InternalError = status.Errorf(codes.Internal, "failed to publish events")) — in place ofcodes.Unavailableacross all failure paths. No node-side behavior change (the durable emitter retransmits on any error), but the router now speaks the same failure dialect as the write-service.Unavailablewith no subscribers).detailis a fixed-length slice indexed by event position: acceptance always wins over rejection, and a rejection only fills an empty slot. A detail-less success (empty results, nil error) accepts the whole batch, clearing other subscribers' rejections — the intent documented in fix(chip-router): populate per-event PublishBatch results #2861's comments but not enforced by its merge (a detail-less success + another subscriber's per-event rejection still reported the event rejected, even though it was delivered). The detail-less shape is what chainlink'schip-testsinksubscriber returns on success, so it stays accepted.PublishBatchresponse whose length differs from the batch is a failed forward. The write-service returns exactly one result per event in input order, and the node-side batch client treats a length mismatch asErrCodeResultsMismatch. The router now logs such a response at Warn and ignores it instead of silently acking the events it does not cover.Pinganswers with a staticpong, like the write-service, instead of inheritingUnimplemented(flagged as P0 surface drift by the atlas parity suite).Image build (
Dockerfile,cmd/chip-router)golang:1.26.6to matchgo.mod.replace framework => ../../cannot resolve inside the container. The router binary only used framework for its global logger, socmd/chip-routernow carries its own zerolog logger mirroringframework.L's setup (CTF_LOG_LEVEL, console writer to stderr; output unchanged), and the Dockerfile drops thereplacefor the isolated build. The deploy-side library (chiprouter.go) keeps its framework dependency and the monoreporeplace.Tests
go test -race -count=20 ./cmd/chip-router/— coverage: single-event success (1 result) / no-subscribers / all-forwards-failed / all-rejected (error carries the rejection reason) / detail-less acceptance overriding a rejection / single-subscriber empty-response success; empty-batch no-op; result-count mismatch (only subscriber →Internal; mismatched list ignored in favor of another subscriber's full detail); a rejection surviving another subscriber's RPC failure;codes.Internalassertions;Ping. Stub simplified to an immutable struct (no mutex/counter).golangci-lint(repo config): 0 issues.