From e56b048aa85965e7c6a986f3c808bd6b8a9181e0 Mon Sep 17 00:00:00 2001 From: "amos.chen" Date: Wed, 16 Sep 2026 16:08:16 +0800 Subject: [PATCH] Fix jedis-4.x-plugin double-stopping the span stack on Redis exceptions AbstractConnectionInterceptor.handleMethodException() called ContextManager.stopSpan(span) explicitly, but afterMethod() always runs afterwards too (InstMethodsInter invokes it in a finally block on every path, including exceptions) and stops the same span again. The second stopSpan() pops whatever is now on top of the stack instead - typically the caller's entry/local span - corrupting the trace for the rest of the request. beforeMethod() also dereferenced the per-Connection dynamic field without a null check. When it is null (observed for some pooled or recycled connections not captured by the constructor interceptor), this throws before createExitSpan() runs, so afterMethod()'s stopSpan() again pops a span that was never pushed by this interceptor. Fix: only log the error in handleMethodException() (matching jedis-2.x-3.x-plugin's safe behavior) and fall back to an "unknown" peer when the dynamic field is null so the exit span is always pushed and the stack stays balanced. Resolves apache/skywalking#14085 --- CHANGES.md | 3 +++ .../v4/AbstractConnectionInterceptor.java | 20 +++++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index c6e9e5862e..2dd610a5cd 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -5,6 +5,9 @@ Release Notes. 9.8.0 ------------------ +* Fix `jedis-4.x-plugin`'s `AbstractConnectionInterceptor` double-stopping the span stack on any + Redis-level exception (or a null dynamic field on a pooled/recycled `Connection`), which corrupted + the parent trace for the rest of the request (apache/skywalking#14085). * Add Spring LDAP 3.3.x-4.x plugin. * Exclude macOS metadata files from source and binary release archives (apache/skywalking#14080). * Fix `NoSuchMethodError: org.apache.skywalking.apm.plugin.spring.webflux.v6.DispatcherHandlerHandleMethodInterceptor` diff --git a/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/AbstractConnectionInterceptor.java b/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/AbstractConnectionInterceptor.java index a1a8a16225..b4b399d83c 100644 --- a/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/AbstractConnectionInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/AbstractConnectionInterceptor.java @@ -53,14 +53,22 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr // Refer to `plugin.jedis.operation_mapping_read`, `plugin.jedis.operation_mapping_write` config item in agent.config String cmd = protocolCommand == null ? UNKNOWN : protocolCommand.toLowerCase(); ConnectionInformation connectionData = (ConnectionInformation) objInst.getSkyWalkingDynamicField(); + // connectionData can be null when this Connection instance wasn't captured by the constructor + // interceptor (e.g. a pooled/recycled connection created through a code path the constructor + // interceptor doesn't cover). Fall back to UNKNOWN instead of throwing here: an exception in + // this method, before createExitSpan() runs, would leave no exit span pushed for this call, + // so afterMethod()'s unconditional stopSpan() would incorrectly pop and close whatever span + // is already on the stack (typically the caller's entry/local span). + String actualTarget = connectionData == null ? UNKNOWN : connectionData.getActualTarget(); + String clusterNodes = connectionData == null ? null : connectionData.getClusterNodes(); // Use cluster information to adapt Virtual Cache if exists, otherwise use real server host - String peer = StringUtil.isBlank(connectionData.getClusterNodes()) ? connectionData.getActualTarget() : connectionData.getClusterNodes(); + String peer = StringUtil.isBlank(clusterNodes) ? actualTarget : clusterNodes; AbstractSpan span = ContextManager.createExitSpan("Jedis/" + cmd, peer); span.setComponent(ComponentsDefine.JEDIS); readKeyIfNecessary(iterator).ifPresent(key -> Tags.CACHE_KEY.set(span, key)); Tags.CACHE_CMD.set(span, cmd); Tags.CACHE_TYPE.set(span, CACHE_TYPE); - TAG_ARGS.set(span, connectionData.getActualTarget()); + TAG_ARGS.set(span, actualTarget); parseOperation(cmd).ifPresent(op -> Tags.CACHE_OP.set(span, op)); SpanLayer.asCache(span); } @@ -84,8 +92,12 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Throwable t) { - AbstractSpan span = ContextManager.activeSpan().log(t).errorOccurred(); - ContextManager.stopSpan(span); + // Do not call ContextManager.stopSpan() here: afterMethod() below always runs afterwards + // (InstMethodsInter invokes it in a finally block, on every path including exceptions) and + // already pops this span. Stopping it a second time here pops one extra span - typically the + // caller's entry/local span - corrupting the trace for the rest of the request. Only log the + // exception on the still-active span, matching the jedis-2.x-3.x-plugin's safe behavior. + ContextManager.activeSpan().log(t).errorOccurred(); } private Optional parseOperation(String cmd) {