ENG-2492: allow 5 minute grace period for daily scheduled jobs #7346
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
adamsachs
changed the title
adamsachs
requested review from
thabofletcher
and removed request for
a team
Greptile OverviewGreptile SummarySuccessfully addresses previous review feedback by implementing support for both string-based cron triggers and Key Changes:
Previous feedback addressed:
Confidence Score: 5/5
Important Files Changed
|
Additional Comments (2)
With
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
|
@greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| trigger == "cron" | ||
| and "misfire_grace_time" | ||
| not in kwargs # respect user-specified grace period | ||
| and _is_daily_or_less_frequent(kwargs) | ||
| ): | ||
| kwargs["misfire_grace_time"] = DEFAULT_DAILY_JOBS_GRACE_PERIOD | ||
| return super().add_job(func, trigger=trigger, **kwargs) # type: ignore[misc] |
There was a problem hiding this comment.
Cron detection is incorrect
_DailyJobGracePeriodMixin.add_job() only applies the grace period when trigger == "cron", but the codebase schedules cron jobs both via trigger="cron" and by passing a CronTrigger instance (e.g. trigger=cron_trigger in src/fides/api/service/privacy_request/email_batch_service.py:160-167). In those cases trigger is not the string "cron", so the 5-minute grace period will never be applied to those daily jobs (including the production job motivating this PR if it uses create_cron_trigger()). This should detect CronTrigger objects as well (and extract their fields) so daily jobs consistently get the intended misfire_grace_time.
| class TestDailyJobGracePeriodMixin: | ||
| @pytest.mark.parametrize( | ||
| "trigger,add_job_kwargs,expect_grace_time", | ||
| [ | ||
| # Daily cron: should auto-inject grace period | ||
| ( | ||
| "cron", | ||
| {"hour": 3, "minute": 0}, | ||
| DEFAULT_DAILY_JOBS_GRACE_PERIOD, | ||
| ), | ||
| ( | ||
| "cron", | ||
| {"hour": "2", "minute": "0", "day": "*", "timezone": "US/Eastern"}, | ||
| DEFAULT_DAILY_JOBS_GRACE_PERIOD, | ||
| ), | ||
| # Hourly/sub-hourly cron: should NOT inject | ||
| ("cron", {"hour": "*"}, None), | ||
| ("cron", {"minute": "*/5"}, None), | ||
| ("cron", {"hour": "*/2", "minute": 0}, None), | ||
| # Non-cron trigger: should NOT inject | ||
| ("interval", {"seconds": 300}, None), | ||
| # Explicit override: should preserve caller's value | ||
| ("cron", {"hour": 3, "misfire_grace_time": 60}, 60), | ||
| ], | ||
| ids=[ | ||
| "daily-cron-injects", | ||
| "daily-cron-with-extras-injects", | ||
| "hourly-cron-skipped", | ||
| "sub-hourly-cron-skipped", | ||
| "every-n-hours-cron-skipped", | ||
| "interval-trigger-skipped", | ||
| "explicit-override-preserved", | ||
| ], | ||
| ) | ||
| def test_background_scheduler_grace_period( | ||
| self, trigger, add_job_kwargs, expect_grace_time | ||
| ): | ||
| sched = GracefulBackgroundScheduler() | ||
| with patch.object(BackgroundScheduler, "add_job") as mock_add_job: | ||
| sched.add_job(lambda: None, trigger=trigger, **add_job_kwargs) | ||
|
|
||
| mock_add_job.assert_called_once() | ||
| call_kwargs = mock_add_job.call_args.kwargs | ||
|
|
||
| if expect_grace_time is not None: | ||
| assert call_kwargs["misfire_grace_time"] == expect_grace_time | ||
| else: | ||
| assert "misfire_grace_time" not in call_kwargs |
There was a problem hiding this comment.
Test misses CronTrigger case
The injected grace period logic is only exercised with trigger="cron" (string) in these tests, but much of the codebase schedules cron jobs with an actual CronTrigger instance (e.g. trigger=cron_trigger). Given the current implementation, those jobs won’t get the grace period, and this test suite wouldn’t catch it. Add a test that schedules with trigger=create_cron_trigger(...) (or a CronTrigger directly) and asserts the grace period is applied.
| def _is_daily_or_less_frequent(trigger_args: dict) -> bool: | ||
| """A cron job with a specific hour value runs at most once per day.""" | ||
| hour = trigger_args.get("hour") | ||
| return hour is not None and "*" not in str(hour) and "/" not in str(hour) |
There was a problem hiding this comment.
I think this needs detection for "," not in str(hour) and "-" not in str(hour) as well to handle cases like hour being 2,14 (twice daily) and 0-3 (4 times)
|
@adamsachs generally looks good, please see my comment about the cron check for is daily or less frequent. Also the greptile gripe seems valid but I'm not familiar enough to say definitively, but I'd say it is probably worht applying this to CronTrigger as well |
|
@greptile |
thabofletcher
left a comment
thabofletcher
left a comment
There was a problem hiding this comment.
💯 - even more robust than my proposed solution (although as you mentioned in the PR I can't test)
2 participants
Related to ENG-2492
Description Of Changes
Increases the grace period (from default 1 second to 5 minutes) automatically on any jobs scheduled for daily execution that use
APScheduler.See ticket comments for more context motivating the change: we saw our 'GVL cache reload' task, which is scheduled to run once daily, miss an execution on a production deployment. the likely root cause was a scheduler 'misfire' that lasted for longer than the default 1s grace period.
as a mitigation, we're overriding the scheduler to detect when scheduling a daily (or less frequent) job, and providing a significantly longer grace period. there should be no negative impact here: these jobs are scheduled to run once daily, so running within 5 minutes after the scheduled time should be totally acceptable.
Code Changes
APSchedulers so that they setmisfire_grace_time=300for any jobs scheduled for daily execution that don't have amisfire_grace_timeotherwise setSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works