[ML] Better cleanup of normalizer quantiles state

bin/autodetect/Main.cc

@@ -26,7 +26,7 @@
 #include <core/CProcessPriority.h>
 #include <core/CProcessStats.h>
 #include <core/CProgramCounters.h>
-#include <core/CStringUtils.h>
+#include <core/CStateFileRemover.h>
 #include <core/CoreTypes.h>
 
 #include <ver/CBuildInfo.h>
@@ -119,14 +119,23 @@ int main(int argc, char** argv) {
     std::size_t maxAnomalyRecords{100};
     bool memoryUsage{false};
     bool validElasticLicenseKeyConfirmed{false};
-    if (ml::autodetect::CCmdLineParser::parse(
-            argc, argv, configFile, filtersConfigFile, eventsConfigFile,
-            modelConfigFile, logProperties, logPipe, delimiter, lengthEncodedInput,
-            timeFormat, quantilesStateFile, deleteStateFiles, bucketPersistInterval,
-            namedPipeConnectTimeout, inputFileName, isInputFileNamedPipe, outputFileName,
-            isOutputFileNamedPipe, restoreFileName, isRestoreFileNamedPipe,
-            persistFileName, isPersistFileNamedPipe, isPersistInForeground,
-            maxAnomalyRecords, memoryUsage, validElasticLicenseKeyConfirmed) == false) {
+    std::unique_ptr<ml::core::CStateFileRemover> removeQuantilesStateOnFailure;
+
+    const bool parseSuccess = ml::autodetect::CCmdLineParser::parse(
+        argc, argv, configFile, filtersConfigFile, eventsConfigFile,
+        modelConfigFile, logProperties, logPipe, delimiter, lengthEncodedInput,
+        timeFormat, quantilesStateFile, deleteStateFiles, bucketPersistInterval,
+        namedPipeConnectTimeout, inputFileName, isInputFileNamedPipe, outputFileName,
+        isOutputFileNamedPipe, restoreFileName, isRestoreFileNamedPipe,
+        persistFileName, isPersistFileNamedPipe, isPersistInForeground,
+        maxAnomalyRecords, memoryUsage, validElasticLicenseKeyConfirmed);
+
+    if (!quantilesStateFile.empty()) {
+        removeQuantilesStateOnFailure = std::make_unique<ml::core::CStateFileRemover>(
+            quantilesStateFile, deleteStateFiles);
+    }
+
+    if (parseSuccess == false) {
         return EXIT_FAILURE;
     }
 
@@ -293,9 +302,6 @@ int main(int argc, char** argv) {
             LOG_FATAL(<< "Failed to restore quantiles and initialize normalizer");
             return EXIT_FAILURE;
         }
-        if (deleteStateFiles) {
-            std::remove(quantilesStateFile.c_str());
-        }
     }
 
     // The categorizer knows how to assign categories to records
@@ -346,5 +352,14 @@ int main(int argc, char** argv) {
     // message indicating early exit then the process has probably core dumped
     LOG_DEBUG(<< "ML anomaly detector job exiting");
 
+    // No need for a warning here so we reset the cleanup function and delete the file explicitly if requested.
+    removeQuantilesStateOnFailure.reset();
+    if (deleteStateFiles) {
+        if (std::remove(quantilesStateFile.c_str()) != 0) {
+            LOG_WARN(<< "Failed to delete quantiles state file '"
+                     << quantilesStateFile << "': " << strerror(errno));
+        }
+    }
+
     return EXIT_SUCCESS;
 }

bin/normalize/Main.cc

@@ -21,6 +21,7 @@
 #include <core/CBlockingCallCancellingTimer.h>
 #include <core/CLogger.h>
 #include <core/CProcessPriority.h>
+#include <core/CStateFileRemover.h>
 #include <core/CoreTypes.h>
 
 #include <ver/CBuildInfo.h>
@@ -63,11 +64,20 @@ int main(int argc, char** argv) {
     bool deleteStateFiles{false};
     bool writeCsv{false};
     bool validElasticLicenseKeyConfirmed{false};
-    if (ml::normalize::CCmdLineParser::parse(
-            argc, argv, modelConfigFile, logProperties, logPipe, bucketSpan,
-            lengthEncodedInput, namedPipeConnectTimeout, inputFileName,
-            isInputFileNamedPipe, outputFileName, isOutputFileNamedPipe, quantilesStateFile,
-            deleteStateFiles, writeCsv, validElasticLicenseKeyConfirmed) == false) {
+    std::unique_ptr<ml::core::CStateFileRemover> removeQuantilesStateOnFailure;
+
+    const bool parseSuccess = ml::normalize::CCmdLineParser::parse(
+        argc, argv, modelConfigFile, logProperties, logPipe, bucketSpan,
+        lengthEncodedInput, namedPipeConnectTimeout, inputFileName,
+        isInputFileNamedPipe, outputFileName, isOutputFileNamedPipe, quantilesStateFile,
+        deleteStateFiles, writeCsv, validElasticLicenseKeyConfirmed);
+
+    if (!quantilesStateFile.empty()) {
+        removeQuantilesStateOnFailure = std::make_unique<ml::core::CStateFileRemover>(
+            quantilesStateFile, deleteStateFiles);
+    }
+
+    if (parseSuccess == false) {
         return EXIT_FAILURE;
     }
 
@@ -158,9 +168,6 @@ int main(int argc, char** argv) {
             LOG_FATAL(<< "Failed to initialize normalizer");
             return EXIT_FAILURE;
         }
-        if (deleteStateFiles) {
-            std::remove(quantilesStateFile.c_str());
-        }
     }
 
     // Now handle the numbers to be normalised from stdin
@@ -176,5 +183,14 @@ int main(int argc, char** argv) {
     // message indicating early exit then the process has probably core dumped
     LOG_DEBUG(<< "ML normalizer exiting");
 
+    // No need for a warning here so we reset the cleanup function and delete the file explicitly if requested.
+    removeQuantilesStateOnFailure.reset();
+    if (deleteStateFiles) {
+        if (std::remove(quantilesStateFile.c_str()) != 0) {
+            LOG_WARN(<< "Failed to delete quantiles state file '"
+                     << quantilesStateFile << "': " << strerror(errno));
+        }
+    }
+
     return EXIT_SUCCESS;
 }

docs/CHANGELOG.asciidoc

@@ -27,6 +27,13 @@
 //=== Bug Fixes
 
 //=== Regressions
+
+== {es} version 9.4.0
+
+=== Enhancements
+
+* Better error handling regarding quantiles state documents (See {ml-pull}[#2894])
+
 == {es} version 9.3.0
 
 === Enhancements

include/core/CStateFileRemover.h

@@ -0,0 +1,61 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the following additional limitation. Functionality enabled by the
+ * files subject to the Elastic License 2.0 may only be used in production when
+ * invoked by an Elasticsearch process with a license key installed that permits
+ * use of machine learning features. You may not use this file except in
+ * compliance with the Elastic License 2.0 and the foregoing additional
+ * limitation.
+ */
+#ifndef INCLUDED_ml_core_CStateFileRemover_h
+#define INCLUDED_ml_core_CStateFileRemover_h
+
+#include <core/CLogger.h>
+
+#include <cstdlib>
+
+namespace ml {
+namespace core {
+
+//! \brief
+//! Ensures that deletion of state files occurs even on process failure.
+//!
+//! DESCRIPTION:\n
+//! A helper to ensure that quantiles state files always get deleted on failure.
+//! They may also be explicitly be deleted on request as well but that is handled separately by the happy path.
+//!
+//! IMPLEMENTATION DECISIONS:\n
+//! Not copyable or moveable. No default construction.
+class CStateFileRemover {
+public:
+    CStateFileRemover() = delete;
+    CStateFileRemover(const CStateFileRemover&) = delete;
+    CStateFileRemover& operator=(const CStateFileRemover&) = delete;
+    CStateFileRemover(CStateFileRemover&&) = delete;
+    CStateFileRemover& operator=(CStateFileRemover&&) = delete;
+    explicit CStateFileRemover(const std::string& quantilesStateFile,
+                               bool deleteStateFiles = false)
+        : m_QuantilesStateFile{quantilesStateFile}, m_DeleteStateFiles{deleteStateFiles} {}
+    ~CStateFileRemover() {
+        // Always delete quantiles state files if requested to do so, even on failure,
+        // else we run the risk of filling the disk after repeated failures.
+        // They should still exist in ES should they need to be examined.
+        if (m_QuantilesStateFile.empty() || m_DeleteStateFiles == false) {
+            return;
+        }
+        LOG_DEBUG(<< "Deleting quantiles state file '" << m_QuantilesStateFile << "'");
+        if (std::remove(m_QuantilesStateFile.c_str()) != 0) {
+            LOG_WARN(<< "Failed to delete quantiles state file '"
+                     << m_QuantilesStateFile << "': " << strerror(errno));
+        }
+    }
+
+private:
+    std::string m_QuantilesStateFile;
+    bool m_DeleteStateFiles{false};
+};
+}
+}
+
+#endif // INCLUDED_ml_core_CStateFileRemover_h