From b88a48cf7fa1c245848a8a9e9f5f079a264115f0 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Thu, 27 Aug 2026 16:05:08 +0200 Subject: [PATCH] Broker: a failed calibration is an Error, and ending a run clears what it left Three things a caller could not see, all in the same lifecycle. A calibration that FAILS now goes to Error rather than Inactive. The wait endpoints turn Error into a 500 carrying the message, while Inactive is a bodiless 502 - so the reason a pedestal or mask sequence failed was thrown away on the way out. Inactive is also where a deliberate Deactivate() leaves the broker, which made a failure indistinguishable from a detector someone had powered down. Initialize() clears Error, so recovery is what it was. The four CANCELLATION paths keep Inactive: a cancel is not a failure, and c29dd67b5 separated the two deliberately. Cancel() and Deactivate() clear start_exception. JFJochStateMachine.h says every entry point that begins new work clears it; these two end work instead, and were missed. A failed /start followed by /deactivate left every later wait call rethrowing a failure from a run on a detector that is no longer powered, until some later /start or /initialize happened to clear it. Start() and Deactivate() clear scan_result. It is only ever assigned at the end of a successful measurement, so a run that failed to start left /scan_result answering with the PREVIOUS run's images, under the new run number and with nothing to mark them as not its own - the same stale-read Initialize() has always cleared. Deactivate() clears it for the same reason it clears the rest: the result describes a measurement on a detector being turned off. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016L1qig74oYQzfUJJZbbxFh --- broker/JFJochStateMachine.cpp | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/broker/JFJochStateMachine.cpp b/broker/JFJochStateMachine.cpp index 218f1434..018904ce 100644 --- a/broker/JFJochStateMachine.cpp +++ b/broker/JFJochStateMachine.cpp @@ -120,7 +120,11 @@ void JFJochStateMachine::CalibrateDetector(std::unique_lock ul) { } catch (const std::exception &e) { logger.Error("Calibration sequence error {}", e.what()); // The calibration is in an undefined state, so the detector has to be initialized again. - SetState(JFJochState::Inactive, e.what(), BrokerStatus::MessageSeverity::Error); + // Error rather than Inactive: /wait_till_done and /wait_until_running answer Error with a 500 + // carrying this message, while Inactive is a bodiless 502 that loses the reason - and Inactive + // is also what a deliberate Deactivate() leaves behind, so a failure would be indistinguishable + // from a detector someone powered down. Initialize() clears Error, so recovery is unchanged. + SetState(JFJochState::Error, e.what(), BrokerStatus::MessageSeverity::Error); c.notify_all(); // ul unlocks on the way out throw; } @@ -162,7 +166,7 @@ bool JFJochStateMachine::TakeDarkMaskInternal(std::unique_lock &ul) ul.lock(); if (mask_output.receiver_output.dark_mask_result.size() != local_experiment.GetPixelsNum()) { - SetState(JFJochState::Inactive, "Mask not collected properly", BrokerStatus::MessageSeverity::Error); + SetState(JFJochState::Error, "Mask not collected properly", BrokerStatus::MessageSeverity::Error); return false; } pixel_mask.LoadDarkBadPixelMask(local_experiment, mask_output.receiver_output.dark_mask_result); @@ -209,7 +213,7 @@ bool JFJochStateMachine::TakePedestalInternalG0(std::unique_lock &ul ul.lock(); if (!ImportPedestalG0(pedestal_output.receiver_output)) { - SetState(JFJochState::Inactive, + SetState(JFJochState::Error, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); return false; @@ -254,7 +258,7 @@ bool JFJochStateMachine::TakePedestalInternalG1(std::unique_lock &ul ul.lock(); if (!ImportPedestalG1G2(pedestal_output.receiver_output, 1, storage_cell)) { - SetState(JFJochState::Inactive, + SetState(JFJochState::Error, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); return false; @@ -297,7 +301,7 @@ bool JFJochStateMachine::TakePedestalInternalG2(std::unique_lock &ul ul.lock(); if (!ImportPedestalG1G2(pedestal_output.receiver_output, 2, storage_cell)) { - SetState(JFJochState::Inactive, + SetState(JFJochState::Error, "Pedestal not collected properly", BrokerStatus::MessageSeverity::Error); return false; @@ -387,6 +391,12 @@ void JFJochStateMachine::Start(const DatasetSettings &settings, bool async) { // previous run's failure behind for the next wait call to report. start_exception = nullptr; + // The same for the previous run's scan result. It is only ever replaced at the END of a + // successful measurement, so a run that fails to start - or is rejected here - would otherwise + // leave /scan_result answering with the last run's images, under this run's number and with + // nothing to say they are not its own. Initialize() already clears it for the same reason. + scan_result = {}; + experiment.ImportDatasetSettings(settings); cancel_sequence = false; @@ -532,6 +542,9 @@ void JFJochStateMachine::MeasurementThread() { void JFJochStateMachine::Cancel() { // This is inconsistency in naming - need to solve later std::unique_lock ul(m); + // Cancelling supersedes a pending start failure: the caller has taken the run in hand, and the + // next wait must report what the cancel did, not what the start before it failed to do. + start_exception = nullptr; if ((state == JFJochState::Calibration) || (state == JFJochState::Measuring)) { services.Cancel(); cancel_sequence = true; @@ -554,6 +567,16 @@ void JFJochStateMachine::Deactivate() { if (IsRunning()) throw WrongDAQStateException("Cannot deactivate while the detector is busy"); + // Powering down supersedes a pending start failure. Without this it outlives the detector it + // belongs to: every later wait call keeps rethrowing a failure from a run on hardware that is no + // longer even on, until the next Initialize() or Start() happens to clear it. + start_exception = nullptr; + + // The same for the last run's scan result, and for the same reason: it describes a measurement on + // a detector that is being turned off, and Initialize() - the only way back from here - clears it + // anyway. Leaving it would let /scan_result answer for a session that has ended. + scan_result = {}; + // Reap the finished thread, but do not let a failure it stored stop the power-off: the state is // Error precisely because that run failed, and leaving the detector powered is worse than // losing an error message that was already reported when it happened.