internal/workflow: always use JSON-marshaled task results

We only realized JSON marshaling of the release tasks was broken when
we retried them because in normal operation we used the in-memory
versions. Change to always use the marshaled results.

For golang/go#51797.

Change-Id: I9dbca62520732a7b00d64dc2481ab35b221fecf0
Reviewed-on: https://go-review.googlesource.com/c/build/+/411894
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

internal/workflow/workflow.go

@@ -518,11 +518,11 @@ func Resume(def *Definition, state *WorkflowState, taskStates map[string]*TaskSt
 			serializedResult: tState.SerializedResult,
 		}
 		if state.serializedResult != nil {
-			ptr := reflect.New(reflect.ValueOf(taskDef.f).Type().Out(0))
-			if err := json.Unmarshal(tState.SerializedResult, ptr.Interface()); err != nil {
+			result, err := unmarshalNew(reflect.ValueOf(taskDef.f).Type().Out(0), tState.SerializedResult)
+			if err != nil {
 				return nil, fmt.Errorf("failed to unmarshal result of %v: %v", taskDef.name, err)
 			}
-			state.result = ptr.Elem().Interface()
+			state.result = result
 		}
 		if tState.Error != "" {
 			state.err = fmt.Errorf("serialized error: %v", tState.Error) // untyped, but hopefully that doesn't matter.
@@ -532,6 +532,14 @@ func Resume(def *Definition, state *WorkflowState, taskStates map[string]*TaskSt
 	return w, nil
 }
 
+func unmarshalNew(t reflect.Type, data []byte) (interface{}, error) {
+	ptr := reflect.New(t)
+	if err := json.Unmarshal(data, ptr.Interface()); err != nil {
+		return nil, err
+	}
+	return ptr.Elem().Interface(), nil
+}
+
 // Run runs a workflow to completion or quiescence and returns its outputs.
 // listener.TaskStateChanged will be called immediately, when each task starts,
 // and when they finish. It should be used only for monitoring and persistence
@@ -612,17 +620,21 @@ func (w *Workflow) runTask(ctx context.Context, listener Listener, state taskSta
 		WorkflowID: w.ID,
 	}
 	in := append([]reflect.Value{reflect.ValueOf(tctx)}, args...)
-	out := reflect.ValueOf(state.def.f).Call(in)
+	fv := reflect.ValueOf(state.def.f)
+	out := fv.Call(in)
 
 	if errIdx := len(out) - 1; !out[errIdx].IsNil() {
 		state.err = out[errIdx].Interface().(error)
 	}
 	state.finished = true
-	if len(out) == 2 {
-		state.result = out[0].Interface()
-	}
-	if state.err == nil {
-		state.serializedResult, state.err = json.Marshal(state.result)
+	if len(out) == 2 && state.err == nil {
+		state.serializedResult, state.err = json.Marshal(out[0].Interface())
+		if state.err == nil {
+			state.result, state.err = unmarshalNew(fv.Type().Out(0), state.serializedResult)
+		}
+		if state.err == nil && !reflect.DeepEqual(out[0].Interface(), state.result) {
+			state.err = fmt.Errorf("JSON marshaling changed result from %#v to %#v", out[0].Interface(), state.result)
+		}
 	}
 	return state
 }

internal/workflow/workflow_test.go

@@ -342,6 +342,23 @@ func TestResume(t *testing.T) {
 	})
 }
 
+type badResult struct {
+	unexported string
+}
+
+func TestBadMarshaling(t *testing.T) {
+	greet := func(_ context.Context) (badResult, error) {
+		return badResult{"hi"}, nil
+	}
+
+	wd := workflow.New()
+	wd.Output("greeting", wd.Task("greet", greet))
+	w := startWorkflow(t, wd, nil)
+	if _, err := w.Run(context.Background(), &verboseListener{t}); err == nil {
+		t.Errorf("running a workflow with bad JSON should give an error, got none")
+	}
+}
+
 type mapListener struct {
 	workflow.Listener
 	states map[uuid.UUID]map[string]*workflow.TaskState