From 01933ee1371a3d90e0180e429f5366f81614b323 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 7 Aug 2026 10:57:03 -0400 Subject: [PATCH 1/7] docs: add ADR for competency mastery concurrency Adds ADR 0004 covering how learner competency mastery is recorded under concurrent, out-of-order grade-change events without a per-event serialization cost. Adjusts ADRs 0002 and 0003 to match: learner status is stored as an in-place ACTIVE row plus a paired append-only HISTORY table, with a unique index on the leaf HISTORY advance that serves as the idempotency key for the append. Co-Authored-By: Claude Opus 5 (1M context) --- .../0002-competency-criteria-model.rst | 31 +++- .../0003-competency-criteria-versioning.rst | 29 +++- .../0004-competency-mastery-concurrency.rst | 137 ++++++++++++++++++ 3 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 0c3e40a15..bea477bb1 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -240,11 +240,14 @@ 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)`` - 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) + 6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` (unique) + 7. ``StudentCompetencyCriteriaStatusHistory(user_id, competency_criteria_id, status_id)`` (unique -- at most one HISTORY row per learner, leaf, and status level, which also serves as the idempotency key for the append in :ref:`openedx-learning-adr-0004`) + 8. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` (unique) + 9. ``StudentCompetencyCriteriaGroupStatusHistory(user_id, competency_criteria_group_id)`` + 10. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` (unique) + 11. ``StudentCompetencyStatusHistory(user_id, oel_tagging_tag_id)`` + 12. ``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) + 13. ``CompetencyMasteryStatuses(status)`` (unique) 6. Learner progress status concepts (``StudentCompetency*Status`` database tables) @@ -257,6 +260,12 @@ Decision - ``StudentCompetencyStatus`` tracks top-level competency demonstration state. - All learner status rows use a shared lookup table (``CompetencyMasteryStatuses``) so status semantics live in one place and student status tables stay structurally consistent. + Append-only history tables: + + - ``StudentCompetencyCriteriaStatusHistory`` + - ``StudentCompetencyCriteriaGroupStatusHistory`` + - ``StudentCompetencyStatusHistory`` + Intended update flow (bottom-up materialization): - A learner event updates one ``StudentCompetencyCriteriaStatus`` row. @@ -422,3 +431,15 @@ 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: + +* Split learner status storage into paired ACTIVE and HISTORY tables: added the append-only + ``StudentCompetencyCriteriaStatusHistory``, ``StudentCompetencyCriteriaGroupStatusHistory``, + and ``StudentCompetencyStatusHistory`` tables and their indexes alongside the in-place ACTIVE + tables, per :ref:`openedx-learning-adr-0005`. +* Made the leaf HISTORY (``StudentCompetencyCriteriaStatusHistory``) index unique on ``(user_id, competency_criteria_id, status_id)``, the + idempotency key for the HISTORY append in :ref:`openedx-learning-adr-0004`. diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 0f0f82515..6481f72f1 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -44,12 +44,21 @@ 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. + - Statuses only increase monotonically as described by :ref:`openedx-learning-adr-0005`; + downward status adjustments (for example ``Demonstrated`` to ``PartiallyAttempted``) are prohibited. + +6. Learner status models/tables as in 5. above each get a separate append-only history table not using ``django-simple-history``: + + - For ``StudentCompetencyCriteriaStatusHistory``, ``StudentCompetencyCriteriaGroupStatusHistory``, and ``StudentCompetencyStatusHistory``, + each status advance is stored as a new row with ``created`` as the write timestamp. + - Existing learner status rows are not updated in place in the history tables. + - Statuses only increase monotonically as described by :ref:`openedx-learning-adr-0005`; + if a change would mean a downward adjustment (for example ``Demonstrated`` to ``PartiallyAttempted``) + or no adjustment, this does not get stored in the history tables. Rejected Alternatives @@ -85,3 +94,13 @@ Rejected Alternatives - Cons: - Requires custom tooling to reconstruct past versions - Does not align with existing publishable versioning patterns + +Changelog +--------- + +2026-07-27: + +* Reworked learner status handling to match :ref:`openedx-learning-adr-0005` and + :ref:`openedx-learning-adr-0004`: Decision 5 now updates learner status rows in place and + monotonically (downward adjustments prohibited), and a new Decision 6 adds separate append-only + HISTORY tables. Previously a single append-only model with no in-place ACTIVE row. 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..aab27e122 --- /dev/null +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -0,0 +1,137 @@ +.. _openedx-learning-adr-0004: + +4. How should learner competency mastery be recorded concurrently and at scale? +================================================================================ + +Status +------ +Proposed. + +Context +------- +When a learner is graded on a subsection (or any other learning instrument associated to a competency +with a competency criteria, like a course or rubric criterion), the platform must evaluate whether that grade +demonstrates any attached competencies and record the learner's mastery. Mastery is recorded at +three levels: the criterion (leaf), the criteria group, and the competency. Per +:ref:`openedx-learning-adr-0002` and :ref:`openedx-learning-adr-0005`, all three levels are +*materialized* (stored), not recomputed on read, so that dashboards and other read surfaces stay +fast. A single grade change therefore writes the changed leaf's status and then re-evaluates and +re-writes the derived rows from that leaf up to the competency root. The re-evaluation +is needed for multiple reasons, including notifications, and badge and certificate issuing. Per +:ref:`openedx-learning-adr-0005`, each level is stored as an ACTIVE row updated in place, holding +the current status for a learner and node, plus an append-only HISTORY row per genuine status +advance. + +**Monotonicity: competency statuses only ever move forward.** Per +:ref:`openedx-learning-adr-0005`, every node, at every level, advances through a small status +lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``) and is never +lowered later. This holds for leaf nodes, group nodes, and top-level competency masteries. + +Two forces shape how recording should happen: + +- **Same-learner correctness.** A grade change writes the changed leaf and then re-derives the + group and competency rows above it. Leaf rows are always correct, since each leaf is a pure + function of its own grade. The derived rows are the hazard: We want to avoid a case where two evaluations for the same learner + that overlap can each read a stale snapshot of the sibling leaf statuses and each write a derived + roll-up computed from an incomplete picture (a *write-skew*). + +- **Throughput.** Grading is bursty and spans a very large number of learners, so the recording + path must keep up under peak load. + +Decision +-------- + +**1. Every write is a monotone merge, never a blind overwrite.** A node's status is written as +``status := max(stored status, newly computed status)`` (a single ``GREATEST``-style ``UPDATE``, +atomic at the row for the duration of that one statement, with no application-level lock). Because +the merge takes the higher of the two values, it is commutative, idempotent, and insensitive to +order. This is why out-of-order delivery and re-delivery are harmless without sequence tracking. + +**2. When a child advances, its parent is recomputed in the same transaction, under a brief row lock on that parent.** +The merge in mechanism 1 makes a single-row write safe, but a *conjunctive* +parent (for example "demonstrated only when all children are demonstrated") is computed by reading +several child rows first, so two overlapping evaluations for one learner could each read a stale +sibling and compute a parent that is too low. To prevent that, recomputing a parent takes a +row-level lock on the parent row (a ``SELECT ... FOR UPDATE``) before reading its children: two +updates that touch the same parent for the same learner take turns, and the second reads the first's +committed children and computes from the complete picture. This correctness argument assumes +``READ COMMITTED`` isolation (the Open edX platform default on MySQL; higher isolation levels are not +supported on the platform): under it the lock's own read and the sibling reads that follow it always +return the latest committed rows, rather than a snapshot fixed at an earlier read in the same +transaction, which is what a higher level such as ``REPEATABLE READ`` would do. Locks are taken child-before-parent up +the path to the root, a consistent order, so concurrent updates cannot deadlock. This is an ordinary +single-row lock. + +**3. Entry point: edx-platform subsection grade change.** edx-platform +computes subsection grades in an async celery task (`recalculate_subsection_grade_v3`) triggered by a score-change signal, not on the +request thread. After that task writes the subsection grade, it calls a public openedx-core function +within the same transaction; this function does the monotone merge and the upward roll-up. This should be generalized as needed to other places that trigger a competency status update. + +**4. The ACTIVE writes, the HISTORY appends, and the roll-ups all commit atomically with the +subsection grade.** The leaf, group, and competency ACTIVE writes from mechanisms 1 and 2, and the +HISTORY row appended for each genuine advance, run inside the same transaction that mechanism 3 +opened for the subsection-grade write, so they commit as a single unit with it. If any step fails, that transaction rolls back and the task retries, leaving +behind neither a partial roll-up nor an ACTIVE status whose advance went unrecorded. A unique +constraint on the advance (learner, node, and status; :ref:`openedx-learning-adr-0002`) makes the +append idempotent, so a retried task or a redelivered grade event collapses to a no-op rather than +writing a duplicate row. + +**5. Only an advance is appended to HISTORY.** The monotone merge in mechanism 1 often leaves a status +where it was, because the newly computed status equals or is lower than the stored one. Those writes +append nothing: a redelivered grade event, a downward grade correction, and a recompute that confirms +the current status all leave HISTORY untouched. So the recorder writes at most one HISTORY row per +learner, node, and step up the lattice, which is what bounds HISTORY to the same order of magnitude as +ACTIVE rather than to grading volume (:ref:`openedx-learning-adr-0005`). + + +Rejected Alternatives +--------------------- + +1. Prevent concurrent writes with a coarser lock, either deployment-wide or per-learner. + + - Pros: + - Correctness comes from a single lock rather than from the monotone-merge argument, so it is + simpler to reason about. + - A per-learner lock (for example a database advisory lock keyed on a hash of the user id) + still lets different learners record in parallel, and gives the same per-learner + serialization the chosen design relies on. + - Cons: + - A single deployment-wide lock serializes recording across every learner, giving up the + throughput the design needs under bursty grading. + - A per-learner lock still serializes a single learner's independent competencies against each + other even when they never contend. + - Either lock adds lock-lifecycle machinery (acquisition, release, and handling a holder that + dies) across a very large key space. + - The chosen design needs no such lock: the monotone merge (mechanism 1) makes each single-row + write safe, and the brief per-parent row lock (mechanism 2) serializes only writers that + actually contend for the same parent row of the same learner, so different learners, and + different competencies of one learner, still record in parallel. + +2. Recompute derived levels on read instead of materializing them. + + - Pros: + - Eliminates the derived group and competency status rows and the roll-up writes entirely, + leaving nothing to keep consistent on write. + - Cons: + - Moves the full bottom-up tree evaluation onto the hot read path, the opposite of what + dashboards and other read surfaces need (a direct indexed lookup). + - Settled against in :ref:`openedx-learning-adr-0002`. + +3. Send an event to openedx-core and update competency statuses in a separate celery task. + + - Pros: + - Decouples the mastery update from the grade write, so grade recording does not depend on + competency code being installed or fast. + - Cons: + - Without a shared transaction, a failure or a lost event leaves the grade and its mastery rows + permanently out of sync (data drift), with no way to roll them back together. + - Recording the ACTIVE writes in the same transaction as the grade (mechanism 3) instead makes + the grade and its mastery consequences commit or fail as a unit. + +4. Append the leaf HISTORY row outside the grade transaction, as a retrying task dispatched with + ``transaction.on_commit``. + + This would be mandatory if the HISTORY table were ever + routed to a separate database alias, since a write on another connection cannot be atomic + with the primary transaction. Since we decided that every status table lives in the main database + (:ref:`openedx-learning-adr-0005`), this is unnecessary. From 952bae2a6db5d019be89e2bac40bbfa5f9a6a130 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 10 Aug 2026 12:39:20 -0400 Subject: [PATCH 2/7] docs: drop history-table decisions from competency ADRs Whether learner status history is stored as separate append-only tables, and whether those are advance-only, is still under discussion. Remove the decision from ADRs 0002, 0003, and 0004 rather than commit to it. ADR 0004 now states only that the competency mastery status writes and the roll-ups commit in the same transaction as the subsection grade, without specifying what else that transaction may carry. Co-Authored-By: Claude Opus 5 (1M context) --- .../0002-competency-criteria-model.rst | 25 +++--------- .../0003-competency-criteria-versioning.rst | 19 +++------ .../0004-competency-mastery-concurrency.rst | 39 +++++-------------- 3 files changed, 21 insertions(+), 62 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index bea477bb1..68a46ac55 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -241,13 +241,10 @@ Decision 4. ``CompetencyCriteria(oel_tagging_objecttag_id)`` 5. ``CompetencyCriteria(competency_criteria_group_id)`` 6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` (unique) - 7. ``StudentCompetencyCriteriaStatusHistory(user_id, competency_criteria_id, status_id)`` (unique -- at most one HISTORY row per learner, leaf, and status level, which also serves as the idempotency key for the append in :ref:`openedx-learning-adr-0004`) - 8. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` (unique) - 9. ``StudentCompetencyCriteriaGroupStatusHistory(user_id, competency_criteria_group_id)`` - 10. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` (unique) - 11. ``StudentCompetencyStatusHistory(user_id, oel_tagging_tag_id)`` - 12. ``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) - 13. ``CompetencyMasteryStatuses(status)`` (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) 6. Learner progress status concepts (``StudentCompetency*Status`` database tables) @@ -260,12 +257,6 @@ Decision - ``StudentCompetencyStatus`` tracks top-level competency demonstration state. - All learner status rows use a shared lookup table (``CompetencyMasteryStatuses``) so status semantics live in one place and student status tables stay structurally consistent. - Append-only history tables: - - - ``StudentCompetencyCriteriaStatusHistory`` - - ``StudentCompetencyCriteriaGroupStatusHistory`` - - ``StudentCompetencyStatusHistory`` - Intended update flow (bottom-up materialization): - A learner event updates one ``StudentCompetencyCriteriaStatus`` row. @@ -437,9 +428,5 @@ Changelog 2026-07-27: -* Split learner status storage into paired ACTIVE and HISTORY tables: added the append-only - ``StudentCompetencyCriteriaStatusHistory``, ``StudentCompetencyCriteriaGroupStatusHistory``, - and ``StudentCompetencyStatusHistory`` tables and their indexes alongside the in-place ACTIVE - tables, per :ref:`openedx-learning-adr-0005`. -* Made the leaf HISTORY (``StudentCompetencyCriteriaStatusHistory``) index unique on ``(user_id, competency_criteria_id, status_id)``, the - idempotency key for the HISTORY append in :ref:`openedx-learning-adr-0004`. +* 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 6481f72f1..566f4f74e 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -48,17 +48,9 @@ For the initial implementation, versioning and traceability of competency achiev - For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``, each status change updates the responsible row. - - Statuses only increase monotonically as described by :ref:`openedx-learning-adr-0005`; + - Statuses only increase monotonically, as relied on by :ref:`openedx-learning-adr-0004`; downward status adjustments (for example ``Demonstrated`` to ``PartiallyAttempted``) are prohibited. - -6. Learner status models/tables as in 5. above each get a separate append-only history table not using ``django-simple-history``: - - - For ``StudentCompetencyCriteriaStatusHistory``, ``StudentCompetencyCriteriaGroupStatusHistory``, and ``StudentCompetencyStatusHistory``, - each status advance is stored as a new row with ``created`` as the write timestamp. - - Existing learner status rows are not updated in place in the history tables. - - Statuses only increase monotonically as described by :ref:`openedx-learning-adr-0005`; - if a change would mean a downward adjustment (for example ``Demonstrated`` to ``PartiallyAttempted``) - or no adjustment, this does not get stored in the history tables. + - How learner status history is retained is not decided here. Rejected Alternatives @@ -100,7 +92,6 @@ Changelog 2026-07-27: -* Reworked learner status handling to match :ref:`openedx-learning-adr-0005` and - :ref:`openedx-learning-adr-0004`: Decision 5 now updates learner status rows in place and - monotonically (downward adjustments prohibited), and a new Decision 6 adds separate append-only - HISTORY tables. Previously a single append-only model with no in-place ACTIVE row. +* Reworked Decision 5 for :ref:`openedx-learning-adr-0004`: learner status rows are now updated in + place and monotonically (downward adjustments prohibited). 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 index aab27e122..5a3663549 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -13,17 +13,16 @@ When a learner is graded on a subsection (or any other learning instrument assoc with a competency criteria, like a course or rubric criterion), the platform must evaluate whether that grade demonstrates any attached competencies and record the learner's mastery. Mastery is recorded at three levels: the criterion (leaf), the criteria group, and the competency. Per -:ref:`openedx-learning-adr-0002` and :ref:`openedx-learning-adr-0005`, all three levels are +:ref:`openedx-learning-adr-0002`, all three levels are *materialized* (stored), not recomputed on read, so that dashboards and other read surfaces stay fast. A single grade change therefore writes the changed leaf's status and then re-evaluates and re-writes the derived rows from that leaf up to the competency root. The re-evaluation is needed for multiple reasons, including notifications, and badge and certificate issuing. Per -:ref:`openedx-learning-adr-0005`, each level is stored as an ACTIVE row updated in place, holding -the current status for a learner and node, plus an append-only HISTORY row per genuine status -advance. +:ref:`openedx-learning-adr-0003`, each level is stored as one row per learner and node, updated in +place, holding that learner's current status for that node. **Monotonicity: competency statuses only ever move forward.** Per -:ref:`openedx-learning-adr-0005`, every node, at every level, advances through a small status +:ref:`openedx-learning-adr-0003`, every node, at every level, advances through a small status lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``) and is never lowered later. This holds for leaf nodes, group nodes, and top-level competency masteries. @@ -67,21 +66,11 @@ computes subsection grades in an async celery task (`recalculate_subsection_grad request thread. After that task writes the subsection grade, it calls a public openedx-core function within the same transaction; this function does the monotone merge and the upward roll-up. This should be generalized as needed to other places that trigger a competency status update. -**4. The ACTIVE writes, the HISTORY appends, and the roll-ups all commit atomically with the -subsection grade.** The leaf, group, and competency ACTIVE writes from mechanisms 1 and 2, and the -HISTORY row appended for each genuine advance, run inside the same transaction that mechanism 3 -opened for the subsection-grade write, so they commit as a single unit with it. If any step fails, that transaction rolls back and the task retries, leaving -behind neither a partial roll-up nor an ACTIVE status whose advance went unrecorded. A unique -constraint on the advance (learner, node, and status; :ref:`openedx-learning-adr-0002`) makes the -append idempotent, so a retried task or a redelivered grade event collapses to a no-op rather than -writing a duplicate row. - -**5. Only an advance is appended to HISTORY.** The monotone merge in mechanism 1 often leaves a status -where it was, because the newly computed status equals or is lower than the stored one. Those writes -append nothing: a redelivered grade event, a downward grade correction, and a recompute that confirms -the current status all leave HISTORY untouched. So the recorder writes at most one HISTORY row per -learner, node, and step up the lattice, which is what bounds HISTORY to the same order of magnitude as -ACTIVE rather than to grading volume (:ref:`openedx-learning-adr-0005`). +**4. The status writes and the roll-ups all commit atomically with the subsection grade.** The +leaf, group, and competency status writes from mechanisms 1 and 2 run inside the same transaction +that mechanism 3 opened for the subsection-grade write, so they commit as a single unit with it. If +any step fails, that transaction rolls back and the task retries, leaving behind no partial +roll-up. Rejected Alternatives @@ -125,13 +114,5 @@ Rejected Alternatives - Cons: - Without a shared transaction, a failure or a lost event leaves the grade and its mastery rows permanently out of sync (data drift), with no way to roll them back together. - - Recording the ACTIVE writes in the same transaction as the grade (mechanism 3) instead makes + - Recording the status writes in the same transaction as the grade (mechanism 3) instead makes the grade and its mastery consequences commit or fail as a unit. - -4. Append the leaf HISTORY row outside the grade transaction, as a retrying task dispatched with - ``transaction.on_commit``. - - This would be mandatory if the HISTORY table were ever - routed to a separate database alias, since a write on another connection cannot be atomic - with the primary transaction. Since we decided that every status table lives in the main database - (:ref:`openedx-learning-adr-0005`), this is unnecessary. From 33327f466cf5fec0189c6b2a51b324cfba2b9bc2 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 10 Aug 2026 12:40:27 -0400 Subject: [PATCH 3/7] docs: scope competency monotonicity to the automatic recorder Monotonicity is a property of the automatic path, not an invariant of the stored data. Grade changes and competency criteria rule changes never lower a status, but staff can, through Django admin or as a deliberate instructor correction, and a direct edit cascades to the ancestors above the edited node. Add an Open Questions section recording that the cascade decision needs confirmation, and that whether a cascade may overwrite a hand-set ancestor status is still undecided. Co-Authored-By: Claude Opus 5 (1M context) --- .../0003-competency-criteria-versioning.rst | 14 +++++-- .../0004-competency-mastery-concurrency.rst | 38 +++++++++++++++++-- 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 566f4f74e..ff4c3b4f7 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -48,8 +48,13 @@ For the initial implementation, versioning and traceability of competency achiev - For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``, each status change updates the responsible row. - - Statuses only increase monotonically, as relied on by :ref:`openedx-learning-adr-0004`; - downward status adjustments (for example ``Demonstrated`` to ``PartiallyAttempted``) are prohibited. + - 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. @@ -93,5 +98,6 @@ Changelog 2026-07-27: * Reworked Decision 5 for :ref:`openedx-learning-adr-0004`: learner status rows are now updated in - place and monotonically (downward adjustments prohibited). Previously append-only, with current - status resolved as the most recent row. How status history is retained is left undecided. + 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 index 5a3663549..b3909d098 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -21,10 +21,25 @@ is needed for multiple reasons, including notifications, and badge and certifica :ref:`openedx-learning-adr-0003`, each level is stored as one row per learner and node, updated in place, holding that learner's current status for that node. -**Monotonicity: competency statuses only ever move forward.** Per +**Monotonicity: the recorder only ever moves a status forward.** Per :ref:`openedx-learning-adr-0003`, every node, at every level, advances through a small status lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``) and is never -lowered later. This holds for leaf nodes, group nodes, and top-level competency masteries. +lowered by the recorder. This holds for leaf nodes, group nodes, and top-level competency masteries, +and against both of the recorder's triggers: neither a downward grade correction nor a change to the +competency criteria rules lowers a recorded status. + +Monotonicity is a property of that automatic path, not an invariant of the stored data. Staff can +set a learner's status directly, through Django admin or as a deliberate instructor correction, and +such an edit may move a status backward. A direct edit does cascade: the ancestors above the edited +node are recomputed up to the competency root, so an instructor correcting a leaf from +``Demonstrated`` to ``AttemptedNotDemonstrated`` lowers the group and competency rows above it too. +That cascade cannot use the monotone merge of mechanism 1, which by construction never lowers +anything, so it overwrites each ancestor with the freshly computed value instead. It does take the +same parent locks as mechanism 2, so it orders correctly against concurrent recorder writes. + +Because the recorder never lowers a status, a direct edit is the only thing that can. A later grade +change re-merges against whatever the edit left behind: it can raise a status an edit lowered, but +it can never re-lower one an edit raised. Two forces shape how recording should happen: @@ -40,7 +55,7 @@ Two forces shape how recording should happen: Decision -------- -**1. Every write is a monotone merge, never a blind overwrite.** A node's status is written as +**1. Every recorder write is a monotone merge, never a blind overwrite.** A node's status is written as ``status := max(stored status, newly computed status)`` (a single ``GREATEST``-style ``UPDATE``, atomic at the row for the duration of that one statement, with no application-level lock). Because the merge takes the higher of the two values, it is commutative, idempotent, and insensitive to @@ -73,6 +88,23 @@ any step fails, that transaction rolls back and the task retries, leaving behind roll-up. +Open Questions +-------------- + +1. **Confirm that a direct staff edit cascades to ancestors.** The Context above decides that it + does: correcting a learner's leaf status by hand recomputes the group and competency rows above + it, downward if that is what the rules produce. The alternative is to leave the ancestors + untouched and require a separate reconciliation step. Cascading is what an instructor issuing a + correction would expect, but it costs something: the recorder is no longer the only writer that + can lower a status, and a stored ancestor status is no longer a pure function of the recorder's + own history. This decision should be confirmed before implementation. + +2. **Decide whether a cascade may overwrite a hand-set ancestor.** If a staff user has set a group + or competency status directly, and a later direct edit below it cascades upward, the recomputed + value overwrites the hand-set one. Whether a hand-set ancestor should instead survive the + cascade is undecided. + + Rejected Alternatives --------------------- From 3fc91464c7bf4ee0f7db60d14da4eb2b64abda03 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 10 Aug 2026 12:41:25 -0400 Subject: [PATCH 4/7] docs: correct lock lifetime and lock ordering in ADR 0004 The row lock taken by the merge UPDATE is held until the transaction commits, not for the duration of the statement. Mechanism 2's lock ordering argument depends on that, so state it correctly. Give the deadlock-freedom argument its missing premise, that the criteria tree gives every node exactly one parent, and close the gap where one grade change advances several leaves at once by fixing their lock order. Attribute the READ COMMITTED default to Django's MySQL backend, which is what actually sets it, and drop the unenforced claim that higher isolation levels are unsupported. Co-Authored-By: Claude Opus 5 (1M context) --- .../0004-competency-mastery-concurrency.rst | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index b3909d098..032c18c28 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -57,7 +57,8 @@ Decision **1. Every recorder write is a monotone merge, never a blind overwrite.** A node's status is written as ``status := max(stored status, newly computed status)`` (a single ``GREATEST``-style ``UPDATE``, -atomic at the row for the duration of that one statement, with no application-level lock). Because +with no application-level lock; the database holds that row's exclusive lock until the transaction +commits, which is what mechanism 2's lock ordering relies on). Because the merge takes the higher of the two values, it is commutative, idempotent, and insensitive to order. This is why out-of-order delivery and re-delivery are harmless without sequence tracking. @@ -69,15 +70,18 @@ sibling and compute a parent that is too low. To prevent that, recomputing a par row-level lock on the parent row (a ``SELECT ... FOR UPDATE``) before reading its children: two updates that touch the same parent for the same learner take turns, and the second reads the first's committed children and computes from the complete picture. This correctness argument assumes -``READ COMMITTED`` isolation (the Open edX platform default on MySQL; higher isolation levels are not -supported on the platform): under it the lock's own read and the sibling reads that follow it always +``READ COMMITTED`` isolation (Django's default for MySQL, which the platform does not override): +under it the lock's own read and the sibling reads that follow it always return the latest committed rows, rather than a snapshot fixed at an earlier read in the same -transaction, which is what a higher level such as ``REPEATABLE READ`` would do. Locks are taken child-before-parent up -the path to the root, a consistent order, so concurrent updates cannot deadlock. This is an ordinary -single-row lock. +transaction, which is what a higher level such as ``REPEATABLE READ`` would do. Locks are taken +child-before-parent up the path to the root. Because the criteria tree gives every node exactly one +parent (:ref:`openedx-learning-adr-0002`), that path is unique, so two updates that touch the same +ancestor always reach it in the same order and cannot deadlock. Where one grade change advances +several leaves at once, because a subsection carries several criteria, those leaves are locked in +primary-key order for the same reason. This is an ordinary single-row lock. **3. Entry point: edx-platform subsection grade change.** edx-platform -computes subsection grades in an async celery task (`recalculate_subsection_grade_v3`) triggered by a score-change signal, not on the +computes subsection grades in an async celery task (``recalculate_subsection_grade_v3``) triggered by a score-change signal, not on the request thread. After that task writes the subsection grade, it calls a public openedx-core function within the same transaction; this function does the monotone merge and the upward roll-up. This should be generalized as needed to other places that trigger a competency status update. From da8117dfbd9314ed578106fc46ccc3d3cf3ff65d Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Mon, 10 Aug 2026 14:42:47 -0400 Subject: [PATCH 5/7] docs: rewrite ADR 0004 in plainer English Break the dense Context and Decision paragraphs into one idea each, cut the nested parentheticals, and drop the write-skew jargon in favor of the plain description already alongside it. Replace mechanism 3's vague "generalized as needed to other places" with what it means: further entry points will call the same function. Content is unchanged. Every mechanism, the monotonicity carve-out for direct staff edits, and the isolation-level argument all say what they said before. Co-Authored-By: Claude Opus 5 (1M context) --- .../0004-competency-mastery-concurrency.rst | 146 +++++++++--------- 1 file changed, 74 insertions(+), 72 deletions(-) diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index 032c18c28..365bdf8d5 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -9,45 +9,44 @@ Proposed. Context ------- -When a learner is graded on a subsection (or any other learning instrument associated to a competency -with a competency criteria, like a course or rubric criterion), the platform must evaluate whether that grade -demonstrates any attached competencies and record the learner's mastery. Mastery is recorded at -three levels: the criterion (leaf), the criteria group, and the competency. Per -:ref:`openedx-learning-adr-0002`, all three levels are -*materialized* (stored), not recomputed on read, so that dashboards and other read surfaces stay -fast. A single grade change therefore writes the changed leaf's status and then re-evaluates and -re-writes the derived rows from that leaf up to the competency root. The re-evaluation -is needed for multiple reasons, including notifications, and badge and certificate issuing. Per -:ref:`openedx-learning-adr-0003`, each level is stored as one row per learner and node, updated in -place, holding that learner's current status for that node. - -**Monotonicity: the recorder only ever moves a status forward.** Per -:ref:`openedx-learning-adr-0003`, every node, at every level, advances through a small status -lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``) and is never -lowered by the recorder. This holds for leaf nodes, group nodes, and top-level competency masteries, -and against both of the recorder's triggers: neither a downward grade correction nor a change to the -competency criteria rules lowers a recorded status. - -Monotonicity is a property of that automatic path, not an invariant of the stored data. Staff can -set a learner's status directly, through Django admin or as a deliberate instructor correction, and -such an edit may move a status backward. A direct edit does cascade: the ancestors above the edited -node are recomputed up to the competency root, so an instructor correcting a leaf from -``Demonstrated`` to ``AttemptedNotDemonstrated`` lowers the group and competency rows above it too. -That cascade cannot use the monotone merge of mechanism 1, which by construction never lowers -anything, so it overwrites each ancestor with the freshly computed value instead. It does take the -same parent locks as mechanism 2, so it orders correctly against concurrent recorder writes. - -Because the recorder never lowers a status, a direct edit is the only thing that can. A later grade -change re-merges against whatever the edit left behind: it can raise a status an edit lowered, but -it can never re-lower one an edit raised. +When a learner is graded on a subsection, the platform must work out whether that grade demonstrates +any attached competencies, and record it. The same goes for any other learning instrument tied to a +competency by a competency criterion, such as a course or a rubric criterion. + +Mastery is recorded at three levels: the criterion (leaf), the criteria group, and the competency. +Per :ref:`openedx-learning-adr-0002`, all three are stored rather than recomputed on read, so that +dashboards and other read surfaces stay fast. Per :ref:`openedx-learning-adr-0003`, each level keeps +one row per learner and node, updated in place. + +So a single grade change writes the changed leaf's status, then re-evaluates and re-writes every +derived row from that leaf up to the competency root. The roll-up is not only for reads: it also +drives notifications, badges, and certificate issuing. + +**Monotonicity: the recorder only ever moves a status forward.** Every node advances through a small +status lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``), and the +recorder never lowers it (:ref:`openedx-learning-adr-0003`). That holds at all three levels, and for +both of the recorder's triggers: neither a downward grade correction nor a change to the competency +criteria rules lowers a recorded status. + +Monotonicity is a property of that automatic path, not of the stored data. Staff can set a learner's +status directly, through Django admin or as a deliberate instructor correction, and a direct edit +may lower it. A direct edit also cascades: the ancestors above the edited node are recomputed up to +the root, so an instructor correcting a leaf from ``Demonstrated`` to ``AttemptedNotDemonstrated`` +lowers the group and competency rows above it too. That cascade cannot use the monotone merge of +mechanism 1, which never lowers anything, so it overwrites each ancestor with the freshly computed +value. It does take the same parent locks as mechanism 2, so it stays ordered against concurrent +recorder writes. + +One consequence: a direct edit is the only thing that can lower a status. A later grade change +merges against whatever the edit left behind, so it can raise a status an edit lowered, but it can +never re-lower one an edit raised. Two forces shape how recording should happen: -- **Same-learner correctness.** A grade change writes the changed leaf and then re-derives the - group and competency rows above it. Leaf rows are always correct, since each leaf is a pure - function of its own grade. The derived rows are the hazard: We want to avoid a case where two evaluations for the same learner - that overlap can each read a stale snapshot of the sibling leaf statuses and each write a derived - roll-up computed from an incomplete picture (a *write-skew*). +- **Same-learner correctness.** Leaf rows are never in doubt, since each leaf is a pure function of + its own grade. The derived rows are the hazard. Two evaluations for the same learner that overlap + can each read a stale snapshot of the sibling leaf statuses, and each write a roll-up computed + from an incomplete picture. - **Throughput.** Grading is bursty and spans a very large number of learners, so the recording path must keep up under peak load. @@ -55,41 +54,44 @@ Two forces shape how recording should happen: Decision -------- -**1. Every recorder write is a monotone merge, never a blind overwrite.** A node's status is written as -``status := max(stored status, newly computed status)`` (a single ``GREATEST``-style ``UPDATE``, -with no application-level lock; the database holds that row's exclusive lock until the transaction -commits, which is what mechanism 2's lock ordering relies on). Because -the merge takes the higher of the two values, it is commutative, idempotent, and insensitive to -order. This is why out-of-order delivery and re-delivery are harmless without sequence tracking. - -**2. When a child advances, its parent is recomputed in the same transaction, under a brief row lock on that parent.** -The merge in mechanism 1 makes a single-row write safe, but a *conjunctive* -parent (for example "demonstrated only when all children are demonstrated") is computed by reading -several child rows first, so two overlapping evaluations for one learner could each read a stale -sibling and compute a parent that is too low. To prevent that, recomputing a parent takes a -row-level lock on the parent row (a ``SELECT ... FOR UPDATE``) before reading its children: two -updates that touch the same parent for the same learner take turns, and the second reads the first's -committed children and computes from the complete picture. This correctness argument assumes -``READ COMMITTED`` isolation (Django's default for MySQL, which the platform does not override): -under it the lock's own read and the sibling reads that follow it always -return the latest committed rows, rather than a snapshot fixed at an earlier read in the same -transaction, which is what a higher level such as ``REPEATABLE READ`` would do. Locks are taken -child-before-parent up the path to the root. Because the criteria tree gives every node exactly one -parent (:ref:`openedx-learning-adr-0002`), that path is unique, so two updates that touch the same -ancestor always reach it in the same order and cannot deadlock. Where one grade change advances -several leaves at once, because a subsection carries several criteria, those leaves are locked in -primary-key order for the same reason. This is an ordinary single-row lock. - -**3. Entry point: edx-platform subsection grade change.** edx-platform -computes subsection grades in an async celery task (``recalculate_subsection_grade_v3``) triggered by a score-change signal, not on the -request thread. After that task writes the subsection grade, it calls a public openedx-core function -within the same transaction; this function does the monotone merge and the upward roll-up. This should be generalized as needed to other places that trigger a competency status update. - -**4. The status writes and the roll-ups all commit atomically with the subsection grade.** The -leaf, group, and competency status writes from mechanisms 1 and 2 run inside the same transaction -that mechanism 3 opened for the subsection-grade write, so they commit as a single unit with it. If -any step fails, that transaction rolls back and the task retries, leaving behind no partial -roll-up. +**1. A recorder write only ever advances the stored value.** Every write is +``status := max(stored status, newly computed status)``, a single ``GREATEST``-style ``UPDATE`` with +no application-level lock. Taking the higher of the two values makes the write idempotent and +insensitive to order, so out-of-order delivery and re-delivery are harmless without sequence +tracking. The database holds that row's exclusive lock until the transaction commits, which is what +mechanism 2's lock ordering relies on. + +**2. When a child advances, its parent is recomputed in the same transaction, under a row lock on +the parent.** A parent's rule can be conjunctive, for example "demonstrated only when all children +are demonstrated", so recomputing it means reading all of its children first. If two children of one +parent advance at the same time, each recomputation could read the other child's old value and write +a parent status that is too low. + +To prevent that, a recomputation locks the parent row (``SELECT ... FOR UPDATE``) before reading the +children. The two writers take turns, and the second reads the first's committed children. + +This relies on ``READ COMMITTED`` isolation, Django's default for MySQL, which the platform does not +override. Under it, the child reads that follow the lock return the latest committed rows. Under +``REPEATABLE READ`` they would instead return a snapshot fixed at an earlier read in the same +transaction, and the second writer would still see the stale child. + +Locks are taken child-before-parent, up the path to the root. The criteria tree gives every node +exactly one parent (:ref:`openedx-learning-adr-0002`), so that path is unique, and two transactions +touching the same ancestor always reach it in the same order. They cannot deadlock. Where one grade +change advances several leaves at once, because a subsection carries several criteria, those leaves +are locked in primary-key order for the same reason. + +**3. Entry point: a subsection grade change in edx-platform.** edx-platform computes subsection +grades in an async celery task (``recalculate_subsection_grade_v3``) triggered by a score-change +signal, not on the request thread. After that task writes the subsection grade, it calls a public +openedx-core function in the same transaction, which does the merge and the roll-up. As other kinds +of competency criteria are defined, completion for example, further entry points will call that same +function. + +**4. The status writes and the roll-ups commit atomically with the subsection grade.** The leaf, +group, and competency writes from mechanisms 1 and 2 run inside the transaction that mechanism 3 +opened for the subsection-grade write, so they commit as one unit with it. If any step fails, the +transaction rolls back and the task retries, leaving no partial roll-up behind. Open Questions @@ -128,7 +130,7 @@ Rejected Alternatives - Either lock adds lock-lifecycle machinery (acquisition, release, and handling a holder that dies) across a very large key space. - The chosen design needs no such lock: the monotone merge (mechanism 1) makes each single-row - write safe, and the brief per-parent row lock (mechanism 2) serializes only writers that + write safe, and the per-parent row lock (mechanism 2) serializes only writers that actually contend for the same parent row of the same learner, so different learners, and different competencies of one learner, still record in parallel. From ef67a55bbb3e48825599b7dba7183ff71de8bdcb Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Tue, 11 Aug 2026 11:27:55 -0400 Subject: [PATCH 6/7] docs: rewrite ADR; surface deadlock question --- .../0004-competency-mastery-concurrency.rst | 196 ++++++++---------- 1 file changed, 85 insertions(+), 111 deletions(-) diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index 365bdf8d5..043fc05d2 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -9,107 +9,61 @@ Proposed. Context ------- -When a learner is graded on a subsection, the platform must work out whether that grade demonstrates -any attached competencies, and record it. The same goes for any other learning instrument tied to a -competency by a competency criterion, such as a course or a rubric criterion. - -Mastery is recorded at three levels: the criterion (leaf), the criteria group, and the competency. -Per :ref:`openedx-learning-adr-0002`, all three are stored rather than recomputed on read, so that -dashboards and other read surfaces stay fast. Per :ref:`openedx-learning-adr-0003`, each level keeps -one row per learner and node, updated in place. - -So a single grade change writes the changed leaf's status, then re-evaluates and re-writes every -derived row from that leaf up to the competency root. The roll-up is not only for reads: it also -drives notifications, badges, and certificate issuing. - -**Monotonicity: the recorder only ever moves a status forward.** Every node advances through a small -status lattice (``AttemptedNotDemonstrated`` to ``PartiallyAttempted`` to ``Demonstrated``), and the -recorder never lowers it (:ref:`openedx-learning-adr-0003`). That holds at all three levels, and for -both of the recorder's triggers: neither a downward grade correction nor a change to the competency -criteria rules lowers a recorded status. - -Monotonicity is a property of that automatic path, not of the stored data. Staff can set a learner's -status directly, through Django admin or as a deliberate instructor correction, and a direct edit -may lower it. A direct edit also cascades: the ancestors above the edited node are recomputed up to -the root, so an instructor correcting a leaf from ``Demonstrated`` to ``AttemptedNotDemonstrated`` -lowers the group and competency rows above it too. That cascade cannot use the monotone merge of -mechanism 1, which never lowers anything, so it overwrites each ancestor with the freshly computed -value. It does take the same parent locks as mechanism 2, so it stays ordered against concurrent -recorder writes. - -One consequence: a direct edit is the only thing that can lower a status. A later grade change -merges against whatever the edit left behind, so it can raise a status an edit lowered, but it can -never re-lower one an edit raised. - -Two forces shape how recording should happen: - -- **Same-learner correctness.** Leaf rows are never in doubt, since each leaf is a pure function of - its own grade. The derived rows are the hazard. Two evaluations for the same learner that overlap - can each read a stale snapshot of the sibling leaf statuses, and each write a roll-up computed - from an incomplete picture. - -- **Throughput.** Grading is bursty and spans a very large number of learners, so the recording - path must keep up under peak load. +A learner's mastery of one competency is stored at three levels of the competency criteria tree: +the leaf criterion that was graded, 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``. +So one grade change updates the leaf and then every row above +it, for a very large number of learners, in bursts. This ADR decides how those writes stay correct +when two of them for the same learner overlap. + +What triggers a change is a subsection grade, or any other learning instrument tied to a +competency by a competency criterion, such as a course grade or a rubric criterion. The rows above +the leaf are stored rather than recomputed on read because they also drive notifications, badges, +and certificate issuing, so the roll-up has to happen when the grade does either way. Decision -------- -**1. A recorder write only ever advances the stored value.** Every write is -``status := max(stored status, newly computed status)``, a single ``GREATEST``-style ``UPDATE`` with -no application-level lock. Taking the higher of the two values makes the write idempotent and -insensitive to order, so out-of-order delivery and re-delivery are harmless without sequence -tracking. The database holds that row's exclusive lock until the transaction commits, which is what -mechanism 2's lock ordering relies on. - -**2. When a child advances, its parent is recomputed in the same transaction, under a row lock on -the parent.** A parent's rule can be conjunctive, for example "demonstrated only when all children -are demonstrated", so recomputing it means reading all of its children first. If two children of one -parent advance at the same time, each recomputation could read the other child's old value and write -a parent status that is too low. - -To prevent that, a recomputation locks the parent row (``SELECT ... FOR UPDATE``) before reading the -children. The two writers take turns, and the second reads the first's committed children. - -This relies on ``READ COMMITTED`` isolation, Django's default for MySQL, which the platform does not -override. Under it, the child reads that follow the lock return the latest committed rows. Under -``REPEATABLE READ`` they would instead return a snapshot fixed at an earlier read in the same -transaction, and the second writer would still see the stale child. - -Locks are taken child-before-parent, up the path to the root. The criteria tree gives every node -exactly one parent (:ref:`openedx-learning-adr-0002`), so that path is unique, and two transactions -touching the same ancestor always reach it in the same order. They cannot deadlock. Where one grade -change advances several leaves at once, because a subsection carries several criteria, those leaves -are locked in primary-key order for the same reason. - -**3. Entry point: a subsection grade change in edx-platform.** edx-platform computes subsection -grades in an async celery task (``recalculate_subsection_grade_v3``) triggered by a score-change -signal, not on the request thread. After that task writes the subsection grade, it calls a public -openedx-core function in the same transaction, which does the merge and the roll-up. As other kinds -of competency criteria are defined, completion for example, further entry points will call that same -function. - -**4. The status writes and the roll-ups commit atomically with the subsection grade.** The leaf, -group, and competency writes from mechanisms 1 and 2 run inside the transaction that mechanism 3 -opened for the subsection-grade write, so they commit as one unit with it. If any step fails, the -transaction rolls back and the task retries, leaving no partial roll-up behind. - - -Open Questions --------------- - -1. **Confirm that a direct staff edit cascades to ancestors.** The Context above decides that it - does: correcting a learner's leaf status by hand recomputes the group and competency rows above - it, downward if that is what the rules produce. The alternative is to leave the ancestors - untouched and require a separate reconciliation step. Cascading is what an instructor issuing a - correction would expect, but it costs something: the recorder is no longer the only writer that - can lower a status, and a stored ancestor status is no longer a pure function of the recorder's - own history. This decision should be confirmed before implementation. - -2. **Decide whether a cascade may overwrite a hand-set ancestor.** If a staff user has set a group - or competency status directly, and a later direct edit below it cascades upward, the recomputed - value overwrites the hand-set one. Whether a hand-set ancestor should instead survive the - cascade is undecided. - +1. **The platform's grading task calls one openedx-core function, in the same atomic transaction as the + grade write.** Subsection grading already happens in a celery task; that task writes the grade + and then calls this function, which updates the leaf and walks up. Writing up the tree stops + where :ref:`openedx-learning-adr-0002`, Decision 6 says it stops. + +2. **Automatic updates only move a status up: each write stores the higher of the stored value and + the newly computed one.** This makes the recorder safe + against celery delivering the same work twice or out of order. Applying one grade event twice + lands on the same value as applying it once. + +3. **Before recomputing a group, lock that group's row.** + If two children advance at the same moment, each recomputation could read the other child as not yet + advanced. Both would then compute the same too-low value. Locking the group first makes the two writers take turns, so the + second one reads the first's committed children. Handling deadlocks is needed: see Unresolved 1. + +4. **A direct staff edit is the exception to all of the above.** An instructor or admin correcting + a status by hand may set any value, including a lower one, and the rows above the edited one are + recomputed and overwritten rather than merged, including any an earlier staff edit set by hand. + So a staff edit or a Django admin change is the only thing that can lower + a status, and a later grade change can raise what an edit lowered but can never re-lower what an + edit raised. + +Unresolved +---------- + +1. How to avoid deadlocks on competency group node locks that a) involve a grade change locking one group and + b) involve a grade change locking multiple groups, which happens when one subsection manifests as multiple leafs + in the same tree. +2. How notifications, badges, and certificates learn that a row moved. + +Assumptions +----------- + +1. Connections run at ``READ COMMITTED`` isolation. Decision 3 depends on it: the read taken after + the lock has to see current data rather than a snapshot from earlier in the transaction. MySQL's + own default is ``REPEATABLE READ``, but Django overrides it to ``READ COMMITTED`` on every + connection and edx-platform leaves that alone. A deployment that changes the setting breaks + decision 3 with no error and no failing test. Rejected Alternatives --------------------- @@ -117,22 +71,23 @@ Rejected Alternatives 1. Prevent concurrent writes with a coarser lock, either deployment-wide or per-learner. - Pros: - - Correctness comes from a single lock rather than from the monotone-merge argument, so it is - simpler to reason about. + - Correctness comes from a single lock rather than from the merge argument in Decision 2, + so it is simpler to reason about. - A per-learner lock (for example a database advisory lock keyed on a hash of the user id) still lets different learners record in parallel, and gives the same per-learner serialization the chosen design relies on. - Cons: - A single deployment-wide lock serializes recording across every learner, giving up the throughput the design needs under bursty grading. - - A per-learner lock still serializes a single learner's independent competencies against each - other even when they never contend. - - Either lock adds lock-lifecycle machinery (acquisition, release, and handling a holder that - dies) across a very large key space. - - The chosen design needs no such lock: the monotone merge (mechanism 1) makes each single-row - write safe, and the per-parent row lock (mechanism 2) serializes only writers that - actually contend for the same parent row of the same learner, so different learners, and - different competencies of one learner, still record in parallel. + - A per-learner lock still serializes a single learner's independent competencies against + each other even when they never contend. + - Either lock adds lock-lifecycle machinery (acquisition, release, and handling a holder + that dies) across a very large key space. + - The chosen design needs no lock beyond the row locks the database already takes for the + statements it issues: the merge (Decision 2) makes each single-row write safe, and the + parent row lock (Decision 3) serializes only writers that actually contend for the same + parent row of the same learner, so different learners, and different competencies of one + learner, still record in parallel. 2. Recompute derived levels on read instead of materializing them. @@ -150,7 +105,26 @@ Rejected Alternatives - Decouples the mastery update from the grade write, so grade recording does not depend on competency code being installed or fast. - Cons: - - Without a shared transaction, a failure or a lost event leaves the grade and its mastery rows - permanently out of sync (data drift), with no way to roll them back together. - - Recording the status writes in the same transaction as the grade (mechanism 3) instead makes - the grade and its mastery consequences commit or fail as a unit. + - Without a shared transaction, a failure or a lost event leaves the grade and its mastery + rows permanently out of sync (data drift), with no way to roll them back together. + - Writing the statuses in the same transaction as the grade (Decision 1) instead makes the + grade and its mastery consequences commit or fail as a unit. + +4. Commit the leaf, then re-read the leaves before rolling up, with no lock. + + - Pros: + - Correct, and lock-free. Every writer commits its leaf before reading, so whichever writer + reads last sees every leaf already committed and computes the true value; the merge in + Decision 2 keeps it. + - Cons: + - The leaf has to commit before the roll-up reads, so the roll-up cannot share the grade's + transaction, which reopens the partial-failure window Decision 1 exists to close. + +5. Detect conflicts optimistically: a version column plus a unique constraint, and the losing write + retries. + + - Pros: + - Contention costs a retry rather than a wait, so no writer ever blocks. + - Cons: + - Every writer needs conflict handling and a retry loop, and repeated contention on one + parent multiplies retries. The row lock in Decision 3 reaches the same result by waiting. From 80cd0ba98b00c714ae8994d98ae01ef3c6d0edb5 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Fri, 14 Aug 2026 10:40:38 -0400 Subject: [PATCH 7/7] docs: rewrite for reconciliation job + retries --- .../0004-competency-mastery-concurrency.rst | 198 ++++++++++-------- 1 file changed, 108 insertions(+), 90 deletions(-) diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index 043fc05d2..9d50d8710 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -9,122 +9,140 @@ Proposed. Context ------- -A learner's mastery of one competency is stored at three levels of the competency criteria tree: -the leaf criterion that was graded, 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`, +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``. -So one grade change updates the leaf and then every row above -it, for a very large number of learners, in bursts. This ADR decides how those writes stay correct -when two of them for the same learner overlap. -What triggers a change is a subsection grade, or any other learning instrument tied to a -competency by a competency criterion, such as a course grade or a rubric criterion. The rows above -the leaf are stored rather than recomputed on read because they also drive notifications, badges, -and certificate issuing, so the roll-up has to happen when the grade does either way. +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. **The platform's grading task calls one openedx-core function, in the same atomic transaction as the - grade write.** Subsection grading already happens in a celery task; that task writes the grade - and then calls this function, which updates the leaf and walks up. Writing up the tree stops - where :ref:`openedx-learning-adr-0002`, Decision 6 says it stops. - -2. **Automatic updates only move a status up: each write stores the higher of the stored value and - the newly computed one.** This makes the recorder safe - against celery delivering the same work twice or out of order. Applying one grade event twice - lands on the same value as applying it once. - -3. **Before recomputing a group, lock that group's row.** - If two children advance at the same moment, each recomputation could read the other child as not yet - advanced. Both would then compute the same too-low value. Locking the group first makes the two writers take turns, so the - second one reads the first's committed children. Handling deadlocks is needed: see Unresolved 1. - -4. **A direct staff edit is the exception to all of the above.** An instructor or admin correcting - a status by hand may set any value, including a lower one, and the rows above the edited one are - recomputed and overwritten rather than merged, including any an earlier staff edit set by hand. - So a staff edit or a Django admin change is the only thing that can lower - a status, and a later grade change can raise what an edit lowered but can never re-lower what an - edit raised. - -Unresolved ----------- - -1. How to avoid deadlocks on competency group node locks that a) involve a grade change locking one group and - b) involve a grade change locking multiple groups, which happens when one subsection manifests as multiple leafs - in the same tree. -2. How notifications, badges, and certificates learn that a row moved. - -Assumptions ------------ - -1. Connections run at ``READ COMMITTED`` isolation. Decision 3 depends on it: the read taken after - the lock has to see current data rather than a snapshot from earlier in the transaction. MySQL's - own default is ``REPEATABLE READ``, but Django overrides it to ``READ COMMITTED`` on every - connection and edx-platform leaves that alone. A deployment that changes the setting breaks - decision 3 with no error and no failing test. +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. Prevent concurrent writes with a coarser lock, either deployment-wide or per-learner. +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: - - Correctness comes from a single lock rather than from the merge argument in Decision 2, - so it is simpler to reason about. - - A per-learner lock (for example a database advisory lock keyed on a hash of the user id) - still lets different learners record in parallel, and gives the same per-learner - serialization the chosen design relies on. + - 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: - - A single deployment-wide lock serializes recording across every learner, giving up the - throughput the design needs under bursty grading. - - A per-learner lock still serializes a single learner's independent competencies against - each other even when they never contend. - - Either lock adds lock-lifecycle machinery (acquisition, release, and handling a holder - that dies) across a very large key space. - - The chosen design needs no lock beyond the row locks the database already takes for the - statements it issues: the merge (Decision 2) makes each single-row write safe, and the - parent row lock (Decision 3) serializes only writers that actually contend for the same - parent row of the same learner, so different learners, and different competencies of one - learner, still record in parallel. - -2. Recompute derived levels on read instead of materializing them. + - 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: - - Eliminates the derived group and competency status rows and the roll-up writes entirely, - leaving nothing to keep consistent on write. + - A single lock replaces the ordering argument in Decision 2. - Cons: - - Moves the full bottom-up tree evaluation onto the hot read path, the opposite of what - dashboards and other read surfaces need (a direct indexed lookup). - - Settled against in :ref:`openedx-learning-adr-0002`. + - 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. -3. Send an event to openedx-core and update competency statuses in a separate celery task. +5. Recalculate the derived levels on every read instead of storing them. - Pros: - - Decouples the mastery update from the grade write, so grade recording does not depend on - competency code being installed or fast. + - No roll-up writes at all, so there is nothing to keep consistent. - Cons: - - Without a shared transaction, a failure or a lost event leaves the grade and its mastery - rows permanently out of sync (data drift), with no way to roll them back together. - - Writing the statuses in the same transaction as the grade (Decision 1) instead makes the - grade and its mastery consequences commit or fail as a unit. + - 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. -4. Commit the leaf, then re-read the leaves before rolling up, with no lock. +6. Send an event to openedx-core and do all the work in a separate celery task. - Pros: - - Correct, and lock-free. Every writer commits its leaf before reading, so whichever writer - reads last sees every leaf already committed and computes the true value; the merge in - Decision 2 keeps it. + - Recording a grade would not depend on the competency code being installed or fast. - Cons: - - The leaf has to commit before the roll-up reads, so the roll-up cannot share the grade's - transaction, which reopens the partial-failure window Decision 1 exists to close. + - 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. -5. Detect conflicts optimistically: a version column plus a unique constraint, and the losing write - retries. +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, so no writer ever blocks. + - Contention costs a retry rather than a wait. - Cons: - - Every writer needs conflict handling and a retry loop, and repeated contention on one - parent multiplies retries. The row lock in Decision 3 reaches the same result by waiting. + - 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.