fix(sensing): finish #611 NaN-panic audit — 7 sites missed by #613#624
Open
therahul-yo wants to merge 1 commit into
Open
fix(sensing): finish #611 NaN-panic audit — 7 sites missed by #613#624therahul-yo wants to merge 1 commit into
therahul-yo wants to merge 1 commit into
Conversation
…uvnet#613 ruvnet#613 fixed adaptive_classifier.rs:94 (the IQR sort) and called the audit done, but the grep used `partial_cmp(b).unwrap()` as a literal and missed seven additional production sites that use comparator variants: adaptive_classifier.rs:205 AdaptiveModel::classify() argmax over softmax probs — same per-frame hot path as ruvnet#611. NaN flows through normalise → logits → softmax and still reaches this site even after the IQR fix. adaptive_classifier.rs:480 train() argmax (training accuracy loop) adaptive_classifier.rs:500 train() per-class argmax main.rs:2446, 2449 count_persons_mincut variance source/sink select csi.rs:602, 605 count_persons_mincut variance source/sink select (duplicate of main.rs logic in csi.rs) For the variance-select sites, note that the *outer* `unwrap_or((0, &0))` only catches an empty iterator — it cannot rescue a panic raised inside the comparator. A single NaN in `variances[]` still aborts the process. Same fix as ruvnet#613: swap `.unwrap()` for `.unwrap_or(std::cmp::Ordering::Equal)` inside the comparator closure. Pure behavioural change, no API surface. Re-audit of the remaining `partial_cmp(...).unwrap()` matches in v2/: they are all inside `#[cfg(test)]` / `#[test]` blocks (spectrogram.rs:269, depth.rs:234, connectivity.rs:477, vital_signs.rs:737) where inputs are controlled and panic-on-NaN is acceptable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #613 (which closed #611). #613 fixed
adaptive_classifier.rs:94and explicitly stated the audit was complete. That audit grepped for the literal patternpartial_cmp(b).unwrap()and missed seven additional production sites that use comparator variants — all share the same crash class as #611 (NaN inf64data →partial_cmpreturnsNone→unwrap()panics the sensing server).Sites fixed
v2/.../adaptive_classifier.rsAdaptiveModel::classify()argmax over softmax probsraw_featuressurvives normalise (NaN / x = NaN) → logits → softmax (exp(NaN)/exp(NaN) = NaN), and the #613 IQR fix does not gate this branch.v2/.../adaptive_classifier.rstrain_from_recordings()argmaxv2/.../main.rscount_persons_mincutvariance source/sink select.unwrap_or((0, &0))only catches an empty iterator — it cannot rescue a panic raised inside the comparator. A single NaN invariances[]aborts the process.v2/.../csi.rscsi.rsFix
Drop-in: swap
.unwrap()for.unwrap_or(std::cmp::Ordering::Equal)inside the comparator closure. This is the same one-line behavioural change as #613 and matches the pattern the same files already use elsewhere (e.g.adaptive_classifier.rs:149-150, 155, and now:94from #613).No API change, no test changes needed — the existing fix(sensing) commit precedent in CHANGELOG describes this as a behavioural fix only.
Re-audit of remaining
partial_cmp(...).unwrap()sitesAfter this PR, the only remaining
partial_cmp(…).unwrap()matches inv2/are inside#[cfg(test)]/#[test]blocks where inputs are controlled and panic-on-NaN is acceptable:wifi-densepose-signal/src/spectrogram.rs:269—#[test] fn test_single_frequency_peakwifi-densepose-pointcloud/src/depth.rs:234— backproject testruv-neural/ruv-neural-signal/src/connectivity.rs:477— coherence testwifi-densepose-sensing-server/src/vital_signs.rs:737—#[test] fn test_fft_peak(#613's PR body listed the first three explicitly as benign; this PR confirms
vital_signs.rs:737is the same category.)Test plan
cd v2 && cargo check -p wifi-densepose-sensing-server --no-default-features(no Rust toolchain available in my environment — relying on CI).cd v2 && cargo test --workspace --no-default-features— 1,031+ passed expected, behavioural change should not affect any test that doesn't already inject NaN.partial_cmp(...).unwrap()after merge — only test/bench sites should remain.Closes the audit gap from #611 / #613. No related open issue — filing this directly since the gap was caught by following the same grep methodology the original audit described.