[PATCH proxmox-backup] api/tools: avoid showing error on missing manifest during file listing

[PATCH proxmox-backup] api/tools: avoid showing error on missing manifest during file listing

[Christian Ebner]

When listing the contents of a datastore, a missing manifest blob
file is currently being logged as error to the systemd journal [0].
The manifest missing is however normal operation in case of a still
ongoing backup. Therefore, refactor the code such that a missing
manifest is not treated as regular error by returning an Option::None
in the read_backup_index() helper, and handle this case accordingly.

The actual check for the missing manifest should further be improved,
but requires a more in depth refactoring, the changes here acting as
a stop gap for not showing the benign error the time being.

[0] error during snapshot file listing: 'unable to load blob '"/mnt/datastore/main/vm/400004/2026-04-24T15:20:18Z/index.json.blob"' - No such file or directory (os error 2)'

Reported-by: Stefan Hanreich <s.hanreich@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 18 ++++++---
 src/tools/mod.rs            | 79 ++++++++++++++++++++++---------------
 2 files changed, 60 insertions(+), 37 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a814c076c..3e52cd581 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -413,11 +413,13 @@ pub async fn list_snapshot_files(
             &backup_dir.group,
         )?;
 
-        let snapshot = datastore.backup_dir(ns, backup_dir)?;
+        let snapshot = datastore.backup_dir(ns, backup_dir.clone())?;
 
         let info = BackupInfo::new(snapshot)?;
 
-        let (_manifest, files) = get_all_snapshot_files(&info)?;
+        let Some((_manifest, files)) = get_all_snapshot_files(&info)? else {
+            bail!("manifest not found for snapshot '{backup_dir}'");
+        };
 
         Ok(files)
     })
@@ -1500,7 +1502,9 @@ pub fn download_file_decoded(
             required_string_param(&param, "file-name")?.try_into()?;
         let backup_dir = datastore.backup_dir(backup_ns.clone(), backup_dir_api.clone())?;
 
-        let (manifest, files) = read_backup_index(&backup_dir)?;
+        let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
+            bail!("manifest not found for snapshot '{}'", backup_dir.dir());
+        };
         for file in files {
             if file.filename == file_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
                 bail!("cannot decode '{}' - is encrypted", file_name);
@@ -1725,7 +1729,9 @@ pub async fn catalog(
 
     let backup_dir = datastore.backup_dir(ns, backup_dir)?;
 
-    let (manifest, files) = read_backup_index(&backup_dir)?;
+    let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
+        bail!("manifest not found for snapshot '{}'", backup_dir.dir());
+    };
     for file in files {
         if file.filename == file_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
             bail!("cannot decode '{file_name}' - is encrypted");
@@ -1866,7 +1872,9 @@ pub fn pxar_file_download(
             (pxar_name.to_owned(), file_path.to_owned())
         };
         let pxar_name: BackupArchiveName = std::str::from_utf8(&pxar_name)?.try_into()?;
-        let (manifest, files) = read_backup_index(&backup_dir)?;
+        let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
+            bail!("manifest not found for snapshot '{}'", backup_dir.dir());
+        };
         for file in files {
             if file.filename == pxar_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
                 bail!("cannot decode '{}' - is encrypted", pxar_name);
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index 56ae4bc10..7c2bcfd61 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -46,8 +46,19 @@ pub fn setup_safe_path_env() {
 
 pub(crate) fn read_backup_index(
     backup_dir: &BackupDir,
-) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-    let (manifest, index_size) = backup_dir.load_manifest()?;
+) -> Result<Option<(BackupManifest, Vec<BackupContent>)>, Error> {
+    let (manifest, index_size) = match backup_dir.load_manifest() {
+        Ok((manifest, index_size)) => (manifest, index_size),
+        Err(err) => {
+            let mut manifest_path = backup_dir.full_path();
+            manifest_path.push(MANIFEST_BLOB_NAME.as_ref());
+            if !manifest_path.exists() {
+                return Ok(None);
+            } else {
+                return Err(err);
+            }
+        }
+    };
 
     let mut result = Vec::new();
     for item in manifest.files() {
@@ -67,13 +78,15 @@ pub(crate) fn read_backup_index(
         size: Some(index_size),
     });
 
-    Ok((manifest, result))
+    Ok(Some((manifest, result)))
 }
 
 pub(crate) fn get_all_snapshot_files(
     info: &BackupInfo,
-) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-    let (manifest, mut files) = read_backup_index(&info.backup_dir)?;
+) -> Result<Option<(BackupManifest, Vec<BackupContent>)>, Error> {
+    let Some((manifest, mut files)) = read_backup_index(&info.backup_dir)? else {
+        return Ok(None);
+    };
 
     let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
         acc.insert(item.filename.clone());
@@ -91,7 +104,7 @@ pub(crate) fn get_all_snapshot_files(
         });
     }
 
-    Ok((manifest, files))
+    Ok(Some((manifest, files)))
 }
 
 /// Helper to transform `BackupInfo` to `SnapshotListItem` with given owner.
@@ -104,7 +117,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
     let owner = Some(owner.to_owned());
 
     match get_all_snapshot_files(info) {
-        Ok((manifest, files)) => {
+        Ok(Some((manifest, files))) => {
             // extract the first line from notes
             let comment: Option<String> = manifest.unprotected["notes"]
                 .as_str()
@@ -129,7 +142,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
 
             let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
 
-            SnapshotListItem {
+            return SnapshotListItem {
                 backup,
                 comment,
                 verification,
@@ -138,31 +151,33 @@ pub(crate) fn backup_info_to_snapshot_list_item(
                 size,
                 owner,
                 protected,
-            }
-        }
-        Err(err) => {
-            eprintln!("error during snapshot file listing: '{err}'");
-            let files = info
-                .files
-                .iter()
-                .map(|filename| BackupContent {
-                    filename: filename.to_owned(),
-                    size: None,
-                    crypt_mode: None,
-                })
-                .collect();
-
-            SnapshotListItem {
-                backup,
-                comment: None,
-                verification: None,
-                fingerprint: None,
-                files,
-                size: None,
-                owner,
-                protected,
-            }
+            };
         }
+        Ok(None) => (),
+        Err(err) => eprintln!("error during snapshot file listing: '{err}'"),
+    }
+
+    // no manifest (possibly ongoing backup) or manifest read error,
+    // fallback to listing without any additional metadata.
+    let files = info
+        .files
+        .iter()
+        .map(|filename| BackupContent {
+            filename: filename.to_owned(),
+            size: None,
+            crypt_mode: None,
+        })
+        .collect();
+
+    SnapshotListItem {
+        backup,
+        comment: None,
+        verification: None,
+        fingerprint: None,
+        files,
+        size: None,
+        owner,
+        protected,
     }
 }
 
-- 
2.47.3

Re: [PATCH proxmox-backup] api/tools: avoid showing error on missing manifest during file listing

[Fabian Grünbichler]

On April 26, 2026 10:03 am, Christian Ebner wrote:
> When listing the contents of a datastore, a missing manifest blob
> file is currently being logged as error to the systemd journal [0].
> The manifest missing is however normal operation in case of a still
> ongoing backup. Therefore, refactor the code such that a missing
> manifest is not treated as regular error by returning an Option::None
> in the read_backup_index() helper, and handle this case accordingly.
> 
> The actual check for the missing manifest should further be improved,
> but requires a more in depth refactoring, the changes here acting as
> a stop gap for not showing the benign error the time being.

how about the following instead? only log the error if the manifest file
has existed before we tried to load and parse it? that should limit the
log spam to
- actually broken manifests
- manifests which disappear right between those two calls

----8<----

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 69949c511..a5456e21b 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -957,6 +957,12 @@ impl BackupDir {
         crate::SnapshotReader::new_do(self.clone())
     }
 
+    /// Returns whether a manifest file exists
+    pub fn has_manifest(&self) -> bool {
+        let manifest_path = self.full_path().join(MANIFEST_BLOB_NAME.as_ref());
+        manifest_path.exists()
+    }
+
     /// Load the manifest without a lock. Must not be written back.
     pub fn load_manifest(&self) -> Result<(BackupManifest, u64), Error> {
         let blob = self.load_blob(MANIFEST_BLOB_NAME.as_ref())?;
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index 56ae4bc10..6cd0c8353 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -103,6 +103,8 @@ pub(crate) fn backup_info_to_snapshot_list_item(
     let protected = info.protected;
     let owner = Some(owner.to_owned());
 
+    let has_manifest = info.backup_dir.has_manifest();
+
     match get_all_snapshot_files(info) {
         Ok((manifest, files)) => {
             // extract the first line from notes
@@ -141,7 +143,9 @@ pub(crate) fn backup_info_to_snapshot_list_item(
             }
         }
         Err(err) => {
-            eprintln!("error during snapshot file listing: '{err}'");
+            if has_manifest {
+                eprintln!("error during snapshot file listing: '{err}'");
+            }
             let files = info
                 .files
                 .iter()

---->8----

> 
> [0] error during snapshot file listing: 'unable to load blob '"/mnt/datastore/main/vm/400004/2026-04-24T15:20:18Z/index.json.blob"' - No such file or directory (os error 2)'
> 
> Reported-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 18 ++++++---
>  src/tools/mod.rs            | 79 ++++++++++++++++++++++---------------
>  2 files changed, 60 insertions(+), 37 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index a814c076c..3e52cd581 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -413,11 +413,13 @@ pub async fn list_snapshot_files(
>              &backup_dir.group,
>          )?;
>  
> -        let snapshot = datastore.backup_dir(ns, backup_dir)?;
> +        let snapshot = datastore.backup_dir(ns, backup_dir.clone())?;
>  
>          let info = BackupInfo::new(snapshot)?;
>  
> -        let (_manifest, files) = get_all_snapshot_files(&info)?;
> +        let Some((_manifest, files)) = get_all_snapshot_files(&info)? else {
> +            bail!("manifest not found for snapshot '{backup_dir}'");
> +        };
>  
>          Ok(files)
>      })
> @@ -1500,7 +1502,9 @@ pub fn download_file_decoded(
>              required_string_param(&param, "file-name")?.try_into()?;
>          let backup_dir = datastore.backup_dir(backup_ns.clone(), backup_dir_api.clone())?;
>  
> -        let (manifest, files) = read_backup_index(&backup_dir)?;
> +        let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
> +            bail!("manifest not found for snapshot '{}'", backup_dir.dir());
> +        };
>          for file in files {
>              if file.filename == file_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
>                  bail!("cannot decode '{}' - is encrypted", file_name);
> @@ -1725,7 +1729,9 @@ pub async fn catalog(
>  
>      let backup_dir = datastore.backup_dir(ns, backup_dir)?;
>  
> -    let (manifest, files) = read_backup_index(&backup_dir)?;
> +    let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
> +        bail!("manifest not found for snapshot '{}'", backup_dir.dir());
> +    };
>      for file in files {
>          if file.filename == file_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
>              bail!("cannot decode '{file_name}' - is encrypted");
> @@ -1866,7 +1872,9 @@ pub fn pxar_file_download(
>              (pxar_name.to_owned(), file_path.to_owned())
>          };
>          let pxar_name: BackupArchiveName = std::str::from_utf8(&pxar_name)?.try_into()?;
> -        let (manifest, files) = read_backup_index(&backup_dir)?;
> +        let Some((manifest, files)) = read_backup_index(&backup_dir)? else {
> +            bail!("manifest not found for snapshot '{}'", backup_dir.dir());
> +        };
>          for file in files {
>              if file.filename == pxar_name.as_ref() && file.crypt_mode == Some(CryptMode::Encrypt) {
>                  bail!("cannot decode '{}' - is encrypted", pxar_name);
> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
> index 56ae4bc10..7c2bcfd61 100644
> --- a/src/tools/mod.rs
> +++ b/src/tools/mod.rs
> @@ -46,8 +46,19 @@ pub fn setup_safe_path_env() {
>  
>  pub(crate) fn read_backup_index(
>      backup_dir: &BackupDir,
> -) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
> -    let (manifest, index_size) = backup_dir.load_manifest()?;
> +) -> Result<Option<(BackupManifest, Vec<BackupContent>)>, Error> {
> +    let (manifest, index_size) = match backup_dir.load_manifest() {
> +        Ok((manifest, index_size)) => (manifest, index_size),
> +        Err(err) => {
> +            let mut manifest_path = backup_dir.full_path();
> +            manifest_path.push(MANIFEST_BLOB_NAME.as_ref());
> +            if !manifest_path.exists() {
> +                return Ok(None);
> +            } else {
> +                return Err(err);
> +            }
> +        }
> +    };
>  
>      let mut result = Vec::new();
>      for item in manifest.files() {
> @@ -67,13 +78,15 @@ pub(crate) fn read_backup_index(
>          size: Some(index_size),
>      });
>  
> -    Ok((manifest, result))
> +    Ok(Some((manifest, result)))
>  }
>  
>  pub(crate) fn get_all_snapshot_files(
>      info: &BackupInfo,
> -) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
> -    let (manifest, mut files) = read_backup_index(&info.backup_dir)?;
> +) -> Result<Option<(BackupManifest, Vec<BackupContent>)>, Error> {
> +    let Some((manifest, mut files)) = read_backup_index(&info.backup_dir)? else {
> +        return Ok(None);
> +    };
>  
>      let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
>          acc.insert(item.filename.clone());
> @@ -91,7 +104,7 @@ pub(crate) fn get_all_snapshot_files(
>          });
>      }
>  
> -    Ok((manifest, files))
> +    Ok(Some((manifest, files)))
>  }
>  
>  /// Helper to transform `BackupInfo` to `SnapshotListItem` with given owner.
> @@ -104,7 +117,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
>      let owner = Some(owner.to_owned());
>  
>      match get_all_snapshot_files(info) {
> -        Ok((manifest, files)) => {
> +        Ok(Some((manifest, files))) => {
>              // extract the first line from notes
>              let comment: Option<String> = manifest.unprotected["notes"]
>                  .as_str()
> @@ -129,7 +142,7 @@ pub(crate) fn backup_info_to_snapshot_list_item(
>  
>              let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
>  
> -            SnapshotListItem {
> +            return SnapshotListItem {
>                  backup,
>                  comment,
>                  verification,
> @@ -138,31 +151,33 @@ pub(crate) fn backup_info_to_snapshot_list_item(
>                  size,
>                  owner,
>                  protected,
> -            }
> -        }
> -        Err(err) => {
> -            eprintln!("error during snapshot file listing: '{err}'");
> -            let files = info
> -                .files
> -                .iter()
> -                .map(|filename| BackupContent {
> -                    filename: filename.to_owned(),
> -                    size: None,
> -                    crypt_mode: None,
> -                })
> -                .collect();
> -
> -            SnapshotListItem {
> -                backup,
> -                comment: None,
> -                verification: None,
> -                fingerprint: None,
> -                files,
> -                size: None,
> -                owner,
> -                protected,
> -            }
> +            };
>          }
> +        Ok(None) => (),
> +        Err(err) => eprintln!("error during snapshot file listing: '{err}'"),
> +    }
> +
> +    // no manifest (possibly ongoing backup) or manifest read error,
> +    // fallback to listing without any additional metadata.
> +    let files = info
> +        .files
> +        .iter()
> +        .map(|filename| BackupContent {
> +            filename: filename.to_owned(),
> +            size: None,
> +            crypt_mode: None,
> +        })
> +        .collect();
> +
> +    SnapshotListItem {
> +        backup,
> +        comment: None,
> +        verification: None,
> +        fingerprint: None,
> +        files,
> +        size: None,
> +        owner,
> +        protected,
>      }
>  }
>  
> -- 
> 2.47.3
> 
> 
> 
> 
> 
> 

Re: [PATCH proxmox-backup] api/tools: avoid showing error on missing manifest during file listing

[Christian Ebner]

On 4/27/26 11:22 AM, Fabian Grünbichler wrote:
> On April 26, 2026 10:03 am, Christian Ebner wrote:
>> When listing the contents of a datastore, a missing manifest blob
>> file is currently being logged as error to the systemd journal [0].
>> The manifest missing is however normal operation in case of a still
>> ongoing backup. Therefore, refactor the code such that a missing
>> manifest is not treated as regular error by returning an Option::None
>> in the read_backup_index() helper, and handle this case accordingly.
>>
>> The actual check for the missing manifest should further be improved,
>> but requires a more in depth refactoring, the changes here acting as
>> a stop gap for not showing the benign error the time being.
> 
> how about the following instead? only log the error if the manifest file
> has existed before we tried to load and parse it? that should limit the
> log spam to
> - actually broken manifests
> - manifests which disappear right between those two calls
> 

That has one major downside though: This is a highly critical code path, 
the content listing having already been reworked quite a bit int the 
past to not be slow.

So your suggested changes would now unconditionally add more syscalls 
and IO?

I think it would be better to refactor the code in the long run, so a 
missing manifest would be treated differently from parsing and other IO 
errors, and bubbled up to the call site for logging.

Re: [PATCH proxmox-backup] api/tools: avoid showing error on missing manifest during file listing

[Fabian Grünbichler]

On April 27, 2026 11:51 am, Christian Ebner wrote:
> On 4/27/26 11:22 AM, Fabian Grünbichler wrote:
>> On April 26, 2026 10:03 am, Christian Ebner wrote:
>>> When listing the contents of a datastore, a missing manifest blob
>>> file is currently being logged as error to the systemd journal [0].
>>> The manifest missing is however normal operation in case of a still
>>> ongoing backup. Therefore, refactor the code such that a missing
>>> manifest is not treated as regular error by returning an Option::None
>>> in the read_backup_index() helper, and handle this case accordingly.
>>>
>>> The actual check for the missing manifest should further be improved,
>>> but requires a more in depth refactoring, the changes here acting as
>>> a stop gap for not showing the benign error the time being.
>> 
>> how about the following instead? only log the error if the manifest file
>> has existed before we tried to load and parse it? that should limit the
>> log spam to
>> - actually broken manifests
>> - manifests which disappear right between those two calls
>> 
> 
> That has one major downside though: This is a highly critical code path, 
> the content listing having already been reworked quite a bit int the 
> past to not be slow.
> 
> So your suggested changes would now unconditionally add more syscalls 
> and IO?

true, I initially had the call in the Err path (which would mean not
differentiating between "manifest does not exist in the first place" and
"manifest disappeared under us", which is probably fine?)

the additional single stat here should be cheap compared to parsing the
full manifest, if we only do it in the Err(path)..

> I think it would be better to refactor the code in the long run, so a 
> missing manifest would be treated differently from parsing and other IO 
> errors, and bubbled up to the call site for logging.

but yeah, doing that is probably also fine, we just need to really do it
;)