fix: send job outputs & job result to Forgejo in sync with each other (#995)

Fixes #994.

First commit ensures that the interpolateResults method is invoked before data is sent to the reporter.  Second commit changes how data is sent to the reporter to include both the result and the job outputs.

<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- bug fixes
  - [PR](https://code.forgejo.org/forgejo/runner/pulls/995): <!--number 995 --><!--line 0 --><!--description Zml4OiBzZW5kIGpvYiBvdXRwdXRzICYgam9iIHJlc3VsdCB0byBGb3JnZWpvIGluIHN5bmMgd2l0aCBlYWNoIG90aGVy-->fix: send job outputs & job result to Forgejo in sync with each other<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/995
Reviewed-by: earl-warren <earl-warren@noreply.code.forgejo.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

internal/pkg/report/reporter.go

@@ -130,6 +130,11 @@ func (r *Reporter) Fire(entry *log.Entry) error {
 					}
 				}
 			}
+			if v, ok := entry.Data["jobOutputs"]; ok {
+				_ = r.setOutputs(v.(map[string]string))
+			} else {
+				log.Panicf("received log entry with jobResult, but without jobOutputs -- outputs will be corrupted for this job")
+			}
 		}
 		if !r.duringSteps() {
 			r.logRows = appendIfNotNil(r.logRows, r.parseLogRow(entry))
@@ -209,7 +214,7 @@ func (r *Reporter) logf(format string, a ...any) {
 	}
 }
 
-func (r *Reporter) GetOutputs() map[string]string {
+func (r *Reporter) cloneOutputs() map[string]string {
 	outputs := make(map[string]string)
 	r.outputs.Range(func(k, v any) bool {
 		if val, ok := v.(string); ok {
@@ -220,10 +225,9 @@ func (r *Reporter) GetOutputs() map[string]string {
 	return outputs
 }
 
-func (r *Reporter) SetOutputs(outputs map[string]string) error {
-	r.stateMu.Lock()
-	defer r.stateMu.Unlock()
-
+// Errors from setOutputs are logged into the reporter automatically; the `errors` return value is only used for unit
+// tests.
+func (r *Reporter) setOutputs(outputs map[string]string) error {
 	var errs []error
 	recordError := func(format string, a ...any) {
 		r.logf(format, a...)
@@ -369,10 +373,9 @@ func (r *Reporter) ReportState() error {
 
 	r.stateMu.RLock()
 	state := proto.Clone(r.state).(*runnerv1.TaskState)
+	outputs := r.cloneOutputs()
 	r.stateMu.RUnlock()
 
-	outputs := r.GetOutputs()
-
 	resp, err := r.client.UpdateTask(r.ctx, connect.NewRequest(&runnerv1.UpdateTaskRequest{
 		State:   state,
 		Outputs: outputs,

internal/pkg/report/reporter_test.go

@@ -78,7 +78,7 @@ func TestReporterSetOutputs(t *testing.T) {
 		reporter, _, _ := mockReporter(t)
 
 		expected := map[string]string{"a": "b", "c": "d"}
-		assert.NoError(t, reporter.SetOutputs(expected))
+		assert.NoError(t, reporter.setOutputs(expected))
 		assertEqual(t, expected, &reporter.outputs)
 	})
 
@@ -93,7 +93,7 @@ func TestReporterSetOutputs(t *testing.T) {
 			"c":       "ABCDEFG", // value too big
 			"d":       "e",
 		}
-		err := reporter.SetOutputs(in)
+		err := reporter.setOutputs(in)
 		assert.ErrorContains(t, err, "ignore output because the length of the value for \"c\" is 7 (the maximum is 5)")
 		assert.ErrorContains(t, err, "ignore output because the key is longer than 5: \"0123456\"")
 		expected := map[string]string{"d": "e"}
@@ -104,11 +104,11 @@ func TestReporterSetOutputs(t *testing.T) {
 		reporter, _, _ := mockReporter(t)
 
 		first := map[string]string{"a": "b", "c": "d"}
-		assert.NoError(t, reporter.SetOutputs(first))
+		assert.NoError(t, reporter.setOutputs(first))
 		assertEqual(t, first, &reporter.outputs)
 
 		second := map[string]string{"c": "d", "e": "f"}
-		assert.ErrorContains(t, reporter.SetOutputs(second), "ignore output because a value already exists for the key \"c\"")
+		assert.ErrorContains(t, reporter.setOutputs(second), "ignore output because a value already exists for the key \"c\"")
 
 		expected := map[string]string{"a": "b", "c": "d", "e": "f"}
 		assertEqual(t, expected, &reporter.outputs)
@@ -284,6 +284,23 @@ func TestReporter_Fire(t *testing.T) {
 
 		assert.Equal(t, int64(3), reporter.state.Steps[0].LogLength)
 	})
+
+	t.Run("jobResult jobOutputs extracted from log entry", func(t *testing.T) {
+		reporter, _, _ := mockReporter(t)
+
+		dataStep0 := map[string]any{
+			"stage":      "Post",
+			"stepNumber": 0,
+			"raw_output": true,
+			"jobResult":  "success",
+			"jobOutputs": map[string]string{"key1": "value1"},
+		}
+		assert.NoError(t, reporter.Fire(&log.Entry{Message: "success!", Data: dataStep0}))
+
+		assert.EqualValues(t, runnerv1.Result_RESULT_SUCCESS, reporter.state.Result)
+		value, _ := reporter.outputs.Load("key1")
+		assert.EqualValues(t, "value1", value)
+	})
 }
 
 func TestReporterReportState(t *testing.T) {
@@ -300,7 +317,7 @@ func TestReporterReportState(t *testing.T) {
 				outputValue1 := "VALUE1"
 				outputKey2 := "KEY2"
 				outputValue2 := "VALUE2"
-				reporter.SetOutputs(map[string]string{
+				reporter.setOutputs(map[string]string{
 					outputKey1: outputValue1,
 					outputKey2: outputValue2,
 				})
@@ -315,7 +332,7 @@ func TestReporterReportState(t *testing.T) {
 			assert: func(t *testing.T, reporter *Reporter, ctx context.Context, err error) {
 				t.Helper()
 				require.ErrorContains(t, err, "not all logs are submitted 1 remain")
-				outputs := reporter.GetOutputs()
+				outputs := reporter.cloneOutputs()
 				assert.Equal(t, map[string]string{
 					"KEY2": "VALUE2",
 				}, outputs)