From a3028b202973014aa3ac9229568d088515769ebe Mon Sep 17 00:00:00 2001 From: ApsisRS contributors Date: Fri, 2 Oct 2026 00:03:22 +0800 Subject: [PATCH] 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 | 2 +- README.zh.md | 2 +- crates/network/apsis-access/src/error.rs | 3 ++ crates/network/apsis-access/src/finder.rs | 38 +++++++++++++++++-- crates/network/apsis-access/tests/finder.rs | 20 ++++++++-- .../network/apsis-constellation/src/walker.rs | 11 +++++- .../network/apsis-routing/tests/dijkstra.rs | 6 ++- crates/ops/apsis-schedule/tests/schedule.rs | 7 +++- crates/orbit/apsis-sgp4/src/model.rs | 8 +++- 9 files changed, 82 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ebf54e4..a7c5a6a 100644 --- a/README.md +++ b/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 diff --git a/README.zh.md b/README.zh.md index aa70768..d329ba1 100644 --- a/README.zh.md +++ b/README.zh.md @@ -70,7 +70,7 @@ apsis-rs = { path = "...", features = ["orbit"] } - 外部输入的失败一律走 `Result` / `Option`，不用 `unwrap` / `expect` 转换 - 允许对**已守卫的内部不变量**用 `expect("…")` 断言不可能的分支（如刚 `push` 后的 `last()`、前置 `is_none()` 已检查后的取值）。这些断言不处理不可信 - 输入，而是把"若此处为空则是编码错误"写成可读的失败信息；全库共 12 处， + 输入，而是把"若此处为空则是编码错误"写成可读的失败信息；全库共 13 处， 逐处均有前置守卫。 - 公共 API 必须有文档注释，注释自包含 diff --git a/crates/network/apsis-access/src/error.rs b/crates/network/apsis-access/src/error.rs index 1049054..b8533ee 100644 --- a/crates/network/apsis-access/src/error.rs +++ b/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) diff --git a/crates/network/apsis-access/src/finder.rs b/crates/network/apsis-access/src/finder.rs index 25b172a..a2af430 100644 --- a/crates/network/apsis-access/src/finder.rs +++ b/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 { self.find_internal(samples, |seconds| { // Interpolate the position between the bracketing samples. The - // caller is responsible for the ordering and the step, which - // were checked before this closure is called. + // order of the sequence is checked by `find_internal` before + // 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}"))); } diff --git a/crates/network/apsis-access/tests/finder.rs b/crates/network/apsis-access/tests/finder.rs index d90ef5c..4e0eebe 100644 --- a/crates/network/apsis-access/tests/finder.rs +++ b/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"); - assert_eq!(error.kind(), AccessErrorKind::InvalidInterval); + let error = finder().find(&backwards).expect_err("time runs backwards"); + 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] diff --git a/crates/network/apsis-constellation/src/walker.rs b/crates/network/apsis-constellation/src/walker.rs index 2757714..c62f76b 100644 --- a/crates/network/apsis-constellation/src/walker.rs +++ b/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, - /// so that a caller can report the value it would use. + /// Returns `None` when the plane count is zero, which is the only case + /// 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 { (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 { if self.total_satellites == 0 { return None; diff --git a/crates/network/apsis-routing/tests/dijkstra.rs b/crates/network/apsis-routing/tests/dijkstra.rs index 8c7de2f..7708715 100644 --- a/crates/network/apsis-routing/tests/dijkstra.rs +++ b/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)); } diff --git a/crates/ops/apsis-schedule/tests/schedule.rs b/crates/ops/apsis-schedule/tests/schedule.rs index b8b858f..2cbb176 100644 --- a/crates/ops/apsis-schedule/tests/schedule.rs +++ b/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); } diff --git a/crates/orbit/apsis-sgp4/src/model.rs b/crates/orbit/apsis-sgp4/src/model.rs index 8cccc7a..6e65c7c 100644 --- a/crates/orbit/apsis-sgp4/src/model.rs +++ b/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 - /// correction. + /// Returns `2 / 3`, the exponent the model uses in the mean motion and + /// 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 }