From a3c101d3a76d453474b8bf7c1c5b8425e2519f38 Mon Sep 17 00:00:00 2001 From: Rudolf Weeber Date: Mon, 15 Jun 2026 12:42:19 +0200 Subject: [PATCH] accumulators: reject delta_N < 1 in core ctor and SI setter (bug-sweep #24) The Python accumulator API documented delta_N as a positive integer sample spacing, but no layer validated it. delta_N <= 0 reached the core unchecked: - Correlator(delta_N=0): m_dt = time_step * 0 = 0, so the m_tau_max <= m_dt guard (e.g. 2.0 <= 0.0) is false; initialize_operations() then computes ceil(1 + log2(inf)) and static_cast(inf) for the hierarchy depth (UB), producing a bad multi_array resize. Observed as MemoryError (std::bad_array_new_length) at construction. This UB does not throw, so the existing parallel_try_catch around construction did not catch it. - MeanVarianceCalculator/TimeSeries/ContactTimes(delta_N=0) constructed fine, then auto_update_accumulators.add(acc) + integrator.run() set frequency=0, making next_update() return 0 -> integrate(0) never advances -> infinite loop (release) / assert abort (debug). - The runtime setter acc.delta_N = 0 was equally unguarded. Fix (PREVENT invalid input, matching the existing tau_lin/tau_max validation convention): add a single chokepoint guard in the core Accumulators::AccumulatorBase ctor (all four accumulators chain to it) that throws std::runtime_error("delta_N must be >= 1") before initialize() runs, killing both the Correlator UB and the auto-update hang. Wrap the previously unwrapped MeanVarianceCalculator and TimeSeries SI ctors in parallel_try_catch so the throw is MPI-coordinated (Correlator/ContactTimes already do this). Guard the SI delta_N setter the same way so acc.delta_N = 0 is rejected and leaves the previous value intact. Extend test_correlator_exceptions and add test_delta_N_validation in accumulator_correlator.py to cover delta_N in (0, -1) at construction and via the runtime setter for Correlator, MeanVarianceCalculator and TimeSeries. Co-Authored-By: Claude Opus 4.8 --- src/core/accumulators/AccumulatorBase.hpp | 7 +++- .../accumulators/AccumulatorBase.hpp | 9 ++++- .../accumulators/MeanVarianceCalculator.hpp | 9 +++-- .../accumulators/TimeSeries.hpp | 8 +++-- testsuite/python/accumulator_correlator.py | 33 +++++++++++++++++++ 5 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/core/accumulators/AccumulatorBase.hpp b/src/core/accumulators/AccumulatorBase.hpp index 19c4b7be2e..a96dc6151d 100644 --- a/src/core/accumulators/AccumulatorBase.hpp +++ b/src/core/accumulators/AccumulatorBase.hpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -37,7 +38,11 @@ namespace Accumulators { class AccumulatorBase { public: AccumulatorBase(::System::System const *system, int delta_N) - : m_system(reinterpret_cast(system)), m_delta_N(delta_N) {} + : m_system(reinterpret_cast(system)), m_delta_N(delta_N) { + if (delta_N < 1) { + throw std::runtime_error("delta_N must be >= 1"); + } + } virtual ~AccumulatorBase() = default; int &delta_N() { return m_delta_N; } diff --git a/src/script_interface/accumulators/AccumulatorBase.hpp b/src/script_interface/accumulators/AccumulatorBase.hpp index c2bc774ae1..537f184564 100644 --- a/src/script_interface/accumulators/AccumulatorBase.hpp +++ b/src/script_interface/accumulators/AccumulatorBase.hpp @@ -27,6 +27,7 @@ #include "core/system/System.hpp" #include +#include #include #include @@ -38,7 +39,13 @@ class AccumulatorBase : public AutoParameters { AccumulatorBase() { add_parameters({{"delta_N", [this](const Variant &v) { - accumulator()->delta_N() = get_value(v); + auto const delta_N = get_value(v); + context()->parallel_try_catch([&]() { + if (delta_N < 1) { + throw std::runtime_error("delta_N must be >= 1"); + } + }); + accumulator()->delta_N() = delta_N; }, [this]() { return accumulator()->delta_N(); }}}); } diff --git a/src/script_interface/accumulators/MeanVarianceCalculator.hpp b/src/script_interface/accumulators/MeanVarianceCalculator.hpp index 31a220d0f9..fe5a3f2c9e 100644 --- a/src/script_interface/accumulators/MeanVarianceCalculator.hpp +++ b/src/script_interface/accumulators/MeanVarianceCalculator.hpp @@ -44,9 +44,12 @@ class MeanVarianceCalculator : public AccumulatorBase { set_from_args(m_obs, params, "obs"); if (m_obs) - m_accumulator = std::make_shared<::Accumulators::MeanVarianceCalculator>( - get_core_system_pointer(params), - get_value_or(params, "delta_N", 1), m_obs->observable()); + ObjectHandle::context()->parallel_try_catch([&]() { + m_accumulator = + std::make_shared<::Accumulators::MeanVarianceCalculator>( + get_core_system_pointer(params), + get_value_or(params, "delta_N", 1), m_obs->observable()); + }); } std::shared_ptr<::Accumulators::MeanVarianceCalculator> diff --git a/src/script_interface/accumulators/TimeSeries.hpp b/src/script_interface/accumulators/TimeSeries.hpp index 7220118305..ea9710b69c 100644 --- a/src/script_interface/accumulators/TimeSeries.hpp +++ b/src/script_interface/accumulators/TimeSeries.hpp @@ -42,9 +42,11 @@ class TimeSeries : public AccumulatorBase { set_from_args(m_obs, params, "obs"); if (m_obs) - m_accumulator = std::make_shared<::Accumulators::TimeSeries>( - get_core_system_pointer(params), - get_value_or(params, "delta_N", 1), m_obs->observable()); + ObjectHandle::context()->parallel_try_catch([&]() { + m_accumulator = std::make_shared<::Accumulators::TimeSeries>( + get_core_system_pointer(params), + get_value_or(params, "delta_N", 1), m_obs->observable()); + }); } Variant do_call_method(std::string const &method, diff --git a/testsuite/python/accumulator_correlator.py b/testsuite/python/accumulator_correlator.py index e15a48e817..832fae3504 100644 --- a/testsuite/python/accumulator_correlator.py +++ b/testsuite/python/accumulator_correlator.py @@ -315,6 +315,20 @@ def create_accumulator(obs1=obs, **kwargs): create_accumulator(tau_lin=3) with self.assertRaisesRegex(RuntimeError, "tau_max must be >= delta_t"): create_accumulator(delta_N=2 * int(10. / self.system.time_step)) + # delta_N <= 0 is meaningless and must be rejected at construction + # (otherwise delta_t=0 leads to log2(inf) UB in the Correlator) + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + create_accumulator(delta_N=0) + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + create_accumulator(delta_N=-1) + # the runtime setter must reject invalid values too + acc = create_accumulator() + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + acc.delta_N = 0 + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + acc.delta_N = -3 + # a rejected setter call must leave the previous value intact + self.assertEqual(acc.delta_N, 1) with self.assertRaisesRegex(ValueError, "correlation operation 'unknown' not implemented"): create_accumulator(corr_operation="unknown") with self.assertRaisesRegex(ValueError, "unknown compression method 'unknown1' for first observable"): @@ -365,6 +379,25 @@ def create_accumulator(obs1=obs, **kwargs): with self.assertRaisesRegex(RuntimeError, "auto_update_accumulators property"): pickle.dumps(acc) + def test_delta_N_validation(self): + """delta_N <= 0 must be rejected by all accumulators (construction and + runtime setter). With delta_N=0 an auto-update would hang the + integrator (frequency=0) and the Correlator hits UB at construction.""" + self.system.part.add(pos=[(0, 0, 0)]) + obs = espressomd.observables.ParticleVelocities(ids=(0,)) + + for ctor in (espressomd.accumulators.MeanVarianceCalculator, + espressomd.accumulators.TimeSeries): + for bad in (0, -1): + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + ctor(obs=obs, delta_N=bad) + # runtime setter + acc = ctor(obs=obs, delta_N=1) + for bad in (0, -2): + with self.assertRaisesRegex(RuntimeError, "delta_N must be >= 1"): + acc.delta_N = bad + self.assertEqual(acc.delta_N, 1) + if __name__ == "__main__": ut.main()