Repository: honeydipper/honeydipper
Pull request #526 (2023-03-03 20:56:27)

fix(core): workflow export failure protection
#### Description
Without the protection, the export failure of the workflow will cause the workflow to get stuck in memory, can not be cleaned up.

#### This PR fixes the following issues



Patch:
diff --git a/internal/workflow/continue.go b/internal/workflow/continue.go
index 3d6da35..80725c8 100644
--- a/internal/workflow/continue.go
+++ b/internal/workflow/continue.go
@@ -127,6 +127,13 @@ func (w *Session) processExport(msg *dipper.Message) {
 	envData := w.buildEnvData(msg)
 	status := msg.Labels["status"]
 
+	defer dipper.SafeExitOnError("session [%s] error on exporting data", w.ID, func(r interface{}) {
+		msg.Labels["status"] = SessionStatusError
+		if len(msg.Labels["reason"]) > 0 {
+			msg.Labels["reason"] += "\n"
+		}
+		msg.Labels["reason"] += fmt.Sprintf("Error on exporting data %+v", r)
+	})
 	if w.inFlyFunction != nil && status != SessionStatusError {
 		export := config.ExportFunctionContext(w.inFlyFunction, envData, w.store.Helper.GetConfig())
 		delete(envData, "sysData")
diff --git a/pkg/dipper/errorHandling.go b/pkg/dipper/errorHandling.go
index 35c3692..590db33 100644
--- a/pkg/dipper/errorHandling.go
+++ b/pkg/dipper/errorHandling.go
@@ -11,9 +11,16 @@ import "github.com/go-errors/errors"
 // SafeExitOnError : use this function in defer statement to ignore errors.
 func SafeExitOnError(args ...interface{}) {
 	if r := recover(); r != nil {
+		l := len(args)
+		if handler, ok := args[l-1].(func(interface{})); ok {
+			l--
+			handler(r)
+		}
 		Logger.Warningf("Resuming after error: %v", r)
 		Logger.Warning(errors.Wrap(r, 1).ErrorStack())
-		Logger.Warningf(args[0].(string), args[1:]...)
+		if l >= 1 {
+			Logger.Warningf(args[0].(string), args[1:l]...)
+		}
 	}
 }
 


Test patch:
diff --git a/internal/workflow/session_context_test.go b/internal/workflow/session_context_test.go
index 4c29623..7329aa0 100644
--- a/internal/workflow/session_context_test.go
+++ b/internal/workflow/session_context_test.go
@@ -109,6 +109,19 @@ contexts:
 	})
 	assert.Equal(t, "bar", w.exported[0]["foo"], "export_on_error should be used if on error")
 	assert.NotContains(t, w.exported[0], "foo2", "export should not be used if on error")
+
+	w = s.newSession("", "uuid12", &config.Workflow{Name: "workflow1", ExportOnError: map[string]interface{}{"foo": "$bar"}, Export: map[string]interface{}{"foo2": "$bar2"}}).(*Session)
+	m := &dipper.Message{
+		Labels: map[string]string{
+			"status": "success",
+			"reason": "test",
+		},
+	}
+	w.prepare(&dipper.Message{}, nil, map[string]interface{}{})
+	assert.NotPanics(t, func() { w.processExport(m) }, "export should not panic if unable to export")
+	assert.Empty(t, w.exported, "export_on_error should fail if there is interpolation error")
+	assert.Equal(t, "test\nError on exporting data config error: invalid path: bar2", m.Labels["reason"], "failed export should append message to reason")
+	assert.Equal(t, "error", m.Labels["status"], "failed export should set message status to fail")
 }
 
 var configStrWithEventContexts = `
diff --git a/pkg/dipper/errorHandling_test.go b/pkg/dipper/errorHandling_test.go
new file mode 100644
index 0000000..bbc1a7d
--- /dev/null
+++ b/pkg/dipper/errorHandling_test.go
@@ -0,0 +1,34 @@
+// Copyright 2023 PayPal Inc.
+
+// This Source Code Form is subject to the terms of the MIT License.
+// If a copy of the MIT License was not distributed with this file,
+// you can obtain one at https://mit-license.org/.
+
+//go:build !integration
+// +build !integration
+
+package dipper
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSafeExitOnError(t *testing.T) {
+	testFunc := func() {
+		defer SafeExitOnError("test found error")
+		panic("test error")
+	}
+	assert.NotPanics(t, testFunc, "testFunc should not panic with SafeExitOnError")
+
+	var caught any
+	testFuncWithHandler := func() {
+		defer SafeExitOnError(func(r any) {
+			caught = r
+		})
+		panic("test error")
+	}
+	assert.NotPanics(t, testFuncWithHandler, "testFuncWithHandler should not panic with SafeExitOnError")
+	assert.Equal(t, "test error", caught, "testFuncWithHandler should run the handler")
+}