Phase 2 (prep): fix rare det-sim crash cause, collapse double hit-merge workaround - #299
Open
SFBayLaser wants to merge 2 commits into
Open
Phase 2 (prep): fix rare det-sim crash cause, collapse double hit-merge workaround#299SFBayLaser wants to merge 2 commits into
SFBayLaser wants to merge 2 commits into
Conversation
SFBayLaser
force-pushed
the
phase2-detsim-fixes
branch
from
August 12, 2026 18:52
443fb27 to
9339aa4
Compare
SFBayLaser
force-pushed
the
phase1-geometry-survey
branch
from
August 12, 2026 19:40
a625e40 to
16e1c6b
Compare
SFBayLaser
force-pushed
the
phase2-detsim-fixes
branch
from
August 12, 2026 19:40
9339aa4 to
94bd90d
Compare
SFBayLaser
force-pushed
the
phase2-detsim-fixes
branch
from
August 13, 2026 18:28
94bd90d to
96fcd21
Compare
…end() For every channel id seen for the first time in an event, the "new channel" branch set should_reset_times=true without ever validating it_dead (bound earlier via deadtime_map.find(id), which is a miss on a brand-new id). The subsequent it_dead->second read was therefore dereferencing std::map::end() unconditionally on every hit at a new (NotZ, Z) channel position -- confirmed as real UB via a standalone repro of the same access pattern under AddressSanitizer (stack-buffer-overflow / container-overflow). In practice the garbage value was usually discarded via the short-circuited deadtime_map.find(id) != deadtime_map.end() check immediately after, which is why this hasn't caused a hard, reliable crash -- its effect depends on heap/stack layout, compiler, and optimization level, matching the long-standing "rare crash" that TMS_Event.cpp's det-sim ordering has been worked around for since 2024 (see commits 93d6b65, 0fd14e1). Fix: only evaluate the zombie-redo branch when it_dead is actually valid (i.e. this channel id has been seen before); a brand-new channel has no prior zombie state to re-check against, matching the existing early-return logic for it_read. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Since 2024 (93d6b65, 0fd14e1), TMS_Event's constructor ran SimulateOpticalModel()/SimulateTimingModel()/MergeCoincidentHits() early (before any pileup overlay), duplicating part of the pipeline that ApplyReconstructionEffects() runs again later -- a workaround for an undiagnosed "rare crash/seg fault" per the original commit message. This session found and fixed a concrete, confirmed cause of that class of crash: SimulateDeadtime() dereferencing deadtime_map.end() (fixed in a5d520a, verified via a standalone ASan repro and run-validated against a real spill). With no crashing file currently available to test against directly, we're proceeding on the assumption that fix resolves the historical crash, and collapsing this workaround now rather than carrying it indefinitely. Changes: - TMS_Event's TG4Event constructor no longer runs any det-sim; it's back to just ProcessTG4Event(). - ApplyReconstructionEffects() runs the full pipeline in its original, pre-workaround order: SimulateOpticalModel() -> SimulateDarkCount() -> SimulateTimingModel() -> SimulateDeadtime() -> MergeCoincidentHits() -> SimulateReadoutNoise() -> SimulatePedestalSubtraction() -- exactly once, with a single merge, instead of split across two calls. - DrawEvents.cpp and CherryPickEvents.cpp construct a TMS_Event but never called FinalizeEvent()/ApplyReconstructionEffects() -- they relied entirely on the constructor's now-removed early calls for any hit merging at all. Added an explicit tms_event.FinalizeEvent() call to both so they keep getting a fully processed event (now the complete pipeline, not just the partial slice they got before). MergeCoincidentHits()'s grouping decision is anchor-time-relative and doesn't depend on prior partial merges, so this doesn't change which hits end up merged together (traced by hand). The one expected numeric difference is for overlaid/pileup events (ConvertToTMSTree.cpp's NerscOverlay path): optical/timing noise for piled-up hits now draws from the combined event's single RNG stream instead of each sub-event's own per-event seed -- a real but expected shift in noise realization, not a bug, analogous to the Phase 1 BarNumber change. The non-overlay path is unaffected (same object, same generator, same calls, just made later). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SFBayLaser
force-pushed
the
phase2-detsim-fixes
branch
from
August 13, 2026 18:47
96fcd21 to
dc8e09d
Compare
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.
Second step of the real-data restructuring proposal (see the companion Phase 1 PR #298 / issue #291; also discussed in #nd_muon_spectrometer_code). Preps
TMS_Event's detector-response pipeline for an eventual split into a sim-only stage and a real-or-simulated signal-processing stage. Heads-up issue: #292.Fixed:
SimulateDeadtime()was dereferencing an invalid map iterator (deadtime_map.end()) for every newly-seen readout channel -- confirmed genuine UB via an isolated ASan repro, and very likely the cause of a long-standing "rare crash" that's been worked around since 2024 via a duplicatedMergeCoincidentHits()call inTMS_Event's constructor.Changed: collapsed that workaround now that its underlying cause has a real fix -- the full detector-sim pipeline (
SimulateOpticalModel->SimulateDarkCount->SimulateTimingModel->SimulateDeadtime->MergeCoincidentHits->SimulateReadoutNoise->SimulatePedestalSubtraction) now runs once, in its original order, instead of split across two calls.DrawEvents/CherryPickEventsgained an explicitFinalizeEvent()call so they still get a fully-processed event.Verification: build clean; deadtime forced on (
Deadtime=500,ZombieTime=20) and run against a full sand_drift spill with no crash, exercising the previously-buggy branch thousands of times. ROOT-level comparison isolating just the merge-collapse (holding the deadtime fix constant) shows small, mixed-direction (~2%)nTracks/hit-count differences consistent with RNG-stream reordering on overlaid pileup events -- not a structural break.Closes #292.