Fix combiner crash#4322
Merged
pinkenburg merged 3 commits intoJun 27, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughFun4AllServer::End now skips EndRun when RUNNUMBER is unset. SingleTriggeredInput now returns early for input files with no data events and adds a verbose deque-size message in ReadEvent. Two Makefile link lists also drop boost_filesystem. ChangesFun4All end-of-run guard
SingleTriggeredInput input handling
Link dependency cleanup
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29778abb-c420-49eb-aa61-abbd9d4ac5b8
📒 Files selected for processing (2)
offline/framework/fun4all/Fun4AllServer.ccoffline/framework/fun4allraw/SingleTriggeredInput.cc
Comment on lines
+1108
to
+1110
| if (rc->FlagExist("RUNNUMBER")) | ||
| { | ||
| EndRun(rc->get_IntFlag("RUNNUMBER")); // call SubsysReco EndRun methods for current run |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an int-specific RUNNUMBER check here.
PHFlag::FlagExist() is type-agnostic, so this branch is still unsafe: it returns true if RUNNUMBER exists as a string/float/etc., and get_IntFlag("RUNNUMBER") then emits the same error/stack trace and falls back to 0. That means the new guard does not fully satisfy the PHFlag contract and can still call EndRun(0) in a mistyped-flag scenario. Please gate this on integer-flag existence specifically, or otherwise avoid get_IntFlag() unless the flag is known to be an int.
Comment on lines
731
to
+739
| int eventvectorsize = FillEventVector(); | ||
| // this seems a unique signature for raw data files which only contain the | ||
| // begin and end run event but no data events. FillEventVector() returns -1 | ||
| // and since no events were read the m_PacketEventDeque is empty | ||
| if (eventvectorsize < 0 && m_PacketEventDeque.empty()) | ||
| { | ||
| std::cout << Name() << ": No data Events in input file " << FileName() << std::endl; | ||
| AllDone(1); | ||
| return; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The no-data-file guard never fires once packet keys have been seeded.
FillEventVector() touches m_PacketEventDeque[pid] before any data event is accepted, so on the begin/end-only input this map is typically non-empty with only empty deques. That makes m_PacketEventDeque.empty() false here, so AllDone(1) is skipped and Fun4AllTriggeredInputManager::run() still proceeds past FillPool() instead of taking the graceful empty-input exit this PR is adding.
Suggested fix
int eventvectorsize = FillEventVector();
- if (eventvectorsize < 0 && m_PacketEventDeque.empty())
+ const bool no_packet_events = std::all_of(
+ m_PacketEventDeque.begin(), m_PacketEventDeque.end(),
+ [](const auto& entry) { return entry.second.empty(); });
+ if (eventvectorsize < 0 && no_packet_events)
{
std::cout << Name() << ": No data Events in input file " << FileName() << std::endl;
AllDone(1);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int eventvectorsize = FillEventVector(); | |
| // this seems a unique signature for raw data files which only contain the | |
| // begin and end run event but no data events. FillEventVector() returns -1 | |
| // and since no events were read the m_PacketEventDeque is empty | |
| if (eventvectorsize < 0 && m_PacketEventDeque.empty()) | |
| { | |
| std::cout << Name() << ": No data Events in input file " << FileName() << std::endl; | |
| AllDone(1); | |
| return; | |
| int eventvectorsize = FillEventVector(); | |
| // this seems a unique signature for raw data files which only contain the | |
| // begin and end run event but no data events. FillEventVector() returns -1 | |
| // and since no events were read the m_PacketEventDeque is empty | |
| const bool no_packet_events = std::all_of( | |
| m_PacketEventDeque.begin(), m_PacketEventDeque.end(), | |
| [](const auto& entry) { return entry.second.empty(); }); | |
| if (eventvectorsize < 0 && no_packet_events) | |
| { | |
| std::cout << Name() << ": No data Events in input file " << FileName() << std::endl; | |
| AllDone(1); | |
| return; |
Build & test reportReport for commit 9171015afe8801b42ed748b6c1a9adec3ba42ecd:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 7c2cf3216d31715150d8ef890ba97669b88f8a79:
Automatically generated by sPHENIX Jenkins continuous integration |
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.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
We have a few raw data files which do not contain data events (daq problem), only the begin and end run event. The event combiner did not expect this and segfaulted somewhere deep in its guts. This PR catches this particular failure and quits gracefully with an error.
A consequence of this is that the runnumber rc flag is not set which triggers a warning and stacktrace when Fun4All calls the EndRun with the runnumber rc flag. Now Fun4All checks if this flag exists and - if not - will not call EndRun for registered modules. This should take care of misleading printouts in some of our logfiles
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
This PR fixes a crash in the raw-data event combiner (
coresoftware) that occurred when an input file contains only begin/end run events (no data events), caused by a DAQ issue. It also prevents follow-on noise where theRUNNUMBERrc flag was not set, which could leadFun4Allto attemptEndRunin an invalid state.Key changes:
SingleTriggeredInput::FillPool: added an early guard to detect “begin/end-only” raw files (no data packets), log the condition, callAllDone(1), and exit cleanly instead of continuing into a failing combiner path.Fun4AllServer::End: added a guard soEndRun()is called only ifrecoConstscontains theRUNNUMBERinteger flag; if missing, it skipsEndRun()for registered modules to reduce misleading warnings/stack traces.-lboost_filesystemfromoffline/framework/fun4all/Makefile.amandsimulation/g4simulation/g4main/Makefile.amlibrary link lists.Potential risk areas:
boost_filesystemlinkage (should be harmless if no longer required, but verify in your build environment).Possible future improvements:
AI can make mistakes—please use best judgment when reading this summary.