docs: ADR 0004 competency mastery concurrency

[jesperhodge]

Splits ADR 0004 out of #657 so it can be reviewed on its own. ADR 0005 is not included here.

What's in this PR

• New: docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst, unchanged from Competency Status Storage ADR #657.
• ADR 0002: the learner-status index list (paired ACTIVE/HISTORY indexes, including the unique leaf HISTORY index that ADR 0004 cites as the idempotency key for its append), the list of append-only history tables, and the matching changelog entry.
• ADR 0003: Decision 5 reworked to in-place, monotonic learner status updates, new Decision 6 for the append-only HISTORY tables, and the matching changelog entry.

Changes to 0002/0003 were filtered from #657, not rewritten. Left behind as ADR-0005-only or unrelated:

• pointing the learner-status user_id foreign keys at settings.AUTH_USER_MODEL (that comes from ADR 0005's rejected alternative 7)
• a whitespace-only fix in ADR 0002

Known gap

ADR 0004 references :ref:openedx-learning-adr-0005`` in five places, and the 0002/0003 edits reference it in four more. Until ADR 0005 lands, those are undefined labels: sphinx emits nine `ref.ref` warnings. The build still succeeds (`SPHINXOPTS = -W` is commented out in `tox.ini`), but the links are dead. Either 0005 merges soon after this, or the references need rewording.

🤖 Generated with Claude Code

[openedx-webhooks]

Thanks for the pull request, @jesperhodge!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[mgwozdz-unicon]

Two things stood out reading through this.

Monotonicity override. ADR 0003's reworded Decision 5 says downward status adjustments "are prohibited," as a flat rule with no exception. ADR 0005 (not part of this PR) has "Reversing a banked status is a separate administrative action, out of scope here," which reads as: overrides are permitted, just not through the normal recorder path and not spec'd here. Since 0005 isn't in this PR, ADR 0003 currently reads as an absolute prohibition with no hint that an override path exists or is intentionally deferred elsewhere. Worth adding a line noting that admin-initiated corrections are out of scope for this ADR rather than prohibited outright, even before 0005's mechanism is settled.

ADR 0005 coupling. I went through every :ref: to openedx-learning-adr-0005 in this PR. In ADR 0004, three of five are load-bearing rather than incidental cross-references: the ACTIVE/HISTORY split in Context, the "Monotonicity" premise that the whole "monotone merge is safe" argument depends on, and Rejected Alternative 4's rationale ("since we decided that every status table lives in the main database"). None of those hold up as written if 0005 lands differently than assumed here. And the 0002/0003 changes aren't generic ADR-4 groundwork, they directly encode 0005's schema: the paired ACTIVE/HISTORY tables and the monotonic-only history-write rule, both citing 0005 as the source. Given that whether to store competency status history at all is still an open question in our design discussions, this PR is more substantively dependent on 0005 than the "known gap" note in the description suggests, it's not just a matter of dangling ref warnings until 0005 lands.

A few ways to handle it:

Option	What it does	Tradeoff
Recombine into #657	Matches the real coupling, no dangling refs	Splitting into a separate PR was wasted effort
Rewrite the load-bearing pieces (ADR 0004's Context/Monotonicity/Rejected-Alt-4, plus 0002/0003's HISTORY-table content) to state them as this PR's own working assumptions instead of citing 0005 as an already-decided fact, and move the 0005-specific schema content back into 0005	Makes this PR genuinely self-contained now; only needs revisiting if 0005 changes something this PR actually assumed	More editing work up front
Leave as-is, merge 0005 right after this	Least work now	Doesn't close the missing-override gap in 0003, and ADR 0004 still isn't mergeable on its own until 0005 lands

I'd lean toward the middle option: pull the 0002/0003 HISTORY-table content back into 0005 where it actually belongs, and reword ADR 0004's three load-bearing spots as assumptions this ADR needs rather than facts cited from an unmerged decision. That keeps this PR mergeable on its own terms and limits the blast radius if the history-storage direction changes.

[mgwozdz-unicon]

On the two open questions: I'd say yes to both. A direct staff edit should cascade to ancestors, and a cascade may overwrite a hand-set ancestor.

Decision 4. This one lost its own idempotency argument, the unique constraint on learner/node/status and its cross-reference to ADR 0002, independent of removing item 5. Was that intentional, or a side effect of editing around item 5's removal?

Decision 2. The first paragraph ends on an unresolved problem, "each recomputation could read the other child's old value and write a parent status that is too low," and the very next paragraph opens by immediately resolving it: "To prevent that, a recomputation locks the parent row..." Since the fix follows the problem directly, I'd merge these two into one paragraph rather than splitting a problem from its ready-made solution.

Decision 3. The Context above already frames the source of variation as different learning instruments, "any other learning instrument tied to a competency by a competency criterion, such as a course or a rubric criterion." This decision's closing sentence switches to a different axis, competency criteria types, with "completion" as the example. Since supporting completion as a competency criteria type isn't something we're likely to do going forward, I'd swap the example for something in the vein of a course grade or a rubric criterion, consistent with how Context already frames it.

[mgwozdz-unicon]

Thanks for the quick turnaround. I have 7 items from my review as well as some final considerations for us.

1. Read-after-write isn't reflected anywhere in the ADR. It was one of the two options coming out of the core arch meeting, alongside dirty-marking plus a scheduled job. Right now only one of the two made it in, not even as a rejected alternative. Could you add read-after-write, at minimum as a rejected alternative, with the reasoning for choosing dirty-marking over it? These two may not actually be solving the same problem: read-after-write recovers from a concurrent race within the same request, but doesn't obviously recover from a worker crashing mid-cascade, which is what the scheduled job in Decision 6 is for. If that is the real distinction, say so in the ADR; if the reason is something else, that's fine too, but it would help to have it written down rather than living only in meeting notes and side conversations.

2. Context's transaction-boundary reasoning conflates two different constraints. The grade itself is stored in SQL (PersistentSubsectionGrade), not MongoDB, and so is the competency status tree, so grade-plus-leaf being one transaction (Decision 1) was never blocked by anything in this paragraph, Mongo/cache/celery/events. The reason the rollup above the leaf isn't also folded into that same transaction is a separate one, already in Rejected Alternative 2's second con: two different grading tasks, each finishing a different assignment for the same group, still race on that group's row no matter how large either task's own transaction is, since each task's transaction hides its writes from the other until it commits. As written, Context reads like the Mongo/cache/celery/events constraint is the whole reason nothing above the leaf is transactional with the grade, when for the SQL-only portion of that question, the real reason is the cross-task race. Worth a sentence separating the two.

3. Decision 2's correctness argument seems incomplete without Decision 3. As written, Decision 2 justifies taking no locks by saying whichever writer reads a parent last computes the correct result. But the race described in the Context is exactly a case where the last reader can still compute a too-low value, both writers see the other's assignment as outstanding. What actually prevents that from sticking is Decision 3's rule to store whichever value is higher. Suggest reordering so Decision 3 comes before Decision 2, or adding a one-line forward reference in Decision 2 pointing to Decision 3.

4. The dirty marker's clear step should say it's atomic. Decision 5 says clearing the marker is conditional on the value passed up still being current, which is the right idea, but it only prevents the race it's meant to prevent if that check and the clear happen as a single atomic statement, for example UPDATE ... WHERE value = :read_value, rather than a read followed by a separate write. Please state that explicitly, the way Decision 3 already calls out "in a single statement" for the raise.

5. Rejected Alternatives 3 and 4 may be saying the same thing twice. Alternative 3, a lock on "the learner's root group row," lists as a con that it makes a learner's unrelated competencies wait for each other. That con only makes sense if "root group row" is one row spanning all of a learner's competencies rather than one per competency tree, in which case it's the same lock scope as Alternative 4's "one per learner" option, making that bullet in Alternative 4 redundant. If "root group row" is meant to be per competency instead, the "unrelated competencies wait" con doesn't belong on Alternative 3. Please clarify which it is, since Decision 7 reuses the same "root group row" language, and trim whichever alternative ends up restating the other.

6. Two sentences read as PR history rather than ADR content: "This was the previous decision here" in Rejected Alternative 1, and "This ADR originally assumed this was available" in Rejected Alternative 2. Neither means anything to a reader who wasn't following this PR. Suggest cutting both.

7. Is "writes to file storage" in the Context accurate? I couldn't find a file-storage write in the current certificate generation path, certificates are web-rendered rather than file-based, but I may be looking at the wrong part of the pipeline. If it isn't accurate, it should come out rather than stay as an uncited claim.

Final Tradeoffs & Implementation Question: One more, for discussion here rather than something to write into the ADR: the choice between read-after-write and dirty-marking-plus-scheduled-job seems to come down to one question: is the crash-safety the scheduled sweep buys worth the operational overhead it adds? Read-after-write's downside is that data can be left wrong if a worker crashes mid-cascade, with nothing scheduled to notice or fix it until an unrelated change happens to touch the same node again. The scheduled sweep's downside is real added scope: something has to be built and then run continuously by whoever operates the deployment. Separately, if a scheduled sweep is the direction we stick with, how much that overhead actually is depends on implementation: instructor_task's pattern (a management command cron'd by whatever the operator already runs) is much lighter than openedx-completion-aggregator's (a separate plugin plus scaled-up Celery workers), which is the closer analog to what's being proposed here. I'm curious for everyone's thoughts on these points.
@jesperhodge @kdmccormick @ormsbee @bradenmacdonald

[jesperhodge]

@mgwozdz-unicon
Thanks a lot for the detailed feedback.

• I don't understand all of the points in your comments, especially point 2. Point 3 contradicts my understanding, so I am not certain whether it's correct. Let's talk about it later so I can understand it better.
• I should state more clearly the reason for moving away from read-after-write: The main reason for the change is that the previous approach of read-after-write assumed that the whole grade change task can be one transaction. This was a wrong assumption. The change here is that now we only wrap the singular grade change and the singular leaf node write in one transaction, then calculate nodes higher up one-by-one. It seems to make read-after-write unnecessary.
• Retrying after a failure is another thing that seems necessary since now there is not one big transaction.
• The repair job is only due to a secondary concern that maybe tasks could silently fail. If we're not worried about that, the repair job is not necessary. The repair job is only an addition for this one concern, so without the repair job the rest of the ADR would still be as now.

[mgwozdz-unicon]

Thanks @jesperhodge , this helps, and it lines up with yesterday's Core Arch meeting too. Here's where all 7 items plus the final tradeoffs question stand. Clearing up 2 and 3 first, since those are the ones you flagged.

Item 2 is about the Context section. As written, the Context section gives one reason ("the grading task cannot be one transaction," because of Mongo, memcached, celery, and events) for why nothing above the leaf shares a transaction with the grade. That reason explains why the whole grading task can't be wrapped in a transaction. It doesn't explain why even a smaller transaction, covering just the leaf and the rollup above it, would also be wrong, since that smaller transaction wouldn't touch Mongo or celery at all. The actual reason the rollup can't join even that smaller transaction is the race already described in Rejected Alternative 2: two tasks each finishing a different assignment for the same group still race on that group's row no matter how large or small either task's own transaction is, because each transaction hides its writes from the other until it commits. Right now the Context section reads as if the Mongo/celery constraint is the whole answer to "why doesn't the rollup share a transaction with the grade," when for the SQL-only part of that question, the real answer is the race. This just needs one sentence in the Context section separating the two reasons. It isn't asking you to change the design.

Item 3: Decision 2 says taking no locks is fine because "whichever writer reads a parent last...computes the correct result." But in the race that the Context section describes, both writers can read the group's children before the other's leaf write has committed, so there's no writer that reads "last" in the sense Decision 2 needs, both can compute the too-low value. What actually makes that safe is Decision 3: because a write can only raise the stored value, never lower it, a wrong low write from either writer can never stick over a right high one, and a later write (retried or triggered by anything else touching that node) raises it correctly. So Decision 2's "whichever reads last is correct" isn't quite true on its own; the correctness guarantee is Decision 3's, not Decision 2's. Suggest either reordering so Decision 3 comes before Decision 2, or adding a one-line forward reference in Decision 2 pointing to Decision 3. This also means read-after-write and the current no-locks design aren't solving the race the same way, worth keeping in mind for item 1 below.

Status on the rest:

1. Read-after-write still isn't written down anywhere in the ADR, not even as a rejected alternative. You've now given the reasoning in this comment (the old approach assumed the whole grading task could be one transaction, which turned out to be wrong). That's exactly what item 1 asked for, it just needs to move from this comment into the ADR itself, as a rejected alternative with that reasoning attached.
2. Still open. Decision 5 says clearing the marker is conditional on the value passed up still being current, which is the right idea, but it only prevents the race it's meant to prevent if that check and the clear happen as a single atomic statement. As written, Decision 5 ends: "Clearing is conditional on the value passed up still being current, otherwise one writer can clear another's marker and lose its work." Suggest adding right after that: "That check and the clear happen in one statement, the same way Decision 3's raise is one statement."
3. Still open. Rejected Alternatives 3 and 4 might be saying the same thing twice. Alternative 3 is "Take one lock on the learner's root group row, then recalculate the whole subtree beneath it," and lists as a con: "It makes a learner's unrelated competencies wait for each other." That con only makes sense if "root group row" means one row per learner, spanning every competency tree they have, not one root per tree. If that's what it means, Alternative 4's "or one per learner" (in "Use a coarser lock, either one per deployment or one per learner") is the same lock scope as Alternative 3, and that half of Alternative 4 could be cut as redundant. If instead "root group row" means one root per competency tree, so a learner has several, the "unrelated competencies wait" con is on the wrong alternative, it belongs on Alternative 4's "one per learner" option instead. Can you confirm which one it is? Decision 7 uses the same "root group row" phrase for its own lock, so whichever meaning you confirm should read consistently there too.
4. Still open, small edit: "This was the previous decision here" (Rejected Alternative 1) and "This ADR originally assumed this was available" (Rejected Alternative 2) both only make sense to someone following this PR. Fine to just delete both sentences.
5. Still open, factual check: the Context section says the grading task "writes to file storage." Can you confirm that's accurate? I couldn't find a file-storage write in the current certificate generation path, certificates render to the browser rather than to a file, but I may be looking at the wrong part of the pipeline. If it's not accurate, it should come out.

Final tradeoffs question: two things got bundled into this question, splitting them.

The operational-overhead half is settled: the "scheduled job" in Decision 6 is a celery task, not a cron job an operator has to configure separately, so that concern doesn't apply.

The other half is still open, and it's really a question for @ormsbee. As written, Decisions 5 and 6 use a "dirty" marker plus a job that scans for markers system-wide. From yesterday's meeting, my understanding is Dave was leaning toward something simpler instead: the roll-up's own celery task already knows the learner's user_id and the subsection's object_id when it starts, so on failure it can just retry with that same input and redo that one tree, no marker, no scan. If that's the direction, here's a specific replacement for Decisions 5 and 6:

5. If a roll-up step fails partway, retry it by re-running the whole roll-up for that learner and that leaf, not by resuming from wherever it stopped. Decision 3 makes every write safe to repeat, so re-running from the leaf costs no more than resuming would, and needs no record of where the previous attempt stopped.
6. The retry is an ordinary celery task retry, not a separate scheduled sweep. The roll-up entry point already has the learner and the leaf when it starts, so the same task that failed can retry with that same input. Nothing needs to scan for or find work to do.

One thing this gives up: a task that's lost outright, not failed-with-a-retry-pending but gone entirely, for example dropped from the queue or out of retries, won't get fixed until something else happens to touch that same learner and leaf again. Decisions 5 and 6 as written today catch that case, because the sweep finds it independently of any future event. If we're fine relying on celery's own retry and delivery guarantees instead, I'd rather make this change now than merge an ADR that says something we don't actually intend to keep.

@ormsbee, could you confirm if Decisions 5 & 6 should get updated to the above suggestion or whether we should continue with the approach of the dirty marker with system-wide cleanup?

[ormsbee]

@mgwozdz-unicon: I agree that for the most part, we can rely on celery's built-in retry mechanisms to address transient failures during roll-up for a given user.

Despite that, I believe that we'll also want some sort of management command or Django admin-triggered task to be able to force roll-ups to be recalculated, to recover from drastic operational failures, content tagging errors, or bugs in the roll-up code. On edx.org, we've sometimes done this sort of recovery based on timestamp ranges for operational failures.

I'd prefer to avoid having an explicit "dirty" flag for this use case, since that's yet another thing that we could have a race condition on. A corrective task like that would often ignore the "dirty" flag anyway, because it's being run to correct a code or content error, as opposed to just an operational failure. We don't have to lay out the specifics of how that recovery mechanism will work here now–I think it's enough to note that we'll want such a mechanism but that it would be manually invoked, and is only meant to be used in unusual circumstances.