Cleanup completed resumed requests

The runner now will cleanup resumed requests as they are completed
(either successfully or unsucessfully). Additionally, the
`FsRequestManager` will cleanup any requests that it cannot resume.

fxrunner/src/bin/main.rs

@@ -88,7 +88,7 @@ async fn fxrunner(log: Logger, options: Options, config: Config) -> Result<(), B
                 shutdown_provider(&options),
                 FirefoxCi::default(),
                 WindowsPerfProvider::default(),
-                FsRequestManager::new(&config.requests_dir),
+                FsRequestManager::new(log.clone(), &config.requests_dir),
             )
             .await?
             {

fxrunner/src/lib/proto.rs

@@ -18,7 +18,9 @@ use tokio::task::spawn_blocking;
 
 use crate::fs::PathExt;
 use crate::osapi::{cpu_and_disk_idle, PerfProvider, ShutdownProvider, WaitForIdleError};
-use crate::request::{NewRequestError, RequestInfo, RequestManager, ResumeRequestError};
+use crate::request::{
+    cleanup_request, NewRequestError, RequestInfo, RequestManager, ResumeRequestError,
+};
 use crate::taskcluster::Taskcluster;
 use crate::zip::{unzip, ZipError};
 
@@ -85,14 +87,7 @@ where
             }
         };
 
-        let cleanup = guard(
+        let cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info));
-            (request_info.clone(), self.log.clone()),
-            |(request_info, log)| {
-                if let Err(e) = std::fs::remove_dir_all(&request_info.path) {
-                    error!(log, "Could not cleanup request"; "request_id" => %request_info.id, "error" => ?e);
-                }
-            },
-        );
 
         self.send(NewRequestResponse {
             request_id: Ok(request_info.id.clone().into_owned()),
@@ -187,13 +182,18 @@ where
     ) -> Result<(), RunnerProtoError<S, T, P>> {
         info!(self.log, "Received resumption request");
 
-        if let Err(e) = self.request_manager.resume_request(&request.id).await {
+        let request_info = match self.request_manager.resume_request(&request.id).await {
-            self.send(ResumeResponse {
+            Ok(request_info) => request_info,
-                result: Err(e.into_error_message()),
+            Err(e) => {
-            })
+                self.send(ResumeResponse {
-            .await?;
+                    result: Err(e.into_error_message()),
-            return Err(e.into());
+                })
-        }
+                .await?;
+                return Err(e.into());
+            }
+        };
+
+        let _cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info));
 
         self.send(ResumeResponse { result: Ok(()) }).await?;
 

fxrunner/src/lib/request.rs

@@ -10,6 +10,8 @@ use std::path::{Path, PathBuf};
 use async_trait::async_trait;
 use rand::distributions::Alphanumeric;
 use rand::prelude::*;
+use scopeguard::{guard, ScopeGuard};
+use slog::error;
 use thiserror::Error;
 use tokio::fs::create_dir;
 
@@ -46,12 +48,16 @@ pub trait RequestManager {
 }
 
 pub struct FsRequestManager {
+    log: slog::Logger,
     path: PathBuf,
 }
 
 impl FsRequestManager {
-    pub fn new(path: &Path) -> Self {
+    pub fn new(log: slog::Logger, path: &Path) -> Self {
-        FsRequestManager { path: path.into() }
+        FsRequestManager {
+            log,
+            path: path.into(),
+        }
     }
 }
 
@@ -119,14 +125,21 @@ impl RequestManager for FsRequestManager {
             });
         }
 
-        if !path.join("profile").is_dir_async().await {
+        let request_info = RequestInfo {
+            path,
+            id: Cow::Borrowed(request_id),
+        };
+
+        let cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info));
+
+        if !request_info.path.join("profile").is_dir_async().await {
             return Err(ResumeRequestError {
                 kind: ResumeRequestErrorKind::MissingProfile,
                 request_id: request_id.into(),
             });
         }
 
-        let firefox_path = path.join("firefox");
+        let firefox_path = request_info.path.join("firefox");
         let bin_path = firefox_path.join("firefox.exe");
         if !firefox_path.is_dir_async().await || !bin_path.is_file_async().await {
             return Err(ResumeRequestError {
@@ -135,10 +148,8 @@ impl RequestManager for FsRequestManager {
             });
         }
 
-        Ok(RequestInfo {
+        drop(ScopeGuard::into_inner(cleanup));
-            path,
+        Ok(request_info)
-            id: Cow::Borrowed(request_id),
-        })
     }
 
     async fn ensure_valid_profile_dir<'a>(
@@ -186,3 +197,18 @@ pub enum NewRequestError {
 fn validate_request_id(request_id: &str) -> bool {
     request_id.len() == REQUEST_ID_LEN && request_id.chars().all(|c| c.is_ascii_alphanumeric())
 }
+
+/// Synchronously cleanup a request given by the request info.
+pub fn cleanup_request(log: slog::Logger, request_info: &RequestInfo<'_>) {
+    // This must be performed synchronously because there is no async version of
+    // the drop trait.
+    //
+    // A future could be spawned that would trigger when the guard goes out of
+    // scope, but we cannot `await` its completion.
+    //
+    // Having a synchronous operation in the failure case seems like an okay
+    // compromise.
+    if let Err(e) = std::fs::remove_dir_all(&request_info.path) {
+        error!(log, "Could not cleanup request"; "request_id" => %request_info.id, "error" => %e);
+    }
+}