Too many recurrences "hang" `task`

[djmitche]

I just started up task in a context where I had a very old task database. It "hung", and some investigation showed it adding tasks for a recurrence. Presumably most of those tasks had due dates in the past, and certainly all of them are not useful.

Perhaps we should either have a limit on the number of recurring tasks to create in a given report run, or just never create recurring tasks in the past.

[ryneeverett]

Never creating recurring tasks with past due dates seems fairly safe and better targeted at the problem of returning to an old database. In an ideal future, I would hope all recurring tasks could either have a due date or a (recurrence overhaul) chained or periodic rtype.

[djmitche]

That fix wasn't right!

The mask is indexed by number of recurrences since the beginning, and the fix didn't take that into account properly.

[djmitche]

I think I have a better fix:

diff --git src/recur.cpp src/recur.cpp
index 524c912d3..bd3f4a08a 100644
--- src/recur.cpp
+++ src/recur.cpp
@@ -74,25 +74,48 @@ void handleRecurrence ()
         Context::getContext ().tdb2.modify (t);
         Context::getContext ().footnote (onExpiration (t));
         continue;
       }
 
       // Get the mask from the parent task.
       auto mask = t.get ("mask");
 
+      // Determine the mask index of the first task in the future.
+      unsigned int i = 0;
+      unsigned int first_future = 0;
+      Datetime now;
+      for (auto& d : due)
+      {
+        if (d > now) {
+          first_future = i;
+          break;
+        }
+        ++i;
+      }
+
       // Iterate over the due dates, and check each against the mask.
       auto changed = false;
-      unsigned int i = 0;
+      i = 0;
       for (auto& d : due)
       {
         if (mask.length () <= i)
         {
           changed = true;
 
+          // Consider any tasks earlier than the most recent past recurrence
+          // to have been "missed".
+          if (i + 1 < first_future)
+          {
+            mask += 'M';
+            ++i;
+            continue;
+          }
+
           Task rec (t);                          // Clone the parent.
           rec.setStatus (Task::pending);         // Change the status.
           rec.set ("uuid", uuid ());             // New UUID.
           rec.set ("parent", t.get ("uuid"));    // Remember mom.
           rec.setAsNow ("entry");                // New entry date.
           rec.set ("due", format (d.toEpoch ()));
 
           if (t.has ("wait"))

The idea is similar, but just counts tasks in the past -- except the latest one, as suggested by Max in the meeting today -- as "missed". I'm not sure it's a good idea to use "M" in the mask. Maybe one of the existing symbols (-, +, or X) would be better. I'm a little unclear on where those are interpreted.

However, this causes some issues with the tests. In particular, this one seems to be relying on some edge cases or bugs:

taskwarrior/test/recurrence.test.py

Lines 139 to 164 in d4649dd

	def test_recurrence_until(self):
	"""Verify that an 'until' date terminates recurrence"""
	self.t("add one due:now+1minute recur:PT1H until:now+125minutes")
	code, out, err = self.t("list rc.verbose:nothing")
	self.assertEqual(out.count("one"), 1)
	# All three expected tasks are shown:
	# - PT1M
	# - PT61M
	# - PT121M
	# - Nothing after PT125M
	self.t.faketime("+3h")
	code, out, err = self.t("list rc.verbose:nothing")
	self.assertEqual(out.count("one"), 3)
	# This test verifies that 'until' propagates to the instances, which
	# was not the original intention, but has been this way for a a while.
	# Given that the original decision was arbitrary, this is no worse.
	# We shall preserve recent behavior.
	self.t.faketime("+24h")
	code, out, err = self.t("( status:pending or status:recurring ) count")
	self.assertEqual("0\n", out)
	self.assertIn("Task 2 'one' expired and was deleted.", err)
	self.assertIn("Task 3 'one' expired and was deleted.", err)
	self.assertIn("Task 4 'one' expired and was deleted.", err)

This passes without my fix, and I started playing with it without my fix. Just adding an extra task list before the assertion line 153 will fail -- the task list itself returns no results, because each task "expired and was deleted". So I think the test is relying on that expiration check occurring before the recurrence.

Also, running task list at 122 minutes, so before the due date, produces four recurrences, not the expected three, at PT1M, PT61M, PT121M, and PT181M. A task info confirms that the last of those has a due later than until.

I think I'm too deep into the buggy guts of the recurrence implementation, so I'm going to step back from this bug. It's been a longstanding bug, so I don't feel too bad about throwing it back in the backlog.

[djmitche]

If anyone else wants to try to fix this, please comment and/or assign yourself!