From 20aa48cce07f8478e5836d39d47cf691cb959fa9 Mon Sep 17 00:00:00 2001 From: Roland Sherwin Date: Wed, 22 May 2024 21:32:12 +0530 Subject: [PATCH] fix(uploader): do not error out immediately on max repayment errors --- sn_client/src/error.rs | 4 ++-- sn_client/src/uploader/tests/mod.rs | 2 +- sn_client/src/uploader/upload.rs | 33 +++++++++++++++++++++-------- 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/sn_client/src/error.rs b/sn_client/src/error.rs index 60404defe9..d01f1068d0 100644 --- a/sn_client/src/error.rs +++ b/sn_client/src/error.rs @@ -140,8 +140,8 @@ pub enum Error { #[error("Too many sequential payment errors reported during upload")] SequentialUploadPaymentError, - #[error("The maximum specified repayments were made for the address: {0:?}")] - MaximumRepaymentsReached(XorName), + #[error("The maximum specified repayments has been reached")] + MaximumRepaymentsReached, #[error("Error occurred when access wallet file")] FailedToAccessWallet, diff --git a/sn_client/src/uploader/tests/mod.rs b/sn_client/src/uploader/tests/mod.rs index b2e1728918..67f0f0ac1a 100644 --- a/sn_client/src/uploader/tests/mod.rs +++ b/sn_client/src/uploader/tests/mod.rs @@ -448,7 +448,7 @@ async fn maximum_repayment_error_should_be_triggered_during_get_store_cost() -> assert_matches!( upload_handle.await?, - Err(ClientError::MaximumRepaymentsReached(_)) + Err(ClientError::MaximumRepaymentsReached) ); let events = events_handle.await?; diff --git a/sn_client/src/uploader/upload.rs b/sn_client/src/uploader/upload.rs index f768fa48af..1759cb4105 100644 --- a/sn_client/src/uploader/upload.rs +++ b/sn_client/src/uploader/upload.rs @@ -36,6 +36,9 @@ use xor_name::XorName; /// The number of repayments to attempt for a failed item before returning an error. /// If value = 1, we do an initial payment & 1 repayment. Thus we make a max 2 payments per data item. +#[cfg(not(test))] +pub(super) const MAX_REPAYMENTS_PER_FAILED_ITEM: usize = 3; +#[cfg(test)] pub(super) const MAX_REPAYMENTS_PER_FAILED_ITEM: usize = 1; /// The maximum number of sequential payment failures before aborting the upload process. @@ -120,6 +123,14 @@ pub(super) async fn start_upload( #[cfg(test)] trace!("UPLOADER STATE: finished uploading all items {uploader:?}"); + if !uploader.max_repayments_reached.is_empty() { + error!( + "The maximum repayments were reached for these addresses: {:?}", + uploader.max_repayments_reached + ); + return Err(ClientError::MaximumRepaymentsReached); + } + let summary = UploadSummary { storage_cost: uploader.upload_storage_cost, royalty_fees: uploader.upload_royalty_fees, @@ -353,16 +364,18 @@ pub(super) async fn start_upload( max_repayments_reached, } => { let _ = uploader.on_going_get_cost.remove(&xorname); - // use the same strategy. The repay different payee is set only if upload fails. - uploader - .pending_to_get_store_cost - .push((xorname, get_store_cost_strategy.clone())); trace!("GetStoreCostErr for {xorname:?} , get_store_cost_strategy: {get_store_cost_strategy:?}, max_repayments_reached: {max_repayments_reached:?}"); - // should we do something more here? + // If max repayments reached, track it separately. Else retry get_store_cost. if max_repayments_reached { - error!("Max repayments reached for {xorname:?}"); - return Err(ClientError::MaximumRepaymentsReached(xorname)); + error!("Max repayments reached for {xorname:?}. Skipping upload for it"); + uploader.max_repayments_reached.insert(xorname); + uploader.all_upload_items.remove(&xorname); + } else { + // use the same strategy. The repay different payee is set only if upload fails. + uploader + .pending_to_get_store_cost + .push((xorname, get_store_cost_strategy.clone())); } uploader.get_store_cost_errors += 1; if uploader.get_store_cost_errors > MAX_SEQUENTIAL_NETWORK_ERRORS { @@ -522,7 +535,7 @@ impl UploaderInterface for Uploader { error!("Encountered error {err:?} when getting store_cost for {xorname:?}",); let max_repayments_reached = - matches!(&err, ClientError::MaximumRepaymentsReached(_)); + matches!(&err, ClientError::MaximumRepaymentsReached); TaskResult::GetStoreCostErr { xorname, @@ -672,6 +685,7 @@ pub(super) struct InnerUploader { pub(super) upload_storage_cost: NanoTokens, pub(super) upload_royalty_fees: NanoTokens, pub(super) upload_final_balance: NanoTokens, + pub(super) max_repayments_reached: BTreeSet, pub(super) uploaded_addresses: BTreeSet, pub(super) uploaded_registers: BTreeMap, pub(super) uploaded_count: usize, @@ -712,6 +726,7 @@ impl InnerUploader { n_errors_during_uploads: Default::default(), push_register_errors: Default::default(), get_store_cost_errors: Default::default(), + max_repayments_reached: Default::default(), make_payments_errors: Default::default(), upload_storage_cost: NanoTokens::zero(), @@ -925,7 +940,7 @@ impl InnerUploader { max_repayments_for_failed_data, ) { // error is used by the caller. - return Err(ClientError::MaximumRepaymentsReached(xorname)); + return Err(ClientError::MaximumRepaymentsReached); } debug!("Filtering out payments from {filter_list:?} during get_store_cost for {xorname:?}");