fix: course cert not created on credentials service#38774
Open
Waleed-Mujahid wants to merge 1 commit into
Open
fix: course cert not created on credentials service#38774Waleed-Mujahid wants to merge 1 commit into
Waleed-Mujahid wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race condition in course certificate sync to the Credentials service by ensuring the Celery task is enqueued only after the certificate transaction commits, preventing workers from querying an uncommitted GeneratedCertificate row.
Changes:
- Wrap
award_course_certificate.delay(...)intransaction.on_commit(...)in theCOURSE_CERT_CHANGEDsignal handler. - Update existing signal-handler tests to account for
on_commitbeing used (via mocking).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openedx/core/djangoapps/programs/signals.py | Enqueue the course-cert sync task on DB commit to avoid pre-commit race conditions. |
| openedx/core/djangoapps/programs/tests/test_signals.py | Adjust tests by mocking transaction.on_commit (needs stronger assertions to actually validate the new behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5e5495c to
eea62de
Compare
COURSE_CERT_CHANGED fires synchronously during GeneratedCertificate.save(), which runs inside the generate_certificate Celery task. The .delay() call was enqueuing award_course_certificate before the DB transaction committed, so the task raced ahead and hit eligible_certificates.get() DoesNotExist — exiting silently with no retry, and never posting the course cert to Credentials. Wrapping in transaction.on_commit() guarantees the cert row is committed before the task is enqueued. Fixes: EDLYPRODUCT-5411
36422da to
e619aef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes
#38776
Root Cause
COURSE_CERT_CHANGEDfires synchronously insideGeneratedCertificate.save(), which itself runs inside thegenerate_certificateCelery task. The.delay()call was enqueuingaward_course_certificatebefore the DB transaction committed, so a second Celery worker picked up the task immediately, calledeligible_certificates.get(), found nothing (the row wasn't committed yet), and returned silently — no retry, no error, no course cert posted to Credentials.The race window is small (~30ms in observed logs) but consistent, and reproducible on every fresh course completion:
Impact
Course certificates are never synced to the Credentials IDA, even when:
CredentialsApiConfig.enable_learner_issuanceisTrueENABLE_LEARNER_RECORDSisTruedownloadableGeneratedCertificatein the LMSThe
eligible_certificatesmanager only excludesaudit_passing/audit_notpassingstatuses — mode (verified,no-id-professional, etc.) is irrelevant. The cert is perfectly eligible; it simply does not exist in the DB at query time.Program certificates are unaffected —
handle_course_cert_awarded(the program cert handler) does not queryeligible_certificates; it delegates toget_completed_programswhich reads from the Credentials API directly.Downstream breakage: Learner Record MFE
The Learner Record MFE (
frontend-app-learner-record) renders program record pages by querying the Credentials service for both the programUserCredentialand individual course-runUserCredentials. When course certs are never posted to Credentials, the MFE shows every course in the program as "Not Earned" — even for learners who have fully completed all courses and hold a program certificate.Example (observed):
Program certificate: ✅ Awarded — making the "Not Earned" course rows even more confusing to learners.
The only workaround is the
notify_credentialsmanagement command backfill, which callsaward_course_certificatedirectly (bypassing the signal handler and therefore bypassing the race), but this needs to be run manually after the fact for each affected learner.Fix
Wrap the
.delay()call intransaction.on_commit()soaward_course_certificateis only enqueued after thegenerate_certificatetransaction commits — guaranteeing theGeneratedCertificaterow exists in the DB when the task runs.Testing
award_course_certificatelog shows "will award a course certificate to user X in course run Y" (the line logged just before the credentials POST, attasks.py)UserCredentialof typecourse-runis created in the Credentials service admin for that learnerNotes
notify_credentialsmanagement command backfill remains the correct remediation for learners affected before this fix is deployed.handle_course_cert_revokeddispatchesrevoke_program_certificates(program-level, not course-level) so is not affected by this race.CertificateDateOverridesignal handlers already usetransaction.on_commit— this fix makeshandle_course_cert_changedconsistent with that pattern.