Cover max_path_cost_factor, salvaged from an abandoned branch

CMakeLists.txt

@@ -230,7 +230,13 @@ if(NETGRAPH_CORE_SANITIZE)
   if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")
     # Must be a CMake list: a single quoted string reaches the compiler as one
     # argument and clang rejects "-fsanitize=address,undefined -fno-omit-frame-pointer".
-    set(SAN_FLAGS -fsanitize=address,undefined -fno-omit-frame-pointer)
+    #
+    # -fno-sanitize-recover=undefined makes UBSan findings FATAL. Without it UBSan
+    # prints a "runtime error:" diagnostic and lets the program continue, so the test
+    # still exits 0 and ctest reports success -- undefined behaviour would be detected
+    # and then silently ignored, which is indistinguishable from not checking at all.
+    set(SAN_FLAGS -fsanitize=address,undefined -fno-omit-frame-pointer
+                  -fno-sanitize-recover=undefined)
     set(SAN_TARGETS netgraph_core _netgraph_core)
     if(TARGET netgraph_core_tests)
       # The test executable links the sanitized static library, so it must be

tests/cpp/flow_policy_tests.cpp

@@ -434,3 +434,74 @@ TEST(FlowPolicyStatic, ValidationRejectsForeignAndDisconnectedBundles) {
     EXPECT_THROW(policy.set_static_paths(0, 2, std::move(b)), std::invalid_argument);
   }
 }
+
+// The max_path_cost_factor gate multiplies best_path_cost_ by the factor and casts
+// the product back to Cost. best_path_cost_ is only updated when dst is reachable
+// (flow_policy.cpp:135), so an UNREACHABLE destination is the one way to reach the
+// gate while it still holds the INT64_MAX "no best path yet" sentinel. Before 0.8.0
+// the multiply-and-cast ran unconditionally there, and INT64_MAX * factor is far
+// outside the int64 range -- undefined behavior.
+//
+// No assertion can distinguish fixed from unfixed here: both return "no path" for an
+// unreachable destination. The check is the UB itself, so this test exists to give
+// UBSan something to instrument. It only has teeth under `make sanitize-test`, which
+// runs ctest -- which is why this lives in C++ and not in the Python suite.
+TEST(FlowPolicyCostBounds, MaxPathCostFactor_UnreachableDst_KeepsSentinelOutOfTheCast) {
+  // 0 -> 1, plus an isolated node 2 that cannot be reached from 0.
+  std::vector<std::int32_t> src{0}, dst{1};
+  std::vector<double> cap{1.0};
+  std::vector<std::int64_t> cost{7};
+  auto g = StrictMultiDiGraph::from_arrays(3, src, dst, cap, cost);
+  FlowGraph fg(g);
+
+  EdgeSelection sel; sel.multi_edge = true; sel.require_capacity = true;
+  sel.tie_break = EdgeTieBreak::Deterministic;
+  auto be = make_cpu_backend(); auto algs = std::make_shared<Algorithms>(be);
+  auto gh = algs->build_graph(g);
+  ExecutionContext ctx(algs, gh);
+
+  FlowPolicyConfig cfg;
+  cfg.flow_placement = FlowPlacement::Proportional;
+  cfg.selection = sel;
+  cfg.require_capacity = true;
+  cfg.max_flow_count = 1;
+  cfg.min_flow_count = 1;  // seeding is what calls get_path_bundle
+  cfg.max_path_cost_factor = 2.0;
+  FlowPolicy policy(ctx, cfg);
+
+  // Reaches the gate with best_path_cost_ == INT64_MAX. Nothing can be placed.
+  auto res = policy.place_demand(fg, /*src=*/0, /*dst=*/2, /*flowClass=*/0, /*volume=*/1.0);
+  EXPECT_NEAR(res.first, 0.0, 1e-9);
+  EXPECT_NEAR(res.second, 1.0, 1e-9);
+}
+
+// A very large (but legal) best cost times a large factor overflows the int64 range
+// too. The bound must be dropped rather than wrapped: a wrapped negative bound would
+// reject the shortest path itself, so a successful placement is what separates the
+// guard from the bug. Unlike the sentinel case above, this one IS assertable.
+TEST(FlowPolicyCostBounds, MaxPathCostFactor_HugeProduct_DropsBoundInsteadOfWrapping) {
+  const std::int64_t big = std::int64_t{1} << 60;  // total stays below the 2^62 ceiling
+  std::vector<std::int32_t> src{0, 1}, dst{1, 2};
+  std::vector<double> cap{1.0, 1.0};
+  std::vector<std::int64_t> cost{big, big};
+  auto g = StrictMultiDiGraph::from_arrays(3, src, dst, cap, cost);
+  FlowGraph fg(g);
+
+  EdgeSelection sel; sel.multi_edge = true; sel.require_capacity = true;
+  sel.tie_break = EdgeTieBreak::Deterministic;
+  auto be = make_cpu_backend(); auto algs = std::make_shared<Algorithms>(be);
+  auto gh = algs->build_graph(g);
+  ExecutionContext ctx(algs, gh);
+
+  FlowPolicyConfig cfg;
+  cfg.flow_placement = FlowPlacement::Proportional;
+  cfg.selection = sel;
+  cfg.require_capacity = true;
+  cfg.max_flow_count = 1;
+  cfg.min_flow_count = 1;
+  cfg.max_path_cost_factor = 1e9;
+  FlowPolicy policy(ctx, cfg);
+
+  auto res = policy.place_demand(fg, /*src=*/0, /*dst=*/2, /*flowClass=*/0, /*volume=*/1.0);
+  EXPECT_NEAR(res.first, 1.0, 1e-9);
+}

tests/py/test_review_regressions.py

@@ -506,3 +506,85 @@ def test_flow_index_constructor_matches_the_binding(self):
             ngc.FlowIndex(src=1, dst=2, flowClass=3, flowId=4)  # positional-only
         with pytest.raises(AttributeError):
             idx.src = 9  # read-only, as the stub's properties declare
+
+
+class TestFlowPolicyMaxPathCostFactor:
+    """`max_path_cost_factor` had no test coverage at all.
+
+    The gate multiplies the best path cost by the factor and casts back to Cost.
+    Before 0.8.0 it did that unconditionally, including while `best_path_cost_` was
+    still the INT64_MAX "no best path yet" sentinel -- the product is then far outside
+    the int64 range and the conversion is undefined behavior. The fix skips the
+    relative bound until a best cost exists and drops it when the product would not
+    fit. Neither branch was exercised by any test.
+
+    Scope: these are *coverage* tests, not detectors of the original UB. They pass
+    against 0.7.2 (pre-fix) too, because an out-of-range float->int conversion is
+    undefined rather than reliably wrong -- in practice it saturates to something that
+    still admits the path. They also cannot reach the sentinel branch at all: a
+    reachable destination updates `best_path_cost_` (flow_policy.cpp:135) before the
+    gate runs. Their value is guarding the *functional* behaviour of the bound.
+
+    Detection of the UB itself lives in C++, in
+    `FlowPolicyCostBounds.MaxPathCostFactor_*` -- `make sanitize-test` builds with
+    sanitizers but runs `ctest`, so it never executes this Python suite. Those tests
+    use an unreachable destination to keep the sentinel live, and fail against the
+    pre-fix conversion.
+    """
+
+    @staticmethod
+    def _policy(algs, gh, *, max_flow_count=1, **cfg_kwargs):
+        sel = ngc.EdgeSelection(
+            multi_edge=True,
+            require_capacity=True,
+            tie_break=ngc.EdgeTieBreak.DETERMINISTIC,
+        )
+        cfg = ngc.FlowPolicyConfig(
+            path_alg=ngc.PathAlg.SPF,
+            flow_placement=ngc.FlowPlacement.PROPORTIONAL,
+            selection=sel,
+            max_flow_count=max_flow_count,
+            **cfg_kwargs,
+        )
+        return ngc.FlowPolicy(algs, gh, cfg)
+
+    def test_factor_applies_before_any_best_cost_exists(self, algs):
+        """First placement: the sentinel path must not be multiplied and cast."""
+        g = _graph(3, [(0, 1, 1.0, 5), (1, 2, 1.0, 5)])
+        gh = algs.build_graph(g)
+        policy = self._policy(algs, gh, max_path_cost_factor=2.0)
+        placed, left = policy.place_demand(ngc.FlowGraph(g), 0, 2, 0, 1.0)
+        assert placed == pytest.approx(1.0)
+        assert left == pytest.approx(0.0)
+
+    def test_huge_factor_does_not_wrap_the_bound(self, algs):
+        """best_cost * factor overflows int64; the bound must be dropped, not wrapped.
+
+        A wrapped (negative) bound would reject the shortest path itself, so a
+        successful placement is what distinguishes the guard from the bug.
+        """
+        big = 1 << 60  # legal: total stays below the 2**62 construction ceiling
+        g = _graph(3, [(0, 1, 1.0, big), (1, 2, 1.0, big)])
+        gh = algs.build_graph(g)
+        policy = self._policy(algs, gh, max_path_cost_factor=1e9)
+        placed, _ = policy.place_demand(ngc.FlowGraph(g), 0, 2, 0, 1.0)
+        assert placed == pytest.approx(1.0)
+
+    def test_factor_still_excludes_a_too_expensive_alternative(self, algs):
+        """The bound must remain functional, not merely safe."""
+        g = _graph(
+            4,
+            [
+                (0, 1, 1.0, 1),
+                (1, 3, 1.0, 1),  # cost 2 route
+                (0, 2, 5.0, 50),
+                (2, 3, 5.0, 50),  # cost 100 route, far beyond 2x
+            ],
+        )
+        gh = algs.build_graph(g)
+        policy = self._policy(
+            algs, gh, max_flow_count=4, max_path_cost_factor=2.0, min_flow_count=1
+        )
+        placed, _ = policy.place_demand(ngc.FlowGraph(g), 0, 3, 0, 6.0)
+        # Only the cost-2 route is within 2x; the cost-100 route must stay unused.
+        assert placed == pytest.approx(1.0)