selection: test deduplication precludes over-delivery

Summary:
this test captures a key structural guarantee of the routing system: that overdelivery is prevented not by explicit guards, but by the semantics of the traversal combined with frame deduplication.

the comment explains that when multiple delivery frames reach the same destination, they must have the same routing lineage and state — making them structurally equivalent. as a result, deduplication ensures only one of them is retained.

by disabling deduplication, the test demonstrates that overdelivery becomes possible, highlighting the value of this mechanism.

i believe this is helpful both as regression protection and as executable documentation of a subtle invariant.

Reviewed By: moonli

Differential Revision: D75027898

fbshipit-source-id: 2a9eed9056e03e6c7d2c3c99db6344def4d9a0dc

Author: shayne-fletcher

ndslice/src/selection/routing.rs

@@ -945,6 +945,7 @@ mod tests {
     use crate::selection::dsl::*;
     use crate::selection::test_utils::RoutedMessage;
     use crate::selection::test_utils::collect_routed_nodes;
+    use crate::selection::test_utils::collect_routed_paths;
     use crate::shape;
 
     // A test slice: (zones = 2, hosts = 4, gpus = 8).
@@ -1621,4 +1622,78 @@ mod tests {
         // Inner selection should still be All(All(True))
         assert!(matches!(hop.selection, Selection::All(_)));
     }
+
+    // This test relies on a deep structural property of the routing
+    // semantics:
+    //
+    //   Overdelivery is prevented not by ad hoc guards, but by the
+    //   structure of the traversal itself — particularly in the
+    //   presence of routing frame deduplication.
+    //
+    // When a frame reaches the final dimension with `selection ==
+    // True`, it becomes a delivery frame. If multiple such frames
+    // target the same coordinate, then:
+    //
+    //   - They must share the same coordinate `here`
+    //   - They must have reached it via the same routing path (by the
+    //     Unique Path Theorem)
+    //   - Their `RoutingFrame` state is thus structurally identical:
+    //       - Same `here`
+    //       - Same `dim` (equal to `slice.num_dim()`)
+    //       - Same residual `selection == True`
+    //
+    // The deduplication logic (via `RoutingFrameKey`) collapses such
+    // structurally equivalent frames. As a result, only one frame
+    // delivers to the target coordinate, and overdelivery is
+    // structurally ruled out.
+    //
+    // This test verifies that behavior holds as expected — and, when
+    // deduplication is disabled, confirms that overdelivery becomes
+    // observable.
+    #[test]
+    fn test_routing_deduplication_precludes_overdelivery() {
+        // Ensure the environment is clean — this test depends on a
+        // known configuration of deduplication behavior.
+        let var = "HYPERACTOR_SELECTION_DISABLE_ROUTING_FRAME_DEDUPLICATION";
+        assert!(
+            std::env::var_os(var).is_none(),
+            "env var `{}` should not be set prior to test",
+            var
+        );
+        let slice = test_slice();
+
+        // Construct a structurally duplicated selection.
+        //
+        // The union duplicates a singleton selection expression.
+        // Without deduplication, this would result in two logically
+        // identical frames targeting the same node — which should
+        // trigger an over-delivery panic in the simulation.
+        let a = range(0, range(0, range(0, true_())));
+        let sel = union(a.clone(), a.clone());
+
+        // Sanity check: with deduplication enabled (default), this
+        // selection does not cause overdelivery.
+        let result = std::panic::catch_unwind(|| {
+            let _ = collect_routed_paths(&sel, &slice);
+        });
+        assert!(result.is_ok(), "Unexpected panic due to overdelivery");
+
+        // Now explicitly disable deduplication.
+        std::env::set_var(var, "1");
+
+        // Expect overdelivery: the duplicated union arms will each
+        // produce a delivery to the same coordinate.
+        let result = std::panic::catch_unwind(|| {
+            let _ = collect_routed_paths(&sel, &slice);
+        });
+
+        // Clean up: restore environment to avoid affecting other
+        // tests.
+        std::env::remove_var(var);
+
+        assert!(
+            result.is_err(),
+            "Expected panic due to overdelivery, but no panic occurred"
+        );
+    }
 }

ndslice/src/selection/test_utils.rs

@@ -55,6 +55,29 @@ macro_rules! assert_round_trip {
     }};
 }
 
+/// Determines whether routing frame deduplication is enabled.
+///
+/// By default, deduplication is enabled to reduce redundant routing
+/// steps and improve performance. However, correctness must not
+/// depend on deduplication.
+///
+/// This behavior can be disabled for debugging or testing purposes by
+/// setting the environment variable:
+/// ```ignore
+/// HYPERACTOR_SELECTION_DISABLE_ROUTING_FRAME_DEDUPLICATION = 1
+/// ```
+/// When disabled, all routing steps—including structurally redundant
+/// ones—will be visited, potentially causing re-entry into previously
+/// seen coordinates. This switch helps validate that correctness
+/// derives from the routing algebra itself—not from memoization or
+/// key-based filtering.
+fn allow_frame_dedup() -> bool {
+    // Default: true (deduplication via memoization and normalization
+    // is enabled unless explicitly disabled).
+    std::env::var("HYPERACTOR_SELECTION_DISABLE_ROUTING_FRAME_DEDUPLICATION")
+        .map_or(true, |val| val != "1")
+}
+
 // == Testing (`collect_routed_paths` mesh simulation) ===
 
 /// Message type used in the `collect_routed_paths` mesh routing
@@ -127,7 +150,12 @@ pub fn collect_routed_paths(selection: &Selection, slice: &Slice) -> RoutedPathT
         let mut visitor = |step: RoutingStep| {
             if let RoutingStep::Forward(next_frame) = step {
                 let key = RoutingFrameKey::new(&next_frame);
-                if seen.insert(key) {
+                let should_insert = if allow_frame_dedup() {
+                    seen.insert(key) // true → not seen before
+                } else {
+                    true // unconditionally insert
+                };
+                if should_insert {
                     let next_rank = slice.location(&next_frame.here).unwrap();
                     let parent_rank = *path.last().unwrap();
                     predecessors
@@ -140,7 +168,12 @@ pub fn collect_routed_paths(selection: &Selection, slice: &Slice) -> RoutedPathT
 
                     match next_frame.action() {
                         RoutingAction::Deliver => {
-                            delivered.insert(next_rank, next_path);
+                            if let Some(previous) = delivered.insert(next_rank, next_path.clone()) {
+                                panic!(
+                                    "over-delivery detected: node {} delivered twice\nfirst: {:?}\nsecond: {:?}",
+                                    next_rank, previous, next_path
+                                );
+                            }
                         }
                         RoutingAction::Forward => {
                             pending.push_back(RoutedMessage::new(next_path, next_frame));