From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 181D21FF09F for ; Thu, 03 Sep 2026 14:16:22 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6C743215FA; Thu, 03 Sep 2026 14:16:21 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup 1/2] fix #7606: verify: s3: handle transient chunk load errors Date: Thu, 3 Sep 2026 14:15:56 +0200 Message-ID: <20260903121557.420018-2-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260903121557.420018-1-c.ebner@proxmox.com> References: <20260903121557.420018-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788437774192 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.675 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 6ZPRPGXJAGZCCQC7URJKS5U736CUOAY5 X-Message-ID-Hash: 6ZPRPGXJAGZCCQC7URJKS5U736CUOAY5 X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Fetching an object for verification might fail when sending the request to the S3 API or while receiving the response. Both are currently handled as hard errors, leading to the manifest state being set to verify failed. This however does not reflect the actual state, as a subsequent verification can work just fine. Therefore, distinguish these error kinds by enum variants, the manifest only marked as failed verification if the error is considered permanent. If only transient errors but no permanent errors occur, strip any verify okay state, since it is not possible to determine the state it is in, but always keep a verify error state in order to avoid allowing backups to reuse this snapshots as reference. Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7606 Signed-off-by: Christian Ebner --- src/backup/verify.rs | 145 +++++++++++++++++++++++++++++++++---------- 1 file changed, 112 insertions(+), 33 deletions(-) diff --git a/src/backup/verify.rs b/src/backup/verify.rs index aeb78c032..e85e120e5 100644 --- a/src/backup/verify.rs +++ b/src/backup/verify.rs @@ -4,7 +4,7 @@ use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; use std::time::Instant; -use anyhow::{Error, bail}; +use anyhow::{Error, format_err}; use http_body_util::BodyExt; use tracing::{error, info, warn}; @@ -39,6 +39,7 @@ struct IndexVerifyState { read_bytes: AtomicU64, decoded_bytes: AtomicU64, errors: AtomicUsize, + fetch_errors: AtomicUsize, datastore: Arc, corrupt_chunks: Arc>>, verified_chunks: Arc>>, @@ -55,6 +56,7 @@ impl IndexVerifyState { read_bytes: AtomicU64::new(0), decoded_bytes: AtomicU64::new(0), errors: AtomicUsize::new(0), + fetch_errors: AtomicUsize::new(0), datastore: Arc::clone(datastore), corrupt_chunks: Arc::clone(corrupt_chunks), verified_chunks: Arc::clone(verified_chunks), @@ -63,6 +65,11 @@ impl IndexVerifyState { } } +enum VerificationError { + Transient(Error), + Permanent(Error), +} + impl VerifyWorker { /// Creates a new VerifyWorker for a given task worker and datastore. pub fn new( @@ -95,27 +102,38 @@ impl VerifyWorker { }) } - fn verify_blob(backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> { - let blob = backup_dir.load_blob(info.filename.as_ref())?; + fn verify_blob(backup_dir: &BackupDir, info: &FileInfo) -> Result<(), VerificationError> { + let blob = backup_dir + .load_blob(info.filename.as_ref()) + .map_err(VerificationError::Permanent)?; let raw_size = blob.raw_size(); if raw_size != info.size { - bail!("wrong size ({} != {})", info.size, raw_size); + return Err(VerificationError::Permanent(format_err!( + "wrong size ({} != {})", + info.size, + raw_size + ))); } let csum = openssl::sha::sha256(blob.raw_data()); if csum != info.csum { - bail!("wrong index checksum"); + return Err(VerificationError::Permanent(format_err!( + "wrong index checksum" + ))); } - match blob.crypt_mode()? { + match blob.crypt_mode().map_err(VerificationError::Permanent)? { CryptMode::Encrypt => Ok(()), CryptMode::None => { // digest already verified above - blob.decode(None, None)?; + blob.decode(None, None) + .map_err(VerificationError::Permanent)?; Ok(()) } - CryptMode::SignOnly => bail!("Invalid CryptMode for blob"), + CryptMode::SignOnly => Err(VerificationError::Permanent(format_err!( + "Invalid CryptMode for blob" + ))), } } @@ -123,7 +141,7 @@ impl VerifyWorker { &self, index: Box, crypt_mode: CryptMode, - ) -> Result<(), Error> { + ) -> Result<(), VerificationError> { let verify_state = Arc::new(IndexVerifyState::new( &self.datastore, &self.corrupt_chunks, @@ -190,7 +208,8 @@ impl VerifyWorker { let chunk_list = self .datastore - .get_chunks_in_order(&*index, skip_chunk, check_abort)?; + .get_chunks_in_order(&*index, skip_chunk, check_abort) + .map_err(VerificationError::Transient)?; let reader_pool = ParallelHandler::new("read chunks", self.read_threads, { let decoder_pool = decoder_pool.channel(); @@ -207,8 +226,12 @@ impl VerifyWorker { } }); for (pos, _) in chunk_list { - self.worker.check_abort()?; - self.worker.fail_on_shutdown()?; + self.worker + .check_abort() + .map_err(VerificationError::Transient)?; + self.worker + .fail_on_shutdown() + .map_err(VerificationError::Transient)?; let info = index.chunk_info(pos).unwrap(); @@ -217,10 +240,14 @@ impl VerifyWorker { continue; // already verified or marked corrupt } - reader_pool.send(info)?; + reader_pool + .send(info) + .map_err(|err| VerificationError::Transient(err.into()))?; } - reader_pool.complete()?; + reader_pool + .complete() + .map_err(|err| VerificationError::Transient(err.into()))?; let elapsed = verify_state.start_time.elapsed().as_secs_f64(); @@ -240,7 +267,15 @@ impl VerifyWorker { ); if verify_state.errors.load(Ordering::SeqCst) > 0 { - bail!("chunks could not be verified"); + return Err(VerificationError::Permanent(format_err!( + "chunks could not be verified" + ))); + } + + if verify_state.fetch_errors.load(Ordering::SeqCst) > 0 { + return Err(VerificationError::Transient(format_err!( + "chunks could not be fetched" + ))); } Ok(()) @@ -296,7 +331,7 @@ impl VerifyWorker { } } Err(err) => { - verify_state.errors.fetch_add(1, Ordering::SeqCst); + verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst); error!("can't verify chunk, load failed - {err}"); } } @@ -310,7 +345,7 @@ impl VerifyWorker { ), ), Err(err) => { - verify_state.errors.fetch_add(1, Ordering::SeqCst); + verify_state.fetch_errors.fetch_add(1, Ordering::SeqCst); error!("can't verify chunk, load failed - {err}"); } } @@ -332,37 +367,63 @@ impl VerifyWorker { } } - fn verify_fixed_index(&self, backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> { + fn verify_fixed_index( + &self, + backup_dir: &BackupDir, + info: &FileInfo, + ) -> Result<(), VerificationError> { let mut path = backup_dir.relative_path(); path.push(info.filename.as_ref()); - let index = self.datastore.open_fixed_reader(&path)?; + let index = self + .datastore + .open_fixed_reader(&path) + .map_err(VerificationError::Permanent)?; let (csum, size) = index.compute_csum(); if size != info.size { - bail!("wrong size ({} != {})", info.size, size); + return Err(VerificationError::Permanent(format_err!( + "wrong size ({} != {})", + info.size, + size + ))); } if csum != info.csum { - bail!("wrong index checksum"); + return Err(VerificationError::Permanent(format_err!( + "wrong index checksum" + ))); } self.verify_index_chunks(Box::new(index), info.chunk_crypt_mode()) } - fn verify_dynamic_index(&self, backup_dir: &BackupDir, info: &FileInfo) -> Result<(), Error> { + fn verify_dynamic_index( + &self, + backup_dir: &BackupDir, + info: &FileInfo, + ) -> Result<(), VerificationError> { let mut path = backup_dir.relative_path(); path.push(info.filename.as_ref()); - let index = self.datastore.open_dynamic_reader(&path)?; + let index = self + .datastore + .open_dynamic_reader(&path) + .map_err(VerificationError::Permanent)?; let (csum, size) = index.compute_csum(); if size != info.size { - bail!("wrong size ({} != {})", info.size, size); + return Err(VerificationError::Permanent(format_err!( + "wrong size ({} != {})", + info.size, + size + ))); } if csum != info.csum { - bail!("wrong index checksum"); + return Err(VerificationError::Permanent(format_err!( + "wrong index checksum" + ))); } self.verify_index_chunks(Box::new(index), info.chunk_crypt_mode()) @@ -438,7 +499,7 @@ impl VerifyWorker { let mut error_count = 0; - let mut verify_result = VerifyState::Ok; + let mut verify_result = Some(VerifyState::Ok); for info in manifest.files() { let result = proxmox_lang::try_block!({ info!(" check {}", info.filename); @@ -453,22 +514,40 @@ impl VerifyWorker { self.worker.fail_on_shutdown()?; if let Err(err) = result { + let err = match err { + VerificationError::Permanent(err) => { + verify_result = Some(VerifyState::Failed); + err + } + VerificationError::Transient(err) => { + if let Some(SnapshotVerifyState { + state: VerifyState::Failed, + .. + }) = manifest.verify_state()? + { + verify_result = Some(VerifyState::Failed); + } else if let Some(VerifyState::Ok) = verify_result { + verify_result = None; + } + err + } + }; info!( "verify {datastore_name}:{backup_dir_name}/{file_name} failed: {err}", file_name = info.filename, ); error_count += 1; - verify_result = VerifyState::Failed; } } - let verify_state = SnapshotVerifyState { - state: verify_result, - upid, - }; - if let Err(err) = { - let verify_state = serde_json::to_value(verify_state)?; + let verify_state = match verify_result { + Some(state) => { + let state = SnapshotVerifyState { state, upid }; + serde_json::to_value(state)? + } + None => serde_json::Value::Null, + }; backup_dir.update_manifest(&self.datastore.backend()?, |manifest| { manifest.unprotected["verify_state"] = verify_state; }) -- 2.47.3