The initial Error state related changes look good.

I am a little wary about adding the new state since we might have state-conditional code that might confuse the Error state with another one. But you should be able to surface any issues while manually testing any of the error conditions. I will run some workflows manually on my end, injecting artificial errors, as well once you have the PR done.

For now I identified that we need to handle the new state at these points, by checking additionally for the Error state:

vreplication/controller.go:100

	// Nothing to do if replication is stopped.
	if params["state"] == binlogplayer.BlpStopped {
		ct.cancel = func() {}
		close(ct.done)
		return ct, nil
	}

vreplication/engine.go:758

		if qr.Rows[0][1].ToString() == binlogplayer.BlpStopped {
			return fmt.Errorf("replication has stopped at %v before reaching position %v, message: %s", current, mPos, qr.Rows[0][2].ToString())
		}

vreplicator.go:210

		// If any of the operations below changed state to Stopped, we should return.
		if settings.State == binlogplayer.BlpStopped {
			return nil
		}

Also the (VExec) Workflow commands:

workflow/server.go:401

		switch {
		case strings.Contains(strings.ToLower(stream.Message), "error"):
			stream.State = "Error"
		case stream.State == "Running" && len(stream.CopyStates) > 0:
			stream.State = "Copying"
		case stream.State == "Running" && int64(time.Now().Second())-timeUpdatedSeconds > 10:
			stream.State = "Lagging"
		}

vexec.go:646

func updateState(message, state string, cs []copyState, timeUpdated int64) string {
	if strings.Contains(strings.ToLower(message), "error") {
		state = "Error"
	} else if state == "Running" && len(cs) > 0 {
		state = "Copying"
	} else if state == "Running" && int64(time.Now().Second())-timeUpdated > 10 /* seconds */ {
		state = "Lagging"
	}
	return state
}

You also need to cherry-pick the fix to the unit tests ...

You also need to cherry-pick the fix to the unit tests ...

Ah, yes, thank you!

@rohit-nayak-ps I'm thinking, we can do this in smaller steps, and only break out of the controller when these errors are encountered AND this is an OnlineDDL workflow type. What do you think?

@rohit-nayak-ps I'm thinking, we can do this in smaller steps, and only break out of the controller when these errors are encountered AND this is an OnlineDDL workflow type. What do you think?

Are you saying you will only check and go into the Error state for OnlineDDLs?

Are you saying you will only check and go into the Error state for OnlineDDLs?

Yes, I propose we will only go into the error state, as well as completely quit the workflow -- only in online ddl. Does this increase confidence in the change?

Yes, I propose we will only go into the error state, as well as completely quit the workflow -- only in online ddl. Does this increase confidence in the change?

Good idea. Let's do that.

Good idea. Let's do that.

@rohit-nayak-ps done. Do I still need to worry about the new state in all aforementioned locations?

I'm also thinking, if this is too complex for now, we can add a column called unrecoverable_error (bool) in _vt.vreplication and update that column, instead of updating the state.

What do you think?

@rohit-nayak-ps done. Do I still need to worry about the new state in all aforementioned locations?

I'm also thinking, if this is too complex for now, we can add a column called unrecoverable_error (bool) in _vt.vreplication and update that column, instead of updating the state.

If you have tested one or two error situations in OnlineDDL then this approach should be fine. It is something we have been planning to add to VReplication workflows anyway and this will be a great start towards it.

We can update the other locations when we expand error handling to other workflows.

sounds good! Yes, tested locally many situations and shortly formally adding tests.

Oh I should note this PR extends #9958 ; so it looks a bit bigger than it would when #9958 is merged.

Tests added!

Ready to review.

OK then!