Skip to content

Commit 7b2100d

Browse files
jesperhodgeclaude
andauthored
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a74c315 commit 7b2100d

3 files changed

Lines changed: 197 additions & 8 deletions

File tree

docs/openedx_learning/decisions/0002-competency-criteria-model.rst

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -240,9 +240,9 @@ Decision
240240
3. ``oel_tagging_objecttag(object_id)``
241241
4. ``CompetencyCriteria(oel_tagging_objecttag_id)``
242242
5. ``CompetencyCriteria(competency_criteria_group_id)``
243-
6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)``
244-
7. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)``
245-
8. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)``
243+
6. ``StudentCompetencyCriteriaStatus(user_id, competency_criteria_id)`` (unique)
244+
7. ``StudentCompetencyCriteriaGroupStatus(user_id, competency_criteria_group_id)`` (unique)
245+
8. ``StudentCompetencyStatus(user_id, oel_tagging_tag_id)`` (unique)
246246
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)
247247
10. ``CompetencyMasteryStatuses(status)`` (unique)
248248

@@ -422,3 +422,11 @@ Rejected Alternatives
422422

423423
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.
424424
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.
425+
426+
Changelog
427+
---------
428+
429+
2026-07-27:
430+
431+
* Made the learner status indexes unique, so there is one row per learner and node. This is what
432+
the in-place, monotone status updates in :ref:`openedx-learning-adr-0004` read, lock, and update.

docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,18 @@ For the initial implementation, versioning and traceability of competency achiev
4444
- 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.
4545
- 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).
4646

47-
5. Learner status models/tables are append-only history and do not use ``django-simple-history``:
47+
5. Learner status models/tables are updated in-place:
4848

49-
- For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``, each status change is stored as a new row with ``created`` as the write timestamp.
50-
- Existing learner status rows are not updated in place.
51-
- Current status is determined by the most recent row for a given learner + target entity (ordered by ``created``, with ``id`` as a tie-breaker).
52-
- Older rows represent the learner status history and remain available for audit/tracing.
49+
- For ``StudentCompetencyCriteriaStatus``, ``StudentCompetencyCriteriaGroupStatus``, and ``StudentCompetencyStatus``,
50+
each status change updates the responsible row.
51+
- Automatic status updates only ever increase a status, as relied on by
52+
:ref:`openedx-learning-adr-0004`. A downward adjustment (for example ``Demonstrated`` to
53+
``PartiallyAttempted``) is never applied by a grade change or by a competency criteria rule
54+
change.
55+
- Direct edits by staff, through Django admin or as a deliberate instructor correction, are
56+
exempt: they may set a status to any value, including a lower one, and the ancestors above the
57+
edited node are recomputed to match.
58+
- How learner status history is retained is not decided here.
5359

5460

5561
Rejected Alternatives
@@ -85,3 +91,13 @@ Rejected Alternatives
8591
- Cons:
8692
- Requires custom tooling to reconstruct past versions
8793
- Does not align with existing publishable versioning patterns
94+
95+
Changelog
96+
---------
97+
98+
2026-07-27:
99+
100+
* Reworked Decision 5 for :ref:`openedx-learning-adr-0004`: learner status rows are now updated in
101+
place, and automatic updates only ever increase a status, with direct staff edits exempt.
102+
Previously append-only, with current status resolved as the most recent row. How status history is
103+
retained is left undecided.
Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
.. _openedx-learning-adr-0004:
2+
3+
4. How should learner competency mastery be recorded concurrently and at scale?
4+
================================================================================
5+
6+
Status
7+
------
8+
Proposed.
9+
10+
Context
11+
-------
12+
A learner's mastery of one competency is stored at three levels of the criteria tree: the graded
13+
leaf criterion, each criteria group above it, and the competency itself. There is one row per
14+
learner and node, updated in place (:ref:`openedx-learning-adr-0002`,
15+
:ref:`openedx-learning-adr-0003`). Each row holds one of three values, lowest to highest:
16+
``AttemptedNotDemonstrated``, ``PartiallyAttempted``, ``Demonstrated``.
17+
18+
One grade change updates the leaf and then every row above it, for many learners at once. This ADR
19+
decides how those updates stay correct when two of them for the same learner overlap.
20+
21+
The problem: a group requires both Assignment A and Assignment B, and celery tasks recomputing
22+
grades and competency statuses for this worker run at the same time. That is likely to happen
23+
when instructor actions trigger multiple subsection grading events.
24+
Each of the two writers sees its own assignment done and the other still outstanding,
25+
so both write "not demonstrated" for the group. Both are wrong, both have finished, and nothing is
26+
left to correct it.
27+
28+
Two constraints shape the answer. First, **the grading task cannot be one transaction**: it reads
29+
MongoDB and memcached, queues further celery tasks, publishes events, and triggers database writes
30+
owned by four other Django apps. Wrapping all of that would roll back other apps' data and publish
31+
events for a grade that never commits. Second, **everything above the leaf is derived**: a group's
32+
value can always be recalculated from the leaves beneath it, so the leaf is the only row that is a
33+
direct consequence of the grade.
34+
35+
That constraint is why the whole grading task can't be one transaction. A separate constraint is why
36+
even a transaction scoped only to the leaf and its rollup would still be wrong: two tasks finishing different
37+
assignments for the same group race on that group's row regardless of transaction size, since each transaction
38+
hides its writes from the other until it commits (Rejected Alternative 2).
39+
40+
Decision
41+
--------
42+
43+
1. **Write the leaf status in the same transaction as the grade. Nothing above it.**
44+
The grading task calls one openedx-core function, which writes the leaf, so the grade and its
45+
leaf commit or fail together.
46+
47+
2. **Schedule rollup as a secondary celery task, asynchronously after the grade is recorded.**
48+
This separation avoids direct contention between concurrent grade changes on shared parent nodes.
49+
The secondary task is automatically retried by celery if it fails.
50+
51+
3. **Rollup commits each level before reading the next, with no locks.**
52+
A writer sees only committed data, so whichever writer reads a parent last sees all its children
53+
at their final values and computes the correct result. This depends on Decision 4, which provides
54+
a reconciliation rule.
55+
56+
4. **Automatic updates may only raise a status, never lower it.**
57+
Each write stores whichever is higher, the stored or the newly computed value, so concurrent
58+
writers cannot overwrite each other. A writer reading stale data can only compute a value that
59+
is too low, and too low is discarded. That is also what makes celery's repeated and out-of-order
60+
delivery harmless.
61+
62+
5. **Add a manually-invoked recovery mechanism.**
63+
For example a management command or Django admin action forces roll-ups to recalculate for a given range, to recover from
64+
operational failures, content tagging errors, or bugs in the roll-up code that celery's retry
65+
won't catch.
66+
67+
6. **Only a direct staff edit may lower a status.** A staff correction may set any value, and the
68+
rows above it are recalculated and overwritten rather than merged. A later grade change can raise
69+
what an edit lowered, but never lower what an edit raised. The staff correction takes a row lock
70+
on the ``StudentCompetencyCriteriaGroupStatus`` row for that learner and the competency's root
71+
criteria group, the group with no parent; no other path takes a lock.
72+
73+
74+
75+
Rejected Alternatives
76+
---------------------
77+
78+
1. Lock each criteria group row before recalculating it.
79+
80+
- Pros:
81+
- Correctness comes from making contending writers take turns, which is easier to prove than
82+
an argument about the order of commits and reads.
83+
- Cons:
84+
- One grade change can affect several leaves of the same tree, so a writer can need several
85+
locks at once, which introduces deadlocks that need their own detection and retry code.
86+
- It puts a lock wait on every grade change. MySQL waits 50 seconds by default, inside a task
87+
allowed 300 seconds in total.
88+
- Correctness would depend on the isolation level, silently, and SQLite has no row locks, so
89+
the test suite could not exercise it.
90+
91+
2. Share one transaction between the grade and the whole roll-up, not just the leaf.
92+
93+
- Pros:
94+
- The grade and every mastery row it touches would commit or fail together, so no roll-up
95+
could ever be left unfinished and Decisions 2 and 5 would be unnecessary.
96+
- Cons:
97+
- The grading task cannot be wrapped in a transaction at all, for the reasons in the Context.
98+
- Wrapping only the roll-up is worse than doing nothing: it hides each writer's changes from
99+
the other until both have finished, which is the problem in the Context again, one level up
100+
the tree and harder to diagnose.
101+
102+
3. Take one lock on the learner's competency root-group status row, then recalculate the whole
103+
subtree beneath it.
104+
105+
- Pros:
106+
- Easy to reason about: one lock, always the same row, so no deadlock and no ordering
107+
argument.
108+
- Cons:
109+
- It puts a lock, and its timeout handling, on every grade change rather than only on the
110+
rare path that lowers a value.
111+
- It needs row locks, which SQLite does not support.
112+
113+
This is the right shape for the paths that lower a value, and Decision 6 uses it there.
114+
115+
4. Use a coarser lock, either one per deployment or one per learner.
116+
117+
- Pros:
118+
- A single lock replaces the ordering argument in Decision 3.
119+
- Cons:
120+
- A deployment-wide lock serializes every learner behind every other, giving up the
121+
throughput bursty grading needs.
122+
- The "one per learner" option makes a learner's unrelated competencies wait for each
123+
other, since one lock would then cover every tree they have.
124+
- Either kind adds machinery for acquiring and releasing locks, and for recovering from a
125+
dead lock holder, across a very large key space.
126+
127+
5. Recalculate the derived levels on every read instead of storing them.
128+
129+
- Pros:
130+
- No roll-up writes at all, so there is nothing to keep consistent.
131+
- Cons:
132+
- It moves a full bottom-up tree evaluation onto every read, the opposite of what dashboards
133+
need.
134+
- Already settled against in :ref:`openedx-learning-adr-0002`. Unresolved item 1 is the
135+
narrower version still open.
136+
137+
6. Send an event to openedx-core and do all the work in a separate celery task.
138+
139+
- Pros:
140+
- Recording a grade would not depend on the competency code being installed or fast.
141+
- Cons:
142+
- openedx-core is a library and cannot own a celery queue, so every caller would supply one.
143+
- The leaf would no longer commit with the grade, giving up the one guarantee Decision 1 is
144+
cheap enough to keep.
145+
146+
7. Detect conflicts optimistically, with a version column and a retry loop for the losing write.
147+
148+
- Pros:
149+
- Contention costs a retry rather than a wait.
150+
- Cons:
151+
- Decision 4 is already optimistic, without the retry loop. A write that loses has computed
152+
a value that is too low, and discarding those is exactly what Decision 4 does.
153+
154+
8. Read-after-write: after writing the leaf, re-read each parent's children before rolling up, to catch a race
155+
with another writer already in flight.
156+
157+
- Pros:
158+
- Recovers from the race within the same request, without a separate marker or job.
159+
- Cons:
160+
- Decision 1 shares a transaction only between the grade and its leaf, and Decision 3 commits each rollup level
161+
separately before reading the next. That leaves no single transaction boundary for a read-after-write check to
162+
run inside: by the time a re-read would happen, the level below has already committed and could change again
163+
before the write completes.
164+
- It also only checks for a race at the moment each parent is read. If the worker crashes mid-cascade before reaching the next read,
165+
nothing notices the rollup was left unfinished. Decision 5's manual recovery mechanism exists to catch that case.

0 commit comments

Comments
 (0)