Repository navigation
Fix agent crashes and lost trace segments found in the plugin audit - #262
Merged
Merged
Conversation
Fix the highest-severity agent bugs: an application crash, a request that the agent breaks, or a span that never ends, so that the whole segment is never reported. - runtime: take the goroutine context snapshot on the creating goroutine's stack before the system stack switch, and not while it holds runtime locks; a contended span lock in newproc1 was a fatal "semacquire not on the G stack" - logrus, zap, toolkit logging: no panic when logging before the agent core is initialized, for example from a package init function - core: end the exit span when its injector fails - grpc, grpc-v184: finish every client stream exit span exactly once from a grpc.OnFinish call option added in newClientStreamWithParams - http, mux, goframe: keep the Flusher, Hijacker, and Unwrap interfaces of the response writer - echov4: serve a request with an undecodable sw8 header untraced instead of answering 500; end the span on a panic or Goexit without recovering it - mongo: no crash with a partial user CommandMonitor; finish the span when a user callback panics - sql: end the span of Conn.Raw - gorm: keep the span on the running statement, so a scope that returns a session does not lose it; end the span when a callback or hook panics - pulsar: no crash on SendAsync with a nil callback, no panic or span on a failed Receive, no broker lookup RPC per message - rocketmq: end the span of a failed SendSync; no panic on an empty send - kratos: no panic on a nil request header (kratos v2.7.2 and later) - go-micro: a tracing failure no longer fails the call Every fix has a regression test that fails on the old code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gorm, gorm-postgres: the span of the operation whose hook panics carries the error log of the Execute hook. grpc: list the server segment of the wrapped stream after the other server segments, in the order they are reported. The validator first matches segments with a loose reference check (endpoint, service, address, type), so listed first it matched the structurally identical server segment of the earlier server-streaming call, and the strict reference check then failed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mrproliu
approved these changes
Oct 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the highest-severity agent bugs found in a full audit of the plugins and the runtime instrumentation. Every bug here either crashes the application, breaks a request that works without the agent, or leaves a span open. An open span means the whole trace segment is never reported, usually the segment of the incoming request, and a collector goroutine leaks.
All of these bugs predate the Go 1.26 move (#260). Each one was reproduced on unfixed
mainfirst, then shown fixed by the same check. Every fix has a regression test that fails on the old code, and most also add a case to the plugin scenario.The fixes
Agent core
fatal error: semacquire not on the G stack, which can't be recovered. It happens when a goroutine starts while another goroutine holds the lock of the starting goroutine's active span, for example a worker tagging an async span throughSpanRef.runtime.newproc1, which runs on the system stack. Waiting for a contendedsync.Mutexthere is a fatal throw.runtime.newprocandruntime.newcoro, right before they switch to the system stack, and stored in a newgfield.newproc1only moves the reference. The snapshot is skipped while the goroutine holds runtime locks, because the runtime starts its GC workers betweenacquiremandreleasem, where blocking is fatal too.runtime.GC()under the same contention, so GC workers start on the traced goroutine. Unfixedmain: 15/15semacquire not on the G stack. Without the runtime-lock guard: 15/15schedule: holding locks. Fixed: 15/15 pass. After the fix, both apps pass on Go 1.26.9 and 1.27.1. Context still reaches new goroutines anditer.Pullcoroutines. A unit test checks the rewritten runtime code.initpanics at startupinitthat runs before the agent core: nil pointer panic, and the process exits.GetLogContextcalledGetOperator().LogReporter(), but the operator is nil until agent core init.skywalking_logrus_context.go:82(logrus v1.10.2) andskywalking_zap_context.go:82(zap v1.28.0). After: both apps exit 0. Unit tests intools/go-agent/instrument/loggerandplugins/toolkit-activation/logging.CreateExitSpanregistered the span, then returnednil, errwithout ending it.TestCreateExitSpanInjectorError: 0 spans reported before, 2 after.grpc (
plugins/grpcandplugins/grpc-v184)NewStreamfails, when a client stream interceptor wraps the stream (logging, metrics, retry, auth), or when the stream ends without aRecvMsg(cancel, connection closed,SendMsgerror).AfterInvokeread the error fromresult[0].AsyncFinishhappened only for a bare*clientStream, and only after aRecvMsghad armed theclientStream.finishhook.newClientStreamWithParams(same signature in v1.83.2 and v1.84.0) adds agrpc.OnFinishcall option to the*clientStreamthat grpc creates, whatever wraps it. grpc calls it once, with the final status, after the stream finishes; thefinishhook is gone. The agent's callback runs before the application's and can't panic into grpc. Each creation gets a generation, so the callback of a failed creation is ignored (grpc before v1.84 calls it). A mutex-guarded state machine ends the span exactly once, including when the stream finishes beforeNewStreamreturns.NewStreamends the span synchronously on an error, when no stream of the call was created, or when a stream interceptor panics after creating it. The stream snapshots are published under the lock, because an interceptor can use the stream on another goroutine beforeNewStreamreturns. The span records the final status, so a failed or canceled stream is an error span.reported spans = 0for the failed, wrapped, canceled, send-error and early-finish cases. They are joined by tests for a cancel before the stream is attached, an interceptor that panics after creating the stream, a creation retried after a failure, and a panicking applicationOnFinish. Each fails on the code before its fix, and all pass with-race. Instrumented compile and run at v1.83.2 and v1.84.0 through a wrapping stream interceptor, with 0 interceptor errors.HTTP servers
w.(http.Flusher)is false).http.NewResponseControllerreturnsErrNotSupported. GoFrame WebSocket upgrades answer 500, because GoFrame's unchecked.(http.Hijacker)panics.FlushnorUnwrap, and the goframe wrapper didn't implementHijackeither.wrapResponseWriterpicks a wrapper type that implementshttp.Flusherandhttp.Hijackerexactly when the wrapped writer does, so type assertions take the same branch as without the agent.UnwrapandFlushErrorservehttp.ResponseController. Status capture is unchanged.flusher=false,deadline=feature not supported, SSE 500, GoFrame upgrade500 ... is not http.Hijacker. After: all pass, including a real GoFrame v2.10.3 WebSocket handshake. Compiled and run at the min and max version of each framework.sw8sw8header (from another tracer or a broken proxy) gets HTTP 500, and anyone can trigger it remotely.next(c).TestMalformedSW8HeaderIsServed: 500 before, 200 after. The scenario sendssw8: x.span.End()wasn't deferred.runtime.Goexit. It doesn't callrecover, which would also stop apanic(nil)underGODEBUG=panicnil=1, so the panic propagates unchanged.TestPanicEndsSpan,TestPanicRecoveredByPreMiddleware,TestNilPanicIsNotSwallowed,TestGoexitEndsSpan: spans lost (and the nil panic swallowed by an intermediate fix) before, reported after.Databases
CommandMonitorSetMonitor(&event.CommandMonitor{Succeeded: f})crashes the process on the first command. A userSucceededorFailedcallback that panics leaves the command span open.deferregistered before calling the user's callback.TestPartialUserMonitorpanics before;TestPanickingUserMonitorFinishesSpanreports 0 spans before. Both pass after. The scenario app panicked inpublishStartedEventbefore the fix.Conn.Rawresults[1], butRawreturns only an error, so it panicked beforeEnd.results[0].TestConnRawEndsSpan: index out of range before, passes after. The mysql scenario's/executesegment was never reported before the fix.tx.WithContext(ctx), every gorm operation's segment is lost. When a gorm hook such asAfterFindpanics and the caller recovers, the segment is lost too.InstanceSetclones the statement when the*gorm.DBcomes from a session and stores the span in the clone, where the end callback never finds it. gorm runs callbacks withoutdefer, so a panic skips the end callback.Settings, under the same statement-address key thatInstanceSetuses. A new hook on(*processor).Execute, whose agent after-hook is deferred, ends a span the end callback left open and marks it as an error, without recovering the panic.TestScopeSessionSpanIsEndedandTestScopeSessionHookPanicEndsSpan: 0 spans with the old storage, pass after.TestHookPanicEndsSpan: 0 spans before, passes after, including the nested sql exit span.Message queues
SendAsync(ctx, msg, nil)connection.go:744before the fix.Receiveresult[0].(pulsar.Message)on a nil message.Receivereceived nothing and creates no span. An entry span would take over the caller's own entry span, such as the one of an HTTP request.TestReceiveFailedNoSpanunder an HTTP entry span: the entry span is renamed before, unchanged after.lookupService.Lookup.NewClienthook stores the client's service URL. The peer andmq.brokerare that URL, as in the Java agent; in a multi-broker cluster this changes them from the owning broker's address.Test*NoLookupRPC: lookups counted before, 0 after. The scenario's expected peer is unchanged.SendSyncsendRet.MessageQueue.QueueIdwas read beforeEnd, but a failed send has a nilMessageQueue.defer span.End(), and guard the nilMessageQueue.TestSendSyncFailedEndsSpan. The scenario's/executesegment was never reported before the fix.msgList[0]was read before the SDK's empty check.TestEmptySendNoPanic.RPC
khttp.Header(&h)and a nilhpanics (assignment to entry in nil map) in middleware that has no recover.has the request header, so the plugin'sRequestHeader().Addwrote into a nil map.Callwith an empty service andclient.WithAddressreturnspeer is emptyand is never sent. The same happens for Stream and Publish.CreateExitSpanerror to the application.TestUntraceableCallsAreDelegated,TestNextKeepsCallerPeerForUntracedCall. An instrumented app at v4.9.0 and v4.11.2 now really attempts the call.Behavior changes to note
mq.brokerare the client's service URL instead of the address of the broker that owns the topic.http.Hijacker, such as HTTP/2, the http plugin's wrapper no longer has aHijackmethod that returns an error. Applications now see the same as without the agent.testing/synctestbubbles, and for GC workers no longer inherit the trace context.Verification
go test -race,go vet, and golangci-lint v2.15.0 report 0 issues in all 16 touched modules.-toolexec,GOWORK=off) at the lowest and highest supported version of every touched framework, and on both sides of the grpc v1.84.0 split. All were also run, with 0 interceptor errors.🤖 Generated with Claude Code