Minor pallet-scheduler documentation/unit test additions (#10511)

# Description

This adds some comments to scheduler pallet calls, a couple of checks to
`Lookup<T>` in some unit tests, and a test that verifies rescheduling of
a task to a full block.

## Integration

N/A

## Review Notes

N/A

---------

Co-authored-by: Dónal Murray <[email protected]>

Author: rockbmb

prdoc/pr_10511.prdoc

@@ -0,0 +1,12 @@
+title: Minor pallet-scheduler documentation/unit test additions
+doc:
+- audience: Runtime Dev
+  description: |-
+    This PR changes the Rust documentation of some extrinsic calls in the scheduler pallet.
+
+    It also adds some checks to the `Lookup<T>` storage item in a unit test to named task cancellation via
+    the anonymous cancellation call, and a unit test for the rescheduling of a failed named/anonymous task
+    to a full agenda.
+crates:
+- name: pallet-scheduler
+  bump: patch

substrate/frame/scheduler/src/lib.rs

@@ -484,7 +484,10 @@ pub mod pallet {
 			Ok(())
 		}
 
-		/// Cancel an anonymously scheduled task.
+		/// Cancel a scheduled task (named or anonymous), by providing the block it is scheduled for
+		/// execution in, as well as the index of the task in that block's agenda.
+		///
+		/// In the case of a named task, it will remove it from the lookup table as well.
 		#[pallet::call_index(1)]
 		#[pallet::weight(<T as Config>::WeightInfo::cancel(T::MaxScheduledPerBlock::get()))]
 		pub fn cancel(origin: OriginFor<T>, when: BlockNumberFor<T>, index: u32) -> DispatchResult {
@@ -586,6 +589,8 @@ pub mod pallet {
 		/// clones of the original task. Their retry configuration will be derived from the
 		/// original task's configuration, but will have a lower value for `remaining` than the
 		/// original `total_retries`.
+		///
+		/// This call **cannot** be used to set a retry configuration for a named task.
 		#[pallet::call_index(6)]
 		#[pallet::weight(<T as Config>::WeightInfo::set_retry())]
 		pub fn set_retry(
@@ -623,6 +628,8 @@ pub mod pallet {
 		/// clones of the original task. Their retry configuration will be derived from the
 		/// original task's configuration, but will have a lower value for `remaining` than the
 		/// original `total_retries`.
+		///
+		/// This is the only way to set a retry configuration for a named task.
 		#[pallet::call_index(7)]
 		#[pallet::weight(<T as Config>::WeightInfo::set_retry_named())]
 		pub fn set_retry_named(
@@ -1021,11 +1028,16 @@ impl<T: Config> Pallet<T> {
 	fn cleanup_agenda(when: BlockNumberFor<T>) {
 		let mut agenda = Agenda::<T>::get(when);
 		match agenda.iter().rposition(|i| i.is_some()) {
+			// Note that `agenda.len() > i + 1` implies that the agenda ends on a sequence of at
+			// least one `None` item(s).
 			Some(i) if agenda.len() > i + 1 => {
 				agenda.truncate(i + 1);
 				Agenda::<T>::insert(when, agenda);
 			},
+			// This branch is taken if `agenda.len() <= i + 1 ==> agenda.len() == i + 1 <==>
+			// agenda.len() - 1 == i` i.e. the agenda's last item is `Some`.
 			Some(_) => {},
+			// All items in the agenda are `None`.
 			None => {
 				Agenda::<T>::remove(when);
 			},

substrate/frame/scheduler/src/tests.rs

@@ -1401,6 +1401,91 @@ fn try_schedule_retry_respects_weight_limits() {
 	});
 }
 
+fn schedule_retry_fails_when_retry_target_block_is_full(named: bool) {
+	let max: u32 = <Test as Config>::MaxScheduledPerBlock::get();
+	let retry_period = 3;
+	let task_name = [42u8; 32];
+
+	new_test_ext().execute_with(|| {
+		// Task will fail until block 100 (effectively always fails in this test).
+		Threshold::<Test>::put((100, 200));
+
+		// Fill block 7 (4 + retry_period) to capacity with dummy tasks.
+		let filler_call =
+			RuntimeCall::Logger(LoggerCall::log { i: 99, weight: Weight::from_parts(10, 0) });
+		let filler_bound = Preimage::bound(filler_call).unwrap();
+		for _ in 0..max {
+			assert_ok!(Scheduler::do_schedule(
+				DispatchTime::At(4 + retry_period),
+				None,
+				127,
+				root(),
+				filler_bound.clone(),
+			));
+		}
+		assert_eq!(Agenda::<Test>::get(4 + retry_period).len() as u32, max);
+
+		// Schedule a task at block 4 that will fail.
+		let failing_call =
+			RuntimeCall::Logger(LoggerCall::timed_log { i: 42, weight: Weight::from_parts(10, 0) });
+		if named {
+			assert_ok!(Scheduler::do_schedule_named(
+				task_name,
+				DispatchTime::At(4),
+				None,
+				127,
+				root(),
+				Preimage::bound(failing_call).unwrap(),
+			));
+			// Set retry config for named task.
+			assert_ok!(Scheduler::set_retry_named(root().into(), task_name, 10, retry_period));
+		} else {
+			assert_ok!(Scheduler::do_schedule(
+				DispatchTime::At(4),
+				None,
+				127,
+				root(),
+				Preimage::bound(failing_call).unwrap(),
+			));
+			// Set retry config for anonymous task.
+			assert_ok!(Scheduler::set_retry(root().into(), (4, 0), 10, retry_period));
+		}
+		assert_eq!(Retries::<Test>::iter().count(), 1);
+
+		// Run to block 4: task fails and tries to schedule retry at block 7, but it's full.
+		System::run_to_block::<AllPalletsWithSystem>(4);
+
+		// The retry config should be removed since scheduling failed.
+		assert_eq!(Retries::<Test>::iter().count(), 0);
+		// Task at block 4 should be gone.
+		assert!(Agenda::<Test>::get(4).is_empty());
+		// Block 7 should still have exactly `max` tasks (the fillers, no retry added).
+		assert_eq!(Agenda::<Test>::get(4 + retry_period).len() as u32, max);
+		// Task 42 never executed.
+		assert!(!logger::log().iter().any(|(_, i)| *i == 42));
+
+		// Verify `RetryFailed` event was emitted.
+		// Note: `id` is always `None` because `as_retry()` clears the task name.
+		let events = frame_system::Pallet::<Test>::events();
+		let retry_failed_event: <Test as frame_system::Config>::RuntimeEvent =
+			Event::RetryFailed { task: (4, 0), id: None }.into();
+		assert!(
+			events.iter().any(|record| record.event == retry_failed_event),
+			"Expected RetryFailed event not found"
+		);
+	});
+}
+
+#[test]
+fn schedule_retry_fails_when_retry_target_block_is_full_anon() {
+	schedule_retry_fails_when_retry_target_block_is_full(false);
+}
+
+#[test]
+fn schedule_retry_fails_when_retry_target_block_is_full_named() {
+	schedule_retry_fails_when_retry_target_block_is_full(true);
+}
+
 /// Permanently overweight calls are not deleted but also not executed.
 #[test]
 fn scheduler_does_not_delete_permanently_overweight_call() {
@@ -2721,6 +2806,8 @@ fn scheduler_v3_named_cancel_named_works() {
 		.unwrap();
 		// Cancel the call by name.
 		assert_ok!(<Scheduler as Named<_, _, _>>::cancel_named(name));
+		// Lookup storage should be empty.
+		assert!(Lookup::<Test>::get(name).is_none());
 		// It did not get executed.
 		System::run_to_block::<AllPalletsWithSystem>(100);
 		assert!(logger::log().is_empty());
@@ -2751,6 +2838,8 @@ fn scheduler_v3_named_cancel_without_name_works() {
 		.unwrap();
 		// Cancel the call by address.
 		assert_ok!(<Scheduler as Anon<_, _, _>>::cancel(address));
+		// Address-cancellation of named task should still clear the lookup storage.
+		assert!(Lookup::<Test>::get(name).is_none());
 		// It did not get executed.
 		System::run_to_block::<AllPalletsWithSystem>(100);
 		assert!(logger::log().is_empty());