Skip to content

[fix] Background task update_config launched multiple times #1128#1141

Merged
nemesifier merged 6 commits intomasterfrom
issues/1128-update_config
Oct 16, 2025
Merged

[fix] Background task update_config launched multiple times #1128#1141
nemesifier merged 6 commits intomasterfrom
issues/1128-update_config

Conversation

@pandafy
Copy link
Member

@pandafy pandafy commented Oct 13, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Fixes #1128

TODOS

  • Add tests

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

What do you think of something like the following?

Before sending the config_modified signal for a device we check for a cache key that is unique per device, if the cache key is set, we don't send the signal otherwise we proceed.

After sending the config_modified signal for a device we can set the cache key (unique for each device) with a short duration, eg: 5 seconds (we need some heuristics to find out the right amount of seconds but it cannot be too long otherwise different changes unrelated to one another may not fire config updates).

I outlined the ideal behavior in #1142. For now, let's focus on mitigating this issue.

Copy link
Member

Choose a reason for hiding this comment

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

Why check if it's None? Wouldn't just check if it's truthy be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are correct. I just wanted to be 100% sure that the cache does not contain the key.

@pandafy pandafy force-pushed the issues/1128-update_config branch from 35ca61a to 098cde7 Compare October 14, 2025 18:11
@pandafy pandafy marked this pull request as ready for review October 14, 2025 18:17
@nemesifier nemesifier moved this from Backlog to In progress in 25.09 Release Oct 14, 2025
@nemesifier nemesifier added the bug label Oct 14, 2025
@coveralls
Copy link

coveralls commented Oct 14, 2025

Coverage Status

coverage: 98.607% (-0.03%) from 98.633%
when pulling f4e3b99 on issues/1128-update_config
into 84fa8fc on master.

@pandafy pandafy force-pushed the issues/1128-update_config branch from 0c78668 to 33a222a Compare October 14, 2025 20:33
Copy link
Member

Choose a reason for hiding this comment

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

This reminds me of the django-allauth rate limiting which we had to disable in tests.

The problem with this approach of modifying each test is that all contributors have to be aware of this, which is not scalable for future maintenance.

We need to look for a solution that doesn't require contributors to be aware of this all the time.

What if we make the timeout configurable with a setting? That would allow us to disable it globally during tests and enable it only in the subset of tests where it is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
prevents emitting duplicate signals inside the same logical window;
prevents emitting duplicate signals inside the same change window;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if that key exists the method returns early without emitting.
if that key exists the method returns without emitting the signal again.

@pandafy pandafy force-pushed the issues/1128-update_config branch from 135531d to f4e3b99 Compare October 15, 2025 17:29
@nemesifier nemesifier merged commit cd6f02a into master Oct 16, 2025
19 checks passed
@nemesifier nemesifier deleted the issues/1128-update_config branch October 16, 2025 14:02
@github-project-automation github-project-automation bot moved this from In progress to Done in 25.09 Release Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[bug] Background task update_config launched multiple times

3 participants

Comments