Description
When a tenant batch contains two identical group (shared subscription) MatchRoutes, DistWorkerCoProc.batchAddRoute throws a NullPointerException and leaves the route cache inconsistent with the KV store.
Observed on main @ eef5e3a.
Root cause
Group routes record "route to its index in the request" in a Map<GlobalTopicFilter, Map<MatchRoute, Integer>>. MatchRoute is a protobuf message and compares by value, so two identical group routes in the same batch collapse into a single map entry, keeping only the later index. The earlier index stays null in the code array:
groupMatchRecords.computeIfAbsent(new GlobalTopicFilter(tenantId, routeDetail.matcher()),
k -> new HashMap<>()).put(route, i); // collapses duplicates, keeps later i
...
for (BatchMatchReply.TenantBatch.Code code : codes) {
batchBuilder.addCode(code); // NPE on the null slot
}The Normal branch is not affected - it explicitly assigns codes[i] = OK for every index in a loop, and it already has explicit duplicate handling plus a test (testAddNormalRouteDuplicatedInOneBatchOnlyCountOnceAndRefresh). The Group branch was missed. The same collapse exists in delGroupMatchRecords in batchRemoveRoute.
Although the route field of TenantBatch is documented as a deduplicated list, BatchMatchCall.makeBatch does not deduplicate client-side, so a caller retrying or re-registering the same shared-subscription receiver in one batch can hit this.
Consequence
The NPE is thrown before MutationResult is returned, but rangeWriter.done() still commits the buffered writes (normal routes + RouteGroup records land in KV), while the co-processor post-actions (routeCache.refresh, deliverExecutorGroup.refreshOrderedSharedSubRoutes, tenant counters, refreshFact) never execute. The batch is reported as ERROR to the client while the route cache stays inconsistent with the KV store until the cache expires - during which publishes are routed using stale data.
A realistic trigger is a shared-subscription receiver re-registering the same group route twice within one batch window (e.g. reconnect storm or rolling restart of multiple replicas of the same subscriber service).
Suggested fix
Record all positions for a route rather than only the last one: the value type of both group maps changes from Integer to List<Integer>, and the consumer assigns the computed code to every recorded index. The same change applies to delGroupMatchRecords.
I have a fix + regression tests (add path and unmatch path) ready in #298.
Description
When a tenant batch contains two identical group (shared subscription)
MatchRoutes,DistWorkerCoProc.batchAddRoutethrows aNullPointerExceptionand leaves the route cache inconsistent with the KV store.Observed on
main@ eef5e3a.Root cause
Group routes record "route to its index in the request" in a
Map<GlobalTopicFilter, Map<MatchRoute, Integer>>.MatchRouteis a protobuf message and compares by value, so two identical group routes in the same batch collapse into a single map entry, keeping only the later index. The earlier index staysnullin the code array:The Normal branch is not affected - it explicitly assigns
codes[i] = OKfor every index in a loop, and it already has explicit duplicate handling plus a test (testAddNormalRouteDuplicatedInOneBatchOnlyCountOnceAndRefresh). The Group branch was missed. The same collapse exists indelGroupMatchRecordsinbatchRemoveRoute.Although the
routefield ofTenantBatchis documented as a deduplicated list,BatchMatchCall.makeBatchdoes not deduplicate client-side, so a caller retrying or re-registering the same shared-subscription receiver in one batch can hit this.Consequence
The NPE is thrown before
MutationResultis returned, butrangeWriter.done()still commits the buffered writes (normal routes + RouteGroup records land in KV), while the co-processor post-actions (routeCache.refresh,deliverExecutorGroup.refreshOrderedSharedSubRoutes, tenant counters,refreshFact) never execute. The batch is reported as ERROR to the client while the route cache stays inconsistent with the KV store until the cache expires - during which publishes are routed using stale data.A realistic trigger is a shared-subscription receiver re-registering the same group route twice within one batch window (e.g. reconnect storm or rolling restart of multiple replicas of the same subscriber service).
Suggested fix
Record all positions for a route rather than only the last one: the value type of both group maps changes from
IntegertoList<Integer>, and the consumer assigns the computed code to every recorded index. The same change applies todelGroupMatchRecords.I have a fix + regression tests (add path and unmatch path) ready in #298.