don't cast Value when pipe is errored

[maxlaverse]

Follow up of #2382

I couldn't find an easy way to get an &edgeState{} as Value of the errored pipe. It seems the simplest solution is to not cast the Value in such a situation. Eventually we could also check if the Value can be cast, and return if it's not the case, but I'm not sure if it brings anything. Thoughts @tonistiigi ?

Having target==nil would end up with the following error.

# Daemon
ERRO[2021-10-01T21:34:06Z] /moby.buildkit.v1.Control/Solve returned error: rpc error: code = Unknown desc = failed to get edge: inconsistent graph state

# Client
error: failed to solve: failed to get edge: inconsistent graph state

/cc @jamesalucas

[maxlaverse]

I also tried to write a test for this, but it's not easy to mess up with graph during the build at the right moment.

[tonistiigi]

We shouldn't return out without processing through all updates. It might be ok to skip the commands inside the current if block. But returning req in https://github.com/moby/buildkit/blob/master/solver/scheduler.go#L355 definitely looks wrong anyway and it has edgeState property.

[maxlaverse]

We shouldn't return out without processing through all updates.

Isn't the loop to process updates outside of the function we're changing here ? In case of an error we're returning at the end of the if block anyway, and everything in between requires the cast to succeed.

returning req in https://github.com/moby/buildkit/blob/master/solver/scheduler.go#L355 definitely looks wrong anyway and it has edgeState property.

Removed!

[tonistiigi]

Ok, I thought this code was iterating over all updates. Still, this error condition is complicated (with the cancel check etc) so I'd rather check the cast failure.

Removed!

Did you try returning the req.edgeState? It looks cleaner than this nil (unless we want to refactor all of these updates to handle nils).

[maxlaverse]

I can return the &req.edgeState here but it doesn't seem to make any difference. It's thrown away because we return an error if I got it right:

buildkit/solver/internal/pipe/pipe.go

Lines 83 to 87 in 4e69662

	if err != nil {
	p.Sender.Finalize(nil, err)
	return
	}
	p.Sender.Finalize(res, nil)

[tonistiigi]

Hmm. Then why wouldn't it panic in this cast for any regular error from a dependant edge?

[tonistiigi]

Ah, ok. The value is thrown away in func error, but not in newPipe error.