Fixes #21960: Allow cable trace to continue through multi-position FrontPorts without position context

[attoba]

Fixes: #21960

When tracing encounters a multi-position FrontPort with an empty position_stack, instead of aborting unconditionally, perform a one-hop lookahead: inspect what each mapped RearPort is cabled to on the far end and check whether all far-end terminations share the same parent device/module.

• If they share a parent, the split is convergent (all strands lead to the same endpoint) and traversal continues correctly.
• If they diverge to different parents, the path is genuinely ambiguous and is flagged as is_split=True.

The existing abort behavior for multi-position RearPorts is preserved unchanged.

Tests added:

• test_225: convergent duplex QSFP model resolves connected_endpoints in both directions.
• test_226: divergent multi-position FrontPort still flags is_split=True.

[jeremystretch]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jeremystretch's task in 2m 36s —— View job

---

Review

Potential concern: No test coverage for the fixed scenario

The removed code path (multi-position FrontPort + empty position_stack → is_split = True) has no corresponding regression test added. The existing tests in test_cablepaths2.py cover multi-position FrontPort cases but always with position context supplied from a breakout cable (so position_stack is populated before the FrontPort is encountered). There is no test that exercises the trace originating from a point where FrontPort.positions > 1 and position_stack is empty, which is the exact scenario this PR fixes. Without a test, this behavior could regress silently.

Subtle behavioral concern: all positions pushed when stack is empty

After the fix, when FrontPort.positions > 1 and position_stack is empty, the code falls through to:

port_mappings = PortMapping.objects.filter(front_port__in=remote_terminations)

This returns all PortMapping records for the FrontPort — one per position. Then at line 953–954:

if any(t.positions > 1 for t in terminations):
    position_stack.append([mapping.rear_port_position for mapping in port_mappings])

All rear_port_position values from all mappings are pushed. In the test_202 topology (FP1 with 4 positions, 4 PortMappings), this pushes [1, 2, 3, 4] — meaning the trace will follow all four fiber positions on the downstream RearPort. The result is a split/complete path covering all endpoints rather than an incomplete/aborted trace. That's arguably more useful, but it's worth confirming this is the intended outcome for issue #21960 rather than just allowing a single-fiber trace to proceed.

The underlying asymmetry with RearPort (still aborts on empty stack) is well-reasoned per the PR description — a FrontPort always maps to exactly one RearPort, so there's no directional ambiguity; the concern is only whether the "fan out all positions" behavior on the downstream leg is acceptable.

[jeremystretch]

@attoba could you please include tests to ensure that this change doesn't introduce a breaking change to existing behavior?

[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.

[jeremystretch]

@attoba are you still interested in working on this?

[jeremystretch]

Removing this branch fails to address the scenario where two front ports map to different remote endpoints:

With the abort gone and the rear ports being single-position, nothing pushes onto position_stack (line 1056), so the split-detection guard at line 887 (position_stack and …) is skipped on the next hop. If the two positions of the front port lead to different remote endpoints, the trace will now complete with two connected_endpoints and is_split = False, rather than being flagged as a split. The issue text explicitly frames the desired behavior as "a path split is only genuinely ambiguous when the branches lead to different endpoints" — but this PR doesn't actually distinguish convergent from divergent; it just stops aborting.

Rather than removing it, consider checking that the mapped ports all share the same parent.

[jeremystretch]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jeremystretch's task in 1m 26s —— View job

---

Review

• Reading changed files
• Analyzing logic and tests
• Posting review

---

Three issues worth addressing before merge:

---

1. IndexError when port_mappings is empty (crash bug)

cables.py:954–955:

rear_ports = [m.rear_port for m in port_mappings]
termination_type = ObjectType.objects.get_for_model(rear_ports[0])  # IndexError if empty

The guard if not port_mappings: break at line 981 comes after this block. If a multi-position FrontPort has no configured PortMapping records, rear_ports is an empty list and rear_ports[0] raises IndexError. The fix is straightforward: add if not port_mappings: before accessing rear_ports[0], or move the guard above this block. Fix this →

---

2. Module ID / Device ID namespace collision in parent identity check

cables.py:971–973:

far_end_parents.add(
    getattr(t, 'module_id', None) or getattr(t, 'device_id', None)
)

module_id and device_id are PKs from separate database tables, so Module #5 and Device #5 are different objects but both contribute the integer 5 to the set. A path where one RearPort ends on Module 5 and another ends on Device 5 would be treated as convergent (set size 1) when it is genuinely divergent. Use a typed tuple to disambiguate: ('module', t.module_id) if getattr(t, 'module_id', None) else ('device', getattr(t, 'device_id', None)). Fix this →

---

3. Test 225 topology requires test_225 to pass cable_position context at fp_b_ce0

In test 225, cable_b_mod is Cable(a_terminations=[if_b], b_terminations=[fp_b_ce0]). fp_b_ce0 has positions=2, so the cable has no position breakdown — position context is not pushed, and the FrontPort is encountered with an empty stack. That's the correct regression scenario. However, the test then expects len(if_b.connected_endpoints) == 1 — a fully-resolved single endpoint. The only way the trace resolves to a single endpoint via both rp_b_tx and rp_b_rx is if the convergence check passes and both rear ports end up on the same interface (if_a) on device A. That topology does have rp_a_tx and rp_a_rx leading back to if_a via fp_a_tx/fp_a_rx, so the test should work — but it is worth verifying that fp_a_tx and fp_a_rx are single-position (positions=1) and will not themselves trigger the multi-position guard. They are created with positions=1, so this is fine. No action needed here — just flagging that the test topology is subtly non-trivial.

---

[jeremystretch]

@attoba thanks for following up on this. Please see if you can address the concerns raised above.

[jeremystretch]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jeremystretch's task in 2m 58s —— View job

---

Review

• Reading changed files
• Analyzing logic and tests
• Posting review

---

Two concerns worth addressing before merge:

---

1. One-hop lookahead does not prevent false "convergent" verdicts in multi-hop topologies

cables.py:969–984 — the convergence check inspects only the immediate far-end parent of each mapped RearPort. If both positions of the multi-position FrontPort connect to different ports on the same intermediate device (same device_id), the set stays at size 1 and the trace continues — even when those ports then diverge further to different final endpoints on different devices. The result is a completed path with multiple connected_endpoints and is_split=False, which is precisely what the old abort was meant to prevent.

Test 226 covers the straightforward divergent case (two strands directly reaching two different devices), but there is no test for the "same intermediate device, different final endpoints" case. If that topology is considered out of scope for this fix, the condition and its limitation should be documented.

---

2. Far-end terminations that have neither device_id nor module_id collapse to ('device', None)

cables.py:974–977:

far_end_parents.add(
    ('module', t.module_id) if getattr(t, 'module_id', None)
    else ('device', getattr(t, 'device_id', None))
)

CableTermination.termination is a generic foreign key that can resolve to a CircuitTermination, PowerFeed, or other objects that carry neither device_id nor module_id. All such objects produce the key ('device', None), so two strands ending on different circuit terminations (or power feeds) would be treated as convergent — no split flagged, no abort. This would be incorrect. At minimum, add an explicit guard or use the content_type + pk of the termination object itself as the identity key, which is unambiguous by construction.

Fix this →

---

[jeremystretch]

Thanks for keeping at this @attoba. Unfortunately this doesn't sufficiently validate that the diverged paths rejoin. I've included some analysis from Claude that breaks down the specific concerns.

1. netbox/dcim/models/cables.py ~1043-1058 — parent-equality is necessary but not sufficient; strands landing on different endpoints of the same device are merged into one wrong path. (CONFIRMED)
The check compares only the immediate far-end termination's parent, never that the strands reconverge on one endpoint. Topology: duplex fp_b (positions=2) → rp_b_1,rp_b_2, cabled to rp_a_1,rp_a_2 on the same device A, which fan out to two different interfaces if_a_1/if_a_2. far_end_parents == {('device', A)} (len 1) → not split → trace continues and terminates at [if_a_1, if_a_2], all same type → is_complete=True, is_split=False. Result: if_b.connected_endpoints == [if_a_1, if_a_2] (two endpoints presented as a complete link), while the reverse trace from either if_a_* returns a single peer if_b — an asymmetric, physically-invalid merge that the removed break prevented. This propagates to connected_endpoints_reachable, destinations, tables, panels, and GraphQL, none of which consult is_split.

2. netbox/dcim/models/cables.py ~1046-1051 — far-end terminations with no device_id/module_id all collapse to ('device', None), so divergent circuits/provider-networks are treated as convergent. (CONFIRMED)
CircuitTermination (and site/provider-network terminations) have neither device_id nor module_id, so ('module', t.module_id) if getattr(t,'module_id',None) else ('device', getattr(t,'device_id',None)) yields ('device', None) for every one. Topology: duplex fp_b → rp_b_1,rp_b_2, cabled to CircuitTerminations on two different circuits. far_end_parents == {('device', None)} (len 1) → not split → the trace merges two unrelated circuits into one path instead of flagging is_split=True.

3. netbox/dcim/models/cables.py ~1057-1058 — the convergent continuation is now reachable with an empty stack and pushes a flat rear_port_position list mixing positions from distinct rear ports. (PLAUSIBLE)
When the converged rear ports themselves have positions > 1, position_stack.append([mapping.rear_port_position for mapping in port_mappings]) pushes one flat list gathered across multiple different rear ports. The next hop applies it uniformly via rear_port_position__in=... to every rear port, so a rear port that legitimately uses only positions 1–2 also matches phantom positions 3–4 from a sibling rear port → extra front ports traced. Under the old code this path aborted as split, so the diff newly exposes this. (Not exercised by the added tests, whose rear ports are all positions=1.)

4. netbox/dcim/models/cables.py ~1046-1051 — the same parent-key expression produces a false split for two components on one device when one is module-mounted and the other is not. (CONFIRMED, low severity)
A module-installed component keys as ('module', module_id) while a directly-mounted one on the same device keys as ('device', device_id), so a genuinely convergent path (both far ends on device X) is flagged is_split=True. This is the conservative direction (matches pre-PR behavior, no new wrong output), but it shows the parent key should resolve to the owning device uniformly rather than device-or-module.

5. netbox/dcim/models/cables.py ~1043 — N+1 query: [m.rear_port for m in port_mappings] loads each RearPort with a separate query. (CONFIRMED)
PortMapping.objects.filter(front_port__in=...) has no select_related('rear_port'), so the new full iteration issues one query per mapping on a hot path run for every FrontPort hop. Add .select_related('rear_port') (also benefits the existing mapping.rear_port use at line 1056).

6. netbox/dcim/models/cables.py ~965-975 — the far-end lookup duplicates the legacy positionless branch at lines 992-1010. (cleanup)
The get_for_model → CableTermination.filter(termination_type, termination_id__in) → cable-end flip → filter(q_filter).prefetch_related('termination') sequence is a near-verbatim copy of lines 992-1010 (the far_end = 'A' if lct.cable_end == 'B' else 'B' block copies 998-1001). Extract a shared "far-end terminations for a set of local terminations" helper and reuse it in both places.

Unfortunately I don't have sufficient time to invest in this right now. You're welcome to continue working at it, but I'm not comfortable altering the tracing logic unless we can verify with sufficient confidence that it won't introduce unintended behavior.