fix: when key not present assume NodeRunning. Fixes 11843

[isubasinghe]

Fixes #11843

When a node is not present in the nodes map, this implies that the DAG is still running.
We errored out which is what causes #11843, this fixes that by assuming NodeRunning.

Verification

Tested on the workflow posted in #11843

[agilgur5]

This is equivalent logic as before, as you wrote in your analysis in #11843 (comment), so I'm fine approving.

But I am wondering if there is a deeper bug (that silently existed before) that we should try to fix here? Since the current logic means it got a nodeId that doesn't exist in the nodes map??
boundaryNode.children should all be valid nodes, so my only guess is that getRetryNodeChildrenIds might be getting an incorrect id, although the logic of that function seemed fine at a glance.

[isubasinghe]

@agilgur5 I think its somewhat impossible to say if the default initialisation was intended or not.
But here is the argument for why this change makes sense:

The nodeId doesn't exist yet because the controller didn't create it yet, this implies code is yet to run.
Therefore the NodeRunning state is implied.

It makes sense to me given the overall design of the controller.

I do not like the fact that the controller even attempts to run this code, because it should know that these nodes have not been created. Making the controller more "context aware" would be nice, it wouldn't need to try paths that it knows it doesn't need to run yet, would probably reduce bugs as well.

[isubasinghe]

To make a more general point, this is why default initialised values are bad, they do not scale (with respect to #users).

When a programmer uses default initialised values, they make an implicit assumption that this behaviour is fine/valid. The
problem here is that default initialised values are not always valid. How do you actually know the programmers intention? One way is via explicit checks, the other is via comments.

So yeah I think we should only allow for code relying on default initialised values, if it has been commented at least.
Explicit is always better though.

[agilgur5]

To make a more general point, this is why default initialised values are bad
Explicit is always better though.

Oh for sure, agreed on that. We're fighting the language design a bit clearly. I do wonder if there's a linter check for this scenario as well though.
(which does feel ironic to me as someone who writes a lot more TS and Python, which are not as respected in the community)

[agilgur5]

The nodeId doesn't exist yet because the controller didn't create it yet, this implies code is yet to run.
Therefore the NodeRunning state is implied.

Wait so does the id exist but the struct around it wasn't created? I actually didn't expect that scenario initially but I suppose that could happen. 😵‍💫

I do not like the fact that the controller even attempts to run this code, because it should know that these nodes have not been created.

So as far as my current understanding of the controller goes, it seems that speculation like this is primarily limited to DAGs as in the assessDAGPhase function. There are still good chunks of the controller I'm not super familiar with yet though.

But yea agreed that making things more deterministic is usually better. Some of the templating changes I'd like to make are conceptually similar (e.g. erroring out earlier at validation with type-checking etc instead of run-time issues)

[agilgur5]

Approving since, as I mentioned above, this fixes the regression and is equivalent behavior to the previous version.

But would still like to continue the conversation here, both for my own edification, and to help with finding root cause

[isubasinghe]

To make a more general point, this is why default initialised values are bad
Explicit is always better though.

Oh for sure, agreed on that. We're fighting the language design a bit clearly. I do wonder if there's a linter check for this scenario as well though. (which does feel ironic to me as someone who writes a lot more TS and Python, which are not as respected in the community)

I definitely prefer mypy and TS over Go, Go has weird footguns like this that it invented for no real reason.

Wait so does the id exist but the struct around it wasn't created? I actually didn't expect that scenario initially but I suppose that could happen. 😵‍💫

Yup the node id exists because it is something that is generated at runtime (which is also not ideal), but its possible the struct wasn't created yet.

So as far as my current understanding of the controller goes, it seems that speculation like this is primarily limited to DAGs as in the assessDAGPhase function.

I maybe wrong, but my assessment was that this kind of speculation is actually relatively common. Feel like a lot of issues I've dealt with were at least indirectly because of this. Might just be my bias.

[agilgur5]

but its possible the struct wasn't created yet.

well that would explain why I couldn't seem to find any deletions in #11451 (comment) -- it's not that they were deleted, it's that they didn't exist yet 😵‍💫

I maybe wrong, but my assessment was that this kind of speculation is actually relatively common.

You probably know this part of the codebase better than me, to be fair. But as a piece of anecdotal evidence, I never encountered these issues while using Workflows and most of our usage was with steps. If this is DAG-specific, that would add up. steps don't need a graph traversal either.