Do not warn on unfinished handlers if workflow failed (#1581)

temporalio · Aug 6, 2024 · c2ce2e6 · c2ce2e6
1 parent a1d05a6
commit c2ce2e6
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 17 deletions.
diff --git a/internal/internal_workflow.go b/internal/internal_workflow.go
@@ -682,7 +682,7 @@ func executeDispatcher(ctx Context, dispatcher dispatcher, timeout time.Duration
 	if len(us) > 0 {
 		env.GetLogger().Warn("Workflow has unhandled signals", "SignalNames", us)
 	}
-	//
+	// Warn if there are any update handlers still running
 	type warnUpdate struct {
 		Name string `json:"name"`
 		ID   string `json:"id"`
@@ -696,7 +696,11 @@ func executeDispatcher(ctx Context, dispatcher dispatcher, timeout time.Duration
 			})
 		}
 	}
-	if len(updatesToWarn) > 0 {
+
+	// Verify that the workflow did not fail. If it did we will not warn about unhandled updates.
+	var canceledErr *CanceledError
+	var contErr *ContinueAsNewError
+	if len(updatesToWarn) > 0 && (rp.error == nil || errors.As(rp.error, &canceledErr) || errors.As(rp.error, &contErr)) {
 		env.GetLogger().Warn(unhandledUpdateWarningMessage, "Updates", updatesToWarn)
 	}
 

diff --git a/internal/workflow_testsuite_test.go b/internal/workflow_testsuite_test.go
@@ -649,44 +649,48 @@ func TestWorkflowAllHandlersFinished(t *testing.T) {
 
 	}
 	// assertExpectedLogs asserts that the logs in the buffer are as expected
-	assertExpectedLogs := func(t *testing.T, buf *bytes.Buffer) {
+	assertExpectedLogs := func(t *testing.T, buf *bytes.Buffer, shouldWarn bool) {
 		logs := parseLogs(buf)
-		require.Len(t, logs, 1)
-		require.Equal(t, unhandledUpdateWarningMessage, logs[0]["msg"])
-		warnedUpdates := parseWarnedUpdates(logs[0]["Updates"])
-		require.Len(t, warnedUpdates, 2)
-		// Order of updates is not guaranteed
-		require.Equal(t, "update", warnedUpdates[0]["name"])
-		require.True(t, warnedUpdates[0]["id"] == "id_1" || warnedUpdates[0]["id"] == "id_2")
-		require.Equal(t, "update", warnedUpdates[1]["name"])
-		require.True(t, warnedUpdates[1]["id"] != warnedUpdates[0]["id"])
-		require.True(t, warnedUpdates[1]["id"] == "id_1" || warnedUpdates[1]["id"] == "id_2")
+		if shouldWarn {
+			require.Len(t, logs, 1)
+			require.Equal(t, unhandledUpdateWarningMessage, logs[0]["msg"])
+			warnedUpdates := parseWarnedUpdates(logs[0]["Updates"])
+			require.Len(t, warnedUpdates, 2)
+			// Order of updates is not guaranteed
+			require.Equal(t, "update", warnedUpdates[0]["name"])
+			require.True(t, warnedUpdates[0]["id"] == "id_1" || warnedUpdates[0]["id"] == "id_2")
+			require.Equal(t, "update", warnedUpdates[1]["name"])
+			require.True(t, warnedUpdates[1]["id"] != warnedUpdates[0]["id"])
+			require.True(t, warnedUpdates[1]["id"] == "id_1" || warnedUpdates[1]["id"] == "id_2")
+		} else {
+			require.Len(t, logs, 0)
+		}
 	}
 
 	t.Run("complete", func(t *testing.T) {
 		var buf bytes.Buffer
 		result, err := runWf("complete", &buf)
 		require.NoError(t, err)
 		require.Equal(t, 2, result)
-		assertExpectedLogs(t, &buf)
+		assertExpectedLogs(t, &buf, true)
 	})
 	t.Run("cancel", func(t *testing.T) {
 		var buf bytes.Buffer
 		_, err := runWf("cancel", &buf)
 		require.Error(t, err)
-		assertExpectedLogs(t, &buf)
+		assertExpectedLogs(t, &buf, true)
 	})
 	t.Run("failure", func(t *testing.T) {
 		var buf bytes.Buffer
 		_, err := runWf("failure", &buf)
 		require.Error(t, err)
-		assertExpectedLogs(t, &buf)
+		assertExpectedLogs(t, &buf, false)
 	})
 	t.Run("continue-as-new", func(t *testing.T) {
 		var buf bytes.Buffer
 		_, err := runWf("continue-as-new", &buf)
 		require.Error(t, err)
-		assertExpectedLogs(t, &buf)
+		assertExpectedLogs(t, &buf, true)
 	})
 }