From 5a5304f38fdcf980b10c0b54838f9d7e2c751c17 Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 21:45:59 -0800 Subject: [PATCH 1/9] fix: Add validity check to TaskHandlerTask --- evennia/scripts/taskhandler.py | 36 ++++++++++++++++-- evennia/scripts/tests.py | 69 ++++++++++++++++++++++++++++++++-- 2 files changed, 99 insertions(+), 6 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index 6233b2f5d0..feb8a48dc1 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -52,6 +52,24 @@ class TaskHandlerTask: self.task_id = task_id self.deferred = TASK_HANDLER.get_deferred(task_id) + def _is_valid(self): + """Check if this task reference still points to the original task. + + A task reference becomes invalid when the original task is completed or + removed and its ID is reassigned to a new, unrelated task. This prevents + accidentally operating on the wrong task. + + Returns: + bool: True if this reference still points to its original task. + + """ + # task data is stored as (comp_time, callback, args, kwargs, persistent, deferred); + # compare the deferred (index 5) by identity to detect ID reuse + task_data = TASK_HANDLER.tasks.get(self.task_id) + if task_data is None: + return False + return task_data[5] is self.deferred + def get_deferred(self): """Return the instance of the deferred the task id is using. @@ -60,6 +78,8 @@ class TaskHandlerTask: None is returned if there is no deferred affiliated with this id. """ + if not self._is_valid(): + return None return TASK_HANDLER.get_deferred(self.task_id) def pause(self): @@ -111,6 +131,8 @@ class TaskHandlerTask: handler. Otherwise it will be the return of the task's callback. """ + if not self._is_valid(): + return False return TASK_HANDLER.do_task(self.task_id) def call(self): @@ -125,19 +147,21 @@ class TaskHandlerTask: handler. Otherwise it will be the return of the task's callback. """ + if not self._is_valid(): + return False return TASK_HANDLER.call_task(self.task_id) def remove(self): """Remove a task without executing it. Deletes the instance of the task's deferred. - Args: - task_id (int): an existing task ID. - Returns: bool: True if the removal completed successfully. + False if the task reference is no longer valid. """ + if not self._is_valid(): + return False return TASK_HANDLER.remove(self.task_id) def cancel(self): @@ -149,6 +173,8 @@ class TaskHandlerTask: False if the cancel did not complete successfully. """ + if not self._is_valid(): + return False return TASK_HANDLER.cancel(self.task_id) def active(self): @@ -159,6 +185,8 @@ class TaskHandlerTask: it is not (has been called) or if the task does not exist. """ + if not self._is_valid(): + return False return TASK_HANDLER.active(self.task_id) @property @@ -190,6 +218,8 @@ class TaskHandlerTask: bool: True the task exists False if it does not. """ + if not self._is_valid(): + return False return TASK_HANDLER.exists(self.task_id) def get_id(self): diff --git a/evennia/scripts/tests.py b/evennia/scripts/tests.py index 5e0f23c9bd..a2d31c18dc 100644 --- a/evennia/scripts/tests.py +++ b/evennia/scripts/tests.py @@ -6,8 +6,6 @@ Unit tests for the scripts package from collections import defaultdict from unittest import TestCase, mock -from parameterized import parameterized - from evennia import DefaultScript from evennia.objects.objects import DefaultObject from evennia.scripts.manager import ScriptDBManager @@ -15,10 +13,10 @@ from evennia.scripts.models import ObjectDoesNotExist, ScriptDB from evennia.scripts.monitorhandler import MonitorHandler from evennia.scripts.ondemandhandler import OnDemandHandler, OnDemandTask from evennia.scripts.scripts import DoNothing, ExtendedLoopingCall +from evennia.scripts.taskhandler import TASK_HANDLER from evennia.scripts.tickerhandler import TickerHandler from evennia.typeclasses.attributes import AttributeProperty from evennia.utils.create import create_script -from evennia.utils.dbserialize import dbserialize from evennia.utils.test_resources import BaseEvenniaTest, EvenniaTest @@ -382,6 +380,71 @@ class TestMonitorHandler(TestCase): self.assertEqual(self.handler.monitors[index][name], {}) +class TestTaskHandlerTask(TestCase): + """Test that TaskHandlerTask correctly handles stale references when task IDs are reused.""" + + def setUp(self): + from twisted.internet import task as twisted_task + + TASK_HANDLER.clock = twisted_task.Clock() + TASK_HANDLER.clear() + + def tearDown(self): + TASK_HANDLER.clear() + + def test_stale_reference_after_id_reuse(self): + """A stale TaskHandlerTask must not operate on a new task that reused its ID.""" + callback1 = mock.Mock(return_value="result1") + callback2 = mock.Mock(return_value="result2") + + # Create first task (gets ID 1) + task1 = TASK_HANDLER.add(5, callback1) + task1_id = task1.get_id() + + # Complete and remove the first task so its ID is freed + TASK_HANDLER.clock.advance(5) + + # Create second task - should reuse ID 1 + task2 = TASK_HANDLER.add(5, callback2) + self.assertEqual(task2.get_id(), task1_id) + + # The stale reference (task1) must not affect the new task (task2) + self.assertFalse(task1.exists()) + self.assertFalse(task1.active()) + self.assertFalse(task1.cancel()) + self.assertFalse(task1.remove()) + self.assertFalse(task1.do_task()) + self.assertFalse(task1.call()) + self.assertIsNone(task1.get_deferred()) + + # The new task must still be intact + self.assertTrue(task2.exists()) + self.assertTrue(task2.active()) + + def test_valid_reference_works_normally(self): + """A valid TaskHandlerTask should work as expected.""" + callback = mock.Mock(return_value="result") + task = TASK_HANDLER.add(5, callback) + + self.assertTrue(task.exists()) + self.assertTrue(task.active()) + self.assertIsNotNone(task.get_deferred()) + + result = task.call() + self.assertEqual(result, "result") + callback.assert_called_once() + + def test_is_valid_after_remove(self): + """After removing a task, its TaskHandlerTask reference should be invalid.""" + callback = mock.Mock() + task = TASK_HANDLER.add(5, callback) + + task.remove() + self.assertFalse(task.exists()) + self.assertFalse(task.active()) + self.assertFalse(task.cancel()) + + class TestOnDemandTask(EvenniaTest): """ Test the OnDemandTask class. From 8e2a9ecf76cdc08a086f939f04028a20c6fce39f Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:00:07 -0800 Subject: [PATCH 2/9] taskhandler: compute now before looping tasks --- evennia/scripts/taskhandler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index feb8a48dc1..bc63a4135a 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -303,11 +303,11 @@ class TaskHandler: """ clean_ids = [] + # if a now time is provided use it (intended for unit testing) + now = self._now if self._now else datetime.now() for task_id, (date, callback, args, kwargs, persistent, _) in self.tasks.items(): if not self.active(task_id): stale_date = date + timedelta(seconds=self.stale_timeout) - # if a now time is provided use it (intended for unit testing) - now = self._now if self._now else datetime.now() # the task was canceled more than stale_timeout seconds ago if now > stale_date: clean_ids.append(task_id) From 6d90a4cf204cd8aa5b2a367cd395da5020cf6925 Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:02:35 -0800 Subject: [PATCH 3/9] taskhandler: remove dead code --- evennia/scripts/taskhandler.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index bc63a4135a..58b69d4f50 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -519,19 +519,14 @@ class TaskHandler: bool: True if the removal completed successfully. """ - d = None # delete the task from the tasks dictionary if task_id in self.tasks: - # if the task has not been run, cancel it self.cancel(task_id) - del self.tasks[task_id] # delete the task from the tasks dictionary + del self.tasks[task_id] # remove the task from the persistent dictionary and ServerConfig if task_id in self.to_save: del self.to_save[task_id] - self.save() # remove from ServerConfig.objects - # delete the instance of the deferred - if d: - del d + self.save() return True def clear(self, save=True, cancel=True): From b0fe4871263ce8577c0f47304e0f6d1d2b11db7e Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:03:32 -0800 Subject: [PATCH 4/9] taskhandler: inline cleanup, save at the end --- evennia/scripts/taskhandler.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index 58b69d4f50..b09b733c81 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -311,8 +311,15 @@ class TaskHandler: # the task was canceled more than stale_timeout seconds ago if now > stale_date: clean_ids.append(task_id) + needs_save = False for task_id in clean_ids: - self.remove(task_id) + self.cancel(task_id) + del self.tasks[task_id] + if task_id in self.to_save: + del self.to_save[task_id] + needs_save = True + if needs_save: + self.save() return True def save(self): From f94f5c581f086200febc488bdd75a77b1e209f6e Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:05:39 -0800 Subject: [PATCH 5/9] taskhandler: remove redundant dict lookups and tuple/list/tuple conversion --- evennia/scripts/taskhandler.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index b09b733c81..515e20335c 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -440,11 +440,8 @@ class TaskHandler: # some tasks may complete before the deferred can be added if task_id in self.tasks: - task = self.tasks.get(task_id) - task = list(task) - task[4] = persistent - task[5] = d - self.tasks[task_id] = task + comp_time, cb, args, kwargs, _, _ = self.tasks[task_id] + self.tasks[task_id] = (comp_time, cb, args, kwargs, persistent, d) else: # the task already completed return False if self.stale_timeout > 0: From 94c503b4e6cdd09d1f625c4609601090edbfe2fb Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:06:25 -0800 Subject: [PATCH 6/9] taskhandler: fix exists() verbosity. --- evennia/scripts/taskhandler.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index 515e20335c..06343fdefb 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -460,10 +460,7 @@ class TaskHandler: bool: True the task exists False if it does not. """ - if task_id in self.tasks: - return True - else: - return False + return task_id in self.tasks def active(self, task_id): """ From 9313659074231c5cfd082374871ba46dd89ed900 Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:07:58 -0800 Subject: [PATCH 7/9] Next: remove misleading comments on active() and self-evident on cancel() --- evennia/scripts/taskhandler.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index 06343fdefb..b847c0e499 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -475,7 +475,6 @@ class TaskHandler: """ if task_id in self.tasks: - # if the task has not been run, cancel it deferred = self.get_deferred(task_id) return not (deferred is not None and deferred.called) else: @@ -495,7 +494,6 @@ class TaskHandler: """ if task_id in self.tasks: - # if the task has not been run, cancel it d = self.get_deferred(task_id) if d is not None: # it is remotely possible for a task to not have a deferred if d.called: From bae500c165baa5625d09582ad4dafad236b2fee9 Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 22:09:09 -0800 Subject: [PATCH 8/9] taskhandler: remove `add()` variable shadowing --- evennia/scripts/taskhandler.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index b847c0e499..d119506871 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -432,10 +432,7 @@ class TaskHandler: self.tasks[task_id] = (comp_time, callback, args, kwargs, persistent, None) # defer the task - callback = self.do_task - args = [task_id] - kwargs = {} - d = deferLater(self.clock, timedelay, callback, *args, **kwargs) + d = deferLater(self.clock, timedelay, self.do_task, task_id) d.addErrback(handle_error) # some tasks may complete before the deferred can be added From 53dbc57ba3236f669667463d5240e5916478d03b Mon Sep 17 00:00:00 2001 From: Jake <73198594+jaborsh@users.noreply.github.com> Date: Wed, 4 Mar 2026 21:57:28 -0800 Subject: [PATCH 9/9] taskhandler: check task membership directly --- evennia/scripts/taskhandler.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/evennia/scripts/taskhandler.py b/evennia/scripts/taskhandler.py index d119506871..0a3e6174a2 100644 --- a/evennia/scripts/taskhandler.py +++ b/evennia/scripts/taskhandler.py @@ -388,9 +388,8 @@ class TaskHandler: delta = timedelta(seconds=timedelay) comp_time = now + delta # get an open task id - used_ids = list(self.tasks.keys()) task_id = 1 - while task_id in used_ids: + while task_id in self.tasks: task_id += 1 # record the task to the tasks dictionary