test: reproduce Linux libstdc++ assertion crash in isolation_tree - #142
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe isolation forest now separates valid and missing feature values when initializing bounds and selecting splits. Regression coverage verifies that leading missing quality metrics do not prevent anomaly model execution or produce non-finite scores. ChangesAnomaly score handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/isolation_forest.cpp (1)
112-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor
is_missing_splitduring path scoring.
isolation_treeroutesNaNvalues to the left child whenis_missing_splitis set, butpath_lengthignores this and uses onlyit->second < node->value. SinceNaNcomparisons are false, scoring traverses the right child instead. Updatepath_lengthto routeNaNvalues left and non-missing values right before applying the numeric comparison.Proposed fix
- if (it->second < node->value) { + if (node->is_missing_split) { + if (std::isnan(it->second)) { + return path_length(node->left.get(), obs, depth + 1); + } + return path_length(node->right.get(), obs, depth + 1); + } + if (it->second < node->value) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/isolation_forest.cpp` around lines 112 - 117, Update path_length to honor is_missing_split when routing split values: detect NaN inputs and traverse the left child, while non-missing inputs traverse the right child before applying the existing numeric comparison. Keep the current comparison-based routing for ordinary numeric splits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@R/utils_anomaly_score.R`:
- Line 214: In .runAnomalyModel, register cleanup immediately after
parallel::makeCluster(cores, outfile = "") by adding an on.exit handler that
calls parallel::stopCluster(cl) with add = TRUE, ensuring the cluster is stopped
on normal returns and errors.
In `@src/isolation_forest.cpp`:
- Around line 108-112: Update the feature-selection and partition logic around
has_valid and is_missing_split so a selected feature with !has_valid is never
used for a non-partitioning split. Select another feature that can produce two
non-empty children, or terminate by returning a leaf when none exists; ensure
recursion never proceeds with an unchanged dataset and an empty right_data.
---
Outside diff comments:
In `@src/isolation_forest.cpp`:
- Around line 112-117: Update path_length to honor is_missing_split when routing
split values: detect NaN inputs and traverse the left child, while non-missing
inputs traverse the right child before applying the existing numeric comparison.
Keep the current comparison-based routing for ordinary numeric splits.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e2f8937-fb6b-43d5-9696-25883d250667
📒 Files selected for processing (3)
.github/workflows/dry-run-build.ymlR/utils_anomaly_score.Rsrc/isolation_forest.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/dry-run-build.yml
| # native C++ assertion/abort messages) to the master process instead of | ||
| # discarding it, to confirm the exact crash text on Linux CI. Revert | ||
| # once confirmed. | ||
| cl = parallel::makeCluster(cores, outfile = "") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching utils_anomaly_score.R:\n'
fd -a 'utils_anomaly_score\.R$' . || true
if [ -f R/utils_anomaly_score.R ]; then
printf '\nRelevant source section:\n'
sed -n '180,255p' R/utils_anomaly_score.R | nl -ba -v180
fi
printf '\nSearch for stopCluster and .runAnomalyModel:\n'
rg -n "stopCluster|makeCluster|\.runAnomalyModel|parLapply|clusterExport" -S .Repository: Vitek-Lab/MSstatsConvert
Length of output: 369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant source section R/utils_anomaly_score.R:\n'
awk 'NR>=180 && NR<=255 { printf "%5d\t%s\n", NR, $0 }' R/utils_anomaly_score.R
printf '\nSearch for stopCluster and parallel/cluster calls:\n'
rg -n "stopCluster|makeCluster|\.runAnomalyModel|parLapply\(|clusterExport\(" -S .
printf '\nOn.exit occurrences in target file:\n'
rg -n "on\.exit|tryCatch|try\(" R/utils_anomaly_score.R || trueRepository: Vitek-Lab/MSstatsConvert
Length of output: 3744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Continue relevant source section R/utils_anomaly_score.R:\n'
awk 'NR>=255 && NR<=320 { printf "%5d\t%s\n", NR, $0 }' R/utils_anomaly_score.R
printf '\nCheck whether .runAnomalyModel has any later return path or cleanup:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('R/utils_anomaly_score.R')
text = p.read_text().splitlines()
for i, line in enumerate(text, start=1):
if 'clusterExport' in line or 'parLapply' in line or 'makeCluster' in line or 'stopCluster' in line or line.strip().startswith('on.exit'):
print(f"{i}: {line.strip()}")
PYRepository: Vitek-Lab/MSstatsConvert
Length of output: 578
Stop the parallel cluster before returning.
.runAnomalyModel creates the cluster but never calls parallel::stopCluster(cl); add on.exit(parallel::stopCluster(cl), add = TRUE) after makeCluster(). This stops workers on both normal return and error paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/utils_anomaly_score.R` at line 214, In .runAnomalyModel, register cleanup
immediately after parallel::makeCluster(cores, outfile = "") by adding an
on.exit handler that calls parallel::stopCluster(cl) with add = TRUE, ensuring
the cluster is stopped on normal returns and errors.
0d878f7 to
1db738a
Compare
2 participants
Motivation and context
A Linux
libstdc++assertion can occur inisolation_treewhen anomaly-model input contains leadingNAorNaNquality metrics. Missing values can corrupt feature bounds during split selection.This change initializes numeric bounds from valid values and handles missing-only features explicitly. It also adds a regression test to confirm that anomaly modeling completes and produces finite scores.
Changes
isolation_tree.Unit tests
NAandNaNquality metrics.Coding guidelines