From a63135363b513ba4bb13f0a721f4b7998cf3f85d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 9 Jan 2024 18:34:55 +0100 Subject: [PATCH] Step status update dont set to running again once it got stoped (#3151) Because of the check `if step.Stopped == 0` without the check there are edgecases where could be the case a stoped steped can be markt as running again, witch is wrong. I do remember we have "running" steps indefinetly in cancled pipelines. This could be the fix, i just did not test that specific. Anyway the func hat a good testcoverage ... so just look at the tests _Source: https://github.com/woodpecker-ci/woodpecker/pull/3143#discussion_r1446138088_ --- server/grpc/rpc.go | 2 +- .../{stepStatus.go => step_status.go} | 8 +- ...stepStatus_test.go => step_status_test.go} | 100 ++++++++---------- 3 files changed, 46 insertions(+), 64 deletions(-) rename server/pipeline/{stepStatus.go => step_status.go} (95%) rename server/pipeline/{stepStatus_test.go => step_status_test.go} (71%) diff --git a/server/grpc/rpc.go b/server/grpc/rpc.go index 428a0e85c..6f505425b 100644 --- a/server/grpc/rpc.go +++ b/server/grpc/rpc.go @@ -137,7 +137,7 @@ func (s *RPC) Update(_ context.Context, id string, state rpc.State) error { return err } - if err := pipeline.UpdateStepStatus(s.store, step, state, currentPipeline.Started); err != nil { + if err := pipeline.UpdateStepStatus(s.store, step, state); err != nil { log.Error().Err(err).Msg("rpc.update: cannot update step") } diff --git a/server/pipeline/stepStatus.go b/server/pipeline/step_status.go similarity index 95% rename from server/pipeline/stepStatus.go rename to server/pipeline/step_status.go index 2258b4c22..3d7bc3767 100644 --- a/server/pipeline/stepStatus.go +++ b/server/pipeline/step_status.go @@ -22,7 +22,7 @@ import ( "go.woodpecker-ci.org/woodpecker/v2/server/model" ) -func UpdateStepStatus(store model.UpdateStepStore, step *model.Step, state rpc.State, started int64) error { +func UpdateStepStatus(store model.UpdateStepStore, step *model.Step, state rpc.State) error { if state.Exited { step.Stopped = state.Finished step.ExitCode = state.ExitCode @@ -34,14 +34,10 @@ func UpdateStepStatus(store model.UpdateStepStore, step *model.Step, state rpc.S if state.ExitCode == 137 { step.State = model.StatusKilled } - } else { + } else if step.Stopped == 0 { step.Started = state.Started step.State = model.StatusRunning } - - if step.Started == 0 && step.Stopped != 0 { - step.Started = started - } return store.StepUpdate(step) } diff --git a/server/pipeline/stepStatus_test.go b/server/pipeline/step_status_test.go similarity index 71% rename from server/pipeline/stepStatus_test.go rename to server/pipeline/step_status_test.go index ffb2a7fa8..7a996cabe 100644 --- a/server/pipeline/stepStatus_test.go +++ b/server/pipeline/step_status_test.go @@ -33,7 +33,10 @@ func (m *mockUpdateStepStore) StepUpdate(_ *model.Step) error { func TestUpdateStepStatusNotExited(t *testing.T) { t.Parallel() + // step in db before update + step := &model.Step{} + // advertised step status state := rpc.State{ Started: int64(42), Exited: false, @@ -42,28 +45,23 @@ func TestUpdateStepStatusNotExited(t *testing.T) { ExitCode: 137, Error: "not an error", } - step := &model.Step{} - err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(1)) - assert.NoError(t, err) - if step.State != model.StatusRunning { - t.Errorf("Step status not equals '%s' != '%s'", model.StatusRunning, step.State) - } else if step.Started != int64(42) { - t.Errorf("Step started not equals 42 != %d", step.Started) - } else if step.Stopped != int64(0) { - t.Errorf("Step stopped not equals 0 != %d", step.Stopped) - } else if step.ExitCode != 0 { - t.Errorf("Step exit code not equals 0 != %d", step.ExitCode) - } else if step.Error != "" { - t.Errorf("Step error not equals '' != '%s'", step.Error) - } + err := UpdateStepStatus(&mockUpdateStepStore{}, step, state) + assert.NoError(t, err) + assert.EqualValues(t, model.StatusRunning, step.State) + assert.EqualValues(t, 42, step.Started) + assert.EqualValues(t, 0, step.Stopped) + assert.EqualValues(t, 0, step.ExitCode) + assert.EqualValues(t, "", step.Error) } func TestUpdateStepStatusNotExitedButStopped(t *testing.T) { t.Parallel() - step := &model.Step{Stopped: int64(64)} + // step in db before update + step := &model.Step{Started: 42, Stopped: 64, State: model.StatusKilled} + // advertised step status state := rpc.State{ Exited: false, // Dummy data @@ -71,25 +69,23 @@ func TestUpdateStepStatusNotExitedButStopped(t *testing.T) { ExitCode: 137, Error: "not an error", } - err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(42)) - assert.NoError(t, err) - if step.State != model.StatusRunning { - t.Errorf("Step status not equals '%s' != '%s'", model.StatusRunning, step.State) - } else if step.Started != int64(42) { - t.Errorf("Step started not equals 42 != %d", step.Started) - } else if step.Stopped != int64(64) { - t.Errorf("Step stopped not equals 64 != %d", step.Stopped) - } else if step.ExitCode != 0 { - t.Errorf("Step exit code not equals 0 != %d", step.ExitCode) - } else if step.Error != "" { - t.Errorf("Step error not equals '' != '%s'", step.Error) - } + err := UpdateStepStatus(&mockUpdateStepStore{}, step, state) + assert.NoError(t, err) + assert.EqualValues(t, model.StatusKilled, step.State) + assert.EqualValues(t, 42, step.Started) + assert.EqualValues(t, 64, step.Stopped) + assert.EqualValues(t, 0, step.ExitCode) + assert.EqualValues(t, "", step.Error) } func TestUpdateStepStatusExited(t *testing.T) { t.Parallel() + // step in db before update + step := &model.Step{Started: 42} + + // advertised step status state := rpc.State{ Started: int64(42), Exited: true, @@ -98,52 +94,42 @@ func TestUpdateStepStatusExited(t *testing.T) { Error: "an error", } - step := &model.Step{} - err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(42)) + err := UpdateStepStatus(&mockUpdateStepStore{}, step, state) assert.NoError(t, err) - - if step.State != model.StatusKilled { - t.Errorf("Step status not equals '%s' != '%s'", model.StatusKilled, step.State) - } else if step.Started != int64(42) { - t.Errorf("Step started not equals 42 != %d", step.Started) - } else if step.Stopped != int64(34) { - t.Errorf("Step stopped not equals 34 != %d", step.Stopped) - } else if step.ExitCode != 137 { - t.Errorf("Step exit code not equals 137 != %d", step.ExitCode) - } else if step.Error != "an error" { - t.Errorf("Step error not equals 'an error' != '%s'", step.Error) - } + assert.EqualValues(t, model.StatusKilled, step.State) + assert.EqualValues(t, 42, step.Started) + assert.EqualValues(t, 34, step.Stopped) + assert.EqualValues(t, 137, step.ExitCode) + assert.EqualValues(t, "an error", step.Error) } func TestUpdateStepStatusExitedButNot137(t *testing.T) { t.Parallel() + // step in db before update + step := &model.Step{Started: 42} + + // advertised step status state := rpc.State{ Started: int64(42), Exited: true, Finished: int64(34), Error: "an error", } - step := &model.Step{} - err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(42)) - assert.NoError(t, err) - if step.State != model.StatusFailure { - t.Errorf("Step status not equals '%s' != '%s'", model.StatusFailure, step.State) - } else if step.Started != int64(42) { - t.Errorf("Step started not equals 42 != %d", step.Started) - } else if step.Stopped != int64(34) { - t.Errorf("Step stopped not equals 34 != %d", step.Stopped) - } else if step.ExitCode != 0 { - t.Errorf("Step exit code not equals 0 != %d", step.ExitCode) - } else if step.Error != "an error" { - t.Errorf("Step error not equals 'an error' != '%s'", step.Error) - } + err := UpdateStepStatus(&mockUpdateStepStore{}, step, state) + assert.NoError(t, err) + assert.EqualValues(t, model.StatusFailure, step.State) + assert.EqualValues(t, 42, step.Started) + assert.EqualValues(t, 34, step.Stopped) + assert.EqualValues(t, 0, step.ExitCode) + assert.EqualValues(t, "an error", step.Error) } func TestUpdateStepStatusExitedWithCode(t *testing.T) { t.Parallel() + // advertised step status state := rpc.State{ Started: int64(42), Exited: true, @@ -152,7 +138,7 @@ func TestUpdateStepStatusExitedWithCode(t *testing.T) { Error: "an error", } step := &model.Step{} - err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(42)) + err := UpdateStepStatus(&mockUpdateStepStore{}, step, state) assert.NoError(t, err) if step.State != model.StatusFailure {