Fix subscription auto-update lifecycle

Signed-off-by: Loren Eteval <loren.eteval@proton.me>

Furious/Service/AGENTS.md

@@ -45,6 +45,10 @@
 - `SubscriptionManager` owns download, decoding, filtering, reconciliation, persistence effects, stale-request
   rejection, and stable-ID auto-update timers. Subscription views invoke commands and render semantic results; they do
   not own this workflow. Remote data is untrusted.
+- Long-lived service timers are created and connected once, then reconciled idempotently. Reapplying unchanged policy
+  must not restart a periodic countdown, reconnect its timeout, or emit a lifecycle transition log.
+- Page visibility and presentation refreshes must not control application-level background scheduler lifecycles.
+  Reconcile schedules at service startup and when the corresponding persisted scheduling policy changes.
 - Subscription reply callbacks stage decoded results only. Persist group status and reconcile profiles after the final
   request-version check; one group's failure must not abort other current groups in the same completion batch.
 - Treat reconnect/disconnect after subscription reconciliation as a post-commit effect. Failure there must be logged

Furious/Service/SubscriptionManager.py

@@ -112,6 +112,7 @@ class SubscriptionManager(HttpGetManager):
 
         self.importer = SubscriptionImportService()
         self.synchronizer = SubscriptionSynchronizer()
+
         self._autoUpdateTimers = {}
         self._requestVersions = {}
         self._activeReplies = {}
@@ -202,24 +203,53 @@ class SubscriptionManager(HttpGetManager):
 
             self._autoUpdateTimers[unique] = timer
 
-        if interval is not None and subscription.get('enabled', True):
-            logger.info(
-                f'start auto update job for subscription '
-                f'({subscription.get("remark", "")}, '
-                f'{subscription.get("webURL", "")}). '
-                f'Interval is {interval // (60 * 1000)} mins'
-            )
+        shouldRun = interval is not None and subscription.get('enabled', True)
 
-            timer.start(interval)
-        else:
-            logger.info(
-                f'stop auto update job for subscription '
-                f'({subscription.get("remark", "")}, '
-                f'{subscription.get("webURL", "")})'
-            )
+        if not shouldRun:
+            if not timer.isActive():
+                return
 
             timer.stop()
 
+            logger.info(
+                f'stop auto update job for subscription '
+                f'({subscription.get("remark", "")}, {unique})'
+            )
+
+            return
+
+        if timer.isActive() and timer.interval() == interval:
+            return
+
+        previousInterval = timer.interval() if timer.isActive() else None
+
+        timer.start(interval)
+
+        if previousInterval is None:
+            logger.info(
+                f'start auto update job for subscription '
+                f'({subscription.get("remark", "")}, {unique}). '
+                f'Interval is {interval // (60 * 1000)} mins'
+            )
+        else:
+            logger.info(
+                f'reschedule auto update job for subscription '
+                f'({subscription.get("remark", "")}, {unique}). '
+                f'Interval changed from {previousInterval // (60 * 1000)} '
+                f'to {interval // (60 * 1000)} mins'
+            )
+
+    def configureAutoUpdate(self, unique: str):
+        """Reconcile the schedule for one known subscription mutation."""
+        subscription = Storage.UserSubs().get(unique)
+
+        if subscription is None:
+            self.removeAutoUpdate(unique)
+
+            return
+
+        self._configureAutoUpdate(unique, subscription)
+
     def refreshAutoUpdates(self):
         """Reconcile service-owned timers with the current subscription repository."""
         subscriptions = Storage.UserSubs()

Furious/Widget/SubscriptionTableView.py

@@ -719,9 +719,6 @@ class SubscriptionTableView(Mixins.QTranslatable, AppQTableView):
         for index, key in enumerate(Storage.UserSubs()):
             self.flushRow(index, Storage.UserSubs()[key])
 
-        if self.subsManager is not None:
-            self.subsManager.refreshAutoUpdates()
-
     def appendNewItem(self, **kwargs):
         """Append new item."""
         (
@@ -777,7 +774,7 @@ class SubscriptionTableView(Mixins.QTranslatable, AppQTableView):
             self.flushRow(row, subsob[unique])
 
         if self.subsManager is not None:
-            self.subsManager.refreshAutoUpdates()
+            self.subsManager.configureAutoUpdate(unique)
 
         self.groupsChanged.emit()
 

tests/test_subscription_manager.py

@@ -24,10 +24,16 @@ import os
 
 os.environ.setdefault('QT_QPA_PLATFORM', 'offscreen')
 
-from PySide6 import QtCore
+from PySide6 import QtCore, QtTest, QtWidgets
 
+from shiboken6 import isValid
+
+from Furious.Repository import Storage
+from Furious.Repository.Subscriptions import SubscriptionGroup
 from Furious.Service.SubscriptionManager import SubscriptionManager
-from tests.support import application
+from Furious.Window.SubscriptionPage import SubscriptionPage
+from Furious.Widget.SubscriptionTableView import SubscriptionTableView
+from tests.support import application, processQtEvents
 
 
 class _Payload:
@@ -81,6 +87,20 @@ class SubscriptionManagerTest(TestCase):
         ):
             return SubscriptionManager()
 
+    @staticmethod
+    def _subscription(**overrides):
+        """Return one enabled five-minute subscription definition."""
+        subscription = {
+            'remark': 'Group A',
+            'webURL': 'https://invalid.test/a',
+            'autoupdate': 'Every 5 mins',
+            'proxy': '',
+            'enabled': True,
+        }
+        subscription.update(overrides)
+
+        return subscription
+
     def testSuccessfulAndInvalidPayloadsProduceSemanticBatchInputs(self):
         manager = self._manager()
         profile = SimpleNamespace(itemRemark='profile')
@@ -396,29 +416,293 @@ class SubscriptionManagerTest(TestCase):
         manager._replySubscriptions.clear()
         manager.deleteLater()
 
-    def testAutoUpdateTimersAreReusedAndKeyedByStableSubscriptionId(self):
-        subscriptions = {
-            'group-a': {
-                'remark': 'Group A',
-                'webURL': 'https://invalid.test/a',
-                'autoupdate': 'Every 5 mins',
-                'proxy': '',
-                'enabled': True,
-            },
-        }
+    def testUnchangedAutoUpdateReconciliationPreservesDeadlineAndConnection(self):
+        """Make repeated full reconciliation a true scheduler no-op."""
+        subscriptions = {'group-a': self._subscription()}
         manager = self._manager(subscriptions)
         timer = manager._autoUpdateTimers['group-a']
+        timerId = timer.timerId()
+
+        QtTest.QTest.qWait(40)
+
+        remainingBefore = timer.remainingTime()
+
+        with (
+            mock.patch(
+                'Furious.Service.SubscriptionManager.Storage.UserSubs',
+                return_value=subscriptions,
+            ),
+            mock.patch(
+                'Furious.Service.SubscriptionManager.logger.info'
+            ) as lifecycleLog,
+        ):
+            for _index in range(100):
+                manager.refreshAutoUpdates()
+
+        self.assertIs(manager._autoUpdateTimers['group-a'], timer)
+        self.assertEqual(timer.timerId(), timerId)
+        self.assertEqual(timer.property('subscriptionId'), 'group-a')
+        self.assertEqual(len(manager._autoUpdateTimers), 1)
+        self.assertLessEqual(timer.remainingTime(), remainingBefore + 5)
+        lifecycleLog.assert_not_called()
+
+        manager.configureHttpProxy = mock.Mock()
+        manager.updateSubsByUnique = mock.Mock()
 
         with mock.patch(
             'Furious.Service.SubscriptionManager.Storage.UserSubs',
             return_value=subscriptions,
         ):
-            manager.refreshAutoUpdates()
-            manager.refreshAutoUpdates()
+            timer.timeout.emit()
 
+        manager.updateSubsByUnique.assert_called_once_with(
+            'group-a', showMessageBox=False
+        )
+        manager.deleteLater()
+
+    def testAutoUpdatePolicyTransitionsReuseTimerAndLogOnlyRealChanges(self):
+        """Start, reschedule, and stop exactly when policy state changes."""
+        subscriptions = {
+            'group-a': self._subscription(autoupdate='Never'),
+        }
+        manager = self._manager(subscriptions)
+        timer = manager._autoUpdateTimers['group-a']
+
+        with (
+            mock.patch(
+                'Furious.Service.SubscriptionManager.Storage.UserSubs',
+                return_value=subscriptions,
+            ),
+            mock.patch(
+                'Furious.Service.SubscriptionManager.logger.info'
+            ) as lifecycleLog,
+        ):
+            manager.configureAutoUpdate('group-a')
+            lifecycleLog.assert_not_called()
+
+            subscriptions['group-a']['autoupdate'] = 'Every 5 mins'
+            manager.configureAutoUpdate('group-a')
+            self.assertTrue(timer.isActive())
+            self.assertEqual(timer.interval(), 5 * 60 * 1000)
+            self.assertIn('start auto update job', lifecycleLog.call_args.args[0])
+
+            activeTimerId = timer.timerId()
+            manager.configureAutoUpdate('group-a')
+            self.assertEqual(timer.timerId(), activeTimerId)
+            self.assertEqual(lifecycleLog.call_count, 1)
+
+            subscriptions['group-a']['autoupdate'] = 'Every 10 mins'
+            manager.configureAutoUpdate('group-a')
+            self.assertIs(manager._autoUpdateTimers['group-a'], timer)
+            self.assertEqual(timer.interval(), 10 * 60 * 1000)
+            self.assertIn('reschedule auto update job', lifecycleLog.call_args.args[0])
+
+            subscriptions['group-a']['enabled'] = False
+            manager.configureAutoUpdate('group-a')
+            self.assertFalse(timer.isActive())
+            self.assertIn('stop auto update job', lifecycleLog.call_args.args[0])
+
+            manager.configureAutoUpdate('group-a')
+            self.assertEqual(lifecycleLog.call_count, 3)
+
+            subscriptions['group-a']['enabled'] = True
+            manager.configureAutoUpdate('group-a')
+            self.assertTrue(timer.isActive())
+            self.assertIn('start auto update job', lifecycleLog.call_args.args[0])
+
+        self.assertEqual(lifecycleLog.call_count, 4)
+        manager.deleteLater()
+
+    def testUnrelatedSubscriptionEditDoesNotRestartItsTimer(self):
+        """Keep the real table edit path outside unchanged timer deadlines."""
+        subscriptions = {'group-a': self._subscription()}
+        manager = self._manager(subscriptions)
+        timer = manager._autoUpdateTimers['group-a']
+
+        QtTest.QTest.qWait(40)
+
+        remainingBefore = timer.remainingTime()
+        timerId = timer.timerId()
+
+        def group(unique):
+            """Return the exact in-memory group edited by the table."""
+            value = subscriptions.get(unique)
+
+            return (
+                SubscriptionGroup.fromMapping(unique, value)
+                if value is not None
+                else None
+            )
+
+        def upsert(value):
+            """Persist the edited group into the isolated test repository."""
+            subscriptions[value.id] = value.toMapping()
+
+        with (
+            mock.patch.object(Storage, 'UserSubs', return_value=subscriptions),
+            mock.patch.object(Storage, 'SubscriptionGroup', side_effect=group),
+            mock.patch.object(Storage, 'upsertSubscriptionGroup', side_effect=upsert),
+        ):
+            table = SubscriptionTableView(subscriptionManager=manager)
+            table.appendNewItem(
+                unique='group-a',
+                remark='Renamed Group',
+                webURL=subscriptions['group-a']['webURL'],
+                enabled=True,
+                autoupdate='Every 5 mins',
+                proxy='',
+                userAgent='',
+                filter='',
+                lastUpdated='',
+            )
+
+        self.assertEqual(subscriptions['group-a']['remark'], 'Renamed Group')
         self.assertIs(manager._autoUpdateTimers['group-a'], timer)
-        self.assertEqual(timer.property('subscriptionId'), 'group-a')
-        self.assertEqual(len(manager._autoUpdateTimers), 1)
+        self.assertEqual(timer.timerId(), timerId)
+        self.assertLessEqual(timer.remainingTime(), remainingBefore + 5)
+        table.deleteLater()
+        manager.deleteLater()
+
+    def testTargetedPolicyChangeDoesNotDisturbOtherSubscriptions(self):
+        """Reconcile only the edited subscription's schedule."""
+        subscriptions = {
+            'group-a': self._subscription(),
+            'group-b': self._subscription(
+                remark='Group B',
+                webURL='https://invalid.test/b',
+                autoupdate='Every 10 mins',
+            ),
+        }
+        manager = self._manager(subscriptions)
+        groupATimer = manager._autoUpdateTimers['group-a']
+        groupBTimer = manager._autoUpdateTimers['group-b']
+        groupBTimerId = groupBTimer.timerId()
+
+        QtTest.QTest.qWait(40)
+
+        groupBRemainingBefore = groupBTimer.remainingTime()
+        subscriptions['group-a']['autoupdate'] = 'Every 10 mins'
+
+        with mock.patch(
+            'Furious.Service.SubscriptionManager.Storage.UserSubs',
+            return_value=subscriptions,
+        ):
+            manager.configureAutoUpdate('group-a')
+
+        self.assertIs(manager._autoUpdateTimers['group-a'], groupATimer)
+        self.assertEqual(groupATimer.interval(), 10 * 60 * 1000)
+        self.assertIs(manager._autoUpdateTimers['group-b'], groupBTimer)
+        self.assertEqual(groupBTimer.timerId(), groupBTimerId)
+        self.assertLessEqual(groupBTimer.remainingTime(), groupBRemainingBefore + 5)
+        self.assertEqual(len(manager._autoUpdateTimers), 2)
+        manager.deleteLater()
+
+    def testRemovingSubscriptionDestroysOnlyItsTimerAndCancelsItsReply(self):
+        """Release one removed subscription without disturbing its sibling."""
+        subscriptions = {
+            'group-a': self._subscription(),
+            'group-b': self._subscription(
+                remark='Group B',
+                webURL='https://invalid.test/b',
+            ),
+        }
+        manager = self._manager(subscriptions)
+        groupATimer = manager._autoUpdateTimers['group-a']
+        groupBTimer = manager._autoUpdateTimers['group-b']
+        groupBTimerId = groupBTimer.timerId()
+        groupAReply = _AbortableReply()
+        groupBReply = _AbortableReply()
+        destroyed = []
+        groupATimer.destroyed.connect(lambda *_args: destroyed.append(True))
+        manager._activeReplies.update(
+            {groupAReply: groupAReply, groupBReply: groupBReply}
+        )
+        manager._replySubscriptions.update(
+            {groupAReply: 'group-a', groupBReply: 'group-b'}
+        )
+        subscriptions.pop('group-a')
+
+        with mock.patch(
+            'Furious.Service.SubscriptionManager.Storage.UserSubs',
+            return_value=subscriptions,
+        ):
+            manager.removeAutoUpdate('group-a')
+
+        self.assertTrue(groupAReply.aborted)
+        self.assertFalse(groupBReply.aborted)
+        self.assertNotIn('group-a', manager._autoUpdateTimers)
+        self.assertIs(manager._autoUpdateTimers['group-b'], groupBTimer)
+        self.assertEqual(groupBTimer.timerId(), groupBTimerId)
+        self.assertTrue(groupBTimer.isActive())
+
+        processQtEvents()
+
+        self.assertEqual(destroyed, [True])
+        self.assertFalse(isValid(groupATimer))
+
+        manager._activeReplies.clear()
+        manager._replySubscriptions.clear()
+        manager.deleteLater()
+
+    def testPageNavigationIsPresentationOnlyForAutoUpdateScheduler(self):
+        """Keep page show/hide cycles outside scheduler policy ownership."""
+        subscriptions = {'group-a': self._subscription()}
+
+        with mock.patch.object(Storage, 'UserSubs', return_value=subscriptions):
+            manager = SubscriptionManager()
+            timer = manager._autoUpdateTimers['group-a']
+            timerId = timer.timerId()
+            manager.refreshAutoUpdates = mock.Mock(
+                side_effect=AssertionError(
+                    'page presentation must not reconcile background schedules'
+                )
+            )
+            serverTable = SimpleNamespace(subsManager=manager)
+            page = SubscriptionPage(serverTable)
+            placeholder = QtWidgets.QWidget()
+            stack = QtWidgets.QStackedWidget()
+            stack.addWidget(page)
+            stack.addWidget(placeholder)
+            stack.show()
+
+            for _index in range(20):
+                stack.setCurrentWidget(placeholder)
+                processQtEvents(1)
+                stack.setCurrentWidget(page)
+                processQtEvents(1)
+
+            self.assertIs(page.table.subsManager, manager)
+            self.assertIs(manager._autoUpdateTimers['group-a'], timer)
+            self.assertEqual(timer.timerId(), timerId)
+            self.assertEqual(len(manager._autoUpdateTimers), 1)
+            manager.refreshAutoUpdates.assert_not_called()
+
+            stack.deleteLater()
+            manager.deleteLater()
+
+        processQtEvents()
+
+    def testManagerDestructionDestroysItsServiceOwnedTimers(self):
+        """Let QObject parent ownership release all scheduler resources."""
+        subscriptions = {'group-a': self._subscription()}
+        manager = self._manager(subscriptions)
+        timer = manager._autoUpdateTimers['group-a']
+        destroyed = []
+        manager.destroyed.connect(lambda *_args: destroyed.append('manager'))
+        timer.destroyed.connect(lambda *_args: destroyed.append('timer'))
+
+        manager.deleteLater()
+        processQtEvents()
+
+        self.assertFalse(isValid(manager))
+        self.assertFalse(isValid(timer))
+        self.assertCountEqual(destroyed, ('manager', 'timer'))
+
+    def testMissingSubscriptionRemovalPrunesTimerDuringFullReconciliation(self):
+        """Retain removal behavior while unchanged groups remain untouched."""
+        subscriptions = {'group-a': self._subscription()}
+        manager = self._manager(subscriptions)
+        timer = manager._autoUpdateTimers['group-a']
 
         subscriptions.clear()