diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 0c3e40a15..68a46ac55 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -240,9 +240,9 @@ Decision 3. ``oel_tagging_objecttag(object_id)`` 4. ``CompetencyCriteria(oel_tagging_objecttag_id)`` 5. ``CompetencyCriteria(competency_criteria_group_id)`` - 6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` - 7. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` - 8. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` + 6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` (unique) + 7. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` (unique) + 8. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` (unique) 9. ``CompetencyRuleProfile(scope_code)`` (unique -- at most one profile per distinct scope value; a plain unique constraint on the three raw nullable scope columns would not enforce this, since SQL never treats two ``NULL`` values as equal and this project's MySQL backend does not support the conditional/partial unique indexes that would otherwise route around that; see the ``scope_code`` column in Decision 3) 10. ``CompetencyMasteryStatuses(status)`` (unique) @@ -422,3 +422,11 @@ Rejected Alternatives 1. Silently does not work on this project's tested and production database backend. Django compiles a conditional ``UniqueConstraint`` to a partial index, which MySQL does not support; Django raises only a non-fatal system-check warning (``models.W036``) and skips creating the constraint, leaving the uniqueness rule completely unenforced at the database level. 2. The gap would surface only as a data-integrity incident under concurrent writes, not as a test or migration failure, since SQLite (used for quick local test runs) does support partial indexes and would mask the problem in that environment. + +Changelog +--------- + +2026-07-27: + +* Made the learner status indexes unique, so there is one row per learner and node. This is what + the in-place, monotone status updates in :ref:`openedx-learning-adr-0004` read, lock, and update. diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 0f0f82515..ff4c3b4f7 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -44,12 +44,18 @@ For the initial implementation, versioning and traceability of competency achiev - A ``CompetencyRuleProfile`` is "in use" if any ``CompetencyCriterion`` assigned to it (``competency_rule_profile_id``) has an associated ``StudentCompetencyCriteriaStatus`` row. Editing an in-use profile's ``rule_type``/``rule_payload`` requires the same warning and confirmation. - The same warning applies when creating a more specific profile causes existing criteria to be reassigned to it, and when an authoring action switches a criterion between a profile assignment and per-criterion overrides (ADR 0002 Decision 4). -5. Learner status models/tables are append-only history and do not use ``django-simple-history``: +5. Learner status models/tables are updated in-place: - - For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``, each status change is stored as a new row with ``created`` as the write timestamp. - - Existing learner status rows are not updated in place. - - Current status is determined by the most recent row for a given learner + target entity (ordered by ``created``, with ``id`` as a tie-breaker). - - Older rows represent the learner status history and remain available for audit/tracing. + - For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``, + each status change updates the responsible row. + - Automatic status updates only ever increase a status, as relied on by + :ref:`openedx-learning-adr-0004`. A downward adjustment (for example ``Demonstrated`` to + ``PartiallyAttempted``) is never applied by a grade change or by a competency criteria rule + change. + - Direct edits by staff, through Django admin or as a deliberate instructor correction, are + exempt: they may set a status to any value, including a lower one, and the ancestors above the + edited node are recomputed to match. + - How learner status history is retained is not decided here. Rejected Alternatives @@ -85,3 +91,13 @@ Rejected Alternatives - Cons: - Requires custom tooling to reconstruct past versions - Does not align with existing publishable versioning patterns + +Changelog +--------- + +2026-07-27: + +* Reworked Decision 5 for :ref:`openedx-learning-adr-0004`: learner status rows are now updated in + place, and automatic updates only ever increase a status, with direct staff edits exempt. + Previously append-only, with current status resolved as the most recent row. How status history is + retained is left undecided. diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst new file mode 100644 index 000000000..9d50d8710 --- /dev/null +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -0,0 +1,148 @@ +.. _openedx-learning-adr-0004: + +4. How should learner competency mastery be recorded concurrently and at scale? +================================================================================ + +Status +------ +Proposed. + +Context +------- +A learner's mastery of one competency is stored at three levels of the criteria tree: the graded +leaf criterion, each criteria group above it, and the competency itself. There is one row per +learner and node, updated in place (:ref:`openedx-learning-adr-0002`, +:ref:`openedx-learning-adr-0003`). Each row holds one of three values, lowest to highest: +``AttemptedNotDemonstrated``, ``PartiallyAttempted``, ``Demonstrated``. + +One grade change updates the leaf and then every row above it, for many learners at once. This ADR +decides how those updates stay correct when two of them for the same learner overlap. + +The problem: a group requires both Assignment A and Assignment B, and celery tasks recomputing +grades and competency statuses for this worker run at the same time. That is likely to happen +when instructor actions trigger multiple subsection grading events. +Each of the two writers sees its own assignment done and the other still outstanding, +so both write "not demonstrated" for the group. Both are wrong, both have finished, and nothing is +left to correct it. + +Two constraints shape the answer. First, **the grading task cannot be one transaction**: it reads +MongoDB and memcached, writes to file storage, queues further celery tasks, publishes events, and +triggers database writes owned by four other Django apps. Wrapping all of that would roll back other +apps' data and publish events for a grade that never commits. Second, **everything above the leaf is +derived**: a group's value can always be recalculated from the leaves beneath it, so the leaf is the +only row that is a direct consequence of the grade. + +Decision +-------- + +1. **Write the leaf status in the same transaction as the grade. Nothing above it.** + The grading task calls one openedx-core function, which writes the leaf, so the grade and its + leaf commit or fail together. Every row above the leaf is written after that transaction commits. + +2. **Roll up one level at a time, committing each level before reading the next. Take no locks.** + A writer sees only committed data, so whichever writer reads a parent last sees all its children + at their final values and computes the correct result. Some writer always reads last, so the tree + ends up correct and no writer waits for another. + +3. **Automatic updates may only raise a status, never lower it.** Each write stores whichever is + higher, the stored or the newly computed value, in a single statement so concurrent writers cannot + overwrite each other. A writer reading stale data can then only compute a value that is too low, + and too low is discarded. That is also what makes celery's repeated and out-of-order delivery + harmless. + +4. **Re-run a failed roll-up rather than undoing the grade.** By Decision 3 a failure leaves rows too + low, never too high, so nothing incorrect needs undoing and re-running is always safe. + +5. **Add a "dirty" marker, set with the value change and cleared once the parent has been + recalculated.** It is set by the same statement that changes the value, so nothing can fail in + between, and cleared whether or not the parent's value changed. Clearing is conditional on the + value passed up still being current, otherwise one writer can clear another's marker and lose its work. + +6. **A scheduled job looks for "dirty" markers and finishes roll-ups that stopped partway.** + The job is scheduled rather than triggered, because a killed worker raises no exception to react + to. openedx-core cannot own a scheduler, so it exposes the entry point and the deployment sets the + interval. In a healthy system no markers are set, so a marker older + than the interval is also the alert. + +7. **Only a direct staff edit may lower a status.** A staff correction may set any value, and the + rows above it are recalculated and overwritten rather than merged. A later grade change can raise + what an edit lowered, but never lower what an edit raised. It is also the only path that takes a + lock, on the learner's root group row. + +Rejected Alternatives +--------------------- + +1. Lock each criteria group row before recalculating it. This was the previous decision here. + + - Pros: + - Correctness comes from making contending writers take turns, which is easier to prove than + an argument about the order of commits and reads. + - Cons: + - One grade change can affect several leaves of the same tree, so a writer can need several + locks at once, which introduces deadlocks that need their own detection and retry code. + - It puts a lock wait on every grade change. MySQL waits 50 seconds by default, inside a task + allowed 300 seconds in total. + - Correctness would depend on the isolation level, silently, and SQLite has no row locks, so + the test suite could not exercise it. + +2. Share one transaction between the grade and the whole roll-up, not just the leaf. This ADR + originally assumed this was available. + + - Pros: + - The grade and every mastery row it touches would commit or fail together, so no roll-up + could ever be left unfinished and Decisions 5 and 6 would be unnecessary. + - Cons: + - The grading task cannot be wrapped in a transaction at all, for the reasons in the Context. + - Wrapping only the roll-up is worse than doing nothing: it hides each writer's changes from + the other until both have finished, which is the problem in the Context again, one level up + the tree and harder to diagnose. + +3. Take one lock on the learner's root group row, then recalculate the whole subtree beneath it. + + - Pros: + - Easy to reason about: one lock, always the same row, so no deadlock and no ordering + argument. + - Cons: + - It puts a lock, and its timeout handling, on every grade change rather than only on the + rare path that lowers a value. + - It makes a learner's unrelated competencies wait for each other, and needs row locks, + which SQLite does not support. + + This is the right shape for the paths that lower a value, and Decision 7 uses it there. + +4. Use a coarser lock, either one per deployment or one per learner. + + - Pros: + - A single lock replaces the ordering argument in Decision 2. + - Cons: + - A deployment-wide lock serializes every learner behind every other, giving up the + throughput bursty grading needs. + - Either kind adds machinery for acquiring and releasing locks, and for recovering from a + dead lock holder, across a very large key space. + +5. Recalculate the derived levels on every read instead of storing them. + + - Pros: + - No roll-up writes at all, so there is nothing to keep consistent. + - Cons: + - It moves a full bottom-up tree evaluation onto every read, the opposite of what dashboards + need. + - Already settled against in :ref:`openedx-learning-adr-0002`. Unresolved item 1 is the + narrower version still open. + +6. Send an event to openedx-core and do all the work in a separate celery task. + + - Pros: + - Recording a grade would not depend on the competency code being installed or fast. + - Cons: + - openedx-core is a library and cannot own a celery queue, so every caller would supply one. + - The leaf would no longer commit with the grade, giving up the one guarantee Decision 1 is + cheap enough to keep. + +7. Detect conflicts optimistically, with a version column and a retry loop for the losing write. + + - Pros: + - Contention costs a retry rather than a wait. + - Cons: + - Decision 3 is already optimistic, without the retry loop. A write that loses has computed + a value that is too low, and discarding those is exactly what Decision 3 does.