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(spell-bus): resubscribe spell on double subscription #1797

Merged
merged 5 commits into from
Sep 28, 2023

Conversation

kmd-fl
Copy link
Contributor

@kmd-fl kmd-fl commented Sep 20, 2023

Description

The spell-event-bus interface doesn't forbid subscription of the same spell on the same event on their own; the code around it was supposed to avoid such situations. However, there's a possibility that the system-service-deployer can deploy system spells before the sorcerer resubscribes old spells (created on the previous run of the node), so the sorcerer can also resubscribe the newly created spells since we don't distinguish active and not-yet-active spells.

This problem occurred during running nox in tests via CreatedSwarm and doesn't yet affect the fully-fledged nox.

I think the best approach to the situation is to create a spell registry that would account for the spell's state: unsubscribed, subscribed, ended execution, etc. However, this approach requires more work and design and will be done in the future, but at the moment, we can introduce a simpler solution that won't resubscribe the running spells.

Motivation

The spell-event-bus doesn't track the list of active spells and will subscribe the already active spells as new ones. This can lead to running spells more often than configured.

This problem can occur only at the start of the nox when the sorcerer resubscribes spells; the public interfaces (used by decider and direct deployment) aren't affected.

Proposed Changes

  • Keep track of active subscriptions in the bus itself
  • If we receive a doubling subscription, resubscribe the spell. I think there are no nice solutions since we can't properly report on the events, but for now, it may be fine.

@gurinderu gurinderu added the e2e Run e2e workflow label Sep 27, 2023
crates/spell-event-bus/src/bus.rs Outdated Show resolved Hide resolved
crates/spell-event-bus/src/bus.rs Outdated Show resolved Hide resolved
@kmd-fl kmd-fl merged commit 7ee8efa into master Sep 28, 2023
14 checks passed
@kmd-fl kmd-fl deleted the fix-rescheduling-spells branch September 28, 2023 13:20
@kmd-fl kmd-fl restored the fix-rescheduling-spells branch September 29, 2023 09:13
@kmd-fl kmd-fl deleted the fix-rescheduling-spells branch January 12, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants