Fix/9548 auto renewal persistence

prdoc/pr_9548.prdoc

@@ -0,0 +1,20 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "[pallet-broker] Fix auto-renewal persistence after failed renewal attempts"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Fixes auto-renewal behavior to match documentation. Previously, when an 
+      auto-renewal attempt failed (e.g., due to insufficient funds), the 
+      auto-renewal setting was removed entirely. Now the setting persists 
+      across multiple periods as documented, allowing automatic retry in 
+      subsequent periods.
+
+      This ensures that temporary issues (like insufficient balance) don't 
+      require manual re-enablement of auto-renewal.
+
+crates:
+  - name: pallet-broker
+    bump: patch

substrate/frame/broker/src/tests.rs

@@ -2247,15 +2247,16 @@ fn auto_renewal_works() {
 			.into(),
 		);
 
-		// Given that core #1 didn't get renewed due to the account not being sufficiently funded,
-		// Task (1003) will now be assigned to that core instead of core #2.
-		assert_eq!(
-			AutoRenewals::<Test>::get().to_vec(),
-			vec![
-				AutoRenewalRecord { core: 0, task: 1001, next_renewal: 10 },
-				AutoRenewalRecord { core: 1, task: 1003, next_renewal: 10 },
-			]
-		);
+	// With the fix, the auto-renewal setting for task 1002 should persist despite the failure.
+	// Task 1002 should still be in the list for the next renewal attempt.
+	assert_eq!(
+		AutoRenewals::<Test>::get().to_vec(),
+		vec![
+			AutoRenewalRecord { core: 0, task: 1001, next_renewal: 10 },
+			AutoRenewalRecord { core: 1, task: 1002, next_renewal: 10 },
+			AutoRenewalRecord { core: 1, task: 1003, next_renewal: 10 },
+		]
+	);
 	});
 }
 
@@ -2295,6 +2296,44 @@ fn disable_auto_renew_works() {
 	});
 }
 
+#[test]
+fn auto_renewal_persists_after_failure() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(100, 3));
+		advance_to(2);
+		let region = Broker::do_purchase(1, u64::max_value()).unwrap();
+
+		// Enable auto-renewal for a task
+		assert_ok!(Broker::do_assign(region, Some(1), 1001, Final));
+		assert_ok!(Broker::do_enable_auto_renew(1001, region.core, 1001, Some(7)));
+		assert_eq!(
+			AutoRenewals::<Test>::get().to_vec(),
+			vec![AutoRenewalRecord { core: 0, task: 1001, next_renewal: 7 }]
+		);
+
+		// Advance to the next sale period WITHOUT funding the sovereign account
+		// This should cause the renewal to fail due to insufficient funds
+		advance_to(7);
+
+		// Verify that renewal failed
+		System::assert_has_event(
+			Event::<Test>::AutoRenewalFailed { core: 0, payer: Some(1001) }.into(),
+		);
+
+		// The key fix: auto-renewal setting should persist despite the failure.
+		// Previously, the setting would be removed. Now it should remain with next_renewal
+		// updated to the next period (10).
+		assert_eq!(
+			AutoRenewals::<Test>::get().to_vec(),
+			vec![AutoRenewalRecord { core: 0, task: 1001, next_renewal: 10 }]
+		);
+
+		// The auto-renewal setting is preserved for future attempts, demonstrating that
+		// the fix works as per documentation: "Even if an auto-renewal attempt fails,
+		// the auto-renewal setting remains active for subsequent sales."
+	});
+}
+
 #[test]
 fn remove_assignment_works() {
 	TestExt::new().endow(1, 1000).execute_with(|| {

substrate/frame/broker/src/tick_impls.rs

@@ -351,28 +351,42 @@ impl<T: Config> Pallet<T> {
 					return Some(record)
 				}
 
-				let Some(payer) = T::SovereignAccountOf::maybe_convert(record.task) else {
-					Self::deposit_event(Event::<T>::AutoRenewalFailed {
-						core: record.core,
-						payer: None,
-					});
-					return None
-				};
+			let Some(payer) = T::SovereignAccountOf::maybe_convert(record.task) else {
+				Self::deposit_event(Event::<T>::AutoRenewalFailed {
+					core: record.core,
+					payer: None,
+				});
+				// Keep the auto-renewal setting active for the next period as per documentation:
+				// "Even if an auto-renewal attempt fails, the auto-renewal setting remains
+				// active for subsequent sales."
+				return Some(AutoRenewalRecord {
+					core: record.core,
+					task: record.task,
+					next_renewal: sale.region_end,
+				})
+			};
+
+			if let Ok(new_core_index) = Self::do_renew(payer.clone(), record.core) {
+				Some(AutoRenewalRecord {
+					core: new_core_index,
+					task: record.task,
+					next_renewal: sale.region_end,
+				})
+			} else {
+				Self::deposit_event(Event::<T>::AutoRenewalFailed {
+					core: record.core,
+					payer: Some(payer),
+				});
 
-				if let Ok(new_core_index) = Self::do_renew(payer.clone(), record.core) {
-					Some(AutoRenewalRecord {
-						core: new_core_index,
-						task: record.task,
-						next_renewal: sale.region_end,
-					})
-				} else {
-					Self::deposit_event(Event::<T>::AutoRenewalFailed {
-						core: record.core,
-						payer: Some(payer),
-					});
-
-					None
-				}
+				// Keep the auto-renewal setting active for the next period as per documentation:
+				// "Even if an auto-renewal attempt fails, the auto-renewal setting remains
+				// active for subsequent sales."
+				Some(AutoRenewalRecord {
+					core: record.core,
+					task: record.task,
+					next_renewal: sale.region_end,
+				})
+			}
 			})
 			.collect::<Vec<AutoRenewalRecord>>()
 			.try_into()