[pbs-devel] [PATCH proxmox-backup] rest-server: cleanup_old_tasks: improve error handling

[pbs-devel] [PATCH proxmox-backup] rest-server: cleanup_old_tasks: improve error handling

[Dominik Csapak]

by not bubbling up most errors, and continuing on. this avoids that we
stop cleaning up because e.g. one directory was missing.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-rest-server/src/worker_task.rs | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index 643e1872..619b8e9d 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -270,18 +270,33 @@ pub fn cleanup_old_tasks(compressed: bool) -> Result<(), Error> {
         for i in 0..256 {
             let mut path = setup.taskdir.clone();
             path.push(format!("{:02X}", i));
-            for file in std::fs::read_dir(path)? {
-                let file = file?;
+            let files = match std::fs::read_dir(path) {
+                Ok(files) => files,
+                Err(_) => continue, // does not exist or no permissions
+            };
+            for file in files {
+                let file = match file {
+                    Ok(file) => file,
+                    Err(err) => {
+                        log::error!("task cleanup: could not check some file in {}: {}", i, err);
+                        continue;
+                    }
+                };
                 let path = file.path();
 
-                let modified = get_modified(file)
-                    .map_err(|err| format_err!("error getting mtime for {:?}: {}", path, err))?;
+                let modified = match get_modified(file) {
+                    Ok(modified) => modified,
+                    Err(err) => {
+                        log::error!("task cleanup: error getting mtime for {:?}: {}", path, err);
+                        continue;
+                    }
+                };
 
                 if modified < cutoff_time {
                     match std::fs::remove_file(path) {
                         Ok(()) => {},
                         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {},
-                        Err(err) => bail!("could not remove file: {}", err),
+                        Err(err) => log::error!("task cleanup: could not remove file: {}", err),
                     }
                 }
             }
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] rest-server: cleanup_old_tasks: improve error handling

[Thomas Lamprecht]

On 25.03.22 13:38, Dominik Csapak wrote:
> by not bubbling up most errors, and continuing on. this avoids that we
> stop cleaning up because e.g. one directory was missing.

continuing is OK but if an those errors are encountered I'd still return an Err(),
and not an Ok(), at the end to make any caller easily distinguish the full OK or
error case.

If we expect such errors to happen there's something wrong with the general approach,
then the known false positives should be filtered.

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-rest-server/src/worker_task.rs | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
> index 643e1872..619b8e9d 100644
> --- a/proxmox-rest-server/src/worker_task.rs
> +++ b/proxmox-rest-server/src/worker_task.rs
> @@ -270,18 +270,33 @@ pub fn cleanup_old_tasks(compressed: bool) -> Result<(), Error> {
>          for i in 0..256 {
>              let mut path = setup.taskdir.clone();
>              path.push(format!("{:02X}", i));
> -            for file in std::fs::read_dir(path)? {
> -                let file = file?;
> +            let files = match std::fs::read_dir(path) {
> +                Ok(files) => files,
> +                Err(_) => continue, // does not exist or no permissions

hmm, silently ignoring ENOENT is one thing, but I'd not do so for other errors, at least
print a note/warning?

> +            };
> +            for file in files {
> +                let file = match file {
> +                    Ok(file) => file,
> +                    Err(err) => {
> +                        log::error!("task cleanup: could not check some file in {}: {}", i, err);
> +                        continue;
> +                    }
> +                };
>                  let path = file.path();
>  
> -                let modified = get_modified(file)
> -                    .map_err(|err| format_err!("error getting mtime for {:?}: {}", path, err))?;
> +                let modified = match get_modified(file) {
> +                    Ok(modified) => modified,
> +                    Err(err) => {
> +                        log::error!("task cleanup: error getting mtime for {:?}: {}", path, err);
> +                        continue;
> +                    }
> +                };
>  
>                  if modified < cutoff_time {
>                      match std::fs::remove_file(path) {
>                          Ok(()) => {},
>                          Err(err) if err.kind() == std::io::ErrorKind::NotFound => {},
> -                        Err(err) => bail!("could not remove file: {}", err),
> +                        Err(err) => log::error!("task cleanup: could not remove file: {}", err),
>                      }
>                  }
>              }