From 9e2daa9cd4dfa2e2fe736e56bb44e28754284927 Mon Sep 17 00:00:00 2001 From: Oliver Lantwin Date: Fri, 3 Jul 2026 11:41:41 +0200 Subject: [PATCH] fix(geant4): fail cleanly on unknown physics lists, count worker kernels - Validate the physics list with IsReferencePhysList before allocating the detector or the leaked G4MTRunManager: GetReferencePhysList aborts the process via a fatal G4Exception on unknown names. Validating up front (as the export_gdml pre-check already does) turns a bad config into a catchable std::runtime_error, which the sticky-init path then reports cleanly instead of crashing the process. - Count only kernels that finish init_worker successfully in the shutdown summary, via a dedicated atomic, rather than the id allocated on entry. Warn when TBB thread churn builds more kernels than configured slots: each new pool thread that ever runs simulate builds its own kernel, silently costing memory and a fresh RNG stream. - Add a physics_list_failure integration test: an unknown list under concurrent init must exit with the clear error and no "constructed twice" run-manager abort. - Correct the reproducibility mechanism in docs/geant4-perf-notes.md: the dominant effect is event-order nondeterminism against a continuous per-kernel RNG stream, observed even in runs with a single kernel; per-event seeding from the stable cell index (PR #54) is the fix. --- CMakeLists.txt | 7 +++ docs/geant4-perf-notes.md | 24 +++++++---- scripts/physics_list_failure.sh | 75 +++++++++++++++++++++++++++++++++ src/geant4_module.cpp | 42 +++++++++++++++--- 4 files changed, 134 insertions(+), 14 deletions(-) create mode 100755 scripts/physics_list_failure.sh diff --git a/CMakeLists.txt b/CMakeLists.txt index 9831688..b5527ea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -203,3 +203,10 @@ add_test( NAME file_source_roundtrip COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/scripts/file_source_roundtrip.sh ) + +# Physics-list failure: an unknown physics_list must fail cleanly (catchable +# error, no abort) even when concurrent init retries race on std::call_once. +add_test( + NAME physics_list_failure + COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/scripts/physics_list_failure.sh +) diff --git a/docs/geant4-perf-notes.md b/docs/geant4-perf-notes.md index 2a7fa01..f644c88 100644 --- a/docs/geant4-perf-notes.md +++ b/docs/geant4-perf-notes.md @@ -59,14 +59,22 @@ Measured with `gun_st_full` (200 events, fixed gun seed): - `phlex -j 1`: **reproducible** — the validation histograms `h_mc_multiplicity`, `h_mc_momentum`, and `h_mc_pdg` are bin-by-bin identical across runs (checked by `scripts/compare_histograms.py`). -- `concurrency: 1` with `-j 12`: **not reproducible.** Even though only - one `simulate` call runs at a time, TBB hands successive calls to - different pool threads; each new thread lazily builds its own - `G4WorkerRunManagerKernel` with its own RNG stream, so the event - sequence samples different streams run to run. This directly confirms - the worker-kernel churn concern: the kernel count is bounded by the - number of distinct TBB threads that ever touch `simulate`, not by the - module's `concurrency`. +- `concurrency: 1` with `-j 12`: **not reproducible**, even in runs + where only a single worker kernel was built (the module now logs the + kernel count at shutdown). The dominant mechanism is event order: with + 12 events in flight upstream, the order in which events reach the + single `simulate` slot varies run to run, and each event starts at a + different point in the kernel's continuous RNG stream. Per-event + seeding fixes this: PR #54 (`feat/g4-per-event-seeding`) reseeds the + CLHEP engine from the stable phlex `data_cell_index` (its `hash()`, the + same per-event index the generators already seed from), so an event's + random stream no longer depends on which `simulate` slot it reaches. + Seeding from the arrival-order `G4Event` id + (`next_event_id_.fetch_add(1)`) would not, because that id itself varies + with arrival order. Kernel multiplication through TBB thread churn + (each distinct pool thread that ever runs `simulate` builds its own + kernel) remains possible on long runs and is now surfaced by a + warning; it was not observed in these short tests. - Consequence for validation: physics comparisons must either run `-j 1` (exact) or compare distributions statistically. The single-threaded benchmark recipes in the justfile now pin `-j 1`. diff --git a/scripts/physics_list_failure.sh b/scripts/physics_list_failure.sh new file mode 100755 index 0000000..5610c9c --- /dev/null +++ b/scripts/physics_list_failure.sh @@ -0,0 +1,75 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: 2026 CERN for the benefit of the SHiP Collaboration +# +# SPDX-License-Identifier: LGPL-3.0-or-later + +# Clean-failure check: an unknown physics_list must make the Geant4 module fail +# with a catchable, clear error rather than aborting the process. Geant4's +# G4PhysListFactory::GetReferencePhysList raises a fatal G4Exception (SIGABRT) +# for an unknown list, so the module validates the name up front and throws a +# std::runtime_error instead. +# +# The module's std::call_once init is sticky: the first failure is captured and +# rethrown on every later simulate call, so phlex drains the events already in +# flight, reports the error, and exits — instead of a retry constructing a +# second G4MTRunManager and hitting the fatal "G4RunManager constructed twice" +# abort. We drive a few events with -j > 1 so several events pass through the +# sticky error, and check below that none of them triggered that abort. +set -uo pipefail + +workdir=$(mktemp -d) +trap 'rm -rf "$workdir"' EXIT + +cat >"$workdir/bad_physics.jsonnet" <<'EOF' +{ + driver: { + cpp: 'generate_layers', + layers: { event: { total: 8 } }, + }, + sources: { + geometry: { cpp: 'geometry_builtin_provider' }, + field: { cpp: 'field_null_provider' }, + gun: { + cpp: 'particle_gun_source', + pdg: 13, + p_min: 20.0, + p_max: 20.0, + max_theta: 0.0, + vertex_z: -2000.0, + }, + }, + modules: { + geant4: { + cpp: 'geant4_module', + physics_list: 'NO_SUCH_LIST', + concurrency: 4, + }, + output: { cpp: 'sim_output_module', mode: 'noop' }, + }, +} +EOF + +out=$(phlex -c <(jsonnet "$workdir/bad_physics.jsonnet") -j 4 2>&1) +rc=$? + +if [ "$rc" -eq 0 ]; then + echo "FAIL: expected failure on unknown physics list, but phlex succeeded" + echo "$out" + exit 1 +fi + +if ! grep -q "Unknown physics list" <<<"$out"; then + echo "FAIL: phlex failed (exit $rc) but not with the clean 'Unknown physics list' error" + echo "$out" + exit 1 +fi + +# Re-constructing the run manager aborts via G4Exception (SIGABRT -> exit 134) +# with "constructed twice"; the sticky-failure path never does. +if [ "$rc" -eq 134 ] || grep -qi "constructed twice" <<<"$out"; then + echo "FAIL: init aborted the run manager instead of failing cleanly (exit $rc)" + echo "$out" + exit 1 +fi + +echo "physics-list check passed: unknown list fails cleanly, no run-manager abort" diff --git a/src/geant4_module.cpp b/src/geant4_module.cpp index 8a1a0f4..8169bc3 100644 --- a/src/geant4_module.cpp +++ b/src/geant4_module.cpp @@ -123,6 +123,9 @@ class Geant4Sim { ~Geant4Sim() { if (!master_constructed_) return; + if (int kernels = built_kernels_.load(); kernels > 0) + spdlog::info("geant4: {} worker kernel(s) built for {} configured slots", + kernels, cfg_.concurrency); // Empty the geometry stores now, on the geometry thread. Their static // singletons' destructors run at process exit on the program's main // thread, which never initialised Geant4's thread-local split-class @@ -287,6 +290,17 @@ class Geant4Sim { "' does not exist"); } + // Validate the physics list before constructing anything. Like export_gdml + // above, GetReferencePhysList raises a fatal G4Exception for an unknown + // list, so query IsReferencePhysList here — before detector_ or the run + // manager exist — to fail with a catchable, clear error instead of aborting + // the process. Failing this early also keeps the failure cheap: nothing has + // been built yet. G4PhysListFactory only consults a static name table (it + // creates no geometry), so validating on the calling thread is safe. + G4PhysListFactory phys_factory; + if (!phys_factory.IsReferencePhysList(cfg_.physics_list)) + throw std::runtime_error("Unknown physics list: " + cfg_.physics_list); + field_ = field; // keep alive for the G4 run detector_ = new ConfigurableDetectorConstruction( *geo, *field, cfg_.sd_mode, cfg_.ke_threshold, cfg_.regions); @@ -306,14 +320,11 @@ class Geant4Sim { rm->SetNumberOfThreads(cfg_.concurrency); rm->SetUserInitialization(detector_); - // The run manager is constructed first so its G4Exception handler is - // installed: G4PhysListFactory raises a fatal G4Exception (and aborts) - // for an unknown list rather than returning null, so this lookup is not - // retryable regardless of ordering. + // Physics list already validated in init_master before the run manager + // was constructed, so GetReferencePhysList won't hit its unknown-list + // abort. G4PhysListFactory factory; auto* physics = factory.GetReferencePhysList(cfg_.physics_list); - if (!physics) - throw std::runtime_error("Unknown physics list: " + cfg_.physics_list); rm->SetUserInitialization(physics); rm->SetUserInitialization(new DirectActionInit()); @@ -351,6 +362,17 @@ class Geant4Sim { void init_worker() { AEGIR_TRACE_EVENT("g4", "init_worker"); int id = next_thread_id_.fetch_add(1); + // TBB does not promise that the same OS threads serve this transform + // for the whole run, so more kernels than configured slots can appear + // (each new thread that ever runs simulate builds one). Geant4 11 + // tolerates thread ids beyond SetNumberOfThreads in this direct- + // injection flow, but each extra kernel costs memory and a fresh RNG + // stream — make it visible instead of silent. + if (id >= cfg_.concurrency) + spdlog::warn( + "geant4: initialising worker kernel #{} beyond the {} configured " + "slots (TBB thread churn)", + id + 1, cfg_.concurrency); AEGIR_TRACE_THREAD_NAME("g4_worker_" + std::to_string(id)); G4Threading::G4SetThreadId(id); G4WorkerThread::BuildGeometryAndPhysicsVector(); @@ -371,6 +393,11 @@ class Geant4Sim { tl_kernel->RunInitialization(); G4StateManager::GetStateManager()->SetNewState(G4State_GeomClosed); + + // Count only here, once the kernel is fully built: next_thread_id_ is + // bumped on entry (to allocate the thread id) and would overcount a worker + // whose init_worker threw partway through. + built_kernels_.fetch_add(1); } Geant4SimConfig cfg_; @@ -384,6 +411,9 @@ class Geant4Sim { ConfigurableDetectorConstruction* detector_ = nullptr; // owned by G4 std::shared_ptr field_; // outlives G4 run std::atomic next_thread_id_{0}; + // Kernels that finished init_worker successfully (unlike next_thread_id_, + // which counts allocated ids). Reported in the shutdown summary. + std::atomic built_kernels_{0}; std::atomic next_event_id_{0}; // Completed-event count (completion order, not event id, so the logged // count is monotonic) and the reference point for the average event rate.