Skip to content

Fix NPE and lost trace context of reactive Lettuce commands when tracing is enabled - #841

Merged
wu-sheng merged 3 commits into
apache:mainfrom
carteruu:fix-lettuce-decorated-command-npe
Oct 31, 2026
Merged

wu-sheng merged 3 commits into
apache:mainfrom
carteruu:fix-lettuce-decorated-command-npe

Conversation

@carteruu

Copy link
Copy Markdown
Contributor

Fix the NullPointerException and the lost trace context of reactive Lettuce commands when Lettuce tracing is enabled (apache/skywalking#14178)

  • Add a unit test to verify that the fix works.
  • Explain briefly why the bug exists and how to fix it.

When Lettuce tracing is enabled, which Spring Boot does by default when there is an ObservationRegistry bean, Lettuce wraps each reactive command in a TracedCommand. RedisSubscriptionConstructorInterceptor stores that wrapper, and RedisSubscriptionSubscribeMethodInterceptor puts the snapshot on it, but RedisChannelWriterInterceptor unwraps DecoratedCommand and creates the span on the innermost command. So the snapshot and the span end up on different objects:

  • The wrapper holds a RedisCommandEnhanceInfo without a span. When it completes or is cancelled, the command interceptors dereference the null span, which is the NPE from the issue.
  • The innermost command has no snapshot, so RedisReactive/local and ContextManager.continued are skipped, and the exit span is not linked to the trace that issued the command.

A second case doesn't need tracing. A reactive command cancelled after subscribing but before any demand has a snapshot and no span, and RedisPublisher.State#cancel calls command.cancel() directly.

This PR fixes both:

  • RedisSubscriptionConstructorInterceptor reuses getSpanCarrierCommand, so the snapshot goes on the command that carries the span. Commands without a decorator are unchanged.
  • RedisCommandEnhanceInfo#getSpan(EnhancedInstance) returns null when a command has no span, and the complete, completeExceptionally and cancel interceptors skip such a command.

RedisReactiveCommandSpanTest uses the real TracedCommand. Before this change snapshotOfDecoratedCommandIsContinuedOnSpanCarrierCommand fails and commandCancelledBeforeWriteHasNoSpanToFinish fails with the NPE. commandWithoutDecoratorStillContinuesSnapshot passes before and after, and guards the existing behavior.

I also ran the agent against real Lettuce on JDK 17, with 3 SET commands plus 1 cancelled before demand, against a minimal fake Redis server:

Agent Lettuce Tracing NPE entries Segment of SET
9.8.0 7.5.2, 6.6.0 on 12 [Lettuce/SET], no reference
9.8.0 7.5.2, 6.6.0 off 1 [Lettuce/SET, RedisReactive/local], with reference
This PR 7.5.2, 6.6.0 on 0 [Lettuce/SET, RedisReactive/local], with reference
This PR 7.5.2, 6.6.0 off 0 [Lettuce/SET, RedisReactive/local], with reference

./mvnw verify passes for lettuce-common and both Lettuce plugins. I did not run the lettuce-* plugin test scenarios locally, and they don't enable Lettuce tracing.

…ing is enabled

Put the snapshot on the command that carries the span instead of the TracedCommand wrapper, and skip the command interceptors for a command without a span.
@carteruu
carteruu force-pushed the fix-lettuce-decorated-command-npe branch from 2b2c90d to 84cb7cd Compare October 30, 2026 15:52
@wu-sheng wu-sheng added bug Something isn't working plugin labels Oct 30, 2026
@wu-sheng wu-sheng added this to the 9.9.0 milestone Oct 30, 2026
ubuntu-latest now resolves to ubuntu-26.04, which ships Docker 29 with the
containerd image store. The pinned infra-e2e action (8c21e43) disables the
containerd snapshotter and restarts dockerd before running the case, so the
agent image built and loaded by `make build docker.java8`
(skywalking/skywalking-java:latest-java8) is no longer visible. Compose then
tries to pull it from Docker Hub and both E2E cases fail with
"pull access denied".

Upstream infra-e2e removed that step in apache/skywalking-infra-e2e#146 and
runs compose through testcontainers-go. Pin the current main
(56ac8befeb5f910f2dc9339e65d07cd18953f90e), whose CI passes on ubuntu-26.04.

The new action installs Go 1.26 itself. Drop the workflow's own
actions/setup-go@v2 (Go 1.24) step: it exports GOROOT, which makes the
action's Go 1.26 toolchain compile against the Go 1.24 standard library
("compile: version go1.24.13 does not match go tool version go1.26.10").
The scenario consumer has no committed offset and starts from the latest
offset, while healthCheck reported success as soon as Tomcat started. When the
health check hit right after startup, the entry request sent "helloWorld"
before the consumer got its partition, so the consumer skipped it, the
request waited forever on the latch and its segment was never reported
(SegmentNotFoundException for GET:/case/spring-kafka-case). On ubuntu-26.04
runners spring-kafka 2.7.14 lost this race on every attempt.

Report healthy only after partitions are assigned, the same way
kafka-scenario does.
@wu-sheng
wu-sheng merged commit 698b224 into apache:main Oct 31, 2026
275 of 289 checks passed
wu-sheng added a commit to apache/skywalking-helm that referenced this pull request Oct 31, 2026
ubuntu-latest now resolves to ubuntu-26.04, which ships Docker 29. The pinned
infra-e2e action (8c21e43) disables the containerd snapshotter and restarts
dockerd before running a case, a workaround upstream removed in
apache/skywalking-infra-e2e#146. It broke the skywalking-java E2E on the new
runners, and the chart E2E has not run on them yet.

Pin the current main (56ac8befeb5f910f2dc9339e65d07cd18953f90e), as
apache/skywalking-java#841 did. The action installs Go 1.26 itself, so drop
the workflow's own Go 1.24 setup.
wu-sheng added a commit to apache/skywalking-helm that referenced this pull request Oct 31, 2026
* Bump skywalking-infra-e2e off the containerd snapshotter workaround

ubuntu-latest now resolves to ubuntu-26.04, which ships Docker 29. The pinned
infra-e2e action (8c21e43) disables the containerd snapshotter and restarts
dockerd before running a case, a workaround upstream removed in
apache/skywalking-infra-e2e#146. It broke the skywalking-java E2E on the new
runners, and the chart E2E has not run on them yet.

Pin the current main (56ac8befeb5f910f2dc9339e65d07cd18953f90e), as
apache/skywalking-java#841 did. The action installs Go 1.26 itself, so drop
the workflow's own Go 1.24 setup.

* Pin docker/login-action to an approved revision

The ASF GitHub Actions allow-list no longer has docker/login-action
af1e73f (v4.4.0), so the Test workflow fails at startup with "is not
allowed in apache/skywalking-helm". Use v4.7.0
(81c6f93d4bc991e510b342a00b419b3bb4efc5c6), which the allow-list
approves without an expiry date. publish-helm.yaml uses the same pin.

* Stop setting SW_QUERY_ZIPKIN twice in the E2E installs

The ubuntu-26.04 runner image ships Helm 4.3.1, whose server-side apply
rejects duplicate env entries. The three OAP 11 cases set
oap.ports.zipkin-query, which already makes the chart emit SW_QUERY_ZIPKIN,
and set oap.env.SW_QUERY_ZIPKIN as well, so `helm install` failed with
"duplicate entries for key [name="SW_QUERY_ZIPKIN"]". Helm 3's client-side
apply accepted the duplicate.

Drop the redundant oap.env setting; the chart still sets the same value.

* Raise the inotify limits for the kind E2E cluster

On the ubuntu-26.04 runners BanyanDB standalone exits at startup with
"failed to create fsnotify watcher: couldn't initialize inotify: too many
open files". The four kind nodes and all their pods share the host's
inotify limits, which Ubuntu sets low, so OAP never became available and
the case timed out. Raise fs.inotify.max_user_watches and
max_user_instances to at least the values the kind known-issues page
recommends, and print the runner's own values first.
wu-sheng added a commit that referenced this pull request Oct 31, 2026
Both scenarios have the race fixed for spring-kafka-2.3.x-scenario in #841.
The consumer has no committed offset and starts from the latest offset, while
healthCheck reported success as soon as Tomcat started. When the entry request
sent "helloWorld" before the consumer got its partition, the consumer skipped
it, the request waited forever on the latch and its segment was never
reported (SegmentNotFoundException for GET:/case/spring-kafka-case).

Report healthy only after partitions are assigned. With auto commit disabled,
the container resolves and commits the position of every assigned partition
before it calls this listener, so a message sent after the health check
passes is always consumed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants