instrumentation: Don't remove content files older than an index

Prevent callbacks from referencing deleted content files by holding onto them
if any index files exist that may contain a reference.

Source/cmInstrumentation.cxx

@@ -229,49 +229,51 @@ void cmInstrumentation::WriteCustomContent()
   }
 }
 
-std::string cmInstrumentation::GetLatestFile(std::string const& dataSubdir,
-                                             std::string const& exclude)
+std::string cmInstrumentation::GetFileByTimestamp(
+  cmInstrumentation::LatestOrOldest order, std::string const& dataSubdir,
+  std::string const& exclude)
 {
   std::string fullDir = cmStrCat(this->timingDirv1, "/data/", dataSubdir);
-  std::string latestFile;
+  std::string result;
   if (cmSystemTools::FileExists(fullDir)) {
     cmsys::Directory d;
     if (d.Load(fullDir)) {
       for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
         std::string fname = d.GetFileName(i);
         if (fname != "." && fname != ".." && fname != exclude &&
-            fname > latestFile) {
-          latestFile = fname;
+            (result.empty() ||
+             (order == LatestOrOldest::Latest && fname > result) ||
+             (order == LatestOrOldest::Oldest && fname < result))) {
+          result = fname;
         }
       }
     }
   }
-  return latestFile;
+  return result;
 }
 
 void cmInstrumentation::RemoveOldFiles(std::string const& dataSubdir)
 {
   std::string const dataSubdirPath =
     cmStrCat(this->timingDirv1, "/data/", dataSubdir);
+  std::string oldIndex =
+    this->GetFileByTimestamp(LatestOrOldest::Oldest, "index");
+  if (!oldIndex.empty()) {
+    oldIndex = cmStrCat(this->timingDirv1, "/data/index/", oldIndex);
+  }
   if (cmSystemTools::FileExists(dataSubdirPath)) {
-    std::string latestFile = this->GetLatestFile(dataSubdir);
+    std::string latestFile =
+      this->GetFileByTimestamp(LatestOrOldest::Latest, dataSubdir);
     cmsys::Directory d;
     if (d.Load(dataSubdirPath)) {
       for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
         std::string fname = d.GetFileName(i);
         std::string fpath = d.GetFilePath(i);
         if (fname != "." && fname != ".." && fname < latestFile) {
-          if (dataSubdir == "trace") {
-            // Check if this trace file shares a name with any existing index
-            // files, in which case it is listed by that index file and a
-            // callback is running, so we shouldn't delete it yet.
-            std::string index = "index-";
-            std::string json = ".json";
-            std::string timestamp = fname.substr(
-              index.size(), fname.size() - index.size() - json.size() - 1);
-            if (cmSystemTools::FileExists(cmStrCat(this->timingDirv1,
-                                                   "/data/index/index-",
-                                                   timestamp, ".json"))) {
+          if (!oldIndex.empty()) {
+            int compare;
+            cmSystemTools::FileTimeCompare(oldIndex, fpath, &compare);
+            if (compare == 1) {
               continue;
             }
           }
@@ -329,7 +331,8 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
   using snippet = std::pair<std::string, std::string>;
   std::vector<snippet> files;
   cmsys::Directory d;
-  std::string last_index_name = this->GetLatestFile("index", index_name);
+  std::string last_index_name =
+    this->GetFileByTimestamp(LatestOrOldest::Latest, "index", index_name);
   if (d.Load(directory)) {
     for (unsigned int i = 0; i < d.GetNumberOfFiles(); i++) {
       std::string fpath = d.GetFilePath(i);
@@ -663,7 +666,8 @@ int cmInstrumentation::InstrumentCommand(
   root["workingDir"] = cmSystemTools::GetLogicalWorkingDirectory();
 
   // Add custom configure content
-  std::string contentFile = this->GetLatestFile("content");
+  std::string contentFile =
+    this->GetFileByTimestamp(LatestOrOldest::Latest, "content");
   if (!contentFile.empty()) {
     root["configureContent"] = cmStrCat("content/", contentFile);
   }

Source/cmInstrumentation.h

@@ -98,8 +98,14 @@ private:
                         Json::Value const& snippetData);
   size_t AssignTargetToTraceThread(std::vector<uint64_t>& workers,
                                    uint64_t timeStart, uint64_t duration);
-  std::string GetLatestFile(std::string const& dataSubdir,
-                            std::string const& exclude = "");
+  enum LatestOrOldest
+  {
+    Latest,
+    Oldest
+  };
+  std::string GetFileByTimestamp(LatestOrOldest latestOrOldest,
+                                 std::string const& dataSubdir,
+                                 std::string const& exclude = "");
   std::string binaryDir;
   std::string timingDirv1;
   std::string userTimingDirv1;

Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -25,6 +25,7 @@ function(instrument test)
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${test})
   set(uuid "2555e60c-9119-46fb-af73-8c54e9b6b326")
   set(v1 ${RunCMake_TEST_BINARY_DIR}/.cmake/instrumentation-${uuid}/v1)
+  set(v1 ${v1} PARENT_SCOPE)
   set(query_dir ${CMAKE_CURRENT_LIST_DIR}/query)
 
   # Clear previous instrumentation data
@@ -177,6 +178,19 @@ instrument(cmake-command-custom-content
   CONFIGURE_ARG "-DN=2"
   CHECK_SCRIPT check-custom-content.cmake
 )
+set(indexDir ${v1}/data/index)
+set(fakeIndex ${indexDir}/index-0.json)
+file(MAKE_DIRECTORY ${indexDir})
+file(TOUCH ${fakeIndex})
+# fakeIndex newer than all content files prevents their deletion
+set(EXPECTED_CONTENT_FILES 2)
+instrument(cmake-command-custom-content
+  NO_WARN NO_CONFIGURE MANUAL_HOOK PRESERVE_DATA
+  CHECK_SCRIPT check-custom-content-removed.cmake
+)
+file(REMOVE ${fakeIndex})
+# old content files will be removed if no index file exists
+set(EXPECTED_CONTENT_FILES 1)
 instrument(cmake-command-custom-content
   NO_WARN NO_CONFIGURE MANUAL_HOOK PRESERVE_DATA
   CHECK_SCRIPT check-custom-content-removed.cmake

Tests/RunCMake/Instrumentation/check-custom-content-removed.cmake

@@ -6,6 +6,6 @@ endif()
 
 file(GLOB content_files ${v1}/data/content/*)
 list(LENGTH content_files num)
-if (NOT ${num} EQUAL 1)
-  add_error("Found ${num} custom content files, expected 1.")
+if (NOT ${num} EQUAL ${EXPECTED_CONTENT_FILES})
+  add_error("Found ${num} custom content files, expected ${EXPECTED_CONTENT_FILES}.")
 endif()