diff --git a/pallets/scheduler/src/lib.rs b/pallets/scheduler/src/lib.rs index 95b28c71..ebd6bce3 100644 --- a/pallets/scheduler/src/lib.rs +++ b/pallets/scheduler/src/lib.rs @@ -55,6 +55,15 @@ //! * Could lead to undefined behavior, such as executing another runtime call with the same //! index. //! +//! ## Known Limitation: Stranded tasks when preimage is unavailable +//! +//! If a task's preimage is not available at execution time, the task remains in the `Agenda` +//! and its `Lookup` entry is preserved, allowing cancellation or rescheduling by name. However, +//! the scheduler will not automatically retry the task in future blocks -- it is only +//! re-attempted when the original block's agenda is serviced again (which does not happen +//! once the block has passed). Callers should ensure preimages are available before the +//! scheduled block, or cancel/reschedule stranded tasks explicitly. +//! //! [`on_initialize`]: frame_support::traits::Hooks::on_initialize // Ensure we're `no_std` when compiling for Wasm. @@ -767,24 +776,28 @@ impl Pallet { Lookup::::try_mutate_exists(id, |lookup| -> DispatchResult { if let Some((when, index)) = lookup.take() { let i = index as usize; - Agenda::::try_mutate(when, |agenda| -> DispatchResult { - if let Some(s) = agenda.get_mut(i) { - if let (Some(ref o), Some(ref s)) = (origin, s.borrow()) { - Self::ensure_privilege(o, &s.origin)?; - } - if let Some(ref s) = s { - Retries::::remove((when, index)); - T::Preimages::drop(&s.call); - } - *s = None; - } - Ok(()) + let task = Agenda::::try_mutate(when, |agenda| { + agenda.get_mut(i).map_or( + Ok(None), + |s| -> Result>, DispatchError> { + if let (Some(ref o), Some(ref s)) = (origin, s.borrow()) { + Self::ensure_privilege(o, &s.origin)?; + } + Ok(s.take()) + }, + ) })?; - Self::cleanup_agenda(when); - Self::deposit_event(Event::Canceled { when, index }); - Ok(()) + if let Some(s) = task { + Retries::::remove((when, index)); + T::Preimages::drop(&s.call); + Self::cleanup_agenda(when); + Self::deposit_event(Event::Canceled { when, index }); + Ok(()) + } else { + Err(Error::::NotFound.into()) + } } else { - return Err(Error::::NotFound.into()); + Err(Error::::NotFound.into()) } }) } @@ -1023,13 +1036,12 @@ impl Pallet { postponed == 0 } - /// Service (i.e. execute) the given task, being careful not to overflow the `weight` counter. - /// - /// This involves: - /// - removing and potentially replacing the `Lookup` entry for the task. - /// - realizing the task's call which can include a preimage lookup. /// Execute a single scheduled task. Returns Ok(()) on successful dispatch, or Err with /// the task back (Some) for retry / postponement, or None if permanently removed. + /// + /// Lookup removal is deferred until the outcome is known: tasks that return to the + /// agenda (preimage unavailable, temporarily overweight) keep their Lookup entry so + /// they can still be cancelled or rescheduled by name. fn service_task( weight: &mut WeightMeter, now: BlockNumberOrTimestampOf, @@ -1038,33 +1050,21 @@ impl Pallet { is_first: bool, task: ScheduledOf, ) -> Result<(), (ServiceTaskError, Option>)> { - // Eagerly remove the name->address lookup. If the task succeeds or gets rescheduled - // via a retry, the new address will be re-inserted by place_task. If it fails, - // the name is freed. - if let Some(ref id) = task.maybe_id { - Lookup::::remove(id); - } - - // Try to retrieve the actual call data. For inline calls this is a no-op decode. - // For lookup calls this fetches from the preimage store. let (call, lookup_len) = match T::Preimages::peek(&task.call) { Ok(c) => c, Err(_) => { - // Preimage not available (not yet submitted or previously dropped). - // Drop our reference, emit an event, and return the task back to the - // agenda so it can be retried if the preimage appears later. Self::deposit_event(Event::CallUnavailable { task: (when, agenda_index), id: task.maybe_id, }); - T::Preimages::drop(&task.call); - let _ = weight.try_consume(T::WeightInfo::service_task( task.call.lookup_len().map(|x| x as usize), task.maybe_id.is_some(), )); + // Keep preimage requested -- the task returns to the agenda and + // whoever resolves it (cancel/reschedule) will drop the reference. return Err((Unavailable, Some(task))); }, }; @@ -1076,6 +1076,9 @@ impl Pallet { match Self::execute_dispatch(weight, task.origin.clone(), call) { Err(()) if is_first => { + if let Some(ref id) = task.maybe_id { + Lookup::::remove(id); + } T::Preimages::drop(&task.call); Self::deposit_event(Event::PermanentlyOverweight { task: (when, agenda_index), @@ -1085,6 +1088,9 @@ impl Pallet { }, Err(()) => Err((Overweight, Some(task))), Ok(result) => { + if let Some(ref id) = task.maybe_id { + Lookup::::remove(id); + } let failed = result.is_err(); let maybe_retry_config = Retries::::take((when, agenda_index)); diff --git a/pallets/scheduler/src/tests.rs b/pallets/scheduler/src/tests.rs index 1708873c..9c735778 100644 --- a/pallets/scheduler/src/tests.rs +++ b/pallets/scheduler/src/tests.rs @@ -1449,29 +1449,24 @@ fn cancel_retries_works() { } #[test] -fn postponed_named_task_cannot_be_rescheduled() { +fn unavailable_preimage_preserves_lookup_for_cancellation() { new_test_ext().execute_with(|| { let call = RuntimeCall::Logger(LoggerCall::log { i: 42, weight: Weight::from_parts(1000, 0) }); let hash = ::Hashing::hash_of(&call); let len = call.using_encoded(|x| x.len()) as u32; - // Important to use here `Bounded::Lookup` to ensure that we request the hash. let hashed = Bounded::Lookup { hash, len }; let name: [u8; 32] = hash.as_ref().try_into().unwrap(); - let address = - Scheduler::do_schedule_named(name, DispatchTime::At(4), 127, root(), hashed.clone()) - .unwrap(); + Scheduler::do_schedule_named(name, DispatchTime::At(4), 127, root(), hashed.clone()) + .unwrap(); assert!(Preimage::is_requested(&hash)); assert!(Lookup::::contains_key(name)); - // Run to a very large block. run_to_block(10); - // It was not executed. assert!(logger::log().is_empty()); - // Preimage was not available assert_eq!( System::events().last().unwrap().event, crate::Event::CallUnavailable { @@ -1481,12 +1476,11 @@ fn postponed_named_task_cannot_be_rescheduled() { .into() ); - // So it should not be requested. - assert!(!Preimage::is_requested(&hash)); - // Postponing removes the lookup. - assert!(!Lookup::::contains_key(name)); + // Preimage stays requested -- the task is still in the agenda. + assert!(Preimage::is_requested(&hash)); + // Lookup is preserved so the task can still be cancelled/rescheduled by name. + assert!(Lookup::::contains_key(name)); - // The agenda still contains the call. let agenda = Agenda::::iter().collect::>(); assert_eq!(agenda.len(), 1); assert_eq!( @@ -1500,24 +1494,10 @@ fn postponed_named_task_cannot_be_rescheduled() { })] ); - // Finally add the preimage. - assert_ok!(Preimage::note_preimage(RuntimeOrigin::signed(0), call.encode())); - - run_to_block(1000); - // It did not execute. - assert!(logger::log().is_empty()); - assert!(!Preimage::is_requested(&hash)); - - // Manually re-schedule the call by name does not work. - assert_err!( - Scheduler::do_reschedule_named(name, DispatchTime::At(1001)), - Error::::NotFound - ); - // Manually re-scheduling the call by address errors. - assert_err!( - Scheduler::do_reschedule(address, DispatchTime::At(1001)), - Error::::Named - ); + // The stranded task can still be cancelled by name. + assert_ok!(Scheduler::do_cancel_named(None, name)); + assert!(!Lookup::::contains_key(name)); + assert!(Agenda::::iter().collect::>().is_empty()); }); } @@ -2228,8 +2208,9 @@ fn unavailable_call_is_detected() { } .into() ); - // It should not be requested anymore. - assert!(!Preimage::is_requested(&hash)); + // Preimage stays requested -- the task is still in the agenda and can be + // cancelled or rescheduled by name. + assert!(Preimage::is_requested(&hash)); }); } #[test]