feat: add periodicity_start parameter to Tasks

[0xDEC0DE]

Adds a new, optional, argument to task creation, periodcity_start, which gives Task authors finer control over scheduling of periodic tasks. Specifically, it allows Tasks to pin their start time to a defined wall-clock time, albeit on a best-effort basis, like all Tasks.

Fixes: Issue #48

[juledwar]

PR Review

Bugs

1. TypeError in next_future_periodic_delta (redis.py)

The diff changes now from a float timestamp to a datetime object, but next_event_time is still a numeric score from Redis:

now = self.start_at()          # datetime object
next_event_time = rv[0][1]     # int from Redis ZSCORE
if next_event_time < now:      # TypeError: '<' not supported between int and datetime

Should be now = self.start_at().timestamp().

2. UnboundLocalError in _snap_to (task.py)

If all three branches (delta.hour, delta.minute, delta.second) are falsy (e.g. timedelta(0) bypasses validation since int(0) > anything is False), mod is never assigned, causing UnboundLocalError on the if not mod: line. A guard for ps <= 0 or a fallback branch is needed.

3. Off-by-one in validation (task.py)

if ps > periodicity.total_seconds():

This allows periodicity_start == periodicity, which is semantically a no-op (snapping to the full period). Should be >=.

---

Design Issues

4. Documentation doesn't match behavior

The docs say:

This will run the task hourly, starting at the next 15-minute boundary (0, 15, 30, 45).

But the code adds periodicity_start on top of the full periodicity delay. With periodicity=1h and periodicity_start=timedelta(minutes=15), at 00:26:21 the first run is at 01:30:00 (one full period + snap offset), not the next 15-minute boundary (00:30:00). The documentation should clarify this is "the first 15-minute boundary that is at least one period away."

5. _snap_to uses fragile datetime arithmetic

delta = epoch + snap_interval  # datetime object
if delta.hour:
    mod = (now - epoch) % timedelta(hours=delta.hour)

This converts a timedelta into a datetime to extract .hour/.minute/.second components. This works only because the validation caps at 12 hours, but it's unintuitive and will silently break for values >= 24h (.hour wraps around). Using divmod on snap_interval.total_seconds() to extract hours/minutes/seconds would be more robust and readable.

6. Lua script uses mutation as control flow (register_periodic_tasks.lua)

if existing_task["periodicity_start"] ~= task["periodicity_start"] then
    next_event_time = next_event_time + task["periodicity_start"]
    existing_task["periodicity"] = nil   -- side-effect to force next branch
end
if existing_task["periodicity"] ~= task["periodicity"] then
    redis.call('zadd', ...)
end

Setting existing_task["periodicity"] = nil to force the subsequent condition to be true is clever but fragile. Combining with or is clearer:

if existing_task["periodicity"] ~= task["periodicity"]
   or existing_task["periodicity_start"] ~= task["periodicity_start"] then

7. start_at() naming is vague

start_at() on Broker reads like "when did this broker start?" It's actually math.ceil of the current UTC timestamp. Something like _now_ceil() would be more descriptive.

---

Minor

• PR title typo: "periodcity" → "periodicity"
• No test for timedelta(0) as periodicity_start — this will hit the UnboundLocalError described above.
• 12-hour hard limit (43200) is an unexplained magic number. Consider a named constant with a comment explaining the rationale.
• periodicity_start default of 0 (int) vs timedelta: The parameter accepts a timedelta but stores an int. This mixed typing could confuse callers inspecting the attribute. Consider storing None as the default and converting only where needed.

---

What looks good

• Validation that periodicity must be set if periodicity_start is set
• The _snap_to offset is computed once at task creation, avoiding repeated clock calls
• Serialization includes periodicity_start, keeping Redis and memory brokers consistent
• Tests cover parametrized periodicities, rollover past midnight, and invalid-value rejection
• Consolidating math.ceil(datetime.now(...).timestamp()) into start_at() on the base class reduces duplication
• docker-compose → docker compose and YAML cleanup are welcome housekeeping

[0xDEC0DE]

Responses

Written with all the love and respect that a clanker doing a code review deserves:

1. TypeError in next_future_periodic_delta (redis.py)

Cool. But that's an existing bug, not a regression introduced by this change. That code path also has zero test coverage, all the tests hit the short-circuit path. Good on you for pointing that out. Oh, wait, you didn't.

2. UnboundLocalError in _snap_to (task.py)

Whoops! PyCharm even lit that up like a Christmas tree, no idea how I missed that. Fixed.

3. Off-by-one in validation (task.py)

This allows periodicity_start == periodicity, which is semantically a no-op (snapping to the full period). Should be >=.

No, in the == case I want to fall through to the snap behavior; just in case.

4. Documentation doesn't match behavior

Get fucked.

5. _snap_to uses fragile datetime arithmetic

The value is capped at 12 hours. How in the fuck is a >= 24h value supposed to happen? You may as well advise guarding against unicorn attacks.

6. Lua script uses mutation as control flow (register_periodic_tasks.lua)

Setting existing_task["periodicity"] = nil to force the subsequent condition to be true is clever but fragile. Combining with or is clearer:

That's... not what that does. It's enforcing the exclusivity of the two parameters.

7. start_at() naming is vague

Get fucked.

• PR title typo: "periodcity" → "periodicity"

Whoopsie!

• No test for timedelta(0) as periodicity_start — this will hit the UnboundLocalError described above.

Yeah, fine.

• 12-hour hard limit (43200) is an unexplained magic number. Consider a named constant with a comment explaining the rationale.

Get fucked.

• periodicity_start default of 0 (int) vs timedelta: The parameter accepts a timedelta but stores an int. This mixed typing could confuse callers inspecting the attribute. Consider storing None as the default and converting only where needed.

Considered. Rejected. The code is pretty clear about what it's doing.

What looks good

I see that Dr. Flattery, the Always-Wrong Robot, has weighed in. Great.