From 273f6b4247862aabfdbf3c9f8c87d2717b7c2944 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sat, 23 May 2026 17:28:44 +0000 Subject: [PATCH] fix(reporter): respect configured log level for job log forwarding (#989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Non-raw_output log entries above the globally configured `log.level` are no longer forwarded to the Gitea job log output - Step output (`raw_output=true`) is always forwarded regardless of level — it is actual job stdout/stderr, not runner internals - State-machine fields (`stepResult`, `jobResult`) are always processed regardless of level, preserving correct tracking for skipped steps (whose `stepResult` is emitted at `DebugLevel` in `step.go`) - Extracts a `shouldAppendLogRow` helper to avoid repeating the combined `!duringSteps() && entry.Level <= log.GetLevel()` guard in three places ## Why not the approach in #677 PR #677 adds `if entry.Level != log.GetLevel() { return nil }` at the top of `Fire()`. That has two bugs: 1. Uses `!=` instead of `>`, so `Error`/`Fatal` entries are dropped when the configured level is `Warn` 2. Returns early before processing `stepResult`/`jobResult` state fields — skipped steps (whose `stepResult` is logged at `DebugLevel`) would never be marked complete This fix instead applies the level guard only at the `r.logRows` append sites, leaving state tracking unconditional. Relates to #409. Reviewed-on: https://gitea.com/gitea/runner/pulls/989 Reviewed-by: Lunny Xiao --- internal/pkg/report/reporter.go | 13 +++++-- internal/pkg/report/reporter_test.go | 53 ++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/internal/pkg/report/reporter.go b/internal/pkg/report/reporter.go index d86f03a6..1a2767b4 100644 --- a/internal/pkg/report/reporter.go +++ b/internal/pkg/report/reporter.go @@ -205,7 +205,7 @@ func (r *Reporter) Fire(entry *log.Entry) error { urgentState = true } } - if !r.duringSteps() { + if r.shouldAppendLogRow(entry) { r.logRows = appendIfNotNil(r.logRows, r.parseLogRow(entry)) } r.unlockAndNotify(urgentState) @@ -219,7 +219,7 @@ func (r *Reporter) Fire(entry *log.Entry) error { } } if step == nil { - if !r.duringSteps() { + if r.shouldAppendLogRow(entry) { r.logRows = appendIfNotNil(r.logRows, r.parseLogRow(entry)) } r.unlockAndNotify(false) @@ -246,7 +246,7 @@ func (r *Reporter) Fire(entry *log.Entry) error { r.logRows = append(r.logRows, row) } } - } else if !r.duringSteps() { + } else if r.shouldAppendLogRow(entry) { r.logRows = appendIfNotNil(r.logRows, r.parseLogRow(entry)) } if v, ok := entry.Data["stepResult"]; ok && isJobStepEntry(entry) { @@ -576,6 +576,13 @@ func (r *Reporter) duringSteps() bool { return true } +// shouldAppendLogRow reports whether a non-raw_output entry should be written +// to the job log: only when we are between steps and the entry's level is +// within the globally configured log level. +func (r *Reporter) shouldAppendLogRow(entry *log.Entry) bool { + return !r.duringSteps() && entry.Level <= log.GetLevel() +} + var stringToResult = map[string]runnerv1.Result{ "success": runnerv1.Result_RESULT_SUCCESS, "failure": runnerv1.Result_RESULT_FAILURE, diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index f12ee1b4..b0c0280d 100644 --- a/internal/pkg/report/reporter_test.go +++ b/internal/pkg/report/reporter_test.go @@ -219,6 +219,59 @@ func TestReporter_Fire(t *testing.T) { }) } +func TestReporter_LogLevelFiltering(t *testing.T) { + // Set global level to Info so Debug entries should be filtered. + origLevel := log.GetLevel() + log.SetLevel(log.InfoLevel) + defer log.SetLevel(origLevel) + + client := mocks.NewClient(t) + client.On("UpdateLog", mock.Anything, mock.Anything).Return(func(_ context.Context, req *connect_go.Request[runnerv1.UpdateLogRequest]) (*connect_go.Response[runnerv1.UpdateLogResponse], error) { + return connect_go.NewResponse(&runnerv1.UpdateLogResponse{ + AckIndex: req.Msg.Index + int64(len(req.Msg.Rows)), + }), nil + }) + client.On("UpdateTask", mock.Anything, mock.Anything).Return(func(_ context.Context, req *connect_go.Request[runnerv1.UpdateTaskRequest]) (*connect_go.Response[runnerv1.UpdateTaskResponse], error) { + return connect_go.NewResponse(&runnerv1.UpdateTaskResponse{}), nil + }) + + ctx, cancel := context.WithCancel(context.Background()) + taskCtx, err := structpb.NewStruct(map[string]any{}) + require.NoError(t, err) + cfg, _ := config.LoadDefault("") + reporter := NewReporter(ctx, cancel, client, &runnerv1.Task{Context: taskCtx}, cfg) + reporter.RunDaemon() + defer func() { + require.NoError(t, reporter.Close("")) + }() + reporter.ResetSteps(2) + + dataStep0 := log.Fields{"stage": "Main", "stepNumber": 0, "raw_output": true} + dataStep0Internal := log.Fields{"stage": "Main", "stepNumber": 0} + + // raw_output entries always appear in job log regardless of level. + require.NoError(t, reporter.Fire(&log.Entry{Message: "step output", Data: dataStep0, Level: log.InfoLevel})) + require.NoError(t, reporter.Fire(&log.Entry{Message: "step debug output", Data: dataStep0, Level: log.DebugLevel})) + assert.Equal(t, int64(2), reporter.state.Steps[0].LogLength, "raw_output entries must always be forwarded") + + // Non-raw_output entries during steps are not added to logRows regardless of level. + require.NoError(t, reporter.Fire(&log.Entry{Message: "internal info", Data: dataStep0Internal, Level: log.InfoLevel})) + require.NoError(t, reporter.Fire(&log.Entry{Message: "internal debug", Data: dataStep0Internal, Level: log.DebugLevel})) + + // stepResult at DebugLevel (skipped step) must still update state even when filtered from log. + require.NoError(t, reporter.Fire(&log.Entry{ + Message: "Skipping step", + Data: log.Fields{ + "stage": "Main", + "stepNumber": 1, + "stepResult": "skipped", + }, + Level: log.DebugLevel, + })) + assert.Equal(t, runnerv1.Result_RESULT_SKIPPED, reporter.state.Steps[1].Result, + "stepResult at DebugLevel must update step state even when log entry is filtered from job log output") +} + // TestReporter_EphemeralRunnerDeletion reproduces the exact scenario from // https://gitea.com/gitea/runner/issues/793: //