[mircealungu]

Fifteen models each had their own copy of the same race-safe find_or_create. Fourteen of those copies had the bug that #893 fixed for ArticleCefrAssessment. This PR moves them all onto one helper, zeeguu/core/model/race_safe.py, which does not have the bug. #893 (ArticleCefrAssessment) is now merged, and its hand-rolled copy moves onto the helper too, in a separate commit (16 models in total).

1. The race

Every copy did the same three steps:

1. SELECT the row (e.g. the (bookmark, article) summary context).
2. If it is missing, INSERT it inside session.begin_nested(), which is a SAVEPOINT.
3. If the INSERT fails with IntegrityError, SELECT again and return the row that the other request created.

Here is what happens when two requests create the same row at once. MySQL runs InnoDB at REPEATABLE READ:

• Request A does its first read in the transaction (any read, e.g. loading the user). MySQL takes a snapshot at that moment. From then on, every plain SELECT in A's transaction sees the database as it was then.
• Request B inserts the row and commits.
• A, step 1: its SELECT reads the snapshot, so it doesn't see B's row. It goes on to insert.
• A, step 2: an INSERT is checked against the real, current table, not the snapshot. B's row is there, so the unique key rejects it: 1062 Duplicate entry. Because the insert ran inside the savepoint, only that insert is rolled back. Whatever A had pending stays, and the session can still be used. Without the savepoint, a failed flush would leave the whole session needing a rollback, and that rollback would discard the caller's other changes.
• A, step 3: the plain re-SELECT reads the same snapshot again. It still doesn't see B's row, so .one() raises NoResultFound. The request fails with a 500.

The fix for step 3 is a locking read: SELECT ... LOCK IN SHARE MODE, written in SQLAlchemy as .with_for_update(read=True). A locking read always reads the latest committed version of a row, not the snapshot, so it finds B's row. UserWord.find_or_create already did this, and #893 did the same for ArticleCefrAssessment.

Reproduced on real MySQL

I wrote a script for local MySQL zeeguu_test (REPEATABLE-READ) with two connections. Connection A reads (so its snapshot is taken). Connection B inserts and commits the row. Then A calls LevelAdaptedArticleSummaryContext.find_or_create(commit=False). All test rows were deleted afterwards.

• On master:

  FAILED: NoResultFound No row was found when one was required
    at zeeguu/core/model/level_adapted_article_summary_context.py:79   (the plain re-query)
    while handling: IntegrityError (1062, "Duplicate entry '812214-3' for key
    'level_adapted_article_summary_context.uq_alsc_bookmark_summary'")
  

• With this PR: OK returned <LevelAdaptedArticleSummaryContext als:4, b:812214>, and the table has exactly one row for the pair.

(I first tried ArticleSummaryContext, which has the same code. But local zeeguu_test doesn't have its unique key yet, because migration 26-08-13--add_unique_bookmark_context_joins.sql was never applied there. Without the key, both inserts went through and the table got two rows. So I used the level-adapted summary context, whose key does exist locally.)

2. The helper, line by line

def find_or_create(session, cls, filters: dict, make, commit=False):
    existing = session.query(cls).filter_by(**filters).one_or_none()
    if existing:
        return existing
    try:
        with session.begin_nested():
            new = make()
            session.add(new)
    except IntegrityError:
        return (
            session.query(cls)
            .filter_by(**filters)
            .with_for_update(read=True)
            .one()
        )
    if commit:
        session.commit()
    return new

• filters is passed to filter_by, so it can hold relationship objects (dict(bookmark=bookmark, article=article)) or ids (dict(article_id=article_id)). This matches what the call sites already filtered on.
• make is a function that builds the new object. It is called only when the row is missing.
• one_or_none(): found, so return it. If there are duplicates it raises MultipleResultsFound, just like the old .one() / .one_or_none() did.
• begin_nested() opens a savepoint. When the block exits, it flushes the INSERT. If the flush fails, it rolls back to the savepoint, so only this insert is lost (see section 1). make() runs inside the block. That way the new object can't be flushed by begin_nested()'s own pre-flush, which runs outside the savepoint. Some old copies built the object before the with.
• except IntegrityError: we lost the race, so we take the winner's row with the locking read. The comment explaining the snapshot is in the helper now, in place of the copies that were in each file.
• commit: commits only when this call created the row. A row that was found, or recovered after losing the race, is returned without a commit. All the old copies behaved this way.

3. Migrated sites

Before/after for one of them, ArticleSummaryContext.find_or_create:

# before
existing = cls.query.filter(cls.bookmark == bookmark, cls.article == article).one_or_none()
if existing:
    return existing
# Insert inside a SAVEPOINT ... (comment)
new = cls(bookmark, article)
try:
    with session.begin_nested():
        session.add(new)
except sqlalchemy.exc.IntegrityError:
    return cls.query.filter(cls.bookmark == bookmark, cls.article == article).one()  # stale snapshot
if commit:
    session.commit()
return new

# after
return find_or_create(
    session,
    cls,
    dict(bookmark=bookmark, article=article),
    lambda: cls(bookmark, article),
    commit=commit,
)

Signatures are unchanged everywhere. The rest of the migrated sites:

Model	commit passed	Note
ArticleTitleContext, ArticleFragmentContext, ExampleSentenceContext, VideoTitleContext, VideoCaptionContext, LevelAdaptedArticleSummaryContext, LevelAdaptedArticleTitleContext	caller's commit (default True)	same shape as above
SearchFilter, SearchSubscription, TopicFilter	True	they always committed after a create
TopicSubscription, UserLanguage	(no commit)	they never committed
UserWord	True	already used the locking read. Its is_user_added update after the lookup is kept unchanged
ArticlePublicCode.for_article	(no commit)	it used to catch any Exception; now it catches IntegrityError like the rest

I removed the import sqlalchemy / IntegrityError imports that were no longer used.

4. Not migrated, and why

• PublicTranslation.store, AudioLessonRefusal.record: these use a savepoint but are not find-or-create. They insert, and if they lose the race they just drop their own row (except: pass). They never re-read, so they don't have the stale-snapshot bug.
• Follow-ups (older copies of the pattern): url.py, text.py, domain_name.py, source_text.py, new_text.py, phrase.py, user_preference.py. These handle the race by committing or rolling back the whole session in retry loops. Moving them to the helper changes their transaction behaviour, so each needs its own look.

5. Why this is safe

• For the 14 copies that re-queried with a plain read, the only change in behaviour is on the losing side of the race: the request now gets the winner's row back where it used to raise NoResultFound.
• UserWord already used the locking read, so its behaviour is the same as before.
• ArticlePublicCode now catches IntegrityError and no longer catches every Exception. Any other error (e.g. a lock timeout) used to fall through to a re-query that would have found nothing anyway.
• Commit behaviour is the same at every site (table above). Nothing changes on the path where the row already exists.
• LOCK IN SHARE MODE holds a shared lock on one row until the transaction ends, and only on the losing side of a race that already failed. The happy path does no locking read.
• No schema change, no migration, no API change.

6. How it was tested

• New zeeguu/core/test/test_race_safe.py:

  • creates when the row is missing;
  • returns the existing row without calling make;
  • doesn't commit by default;
  • commit=True commits a created row but not a found one;
  • the lost race: the winner's row is committed, the first lookup is made to miss it (as a stale snapshot would), and the caller has a pending change. The helper returns the winner's row, there is still exactly one row, and the caller's change survives.

  The tests run on SQLite, which ignores with_for_update and has no snapshot. They cover the savepoint and recovery path. The locking read itself is covered by the MySQL reproduction above.

• The existing test_bookmark_context_uniqueness.py passes.

• MySQL reproduction: before and after results in section 1.

• Full suite: 1289 passed, 18 skipped.

Added after #893 merged

The branch is rebased on master. Commit 1ce16308 replaces the copy in ArticleCefrAssessment.find_or_create with a call to the helper. There is one difference: the old copy also committed after recovering the other request's row, and the helper doesn't. All six callers pass commit=False (four in article.py, two in endpoints/cefr_assessment.py), so none of them can notice. Full suite after the rebase: 1281 passed, 17 skipped.

🤖 Generated with Claude Code

[github-actions]

ArchLens - No architecturally relevant changes to the existing views