fix: fix race condition where library is not in the store yet#826
fix: fix race condition where library is not in the store yet#826marslanabdulrauf wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of the ol_openedx_git_auto_export plugin when exporting newly-created courses/libraries to GitHub by handling “content not found (yet)” scenarios and deferring certain Celery dispatches until after DB commit, reducing transaction-timing races.
Changes:
- Introduces
ContentNotFoundErrorand updatesget_content_infoto raise it when courses/libraries can’t be found (including handlingContentLibraryNotFoundfrom the v2 libraries API). - Updates Celery tasks to treat missing content as a non-fatal/expected condition (logging and skipping/returning cleanly).
- Defers the library v2 “create repo + export” task dispatch using
transaction.on_commit(...)to avoid running before the creating transaction commits.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/utils.py |
Raises a plugin-specific not-found exception for v1/v2 library and course lookups. |
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/tasks.py |
Handles ContentNotFoundError in async tasks; adjusts Studio URL path composition for repo description. |
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/signals.py |
Uses transaction.on_commit for v2 library creation task dispatch to prevent pre-commit races. |
src/ol_openedx_git_auto_export/ol_openedx_git_auto_export/exceptions.py |
Adds the new ContentNotFoundError exception type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@marslanabdulrauf is this ready for review? Could you please add the |
arslanashraf7
left a comment
arslanashraf7
left a comment
There was a problem hiding this comment.
LGTM with a small comment 👍
|
|
||
| # Determine URL path based on content type | ||
| url_path = f"{content_info['content_type']}/{context_key_str}" | ||
| url_path = f"authoring/{content_info['content_type']}/{context_key_str}" |
There was a problem hiding this comment.
My understanding is that this is only for informational purposes. Or is there more to it?
nit because this impacts only repo description change in my opinion: But in any case, I think this should be configurable because this value comes from infra, and it can change over time. So, making it configurable would allow us to keep up to date with the original value in infra without any code change and a new plugin deployment. This could be a new setting that defaults to authoring.
There was a problem hiding this comment.
Yes it is only for informational purposes.
/authoring is usually hardcoded in ol-infrastructure as well, no variable there.
There was a problem hiding this comment.
Yes, that's True. But imagine this scenario:
If they change the /authoring to something like /author. What will we need? 1) We will need to update this plugin code and change this line. 2) Deploy a new plugin on PyPI. 3) Change the plugin pin in infra. 4) Rebuild the Open edX instance deployment.
Whereas if this were a configurable setting in the plugin, we would need only: 1) update the config value to /author in infra; 2) rebuild the Open edX instance.
That being said, this is still a nit, so it's up to you to make this change or merge as is.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/12267
Description (What does it do?)
This pull request improves the reliability of exporting courses and libraries to GitHub by introducing better error handling for cases where content is not yet available in the database due to transaction timing. It adds a custom exception, ensures Celery tasks are dispatched only after database commits, and updates error handling in utility and task functions.
How can this be tested?