diff --git a/Furious/Service/AGENTS.md b/Furious/Service/AGENTS.md index da43f24..6096ed7 100644 --- a/Furious/Service/AGENTS.md +++ b/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 diff --git a/Furious/Service/SubscriptionManager.py b/Furious/Service/SubscriptionManager.py index 5089403..45942b0 100644 --- a/Furious/Service/SubscriptionManager.py +++ b/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() diff --git a/Furious/Widget/SubscriptionTableView.py b/Furious/Widget/SubscriptionTableView.py index 88055af..45690ca 100644 --- a/Furious/Widget/SubscriptionTableView.py +++ b/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() diff --git a/tests/test_subscription_manager.py b/tests/test_subscription_manager.py index a6ddd9d..e18958b 100644 --- a/tests/test_subscription_manager.py +++ b/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()