AAP-70288 Catch errors on re-compute of schedule RRULE computed fields#16400
AAP-70288 Catch errors on re-compute of schedule RRULE computed fields#16400AlanCoding wants to merge 5 commits intoansible:develfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds defensive handling for ValueError from rrule parsing in the Schedule model and guards scheduler processing to log and skip or clear faulty schedules instead of aborting; includes tests for DST/rrule edge cases and scheduler resilience. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant ScheduleModel as Schedule
participant Logger as Logger
participant JobService as JobCreator
Note over Scheduler,ScheduleModel: periodic run iterates schedules
Scheduler->>ScheduleModel: call update_computed_fields()
alt update_computed_fields succeeds
ScheduleModel-->>Scheduler: computed fields updated
Scheduler->>JobService: enqueue/create job if due
else update_computed_fields raises ValueError
ScheduleModel-->>Logger: log warning/error (includes schedule id/rrule)
Logger-->>Scheduler: exception handled
Scheduler->>ScheduleModel: force-update next_run=None, dtend=None
Scheduler-->>Scheduler: skip further processing for this schedule (continue)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I hit an issue, some RRULE values would get accepted by PATCH requests. I updated this so that those should return a 400 error, and this was a pre-existing bug. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/models/schedules.py`:
- Around line 329-341: The parse error for Schedule.rrulestr in
Schedule.update_computed_fields is currently swallowed after clearing next_run,
which allows a stale next_run to still be used by the scheduler; instead, after
logging and setting self.next_run and self.dtend to None, re-raise the
ValueError (or raise a new descriptive exception) so callers like
awx/main/tasks/system.py receive a failure signal and can skip/continue that
schedule; reference Schedule.update_computed_fields, Schedule.rrulestr and the
except ValueError block to locate where to change the behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff9755a8-f65c-4be0-88d0-f65199194c5f
📒 Files selected for processing (2)
awx/main/models/schedules.pyawx/main/tests/functional/api/test_schedules.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/models/schedules.py`:
- Around line 329-340: The except block for Schedule.rrulestr currently clears
in-memory self.next_run and self.dtend but re-raises before persisting; update
the handler in the Schedule class to set self.next_run = None and self.dtend =
None and then persist those changes (e.g., call self.save() or
self.save(update_fields=['next_run','dtend'])) before re-raising the ValueError
so the DB state reflects the cleared values and the broken schedule won't be
repeatedly reselected; ensure you still re-raise the original exception after
the save.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5af1687c-aef0-4437-a0c4-6e8dbb956cfa
📒 Files selected for processing (1)
awx/main/models/schedules.py
| try: | ||
| future_rs = Schedule.rrulestr(self.rrule) | ||
| except ValueError as e: | ||
| logger.error( | ||
| "Schedule id=%s has an invalid rrule that cannot be parsed: %s. Error: %s", | ||
| self.id, | ||
| self.rrule, | ||
| e, | ||
| ) | ||
| self.next_run = None | ||
| self.dtend = None | ||
| raise |
There was a problem hiding this comment.
Persisted-state gap: next_run/dtend are cleared in memory but not saved before re-raise.
At Line 340, re-raising exits before any save, so scheduler catch-and-continue paths can leave stale DB values and repeatedly re-select the same broken schedule.
Proposed fix
def update_computed_fields(self):
- changed = self.update_computed_fields_no_save()
+ try:
+ changed = self.update_computed_fields_no_save()
+ except ValueError:
+ # Fields were nulled in update_computed_fields_no_save(); persist that state.
+ super(Schedule, self).save(update_fields=['next_run', 'dtend', 'modified'])
+ raise
if not changed:
return
emit_channel_notification('schedules-changed', dict(id=self.id, group_name='schedules'))As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/models/schedules.py` around lines 329 - 340, The except block for
Schedule.rrulestr currently clears in-memory self.next_run and self.dtend but
re-raises before persisting; update the handler in the Schedule class to set
self.next_run = None and self.dtend = None and then persist those changes (e.g.,
call self.save() or self.save(update_fields=['next_run','dtend'])) before
re-raising the ValueError so the DB state reflects the cleared values and the
broken schedule won't be repeatedly reselected; ensure you still re-raise the
original exception after the save.
There was a problem hiding this comment.
...seems valid, unfortunately.
There was a problem hiding this comment.
@AlanCoding, glad the concern checks out. Here's a clean fix plan:
- Catch
ValueErrorinupdate_computed_fieldsafterupdate_computed_fields_no_savesets the fields toNone, persist those cleared values, then re-raise so callers (scheduler) still see the error but the DB is consistent.
Failed to handle agent chat message. Please try again.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 719aeff. Configure here.
| utc = tzutc() | ||
| # All rules in a ruleset will have the same dtstart so we can just take the first rule | ||
| tzinfo = Schedule.rrulestr(self.rrule)._rrule[0]._dtstart.tzinfo | ||
| try: |
There was a problem hiding this comment.
Missing root-cause catch permanently disables valid schedules
High Severity
The PR description states the fix "catches ValueError in _fast_forward_rrule and falls back to the original rrule," but _fast_forward_rrule has no such catch around rrule.replace() at line 100. Without it, the ValueError propagates to the new handlers in awx_periodic_scheduler, which set next_run=None via direct DB update. Since next_run=None never matches the before() or between() query filters, the schedule is permanently disabled — even after DST ends and the fast-forward would succeed again. Because fast-forwarding is purely a performance optimization, catching ValueError in _fast_forward_rrule and returning the original rrule would let valid schedules keep running instead of being irreversibly killed.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 719aeff. Configure here.
|
Baby Yolo ResultsRan containerized Baby Yolo build #132 (Apr 20) and compared against reference build #129 (devel, same day).
Verdict: This PR does not introduce test regressions. |
SUMMARY
DST - daylight savings time
ValueErrorin_fast_forward_rrulethat causes HTTP 500 on the schedules page when an HOURLY rrule with BYHOUR constraints is fast-forwarded across a DST boundaryawx_periodic_schedulerso one bad schedule cannot crash the task and block all other schedules from runningThe
_fast_forward_rruleoptimization advances the dtstart of HOURLY/MINUTELY rrules close to the current time, computing in UTC then converting back to local timezone. With rrules whose BYHOUR values are aligned to a specific residue mod INTERVAL (e.g., BYHOUR=1,5,9,13,17,21 with INTERVAL=4 — all 1 mod 4), this alignment holds during EST (UTC-5) but breaks during EDT (UTC-4). The 1-hour DST shift changes the residue, dateutil finds no valid hours, and raisesValueError: Invalid rrule byxxx generates an empty set.This propagates uncaught through thetimezone/untilserializer properties on GET and throughupdate_computed_fieldsin the periodic scheduler — crashing the schedules page and preventing all scheduled jobs from launching.Bad rrules enter the database through two confirmed pathways: PATCH during EST (validation passes because fast-forward alignment holds in standard time, then breaks when EDT starts) and POST with a future dtstart (fast-forward is skipped entirely, then fails once dtstart passes and the clock is in EDT). Both are covered by new functional tests with deterministic mocked time.
The fix catches
ValueErrorin_fast_forward_rruleand falls back to the original rrule (safe, since fast-forwarding is purely a performance optimization), adds defense-in-depth inupdate_computed_fields_no_saveto setnext_run = Nonefor any unparseable rrule, and wraps both unprotectedupdate_computed_fields()calls inawx_periodic_schedulerwith try/except — matching the safe pattern already used in_run_dispatch_startup_common.ISSUE TYPE
COMPONENT NAME
Note
Medium Risk
Touches schedule recomputation and the periodic scheduler loop; while mostly defensive error handling, it can change how invalid schedules are persisted/ignored and could impact job launch timing if parsing behavior regresses.
Overview
Prevents schedule reads and the periodic scheduler from crashing when an
rrulebecomes unparsable (notably DST-triggeredBYxxxempty-set errors).Schedule.timezone/Schedule.untilnow catchValueErrorfromSchedule.rrulestrand return empty fallbacks, andawx_periodic_schedulernow wrapsupdate_computed_fields()so a single bad schedule is logged, hasnext_run/dtendnulled, and is skipped without blocking other schedules.Adds functional tests covering DST/
BYHOURfailure scenarios (PATCH rejected with 400; GET stays 200 even if bad data exists in DB) and a task-level test ensuring the scheduler continues processing healthy schedules when one schedule recompute raises.Reviewed by Cursor Bugbot for commit 719aeff. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit