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

@@ -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")
 	}
 

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)
 }
 

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 {
+	err := UpdateStepStatus(&mockUpdateStepStore{}, step, state)
-		t.Errorf("Step status not equals '%s' != '%s'", model.StatusRunning, step.State)
+	assert.NoError(t, err)
-	} else if step.Started != int64(42) {
+	assert.EqualValues(t, model.StatusRunning, step.State)
-		t.Errorf("Step started not equals 42 != %d", step.Started)
+	assert.EqualValues(t, 42, step.Started)
-	} else if step.Stopped != int64(0) {
+	assert.EqualValues(t, 0, step.Stopped)
-		t.Errorf("Step stopped not equals 0 != %d", step.Stopped)
+	assert.EqualValues(t, 0, step.ExitCode)
-	} else if step.ExitCode != 0 {
+	assert.EqualValues(t, "", step.Error)
-		t.Errorf("Step exit code not equals 0 != %d", step.ExitCode)
-	} else if step.Error != "" {
-		t.Errorf("Step error not equals '' != '%s'", 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 {
+	err := UpdateStepStatus(&mockUpdateStepStore{}, step, state)
-		t.Errorf("Step status not equals '%s' != '%s'", model.StatusRunning, step.State)
+	assert.NoError(t, err)
-	} else if step.Started != int64(42) {
+	assert.EqualValues(t, model.StatusKilled, step.State)
-		t.Errorf("Step started not equals 42 != %d", step.Started)
+	assert.EqualValues(t, 42, step.Started)
-	} else if step.Stopped != int64(64) {
+	assert.EqualValues(t, 64, step.Stopped)
-		t.Errorf("Step stopped not equals 64 != %d", step.Stopped)
+	assert.EqualValues(t, 0, step.ExitCode)
-	} else if step.ExitCode != 0 {
+	assert.EqualValues(t, "", step.Error)
-		t.Errorf("Step exit code not equals 0 != %d", step.ExitCode)
-	} else if step.Error != "" {
-		t.Errorf("Step error not equals '' != '%s'", 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)
-	err := UpdateStepStatus(&mockUpdateStepStore{}, step, state, int64(42))
 	assert.NoError(t, err)
-
+	assert.EqualValues(t, model.StatusKilled, step.State)
-	if step.State != model.StatusKilled {
+	assert.EqualValues(t, 42, step.Started)
-		t.Errorf("Step status not equals '%s' != '%s'", model.StatusKilled, step.State)
+	assert.EqualValues(t, 34, step.Stopped)
-	} else if step.Started != int64(42) {
+	assert.EqualValues(t, 137, step.ExitCode)
-		t.Errorf("Step started not equals 42 != %d", step.Started)
+	assert.EqualValues(t, "an error", step.Error)
-	} 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)
-	}
 }
 
 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 {
+	err := UpdateStepStatus(&mockUpdateStepStore{}, step, state)
-		t.Errorf("Step status not equals '%s' != '%s'", model.StatusFailure, step.State)
+	assert.NoError(t, err)
-	} else if step.Started != int64(42) {
+	assert.EqualValues(t, model.StatusFailure, step.State)
-		t.Errorf("Step started not equals 42 != %d", step.Started)
+	assert.EqualValues(t, 42, step.Started)
-	} else if step.Stopped != int64(34) {
+	assert.EqualValues(t, 34, step.Stopped)
-		t.Errorf("Step stopped not equals 34 != %d", step.Stopped)
+	assert.EqualValues(t, 0, step.ExitCode)
-	} else if step.ExitCode != 0 {
+	assert.EqualValues(t, "an error", step.Error)
-		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)
-	}
 }
 
 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 {