This is an automated email from the ASF dual-hosted git repository.
sruehl pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
The following commit(s) were added to refs/heads/develop by this push:
new 220b7760cb feat(plc4go): surface corrupt error chains instead of
silently swallowing them
220b7760cb is described below
commit 220b7760cb8723beeefb89ae80900ab1d96af955
Author: Sebastian Rühl <[email protected]>
AuthorDate: Thu Jul 2 13:03:02 2026 +0200
feat(plc4go): surface corrupt error chains instead of silently swallowing
them
The panic guards added for improperly constructed error chains (typed-nil
*net.OpError wrapped via %w) recover silently, which converts a loud
worker-killing signal into an invisible one - future occurrences of the
underlying anomaly would go unnoticed. Restore the signal without
introducing any logging at the utility level:
- errors.SanitizeError: returns the error unchanged when its chain walks
safely (including multi-child Join nodes, Error() panics, and cyclic
chains). A corrupt chain is flattened to its message and joined with the
new errors.ErrCorruptErrorChain sentinel - safe to walk, keeps the
original text, and is detectable via errors.Is wherever the error is
eventually logged or propagated. Joining the ORIGINAL error would re-arm
the poisoned chain one level deeper (Join's Unwrap hands it back to every
walk), hence the flatten.
- defaultCodec.handleTransportError sanitizes once at the entry point, so
classification, TransportError wrapping, expectation fan-out, and client
propagation all operate on a safe chain that still carries the anomaly.
- errors.MarshalStack now reports a recovered panic as the marshaled
payload (under the "stack" key of the log event that was already logging
the error) instead of returning nil - scoped to the exact log line, no
global logger involved.
Tests cover: clean-chain identity, typed-nil in chain and at top level,
panicking Error(), cyclic chains, Join with poisoned child, sentinel
survival through TransportError wrapping to the transport-error handler,
and the MarshalStack panic marker.
---
plc4go/spi/default/DefaultCodec.go | 12 +-
.../DefaultCodecTransportErrorPanic_test.go | 34 ++++++
plc4go/spi/errors/marshal.go | 13 ++-
plc4go/spi/errors/sanitize.go | 104 ++++++++++++++++++
plc4go/spi/errors/sanitize_test.go | 122 +++++++++++++++++++++
5 files changed, 280 insertions(+), 5 deletions(-)
diff --git a/plc4go/spi/default/DefaultCodec.go
b/plc4go/spi/default/DefaultCodec.go
index b328265627..e60f92a97d 100644
--- a/plc4go/spi/default/DefaultCodec.go
+++ b/plc4go/spi/default/DefaultCodec.go
@@ -543,10 +543,14 @@ func (m *defaultCodec) handleTransportError(workerLog
zerolog.Logger, err error)
if err == nil {
return true
}
- // Use the panic-guarded transports.ErrorIs here: the error chain
delivered by a
- // transport can contain improperly constructed values (e.g. a typed-nil
- // *net.OpError), and stdErrors.Is dereferences them while unwrapping -
which
- // killed the receive worker with a recovered nil-pointer panic in the
field.
+ // Defuse improperly constructed error chains (e.g. a typed-nil
*net.OpError
+ // wrapped via %w) once at the entry point: everything below -
classification,
+ // wrapping, expectation fan-out, logging - walks the chain repeatedly,
and
+ // unguarded walks dereference such values, which killed the receive
worker
+ // with a recovered nil-pointer panic in the field. A corrupt chain is
+ // flattened and tagged with errors.ErrCorruptErrorChain so the anomaly
+ // stays visible downstream instead of being silently swallowed.
+ err = errors.SanitizeError(err)
if transports.ErrorIs(err, context.Canceled) {
workerLog.Debug().Msg("receive aborted due to context
cancellation")
return false
diff --git a/plc4go/spi/default/DefaultCodecTransportErrorPanic_test.go
b/plc4go/spi/default/DefaultCodecTransportErrorPanic_test.go
index 161a764b07..10d9331e60 100644
--- a/plc4go/spi/default/DefaultCodecTransportErrorPanic_test.go
+++ b/plc4go/spi/default/DefaultCodecTransportErrorPanic_test.go
@@ -20,13 +20,18 @@
package _default
import (
+ stderrors "errors"
"fmt"
"net"
"testing"
+ "time"
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ spiErrors "github.com/apache/plc4x/plc4go/spi/errors"
"github.com/apache/plc4x/plc4go/spi/testutils"
+ "github.com/apache/plc4x/plc4go/spi/transports"
)
// A transport error chain can contain improperly constructed values - most
@@ -45,3 +50,32 @@ func
Test_defaultCodec_handleTransportError_typedNilOpErrorInChain(t *testing.T)
assert.False(t, keepRunning, "poisoned chain must be treated as
fatal and stop the worker")
})
}
+
+// The sanitized error handed to the transport-error handler (and thus to the
+// owning connection / client code) must be safe to walk, carry the
+// ErrCorruptErrorChain sentinel, and preserve the original message text.
+func Test_defaultCodec_handleTransportError_sanitizedErrorReachesHandler(t
*testing.T) {
+ var nilOpErr *net.OpError
+ poisoned := fmt.Errorf("read failed: %w", error(nilOpErr))
+
+ received := make(chan error, 1)
+ codec := &defaultCodec{
+ transportErrorHandler: func(kind transports.TransportErrorKind,
err error) {
+ assert.Equal(t, transports.TransportErrorFatal, kind)
+ received <- err
+ },
+ }
+ assert.NotPanics(t, func() {
+ codec.handleTransportError(testutils.ProduceTestingLogger(t),
poisoned)
+ })
+
+ select {
+ case err := <-received:
+ require.NotNil(t, err)
+ assert.NotPanics(t, func() { _ = stderrors.Is(err,
stderrors.New("x")) }, "handler error must be safe to walk")
+ assert.True(t, stderrors.Is(err,
spiErrors.ErrCorruptErrorChain), "sentinel must survive TransportError
wrapping")
+ assert.Contains(t, err.Error(), "read failed", "original
message must be preserved")
+ case <-time.After(5 * time.Second):
+ t.Fatal("transport error handler was never invoked")
+ }
+}
diff --git a/plc4go/spi/errors/marshal.go b/plc4go/spi/errors/marshal.go
index f5093f4254..f375cd2c74 100644
--- a/plc4go/spi/errors/marshal.go
+++ b/plc4go/spi/errors/marshal.go
@@ -19,6 +19,8 @@
package errors
+import "fmt"
+
// Constants compatible with github.com/rs/zerolog/pkgerrors so callers can do
// entry[errors.StackSourceFileName] and keep working when swapped over.
const (
@@ -27,6 +29,10 @@ const (
StackSourceFunctionName = "func"
)
+// StackMarshalPanicKey is the key under which MarshalStack reports a panic it
+// recovered while walking the error chain (instead of stack frames).
+const StackMarshalPanicKey = "error"
+
// MarshalStack walks err's chain looking for a value that implements
//
// interface{ StackTrace() StackTrace }
@@ -39,10 +45,15 @@ const (
// values (e.g. a typed-nil *net.OpError wrapped via %w) whose Unwrap method
// dereferences a nil receiver. This marshaler runs inside zerolog on every
// logged error, so a panic here would take down whatever worker is logging.
+// Instead of swallowing the anomaly, the recovered panic is reported as the
+// marshaled payload - it lands under the "stack" key of the very log event
+// that was logging the error, so no logger is needed at this level.
func MarshalStack(err error) (result any) {
defer func() {
if r := recover(); r != nil {
- result = nil
+ result = []map[string]string{{
+ StackMarshalPanicKey: fmt.Sprintf("panic while
extracting stack from error chain: %v", r),
+ }}
}
}()
type stackTracer interface {
diff --git a/plc4go/spi/errors/sanitize.go b/plc4go/spi/errors/sanitize.go
new file mode 100644
index 0000000000..3d08bf64ff
--- /dev/null
+++ b/plc4go/spi/errors/sanitize.go
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package errors
+
+import (
+ stderrors "errors"
+ "fmt"
+)
+
+// ErrCorruptErrorChain marks an error whose original chain could not be walked
+// safely. Detect it with errors.Is(err, ErrCorruptErrorChain) after the error
+// passed through SanitizeError.
+var ErrCorruptErrorChain = stderrors.New("corrupt error chain (panic during
unwrap)")
+
+// sanitizeMaxDepth bounds the chain walk so a (broken) cyclic chain classifies
+// as corrupt instead of hanging the caller.
+const sanitizeMaxDepth = 256
+
+// SanitizeError returns err unchanged when its chain can be walked safely.
+//
+// If walking the chain panics - which happens when improperly constructed
+// values (e.g. a typed-nil *net.OpError wrapped via %w) are part of the chain,
+// as their Unwrap/Error methods dereference a nil receiver - it returns a
+// flattened copy of the error message joined with ErrCorruptErrorChain. The
+// result is safe for errors.Is/As/Unwrap, keeps the original message in the
+// error string, and carries the anomaly as an Is-detectable sentinel, so it
+// stays visible wherever the caller eventually logs or propagates the error.
+// No logging happens at this level.
+//
+// Note: joining the ORIGINAL error with the sentinel would not defuse
+// anything - Join's Unwrap() []error hands the poisoned chain right back to
+// every future walk. That is why the message is flattened instead.
+func SanitizeError(err error) error {
+ if err == nil {
+ return nil
+ }
+ if chainWalksSafely(err) {
+ return err
+ }
+ return stderrors.Join(stderrors.New(safeErrorString(err)),
ErrCorruptErrorChain)
+}
+
+// chainWalksSafely performs the same traversal errors.Is would (including
+// multi-child Join nodes) and reports whether it completes without panicking.
+func chainWalksSafely(err error) (safe bool) {
+ defer func() {
+ if r := recover(); r != nil {
+ safe = false
+ }
+ }()
+ return walkChain(err, 0)
+}
+
+func walkChain(err error, depth int) bool {
+ for err != nil {
+ if depth > sanitizeMaxDepth {
+ return false
+ }
+ depth++
+ _ = err.Error() // Error() itself may dereference a nil receiver
+ switch x := err.(type) {
+ case interface{ Unwrap() error }:
+ err = x.Unwrap()
+ case interface{ Unwrap() []error }:
+ for _, child := range x.Unwrap() {
+ if !walkChain(child, depth) {
+ return false
+ }
+ }
+ return true
+ default:
+ return true
+ }
+ }
+ return true
+}
+
+// safeErrorString renders err.Error() with a panic guard, falling back to a
+// placeholder when even the message cannot be produced.
+func safeErrorString(err error) (s string) {
+ defer func() {
+ if r := recover(); r != nil {
+ s = fmt.Sprintf("(error message unavailable: panic in
Error(): %v)", r)
+ }
+ }()
+ return err.Error()
+}
diff --git a/plc4go/spi/errors/sanitize_test.go
b/plc4go/spi/errors/sanitize_test.go
new file mode 100644
index 0000000000..4d670ab7c1
--- /dev/null
+++ b/plc4go/spi/errors/sanitize_test.go
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package errors
+
+import (
+ stderrors "errors"
+ "fmt"
+ "net"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+// panickyError panics in Error() - simulating a node whose message cannot even
+// be rendered.
+type panickyError struct{}
+
+func (p *panickyError) Error() string { panic("boom in Error()") }
+
+// selfWrapping unwraps to itself - a (broken) cyclic chain.
+type selfWrapping struct{}
+
+func (s *selfWrapping) Error() string { return "cyclic" }
+func (s *selfWrapping) Unwrap() error { return s }
+
+func poisonedChain() error {
+ var nilOpErr *net.OpError
+ return fmt.Errorf("read failed: %w", error(nilOpErr))
+}
+
+func TestSanitizeError_nil(t *testing.T) {
+ assert.Nil(t, SanitizeError(nil))
+}
+
+func TestSanitizeError_cleanChainReturnedUnchanged(t *testing.T) {
+ inner := stderrors.New("inner")
+ err := fmt.Errorf("outer: %w", inner)
+ sanitized := SanitizeError(err)
+ assert.Equal(t, err, sanitized, "clean chains must pass through
untouched")
+ assert.True(t, stderrors.Is(sanitized, inner), "chain semantics must be
preserved")
+}
+
+func TestSanitizeError_typedNilInChain(t *testing.T) {
+ sanitized := SanitizeError(poisonedChain())
+ require.NotNil(t, sanitized)
+
+ // The sanitized error must be safe to walk...
+ assert.NotPanics(t, func() {
+ _ = stderrors.Is(sanitized, ErrCorruptErrorChain)
+ var target *net.OpError
+ _ = stderrors.As(sanitized, &target)
+ })
+ // ...carry the sentinel...
+ assert.True(t, stderrors.Is(sanitized, ErrCorruptErrorChain))
+ // ...and preserve the original message text.
+ assert.Contains(t, sanitized.Error(), "read failed")
+}
+
+func TestSanitizeError_typedNilAtTopLevel(t *testing.T) {
+ var nilOpErr *net.OpError
+ sanitized := SanitizeError(error(nilOpErr))
+ require.NotNil(t, sanitized)
+ assert.True(t, stderrors.Is(sanitized, ErrCorruptErrorChain))
+ assert.NotPanics(t, func() { _ = stderrors.Is(sanitized,
stderrors.New("x")) })
+}
+
+func TestSanitizeError_panickyErrorMessage(t *testing.T) {
+ err := fmt.Errorf("outer: %w", error(&panickyError{}))
+ // Chain walk panics via Error() on the child -> corrupt. Note
fmt.Errorf
+ // pre-renders the message, so the flattened string comes from the
wrapper
+ // and stays usable; a top-level panicky Error() falls back to a
placeholder.
+ sanitized := SanitizeError(err)
+ assert.True(t, stderrors.Is(sanitized, ErrCorruptErrorChain))
+
+ topLevel := SanitizeError(error(&panickyError{}))
+ assert.True(t, stderrors.Is(topLevel, ErrCorruptErrorChain))
+ assert.Contains(t, topLevel.Error(), "error message unavailable")
+}
+
+func TestSanitizeError_cyclicChain(t *testing.T) {
+ sanitized := SanitizeError(error(&selfWrapping{}))
+ assert.True(t, stderrors.Is(sanitized, ErrCorruptErrorChain), "cyclic
chains must classify as corrupt instead of hanging")
+ assert.Contains(t, sanitized.Error(), "cyclic")
+}
+
+func TestSanitizeError_joinWithPoisonedChild(t *testing.T) {
+ joined := stderrors.Join(stderrors.New("healthy"), poisonedChain())
+ sanitized := SanitizeError(joined)
+ assert.True(t, stderrors.Is(sanitized, ErrCorruptErrorChain))
+ assert.Contains(t, sanitized.Error(), "healthy")
+ assert.NotPanics(t, func() { _ = stderrors.Is(sanitized,
ErrCorruptErrorChain) })
+}
+
+func TestMarshalStack_poisonedChainReportsPanicMarker(t *testing.T) {
+ var result any
+ assert.NotPanics(t, func() {
+ result = MarshalStack(poisonedChain())
+ })
+ require.NotNil(t, result, "the recovered panic must be reported, not
swallowed")
+ marker, ok := result.([]map[string]string)
+ require.True(t, ok)
+ require.Len(t, marker, 1)
+ assert.Contains(t, marker[0][StackMarshalPanicKey], "panic while
extracting stack")
+}
