Fixes: 17292 - Detect infinite loop condition while iterating terminations in CablePath.from_origin

[bctiemann]

Fixes: #17292

Adds a check to Step 4 of CablePath.from_origin to make sure the path length is not exceeding CABLE_TRACE_MAX_LENGTH (a new setting).

Also adds an optional max_length parameter to from_origin for use as a testing hook, to determine whether the loop detection is working.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[DanSheps]

If I am understanding this correctly, this iwll only detect a loop in the last element, not if it is 3 elements back

Eg:

FP1 <> RP1 <> RP2 <> FP2 <> FP3 <> RP3 <> RP4 <> FP4 <> FP1

Could youy confirm this?

[bctiemann]

Please re-review; I moved the check to earlier in the process and made it more straightforward. It only checks cables in the body of the trace, not the beginning and end terminations, but I doubt there's risk there.

[DanSheps]

Is this test suppose to fail or succeed? As it stands I don't see a loop in the topology unless I am missing something.

[bctiemann]

If there's an infinite loop then the test will just keep executing. I'm not sure of an elegant way to test the infinite looping condition, but as long as it executes without the whole test process hanging then the test passes.

I've tried a few different approaches for making it assert if the execution takes too long or loops too many times, mostly involving timeouts and/or threading, but I don't know if it's worth going to that extreme. What would you suggest?

In any case if you comment out lines 600-602 in cables.py, the test will hang indefinitely.

[DanSheps]

IMO, for testing purposes we may want to add a parameter that sets a maximum path length (might not be bad anyways to set it to an unreasonable number) and still create a loop.

IDK if it is worth it though TBH. It is just this test doesn't really do anything as far as testing for a "loop" though., which was more my point. Might not be worth including a test even for this.

[jeremystretch]

It seems like this condition can be avoided by checking for the presence of each encountered termination in the path so far: If we see the same termination twice, we're in a loop. Could this be implemented under Cable.trace()?

[bctiemann]

Short answer -- no, I don't think so. The from_origin method that has the infinite loop possibility in it is called from a variety of places, including create_cablepath and retrace_cable_paths, and defending against it at that level would have to be done in all those locations. Similarly Cable.trace is not called in the post-save signal flow that triggers the infinite loop condition I found; it goes straight to from_origin where it falls into the loop. I think the only reasonable place to handle this is in from_origin itself.

[bctiemann]

I'm going to close this PR, as it's sucking up a lot of time that is better spent elsewhere. I believe the solution here is the correct and least invasive way to deal with this issue, so if anyone picks up this issue in the future this is a potential valid place to start from.