From 5dfc4619c6b9c73c3cf7b8d6838726b0b34c5a1d Mon Sep 17 00:00:00 2001 From: Mircea Lungu Date: Sun, 25 Oct 2026 20:02:08 +0100 Subject: [PATCH 1/2] translate_and_adapt: no 500 when two requests create the same article Two requests for the same upload (double tap, retry while the first waits on the LLM) both missed Article.find and both inserted an article at the same url; the second hit the unique key on article.url_id (1062) and returned a 500. The insert now goes through race_safe, so the loser gets the winner's article back. A new find_or_create_and_report (find_or_create is now a wrapper around it) also says whether this call created the row, so fragments are only created by the request that created the article. The article and its fragments go in one commit. Co-Authored-By: Claude Opus 5.5 --- zeeguu/api/endpoints/article_upload.py | 54 +++++---- .../test_article_upload_translate_race.py | 113 ++++++++++++++++++ zeeguu/core/model/race_safe.py | 17 ++- zeeguu/core/test/test_race_safe.py | 36 +++++- 4 files changed, 195 insertions(+), 25 deletions(-) create mode 100644 zeeguu/api/test/test_article_upload_translate_race.py diff --git a/zeeguu/api/endpoints/article_upload.py b/zeeguu/api/endpoints/article_upload.py index df40eaaf1..464d1cf64 100644 --- a/zeeguu/api/endpoints/article_upload.py +++ b/zeeguu/api/endpoints/article_upload.py @@ -198,6 +198,7 @@ def article_upload_translate_and_adapt(upload_id): from zeeguu.core.model.url import Url from zeeguu.core.model.source import Source from zeeguu.core.model.source_type import SourceType + from zeeguu.core.model.race_safe import find_or_create_and_report from datetime import datetime user = User.find_by_id(flask.g.user_id) @@ -260,28 +261,39 @@ def article_upload_translate_and_adapt(upload_id): ) clean_summary = result.get("summary") or (result["content"][:200] + "...") - - translated_article = Article( - translated_url, - result["title"], - None, - source_obj, - clean_summary, - datetime.now(), - None, - target_lang_obj, - result["content"], - None, + img_url = Url.find_or_create(db_session, upload.image_url) if upload.image_url else None + + def make_translated_article(): + article = Article( + translated_url, + result["title"], + None, + source_obj, + clean_summary, + datetime.now(), + None, + target_lang_obj, + result["content"], + None, + ) + article.cefr_level = user_level + article.source_upload_id = upload.id + article.img_url = img_url + return article + + # A second request for the same upload (double tap, retry) can miss the + # Article.find above while the first one waits on the LLM. Both then + # insert at this url; the loser gets the winner's article back. + translated_article, created = find_or_create_and_report( + db_session, Article, dict(url=translated_url), make_translated_article ) - translated_article.cefr_level = user_level - translated_article.source_upload_id = upload.id - if upload.image_url: - translated_article.img_url = Url.find_or_create(db_session, upload.image_url) - - db_session.add(translated_article) - db_session.commit() - - translated_article.create_article_fragments(db_session) + # Fragments only for an article this request created: the loser must not + # add a second set to the winner's. One commit for article and fragments, + # so whoever sees the article sees its fragments too. For the loser the + # commit also ends its REPEATABLE READ snapshot, which predates the + # winner's commit and would show the article without fragments. + if created: + translated_article.create_article_fragments(db_session) db_session.commit() _ensure_personal_copy(user, translated_article) diff --git a/zeeguu/api/test/test_article_upload_translate_race.py b/zeeguu/api/test/test_article_upload_translate_race.py new file mode 100644 index 000000000..7c2d5b2d1 --- /dev/null +++ b/zeeguu/api/test/test_article_upload_translate_race.py @@ -0,0 +1,113 @@ +""" +Two requests to /article_upload//translate_and_adapt for the same upload +(a double tap, or a retry while the first waits on the LLM) both miss the +existing-article lookup and both insert an article at the same url. The +loser used to hit the unique key on article.url_id and return a 500. +""" + +from unittest import mock + +from sqlalchemy.orm import Query + +from zeeguu.api.test.fixtures import logged_in_client +from zeeguu.core.model import Article +from zeeguu.core.model.article_fragment import ArticleFragment +from zeeguu.core.model.db import db + +UPLOAD_URL = "https://example.com/race/bau-von-trumps-triumphbogen" +TEXT = ( + "Der Bau des Triumphbogens hat begonnen. Er wird sechsundsiebzig Meter " + "hoch sein und steht in der Hauptstadt. Viele Leute sprechen darüber." +) + + +def _llm_output(): + # Each call returns a different text, as the real LLM does: the loser's + # fragments would differ from the winner's and add a second set. + calls = [] + + def translate_and_adapt(self, **kwargs): + calls.append(kwargs) + n = len(calls) + return { + "title": f"Triumphbogen {n}", + "content": f"Erster Absatz, Version {n}.\n\nZweiter Absatz, Version {n}.", + "summary": f"Zusammenfassung {n}", + } + + return translate_and_adapt, calls + + +def _create_upload(client): + upload = client.post( + "/article_upload/create", + data=dict(url=UPLOAD_URL, text_content=TEXT, title="Triumphbogen"), + ) + return upload["id"] + + +def _translate(client, upload_id): + return client.response_from_post( + f"/article_upload/{upload_id}/translate_and_adapt" + ) + + +def _fragment_count(article_id): + return ArticleFragment.query.filter_by(article_id=article_id).count() + + +def _articles_from(upload_id): + return Article.query.filter_by(source_upload_id=upload_id).all() + + +def _cleanup(upload_id): + # article and article_fragment survive between API tests (conftest's + # _KEEP_TABLES), so remove what this test made. + for article in _articles_from(upload_id): + ArticleFragment.query.filter_by(article_id=article.id).delete() + db.session.delete(article) + db.session.commit() + + +def test_losing_the_insert_race_returns_the_winners_article(logged_in_client): + upload_id = _create_upload(logged_in_client) + translate_and_adapt, llm_calls = _llm_output() + + with mock.patch( + "zeeguu.core.llm_services.simplification_service." + "SimplificationService.translate_and_adapt", + translate_and_adapt, + ): + try: + # The winner: the first request creates the article. + first = _translate(logged_in_client, upload_id) + assert first.status_code == 200 + winner_id = first.get_json()["id"] + winner_fragments = _fragment_count(winner_id) + assert winner_fragments > 0 + + # The loser: its lookups miss the winner's article (on MySQL: its + # REPEATABLE READ snapshot predates the winner's commit), so it + # calls the LLM and inserts at the same url. + real_one_or_none = Query.one_or_none + article_lookups = [] + + def first_article_lookup_misses(query): + if query.column_descriptions[0]["entity"] is Article: + article_lookups.append(query) + if len(article_lookups) == 1: + return None + return real_one_or_none(query) + + with mock.patch.object(Article, "find", return_value=None), \ + mock.patch.object(Query, "one_or_none", first_article_lookup_misses): + second = _translate(logged_in_client, upload_id) + + assert len(llm_calls) == 2, "the loser did reach the insert" + assert second.status_code == 200 + assert second.get_json()["id"] == winner_id + assert second.get_json()["title"] == "Triumphbogen 1" + assert len(_articles_from(upload_id)) == 1 + assert _fragment_count(winner_id) == winner_fragments + finally: + _cleanup(upload_id) diff --git a/zeeguu/core/model/race_safe.py b/zeeguu/core/model/race_safe.py index 627d2b1b3..cdbcce956 100644 --- a/zeeguu/core/model/race_safe.py +++ b/zeeguu/core/model/race_safe.py @@ -12,9 +12,20 @@ def find_or_create(session, cls, filters: dict, make, commit=False): Commits only when it created the row and `commit` is set; a found or recovered row is returned without committing, as the per-model copies did. """ + row, _ = find_or_create_and_report(session, cls, filters, make, commit) + return row + + +def find_or_create_and_report(session, cls, filters: dict, make, commit=False): + """ + As find_or_create, but returns `(row, created)`: `created` is True only + when this call inserted the row, and False when it found it or lost the + race to another request. For callers that must do follow-up work (e.g. + child rows) only for a row they created. + """ existing = session.query(cls).filter_by(**filters).one_or_none() if existing: - return existing + return existing, False # Two requests that both miss the query above both insert, and the unique # key lets exactly one of them through. @@ -40,8 +51,8 @@ def find_or_create(session, cls, filters: dict, make, commit=False): .filter_by(**filters) .with_for_update(read=True) .one() - ) + ), False if commit: session.commit() - return new + return new, True diff --git a/zeeguu/core/test/test_race_safe.py b/zeeguu/core/test/test_race_safe.py index faf768c38..c783f9b9a 100644 --- a/zeeguu/core/test/test_race_safe.py +++ b/zeeguu/core/test/test_race_safe.py @@ -4,7 +4,7 @@ from zeeguu.core.model.article_summary_context import ArticleSummaryContext from zeeguu.core.model.db import db -from zeeguu.core.model.race_safe import find_or_create +from zeeguu.core.model.race_safe import find_or_create, find_or_create_and_report from zeeguu.core.test.model_test_mixin import ModelTestMixIn from zeeguu.core.test.rules.article_rule import ArticleRule from zeeguu.core.test.rules.bookmark_rule import BookmarkRule @@ -94,3 +94,37 @@ def first_lookup_misses(query): assert self._count() == 1 session.refresh(self.article) assert self.article.title == "changed by the caller" + + def test_report_says_whether_this_call_created_the_row(self): + created_row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + assert created + + found_row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + assert found_row is created_row + assert not created + + def test_report_says_not_created_when_losing_the_race(self): + winner = self._make() + session.add(winner) + session.commit() + + real_one_or_none = Query.one_or_none + lookups = [] + + def first_lookup_misses(query): + lookups.append(query) + if len(lookups) == 1: + return None + return real_one_or_none(query) + + with mock.patch.object(Query, "one_or_none", first_lookup_misses): + row, created = find_or_create_and_report( + session, ArticleSummaryContext, self.filters, self._make + ) + + assert row.id == winner.id + assert not created From b36d22517e43802588376043e18be7fd9e71fb9e Mon Sep 17 00:00:00 2001 From: Mircea Lungu Date: Sun, 25 Oct 2026 20:40:15 +0100 Subject: [PATCH 2/2] translate_and_adapt: insert directly when the translated url is None; race_safe re-raises other-key collisions Url.find_or_create returns None for a path over 255 chars, and the "#translated-from-..." suffix can push an upload's url over. The helper would then look up `url_id IS NULL` and return an unrelated article (or raise MultipleResultsFound). With no url, insert directly as master did. race_safe: when the insert collides but no row matches `filters`, the collision was on another unique key. Re-raise that IntegrityError instead of a NoResultFound that hides it. Unchanged when the row is found. Co-Authored-By: Claude Opus 5.5 --- zeeguu/api/endpoints/article_upload.py | 20 +++++++--- .../test_article_upload_translate_race.py | 40 ++++++++++++++++++- zeeguu/core/model/race_safe.py | 17 +++++--- zeeguu/core/test/test_race_safe.py | 22 ++++++++++ 4 files changed, 86 insertions(+), 13 deletions(-) diff --git a/zeeguu/api/endpoints/article_upload.py b/zeeguu/api/endpoints/article_upload.py index 464d1cf64..fbf5470e0 100644 --- a/zeeguu/api/endpoints/article_upload.py +++ b/zeeguu/api/endpoints/article_upload.py @@ -281,12 +281,20 @@ def make_translated_article(): article.img_url = img_url return article - # A second request for the same upload (double tap, retry) can miss the - # Article.find above while the first one waits on the LLM. Both then - # insert at this url; the loser gets the winner's article back. - translated_article, created = find_or_create_and_report( - db_session, Article, dict(url=translated_url), make_translated_article - ) + if translated_url is None: + # Url.find_or_create gives None when the path is over 255 chars, and + # the "#translated-from-..." suffix can push an upload's url over. + # With no url to key on, the helper would look up `url_id IS NULL` + # and find unrelated articles; insert directly, as before. + translated_article, created = make_translated_article(), True + db_session.add(translated_article) + else: + # A second request for the same upload (double tap, retry) can miss + # the Article.find above while the first one waits on the LLM. Both + # then insert at this url; the loser gets the winner's article back. + translated_article, created = find_or_create_and_report( + db_session, Article, dict(url=translated_url), make_translated_article + ) # Fragments only for an article this request created: the loser must not # add a second set to the winner's. One commit for article and fragments, # so whoever sees the article sees its fragments too. For the loser the diff --git a/zeeguu/api/test/test_article_upload_translate_race.py b/zeeguu/api/test/test_article_upload_translate_race.py index 7c2d5b2d1..6704d47a0 100644 --- a/zeeguu/api/test/test_article_upload_translate_race.py +++ b/zeeguu/api/test/test_article_upload_translate_race.py @@ -38,10 +38,10 @@ def translate_and_adapt(self, **kwargs): return translate_and_adapt, calls -def _create_upload(client): +def _create_upload(client, url=UPLOAD_URL): upload = client.post( "/article_upload/create", - data=dict(url=UPLOAD_URL, text_content=TEXT, title="Triumphbogen"), + data=dict(url=url, text_content=TEXT, title="Triumphbogen"), ) return upload["id"] @@ -111,3 +111,39 @@ def first_article_lookup_misses(query): assert _fragment_count(winner_id) == winner_fragments finally: _cleanup(upload_id) + + +def test_an_upload_whose_translated_url_is_too_long_gets_its_own_article( + logged_in_client, +): + # Url.find_or_create gives None for a path over 255 chars. These paths + # fit, but not with the "#translated-from-de-to-de-A2" suffix, so both + # translated articles get no url. Each must still get its own article, + # not the other one (an `url_id IS NULL` lookup would find it). + def long_url(n): + path = f"/race/{n}-" + "x" * 240 + assert len(path) <= 255 + return "https://example.com" + path + + upload_ids = [_create_upload(logged_in_client, long_url(n)) for n in (1, 2)] + translate_and_adapt, llm_calls = _llm_output() + + with mock.patch( + "zeeguu.core.llm_services.simplification_service." + "SimplificationService.translate_and_adapt", + translate_and_adapt, + ): + try: + responses = [_translate(logged_in_client, u) for u in upload_ids] + + assert [r.status_code for r in responses] == [200, 200] + first, second = [r.get_json() for r in responses] + assert first["id"] != second["id"] + assert second["title"] == "Triumphbogen 2" + for upload_id in upload_ids: + [article] = _articles_from(upload_id) + assert article.url is None + assert _fragment_count(article.id) > 0 + finally: + for upload_id in upload_ids: + _cleanup(upload_id) diff --git a/zeeguu/core/model/race_safe.py b/zeeguu/core/model/race_safe.py index cdbcce956..7159579a1 100644 --- a/zeeguu/core/model/race_safe.py +++ b/zeeguu/core/model/race_safe.py @@ -44,14 +44,21 @@ def find_or_create_and_report(session, cls, filters: dict, make, commit=False): # A locking read (SELECT ... LOCK IN SHARE MODE). Under REPEATABLE # READ a plain SELECT would reuse this transaction's snapshot, taken # at its first read, before the winner committed: it would find - # nothing and .one() would raise NoResultFound. A locking read sees - # the latest committed row. (SQLite ignores it; tests don't race.) - return ( + # nothing. A locking read sees the latest committed row. (SQLite + # ignores it; tests don't race.) + winner = ( session.query(cls) .filter_by(**filters) .with_for_update(read=True) - .one() - ), False + .one_or_none() + ) + if winner is None: + # No row matches `filters`, so the insert collided on some other + # unique key: there is no winner to return. Re-raise the + # IntegrityError, which names that key, rather than hide it + # behind a NoResultFound. + raise + return winner, False if commit: session.commit() diff --git a/zeeguu/core/test/test_race_safe.py b/zeeguu/core/test/test_race_safe.py index c783f9b9a..2dedb652f 100644 --- a/zeeguu/core/test/test_race_safe.py +++ b/zeeguu/core/test/test_race_safe.py @@ -1,5 +1,8 @@ from unittest import mock +import pytest + +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Query from zeeguu.core.model.article_summary_context import ArticleSummaryContext @@ -128,3 +131,22 @@ def first_lookup_misses(query): assert row.id == winner.id assert not created + + def test_collision_on_another_unique_key_raises_the_integrity_error(self): + # The row make() builds collides on (bookmark, article), but the + # filters don't cover that key, so there is no winner to return. + existing = self._make() + session.add(existing) + session.commit() + + self.article.title = "changed by the caller" + + with pytest.raises(IntegrityError): + find_or_create( + session, ArticleSummaryContext, dict(id=existing.id + 1000), self._make + ) + session.commit() + + assert self._count() == 1 + session.refresh(self.article) + assert self.article.title == "changed by the caller"