fix: reject an unordered sample sequence in the access search

The search binary searches the sample list to interpolate between the two
samples that straddle a crossing, and a binary search over an unordered
list returns a wrong index instead of failing. Only the first and last
times were checked, so a sequence whose ends were in order but whose
middle was not refined each crossing against the wrong pair of positions
and reported a pass that never happened. On a circular ground track,
shuffling two samples changed the result from three passes to two and
still returned Ok.

The whole sequence is now checked, with a new UnorderedSamples error kind
that says which pair is out of order. The test covers the middle-of-
sequence case that the end check missed.

Two comments also described something other than what the code does, and
two Option-returning accessors did not say what makes them None:

- GravityModel::x2o3 is the constant 2/3, used as an exponent in the
  mean motion and drag corrections, not 2 * xke^2 as its comment claimed;
  the sgp4 reference implementation uses the same constant the same way.
- WalkerPattern::plane_spacing and phase_offset returned None without
  saying so, which is the case a caller has to handle.

Both READMEs claimed twelve expect sites where there are thirteen, because
apsis-math/src/parallel.rs added one and the count was not carried over.

README.md

@@ -72,7 +72,7 @@ that does not run simulations.
   input is always a `Result` or an `Option`. An `expect` is allowed on an
   internal invariant that a check just above already guards, such as the
   element just pushed onto a vector, or a value whose absence the line above
-  ruled out. There are twelve such sites and each is listed with its guard in
+  ruled out. There are thirteen such sites and each is listed with its guard in
   `docs/design/errors.md`.
 - Every public item carries a documentation comment, and the comment is
   self-contained

README.zh.md

@@ -70,7 +70,7 @@ apsis-rs = { path = "...", features = ["orbit"] }
   - 外部输入的失败一律走 `Result` / `Option`，不用 `unwrap` / `expect` 转换
   - 允许对**已守卫的内部不变量**用 `expect("…")` 断言不可能的分支（如刚 `push`
     后的 `last()`、前置 `is_none()` 已检查后的取值）。这些断言不处理不可信
-    输入，而是把"若此处为空则是编码错误"写成可读的失败信息；全库共 12 处，
+    输入，而是把"若此处为空则是编码错误"写成可读的失败信息；全库共 13 处，
     逐处均有前置守卫。
 - 公共 API 必须有文档注释，注释自包含
 

crates/network/apsis-access/src/error.rs

@@ -14,6 +14,8 @@ pub enum AccessErrorKind {
     InvalidTolerance,
     /// The search interval is empty or not ordered.
     InvalidInterval,
+    /// The sample sequence is not in increasing time order.
+    UnorderedSamples,
     /// A supplied satellite position is not usable.
     InvalidPosition,
 }
@@ -25,6 +27,7 @@ impl fmt::Display for AccessErrorKind {
             Self::InvalidSampleStep => "the sampling step is not usable",
             Self::InvalidTolerance => "the crossing tolerance is not usable",
             Self::InvalidInterval => "the search interval is not usable",
+            Self::UnorderedSamples => "the sample sequence is not ordered",
             Self::InvalidPosition => "a satellite position is not usable",
         };
         f.write_str(text)

crates/network/apsis-access/src/finder.rs

@@ -122,12 +122,16 @@ impl AccessFinder {
     /// rather than extrapolated beyond it.
     ///
     /// Returns an error when the settings are not usable, when the sample
-    /// sequence has fewer than two entries, or when a sample is not usable.
+    /// sequence has fewer than two entries, when a sample is not usable, or
+    /// when the sequence is not in increasing time order. The order is
+    /// checked rather than assumed, because the interpolation between two
+    /// samples binary searches the sequence and a binary search over an
+    /// unordered list returns a wrong index without failing.
     pub fn find(&self, samples: &[OrbitSample]) -> Result<PassSummary, AccessError> {
         self.find_internal(samples, |seconds| {
             // Interpolate the position between the bracketing samples. The
-            // caller is responsible for the ordering and the step, which
+            // order of the sequence is checked by `find_internal` before
-            // were checked before this closure is called.
+            // this closure runs, and the step is the caller's to choose.
             let index = match samples.binary_search_by(|sample| {
                 sample
                     .seconds
@@ -172,9 +176,35 @@ impl AccessFinder {
                 .with_detail("at least two samples are needed"));
         }
 
+        // The whole sequence must be in strictly increasing time, not just its
+        // ends. The interpolation between two samples binary searches this
+        // list, and a binary search over an unordered list returns a wrong
+        // index rather than failing, so an unordered sequence would refine a
+        // crossing against the wrong pair of positions and report a pass that
+        // never happened. Checking only the ends, as this once did, let that
+        // through silently.
+        for (index, pair) in samples.windows(2).enumerate() {
+            // The test is written out rather than as `!(a < b)` so that it
+            // also rejects a pair whose times are not finite, and so that a
+            // reader can see that equality and disorder are both refused.
+            let ordered = pair[0].seconds.is_finite()
+                && pair[1].seconds.is_finite()
+                && pair[0].seconds < pair[1].seconds;
+            if !ordered {
+                return Err(
+                    AccessError::new(AccessErrorKind::UnorderedSamples).with_detail(format!(
+                        "sample {} at {} s does not precede {} s",
+                        index + 1,
+                        pair[0].seconds,
+                        pair[1].seconds
+                    )),
+                );
+            }
+        }
+
         let first = samples[0].seconds;
         let last = samples[samples.len() - 1].seconds;
-        if !first.is_finite() || !last.is_finite() || last <= first {
+        if !first.is_finite() || !last.is_finite() {
             return Err(AccessError::new(AccessErrorKind::InvalidInterval)
                 .with_detail(format!("times run from {first} to {last}")));
         }

crates/network/apsis-access/tests/finder.rs

@@ -380,12 +380,26 @@ fn too_few_samples_are_rejected() {
 fn an_unordered_sample_sequence_is_rejected() {
     let samples = reference_samples(1.0, 1_000.0, 0.0);
     assert!(samples.is_empty());
-    let unordered = [
+    // The ends run backwards.
+    let backwards = [
         OrbitSample::new(100.0, reference_position(100.0)),
         OrbitSample::new(50.0, reference_position(50.0)),
     ];
-    let error = finder().find(&unordered).expect_err("time runs backwards");
+    let error = finder().find(&backwards).expect_err("time runs backwards");
-    assert_eq!(error.kind(), AccessErrorKind::InvalidInterval);
+    assert_eq!(error.kind(), AccessErrorKind::UnorderedSamples);
+
+    // The ends are in order but the middle is not. This is the case that a
+    // check of the ends alone misses: the interpolation binary searches the
+    // list, and a binary search over an unordered list returns a wrong index
+    // without failing, so the search would refine a crossing against the
+    // wrong pair of positions and report a pass that never happened.
+    let mut shuffled = reference_samples(60.0, 0.0, 1_200.0);
+    assert!(shuffled.len() > 6, "the reference track is long enough");
+    shuffled.swap(2, 4);
+    let error = finder()
+        .find(&shuffled)
+        .expect_err("the middle is out of order");
+    assert_eq!(error.kind(), AccessErrorKind::UnorderedSamples);
 }
 
 #[test]

crates/network/apsis-constellation/src/walker.rs

@@ -44,8 +44,12 @@ impl WalkerPattern {
     /// Returns the spacing between adjacent planes in right ascension, in
     /// radians.
     ///
-    /// Returns the spacing regardless of whether the pattern is consistent,
+    /// Returns `None` when the plane count is zero, which is the only case
-    /// so that a caller can report the value it would use.
+    /// in which the spacing is undefined.
+    ///
+    /// The spacing does not depend on the pattern being consistent, so a
+    /// caller can report the value it would use even for a pattern that
+    /// cannot be instantiated.
     pub fn plane_spacing(self) -> Option<f64> {
         (self.planes > 0).then(|| 2.0 * PI / self.planes as f64)
     }
@@ -54,6 +58,9 @@ impl WalkerPattern {
     ///
     /// This is `f * 360 / t` degrees, where `f` is the phase factor and `t`
     /// the total satellite count.
+    ///
+    /// Returns `None` when the satellite count is zero, which is the only
+    /// case in which the offset is undefined.
     pub fn phase_offset(self) -> Option<f64> {
         if self.total_satellites == 0 {
             return None;

crates/network/apsis-routing/tests/dijkstra.rs

@@ -305,9 +305,13 @@ fn a_route_over_a_series_validates_its_arguments() {
 
 #[test]
 fn the_path_tree_type_is_public() {
-    // The type must be nameable by a caller that stores a routing table.
+    // The type must be nameable by a caller that stores a routing table,
+    // and what it hands back must be the tree for the source asked for.
     fn takes_tree(_tree: &PathTree) {}
     let topology = topology_with(2, &[(0, 1)]);
     let tree = shortest_paths_from(&topology, 0).expect("valid");
     takes_tree(&tree);
+    assert_eq!(tree.source, 0);
+    assert_eq!(tree.distance(0), Some(0.0));
+    assert_eq!(tree.distance(1), Some(1.0));
 }

crates/ops/apsis-schedule/tests/schedule.rs

@@ -480,10 +480,15 @@ fn an_invalid_placement_is_reported() {
 
 #[test]
 fn the_evaluation_type_is_nameable() {
-    // The type must be usable by a caller that stores the results.
+    // The type must be usable by a caller that stores the results, and an
+    // empty schedule against one resource must report itself as empty.
     fn takes(_evaluation: &Evaluation) {}
     let resources = ResourceSet::from_resources(vec![resource(0, 1.0)]).expect("one");
     let schedule = Schedule::new();
     let evaluation = schedule.evaluate(&[], &resources);
     takes(&evaluation);
+    assert_eq!(evaluation.placed_count, 0);
+    assert_eq!(evaluation.offered_count, 0);
+    assert_eq!(evaluation.scheduled_seconds, 0.0);
+    assert_eq!(evaluation.resource_usage.len(), 1);
 }

crates/orbit/apsis-sgp4/src/model.rs

@@ -100,8 +100,12 @@ impl GravityModel {
         self.j3 / self.j2
     }
 
-    /// Returns `2 * xke^2`, a term that appears in the mean motion
+    /// Returns `2 / 3`, the exponent the model uses in the mean motion and
-    /// correction.
+    /// drag corrections.
+    ///
+    /// It appears as a power rather than a factor, in terms such as
+    /// `(xke / mean_motion)^(2/3)`, so it is a constant and does not depend
+    /// on this model.
     pub fn x2o3(self) -> f64 {
         2.0 / 3.0
     }