[mircealungu]

Moves four of the seven older find_or_creates onto race_safe.find_or_create (#897): Url, DomainName, NewText, SourceText. Refactor only. Three more (Phrase, UserPreference, Text) are left alone, for the reasons in section 3.

1. What the old pattern did wrong

DomainName.find_or_create on master (Url, NewText, SourceText have the same shape):

try:
    return cls.find(_domain)
except sqlalchemy.orm.exc.NoResultFound:
    try:
        new = cls(_domain)
        session.add(new)
        session.commit()
        return new
    except sqlalchemy.exc.IntegrityError:
        for i in range(10):
            try:
                session.rollback()          # <- the whole session
                d = cls.find(_domain)       # <- plain SELECT
                return d
            except:
                time.sleep(0.1)
                continue

1. The full rollback. When two requests insert the same row, the loser's insert hits the unique key. session.rollback() then rolls back the loser's whole transaction, including anything its caller had changed but not yet committed. The caller isn't told and carries on. Its objects are now expired, and its changes are gone.
2. The retry loop. Up to 10 rounds of rollback, re-read and sleep. On MySQL one round is enough, so the loop does nothing useful. It hides real failures: DomainName swallows every exception with a bare except:, and Url can fall out of the loop and return None.
3. The plain re-read. It finds the winner's row only because of the full rollback. The rollback ends the transaction, so the next SELECT gets a fresh REPEATABLE READ snapshot. Recovering without throwing away the caller's work means staying in the same transaction, and then a plain SELECT reuses the old snapshot and misses the row. That's why the helper uses a locking read (LOCK IN SHARE MODE), which sees the latest committed row.
4. commit=False wasn't race-safe at all (NewText, SourceText). With commit=False they only session.add() the new row and never flush, so the except IntegrityError can't fire. A lost race came back as an IntegrityError at the caller's later commit, outside any recovery.

The helper rolls back only a savepoint around its own insert, then gets the winner's row with a locking read. The caller's pending work stays in the session.

2. Per site

Site	Before	After	Behaviour difference
DomainName.find_or_create(session, url)	find by domain_name; add + commit; on IntegrityError: full rollback, retry up to 10×	find_or_create(session, cls, {"domain_name": _domain}, …, commit=True)	Race path only: keeps the caller's pending work; re-raises the IntegrityError if no winner row exists, instead of swallowing it
Url.find_or_create(session, url, title)	resolve domain; None if path > 255; find by (path, domain); add + commit; on IntegrityError: full rollback, re-resolve domain, retry up to 10×, Sentry message per failed round	domain and the 255-char check unchanged; find_or_create(session, cls, {"path": path, "domain": domain}, …, commit=True)	Race path only: as above. The prints and the "Exception of second degree in url" Sentry message go away, along with the loop they were in
NewText.find_or_create(session, text, commit=True)	strip; find by long_hash; add, commit if commit; full rollback + retry	find_or_create(session, cls, {"content_hash": long_hash(clean_text)}, …, commit=commit)	Race path as above. With commit=False the new row is now flushed (inside the savepoint), so it has an id straight away, and a lost race is recovered rather than failing at the caller's commit
SourceText.find_or_create(session, text, commit=True)	same as NewText with text_hash	same with text_hash	same as NewText

Every unique key matches its filters, which the helper needs:

• domain_name.domain_name: unique=True in the model.
• url (path, domain_name_id): _path_domain_unique_constraint in the model (local MySQL calls it unique_domain_and_path).
• new_text.content_hash, source_text.content_hash: UNIQUE HASH_INDEX from tools/migrations/25-02-28--01-new-content-tables copy.sql. The models never declared it, so the SQLite test DB didn't have it. The first commit declares it, the way UserWord declares unique_user_word. create_all doesn't alter existing tables, so the declaration has no effect on prod.

On my local zeeguu_test (MySQL) all four keys exist, and the four lookup columns have binary collations (utf8mb3_bin / utf8mb4_bin). The WHERE and the key therefore agree on what counts as equal. I didn't check prod. If prod lacked one of these keys, the helper would act like the old code (no IntegrityError, so a duplicate row), so nothing gets worse. Worth a look: SHOW INDEX FROM new_text / source_text.

3. Skipped sites

• Phrase.find_or_create: needs a decision, not a refactor.

  • Prod probably has no unique key. 25-05-21--rename_user_word_to_phrase.sql dropped unique_word_language, and local MySQL has only non-unique indexes on phrase. The model's UniqueConstraint(content, language_id) exists only in the SQLite test DB. In prod, two racing requests would both insert a row and get no IntegrityError, so the helper has nothing to recover.
  • The lookup is Phrase.find: case-insensitive on content_lower, then a Python re-check with Meaning._canonicalize. A filter_by dict can't express that. The column's collation (utf8mb4_unicode_ci) also ignores accents, so a unique key on it would make accompagne and accompagné collide.
  • The create path also starts _calculate_rank_async(new.id) after the commit.

  Deciding what a phrase's identity is (and whether to add a key) is a separate question.

• UserPreference.find_or_create: no unique key on (user_id, key) in the model or in local MySQL (only a non-unique user_id index). A race therefore inserts a duplicate preference and never raises IntegrityError, so the retry loop is dead code and the helper would add nothing. A fix needs a prod duplicate check, a dedupe and a migration. Drop what nobody reads of user_preference #905 / c6b0918 reworked user_word.user_preference, a different thing; this model wasn't touched.

• Text.find_or_create: the legacy text table. Text migration: stop dual-write to legacy text table (Deploy 1/3) #650 (open) stops writing to it. It also doesn't fit the helper. The lookup filters on (content_hash, article), but local MySQL's unique key is on content_hash alone. When the same sentence already exists for another article, the helper would re-raise the IntegrityError, where the old fallback (content_hash only) returns the other article's row. Changing behaviour in code that's being retired isn't worth it.

4. Why this is safe

Commit behaviour. The helper's commit=True commits only when it created the row. The old code was the same: it committed after creating, never after finding, and on the race path it rolled back without committing. So the found and created paths behave as before. Only the race path differs, as described above.

Callers.

• Url.find_or_create has about 30 call sites (crawler, article/upload/translate endpoints, starred_article, video, yt_channel, skipped_url, tools). Some rely on the commit-on-create, e.g. save_translated_article's docstring and endpoints/article.py ("Url.find_or_create commits, and must not run inside save_translated_article's savepoint"). That's kept. They all use the returned .id or relationship, which a created, found or recovered row all have.
• DomainName.find_or_create: Url.find_or_create and tools/fix_feed.py. Commit-on-create is kept.
• NewText.find_or_create: Caption.create (commit=False), ArticleFragment.find_or_create and BookmarkContext.find_or_create (pass commit through).
• SourceText.find_or_create: Source.find_or_create (passes commit through).
• With commit=True (the default) nothing changes. With commit=False (article fragments while building an article, Source from the crawler/upload paths, captions) the row is now flushed early. It's still in the caller's transaction, so it's still undone if the caller rolls back.
• One knock-on effect: ArticleFragment.find_or_create(commit=False) filters on text_id=text_row.id. For a brand-new text that used to be None (text_id IS NULL, no match, so a new fragment). Now it's the real id, which no committed fragment can reference yet, so the result is the same (a new fragment).

5. How it was tested

zeeguu/core/test/test_legacy_find_or_create_races.py, one test per site. The row already exists, the site's first lookup is forced to miss (as on MySQL when the loser's snapshot is older than the winner's commit), and the caller has a pending change (a user's name). Each test checks three things: the loser gets the winner's id, only one row exists, and the pending change survives the commit. One more test checks that Url still returns None for a path over 255 chars.

• On master (with only the HASH_INDEX declarations applied, so SQLite has the keys) all four race tests fail:
  • DomainName and Url lose the caller's change (assert 'George Wright' == 'changed by the caller').
  • NewText and SourceText (commit=False) return an unflushed duplicate (assert None == 1).
• Full suite on this branch (rebased on current master): 1321 passed, 17 skipped, 1 failed. The failure is test_cloze_alternatives.py::test_a_translation_verdict_is_not_a_cloze_verdict, which is flaky on master too. Run alone 20 times, it failed on the 5th run here and the 7th on master (True is not false).
• Local MySQL (zeeguu_test, REPEATABLE READ), Url. Request A starts its snapshot and changes another url's title (pending). Request B inserts and commits A's url on a separate connection. A's plain SELECT can't see B's row. Then A calls Url.find_or_create:
  • master: doing a rollback … Found url after recovering from race. It returns the winner, but the pending title is lost ('original title').
  • this branch: it returns the winner and keeps the pending title ('changed by the caller').
  • The script deletes its rows afterwards.

Review follow-up

A code review found that the test rules NewTextRule and SourceTextRule built NewText(content) / SourceText(content) directly. With HASH_INDEX now declared, the test DB enforces the unique content_hash, so a second rule for the same text would raise IntegrityError. They now go through find_or_create. A new test, test_text_rules_reuse_the_row_for_the_same_content, fails without that change. Full suite: 1323 passed, 17 skipped.

Prod indexes were checked on 25 Oct, and all four keys exist and match the lookups: url.unique_domain_and_path (domain_name_id, path), domain_name.domain_name, new_text.HASH_INDEX, source_text.HASH_INDEX.

🤖 Generated with Claude Code

[github-actions]

ArchLens detected architectural changes in the following views: