From 5aaae0f567cbdd3eefab8f3cdc4ad3ceff7ad57e Mon Sep 17 00:00:00 2001
From: Dominic Hamon <dominic+github@google.com>
Date: Tue, 13 Aug 2024 14:50:21 +0100
Subject: [PATCH] Only insert executable into cache once it starts

The observability hooks are created in `Executable::new` so we shouldn't
miss any messages.  This ensures that we only add started executables to
the cache, which means that we don't end up with orphan executables in
the cache.
---
 .../cell_service/executables/executables.rs   | 34 +++++++++++--------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/auraed/src/cells/cell_service/executables/executables.rs b/auraed/src/cells/cell_service/executables/executables.rs
index 231ad2630..2df13e1f8 100644
--- a/auraed/src/cells/cell_service/executables/executables.rs
+++ b/auraed/src/cells/cell_service/executables/executables.rs
@@ -42,27 +42,30 @@ impl Executables {
         }
 
         let executable_name = executable_spec.name.clone();
-        // `or_insert` will always insert as we've already assured ourselves that the key does not exist.
-        let executable = self
-            .cache
-            .entry(executable_name.clone())
-            .or_insert_with(|| Executable::new(executable_spec));
-
-        // TODO: if we fail to start, the exe remains in the cache and start cannot be called again
-        // solving ^^ was a borrow checker fight and I (future-highway) lost this round.
+        let mut executable = Executable::new(executable_spec);
+
+        // start the exe before we add it to the cache, as otherwise a failure leads to the
+        // executable remaining in the cache and start cannot be called again.
         executable.start().map_err(|e| {
             ExecutablesError::FailedToStartExecutable {
-                executable_name,
+                executable_name: executable_name.clone(),
                 source: e,
             }
         })?;
 
-        Ok(executable)
+        // `or_insert` will always insert as we've already assured ourselves that the key does not
+        // exist.
+        let inserted_executable =
+            self.cache.entry(executable_name).or_insert_with(|| executable);
+
+        Ok(inserted_executable)
     }
 
     pub fn get(&self, executable_name: &ExecutableName) -> Result<&Executable> {
         let Some(executable) = self.cache.get(executable_name) else {
-            return Err(ExecutablesError::ExecutableNotFound { executable_name: executable_name.clone() });
+            return Err(ExecutablesError::ExecutableNotFound {
+                executable_name: executable_name.clone(),
+            });
         };
         Ok(executable)
     }
@@ -72,7 +75,9 @@ impl Executables {
         executable_name: &ExecutableName,
     ) -> Result<ExitStatus> {
         let Some(executable) = self.cache.get_mut(executable_name) else {
-            return Err(ExecutablesError::ExecutableNotFound { executable_name: executable_name.clone() });
+            return Err(ExecutablesError::ExecutableNotFound {
+                executable_name: executable_name.clone(),
+            });
         };
 
         let exit_status = executable.kill().await.map_err(|e| {
@@ -84,7 +89,8 @@ impl Executables {
 
         let Some(exit_status) = exit_status else {
             // Exes that never started return None
-            let executable = self.cache.remove(executable_name).expect("exe in cache");
+            let executable =
+                self.cache.remove(executable_name).expect("exe in cache");
             return Err(ExecutablesError::ExecutableNotFound {
                 executable_name: executable.name,
             });
@@ -112,4 +118,4 @@ impl Executables {
             let _ = self.cache.remove(&name);
         }
     }
-}
\ No newline at end of file
+}