Skip to content

Fix bugs in TrkReco: break→continue, division by zero, bounds checks, shadowing, dead code#1785

Merged
oksuzian merged 2 commits intomainfrom
copilot/review-issues-in-trkreco
Apr 8, 2026
Merged

Fix bugs in TrkReco: break→continue, division by zero, bounds checks, shadowing, dead code#1785
oksuzian merged 2 commits intomainfrom
copilot/review-issues-in-trkreco

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 7, 2026

Summary

Fixes critical and high-severity bugs in TrkReco identified during code review.

Critical Fixes

breakcontinue in track matching loops

Files: SelectReflections_module.cc:98, SelectSameTrack_module.cc:116

Both modules had break instead of continue when no intersection was found for a track. The comment says "skip testing for a match with this track", but break terminates the entire outer loop over all tracks, silently skipping all remaining candidates. Changed to continue to correctly advance to the next track.

Missing histogram bounds check in initFZ_2()

File: RobustHelixFit.cc:580

The histogram hist[20] fill loop computed bin = (x-minX)/stepX without bounds checking. The physics range [_mindfdz, _maxdfdz] is user-configurable via fcl but the histogram parameters are hardcoded, creating a fragile coupling that could lead to out-of-bounds writes. Added if(bin >= 0 && bin < int(nbinsX)) guard.

High-Severity Fixes

Division by zero guards

  • SimpleKalSeedSelector_tool.cc:32: When both tracks have zero active hits, ntest + ncurrent == 0 produces NaN. Added early fallback to fit consistency comparison.
  • SelectSameTrack_module.cc:147: When prihits is empty, nover/prihits.size() divides by zero. Added if(prihits.empty()) continue; guard before the overlap computation.
  • TrkUtilities.cc:50: overlap(SHIV) divides by min(size1, size2) which is 0 when either vector is empty. Added early return of 0.0.

Medium-Severity Fixes

  • TrackMatching_module.cc:201: Renamed inner ntrks to ntrks_handle to avoid shadowing the outer ntrks used for validation.
  • SelectReflections_module.cc:133, SelectSameTrack_module.cc:165: Changed numeric_limits<float>::max() to numeric_limits<double>::max() to match the double variable type.
  • RobustHelixFinderData.cc:66-70: Removed duplicate assignments of _nStrawHits, _nComboHits, _nFiltComboHits, _nFiltStrawHits in clearTempVariables().

Files Changed

  • TrkReco/src/SelectReflections_module.cc
  • TrkReco/src/SelectSameTrack_module.cc
  • TrkReco/src/SimpleKalSeedSelector_tool.cc
  • TrkReco/src/TrkUtilities.cc
  • TrkReco/src/TrackMatching_module.cc
  • TrkReco/src/RobustHelixFit.cc
  • TrkReco/src/RobustHelixFinderData.cc

Copilot AI and others added 2 commits April 7, 2026 13:48
@FNALbuild
Copy link
Copy Markdown
Collaborator

Hi @Copilot,
You have proposed changes to files in these packages:

  • TrkReco

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

📝 The author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

Copy link
Copy Markdown
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look correct or innocuous.

@oksuzian oksuzian marked this pull request as ready for review April 7, 2026 16:12
@oksuzian
Copy link
Copy Markdown
Collaborator

oksuzian commented Apr 7, 2026

@FNALbuild run build test

@FNALbuild
Copy link
Copy Markdown
Collaborator

⌛ The following tests have been triggered for 8fd3bcf: build (Build queue - API unavailable)

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 8fd3bcf.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 8fd3bcf at e229837
build (prof) Log file. Build time: 04 min 08 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO ➡️ TODO (0) FIXME (4) in 7 files
clang-tidy ➡️ 9 errors 239 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 8fd3bcf after being merged into the base branch at e229837.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

@oksuzian oksuzian removed their request for review April 7, 2026 20:50
@oksuzian oksuzian merged commit 5ec09f9 into main Apr 8, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants