Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(spells): do not run spells with passed (ended) Clock trigger #1452

Merged
merged 3 commits into from
Feb 10, 2023

Conversation

kmd-fl
Copy link
Contributor

@kmd-fl kmd-fl commented Feb 10, 2023

Right now all spells will be rerun after the node restart, even the ones that ended before restarting.

So the new rule is: the spell won't be restarted if in its trigger config end_at <= now or it's a one-shot spell that started in the past.

Not sure about the following events:

  1. The spell is a one-shot that should be run immediately or was running, but the node was restarted during the process.
  2. The periodic spell with end_at was added, then the node was shut down until end_at, so the spell was never executed.

These events don't seem to be very relevant though.

@kmd-fl kmd-fl requested review from folex and justprosh February 10, 2023 10:17
@folex folex changed the title fix(spells): do not resubscribed ended spells fix(spells): do not run spells with passed (ended) Clock trigger Feb 10, 2023
self.spell_event_bus_api
.subscribe(spell_id.clone(), config.clone())
.await?;
} else {
log::warn!("Spell {spell_id} is not rescheduled since its config is either not found or not reschedulable");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be info, not warn

@kmd-fl kmd-fl enabled auto-merge (squash) February 10, 2023 11:53
@kmd-fl kmd-fl merged commit fe2184a into master Feb 10, 2023
@kmd-fl kmd-fl deleted the fix-spell-resubscribing branch February 10, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants