feat: add clear expired tokens task in ol-social-auth#778
Conversation
| ) | ||
| if not hasattr(settings, "CELERY_BEAT_SCHEDULE"): | ||
| settings.CELERY_BEAT_SCHEDULE = {} | ||
| settings.CELERY_BEAT_SCHEDULE["clear_expired_tokens"] = { |
There was a problem hiding this comment.
Let's distinguish it.
| settings.CELERY_BEAT_SCHEDULE["clear_expired_tokens"] = { | |
| settings.CELERY_BEAT_SCHEDULE["ol_clear_expired_tokens"] = { |
| if not hasattr(settings, "CELERY_BEAT_SCHEDULE"): | ||
| settings.CELERY_BEAT_SCHEDULE = {} | ||
| settings.CELERY_BEAT_SCHEDULE["clear_expired_tokens"] = { | ||
| "task": "ol_social_auth.tasks.clear_expired_tokens", |
There was a problem hiding this comment.
Let's do it here as well
| "task": "ol_social_auth.tasks.clear_expired_tokens", | |
| "task": "ol_social_auth.tasks.ol_clear_expired_tokens", |
| settings.CELERY_BEAT_SCHEDULE = {} | ||
| settings.CELERY_BEAT_SCHEDULE["clear_expired_tokens"] = { | ||
| "task": "ol_social_auth.tasks.clear_expired_tokens", | ||
| "schedule": crontab(hour=9, minute=0, day_of_week="monday"), |
There was a problem hiding this comment.
Why not midnight?
| "schedule": crontab(hour=9, minute=0, day_of_week="monday"), | |
| "schedule": crontab(hour=0, minute=0, day_of_week="monday"), |
There was a problem hiding this comment.
I’m not entirely sure, but this task is scheduled for 9 AM in our other applications as well; for example, MITxOnline and MITxPRO
@pdpinch, what are your thoughts on this?
There was a problem hiding this comment.
If it's a task that we care about succeeding, I prefer to do it during business hours, so we have a better chance of noticing a problem and fixing it.
For this task, like this, really, any time would be fine.
|
|
||
|
|
||
| def plugin_settings(settings): | ||
| """Add clear_expired_tokens to the Celery beat schedule.""" |
There was a problem hiding this comment.
The docstrings are for the while plugin_settings, so they need to be generic.
| # Suppress debug-level logs from oauth2_provider during cleanup. | ||
| # Its batch_delete debug logs lack the 'userid' field expected by | ||
| # Open edX's custom log formatter, causing noisy ValueError tracebacks. |
There was a problem hiding this comment.
Can you explain this a little bit more? I am having a hard time getting it. Did we copy it from Open edX?
There was a problem hiding this comment.
This wasn't copied from Open edX — it's a workaround for an incompatibility between django-oauth-toolkit and Open edX's custom log formatter.
The problem: clear_expired() internally calls batch_delete(), which emits debug-level log messages like "10000 tokens deleted, X left". Open edX configures a custom log formatter that expects every log record to have a userid field (used for request-scoped user tracking). The oauth2_provider library doesn't add this field to its log records, so every debug message it emits causes a ValueError: Formatting field not found in record: 'userid' — producing a noisy traceback in the logs on every batch deletion.
The fix: Before calling clear_expired(), we temporarily raise the oauth2_provider logger level to INFO, which silences the debug-level batch messages. After the call completes (or fails), we restore the original level. This way the deletion still happens correctly — we just suppress the log messages that would trigger the formatter error.
To reproduce the issue, run the clear_expired() fn without suppressing the logs.
|
|
||
| **Behavior:** | ||
|
|
||
| * Runs every **Monday at 9:00 AM** (server time) via Celery Beat by default. The schedule can be customized by overriding the ``clear_expired_tokens`` entry in ``CELERY_BEAT_SCHEDULE``. |
There was a problem hiding this comment.
this will change too if we decide to run the task at midnight.
for more information, see https://pre-commit.ci
| if not hasattr(settings, "CELERY_BEAT_SCHEDULE"): | ||
| settings.CELERY_BEAT_SCHEDULE = {} | ||
| settings.CELERY_BEAT_SCHEDULE["ol_clear_expired_tokens"] = { | ||
| "task": "ol_social_auth.tasks.ol_clear_expired_tokens", | ||
| "schedule": crontab(hour=9, minute=0, day_of_week="monday"), |
There was a problem hiding this comment.
I was able to test it manually via Django shell, but I wasn't able to test it via the beat schedule and the task being run in auto mode. Were you able to test it?
Also, Open edX has CELERYBEAT_SCHEDULE in the settings in common.py instead of CELERY_BEAT_SCHEDULE. Could you tell if there was any specific reason for using a new env key rather than using the existing openedx one? And also, could this be the reason the task schedule isn't working?
There was a problem hiding this comment.
Good catch. I’ve updated the setting name in dce4d4b and verified that the tasks are running as expected according to the beat schedule.
arslanashraf7
left a comment
There was a problem hiding this comment.
LGTM 👍 One question for @blarghmatey before merging.
| def plugin_settings(settings): | ||
| """Settings for the ol-social-auth plugin.""" # noqa: D401 | ||
| settings.OAUTH2_PROVIDER["REFRESH_TOKEN_EXPIRE_SECONDS"] = ( | ||
| 30 * 24 * 60 * 60 # 30 days |
There was a problem hiding this comment.
I'd suggest checking with @blarghmatey this time range as well, since we're using service tokens before merging the PR. Since DevOps creates service tokens, I'm not sure what expiry time they set. I want to make sure they will not be deleted automatically.
There was a problem hiding this comment.
The OAuth service accounts that we rely on are using client credentials grants so we're not worried about token expiration. Thanks for checking.
* feat: add task * fix: issues * chore: bump version * docs: update readme * fix: issues * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: rename to CELERYBEAT_SCHEDULE --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: arslanashraf7 <34372316+arslanashraf7@users.noreply.github.com>
Reverts the changes introduced by two commits that were accidentally included in this branch during an earlier rebase on a shallow clone: - f71e486 (feat: add clear expired tokens task in ol-social-auth #778) - 7226134 ([pre-commit.ci] pre-commit autoupdate #783) These commits are already in main; including them here caused the PR to show unrelated ol_social_auth and pre-commit changes. Co-authored-by: arslanashraf7 <34372316+arslanashraf7@users.noreply.github.com>
What are the relevant tickets?
https://github.com/mitodl/hq/issues/10116
Description (What does it do?)
This PR adds a
clear_expired_tokenstask in the ol-social-auth plugin to periodically delete the expired access and refresh tokens from the edX database.Screenshots (if appropriate):
How can this be tested?
Additional Context