[SE-4101] fix: address VisibleBlocks caching race condition

[gabor-boros]

This PR is trying to address a hard-to-reproduce VisibleBlocks per-user caching issue in which case bulk create results an IntegrityError, though it shouldn't

Discussions: Forum post

Dependencies: None

Sandbox URL: N/A - We were not able to produce a sandbox which can "consistently" show the issue.

Merge deadline: None

Testing instructions:

1. Read through the code

Author notes and concerns:

This fix is deployed for one of our clients and it is working well. We did no see issues since we applied the patch.

1. The issue is really hard to reproduce as it is based on a possible race condition

Reviewers

• @pkulkark

[openedx-webhooks]

Thanks for the pull request, @gabor-boros! I've created OSPR-5743 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[natabene]

@gabor-boros Thank you for your contribution. Please let me know once it is ready for our review.

[gabor-boros]

@natabene This is ready for your review, thank you! 🎉

[nedbat]

Let me know when this merges, it could be applied to Koa and Lilac.

[natabene]

@nsprenkle Hi Nathan, this is good for your review when you have a moment.

[nsprenkle]

@gabor-boros, in PROD this is probably caused by a race condition. In dev environments, I can reliably reproduce by doing some database manipulation.

TL;DR - we need to force a creation that conflicts with an existing visible blocks cache entry. A way to do this is turning off the assume_zero_if_absent functionality and deleting a user's grade data from the database.

Steps to reproduce

1. As a student, visit a course set up with Persistent grades enabled (default) and submit some graded responses.
2. This creates some subsection grades and visible block entries. Verify with the queries below:
   1. Get user ID: select id from auth_user where username = <username>
   2. Verify that there subsection grade entries for user: select id, created, modified, usage_key from grades_persistentsubsectiongrade where user_id = <user-id> and course_id = <course-id>;
3. As the student, visit the progress page. It should load normally with grades.
4. As staff, disable assume zero flag from Django admin:
   1. Django-waffle > Switches > grades.assume_zero_if_absent = inactive
5. As super user, delete persisted course grade and subsection grades for user
   1. Go to SQL shell. For Devstack: make mysql-shell-edxapp
   2. Delete course grades for user (see queries): delete from grades_persistentcoursegrade where user_id = <user-id> and course_id =<course-id>;
   3. Get subsection grade IDs for user: select id from grades_persistentsubsectiongrade where user_id = <user-id> and course_id = <course-id>;
   4. Delete subsection grade overrides for user: delete from grades_persistentsubsectiongradeoverride where grade_id in(<subsection-grade-ids>);
   5. Delete subsection grades for user: delete from grades_persistentsubsectiongrade where user_id = <user-id> and course_id = <course-id>;
6. As a student, revisit the progress page. This will see missing persisted grades and try to create (instead of update) existing cache entry, causing a collision.

[nsprenkle]

From testing, this does appear to fix the issue. It also only runs once when the error condition first occurs (instead of every re-query for course grade) which is nice. So, although it slows the query down one time during these race conditions (which I still don't understand well), it should be a one-time impact.

[nsprenkle]

@mattcarter, since we had talked about this briefly, I'm comfortable approving insofar as this appears to fix the issue but wanted to bring you in for any further DB related concerns.

[nsprenkle]

@gabor-boros, this looks good for fixing the bug 👍

One request: can you add a log statement at the top level of the except IntegrityError with user/course? I think it might be helpful to start gathering data on when/where this error occurs.

[gabor-boros]

@nsprenkle
First of all, thank you for reviewing the Pull Request!

One request: can you add a log statement at the top level of the except IntegrityError with user/course?

That's actually a pretty good idea, I'll add that change today

[gabor-boros]

@nsprenkle added the logging statement. The wording may not the best. If you have an idea about a better message, feel free to let me know and I'll adjust the log message.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[nsprenkle]

Fabulous :)

[openedx-webhooks]

@gabor-boros 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[nsprenkle]

@nedbat, resurfacing for possible inclusion into Koa & Lilac

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[nedbat]

@nsprenkle thanks, I've cherry-picked this onto Koa and Lilac.