Repository navigation
translate_and_adapt: no 500 when two requests create the same article - #909
Merged
Merged
Conversation
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 <noreply@anthropic.com>
… 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

1. What happened
FMD alert, 22 Oct 11:24:04:
A user asked Zeeguu to translate and adapt an uploaded article ("Bau von Trumps 76-Meter-Triumphbogen ..."). Two requests for the same upload arrived close together, most likely a double tap or a retry while the first one was still waiting for the LLM. Both requests called the LLM. The first one saved the translated article. The second one tried to save a second article at the same URL, the database refused it (
article.url_idis unique), and the user got a 500.2. Why it happened
POST /article_upload/<id>/translate_and_adapt,zeeguu/api/endpoints/article_upload.pyon master:Url.find_or_create,Source.find_or_create, buildArticle(...), thenThe check in step 2 and the insert in step 4 are seconds apart because of the LLM call. Request A and request B both do step 2 before either of them reaches step 4, so both see "no article yet". A inserts and commits. B then inserts a second article with the same
url_id, and the unique key rejects it.3. The fix
3a. The insert goes through
race_safe(endpoint)Step 4 now uses the shared race-safe helper from #897 instead of
add+commit:SELECTs the article attranslated_url. If it is already there, it returns it.make_translated_article()inside a savepoint.IntegrityError(the other request won), it rolls back only that savepoint and fetches the winner's article with a locking read (LOCK IN SHARE MODE). A plain re-read would not see it under REPEATABLE READ (see Share one race-safe find_or_create helper (locking re-read) across 15 models #897).So the loser now gets the winner's article and continues as if it had found it in step 2: personal copy,
user_article_info, 200.The LLM call stays where it was, before the insert. Both requests still pay for the LLM (see follow-ups); the goal here is only to get rid of the 500.
make_translated_article()contains exactly what the old code did to build the article (cefr_level,source_upload_id,img_url). The one thing that moved isUrl.find_or_create(db_session, upload.image_url): it now runs before the helper rather than inside the builder, becauseUrl.find_or_createcommits, and a commit must not happen inside the helper's savepoint.3b. Fragments: only for an article this request created
If the loser also ran
create_article_fragments, it would add a second set of fragments to the winner's article, made from its own (different) LLM text. So fragments must be created only when this request inserted the article.The helper did not say whether it created the row. I added a small function next to it,
find_or_create_and_report, which returns(row, created).find_or_createis now a two-line wrapper around it and still returns just the row:The endpoint then does:
Why this and not a check in the endpoint such as "does the article already have fragments?": that check would itself be racy. On master the winner commits the article first and only then creates the fragments and commits again. A loser arriving in between would see an article with no fragments yet and create its own set. Only the insert itself knows who won.
3c. Article and fragments in one commit
On master the article and its fragments were two commits (lines 282 and 285). Now they are one, after the
if created:block. There are two reasons:user_article_info(..., with_content=True)would then tokenize zero fragments and write that into the tokenization cache. The commit starts a fresh snapshot, so the loser sees the fragments. On local MySQL (below): before its commit the loser saw 0 fragments, after the commit it saw 2.A side effect: if
create_article_fragmentsraises, the article is no longer left committed without fragments. Nothing is saved.4. Why this is safe
race_safe.find_or_createare unchanged.find_or_createhas the same signature and return value. Its body is the same code, now living infind_or_create_and_report: the only differences are the, False/, Trueadded to the threereturns, and the wrapper drops that flag.commit=Truestill commits only a row it created. The 5 existing helper tests pass unchanged.img_urlis looked up a few lines earlier.Url.find_or_createandSource.find_or_createin this path. I checked whether they can fail in the same race:Url.find_or_create(db_session, translated_url_key)can collide on its unique(path, domain)key when both requests create the URL. It catches theIntegrityError, doessession.rollback()and reads the row again. The rollback also starts a fresh snapshot, so the re-read finds the winner's URL. This endpoint has nothing pending at that point, so the full rollback loses nothing. The alert confirms this: the loser got past it and failed on the article insert, with the sharedurl_id6974326. Not broken here, so I left it alone.Source.find_or_create(db_session, result["content"], ...): the two LLM outputs differ, so each request creates its own source; thesourcetable has no unique key that they would share. Not broken here.Url.find_or_create(db_session, upload.image_url): both requests find the same existing URL, and it collides the same way as above if not.article. The model also declaresimg_url_idandsource_idasunique=True. If prod has those keys and an insert collides on one of them (rather than onurl_id), this PR does not recover it: no article matches theurl, so there is no winner to return. That was a 500 on master too. With this PR it surfaces as the originalIntegrityErrornaming the key (see 6b), not a misleadingNoResultFound. Whether prod has these keys at all is unknown; check with:5. How it was tested
New endpoint test,
zeeguu/api/test/test_article_upload_translate_race.py. The LLM is mocked and returns a different text on each call, as the real one does. The first request creates the article. The second request is made to miss both lookups (step 2'sArticle.find, and the helper's firstSELECT), as on MySQL where it reads an older snapshot. So it really calls the LLM and really inserts at the same URL. It checks that:On master this test fails with
UNIQUE constraint failed: article.url_id, the same error as the alert.Two new helper tests in
zeeguu/core/test/test_race_safe.py:createdis True when the call inserts, False when it finds the row, and False when it loses the race.Local MySQL (
zeeguu_test, REPEATABLE-READ), a scratch script with two sessions. Request A takes its snapshot; request B creates the article plus fragments at the translated URL and commits; then A runs the insert step. All rows created (url, domain, sources, texts, articles, fragments) were deleted afterwards.IntegrityError (1062, "Duplicate entry '6703913' for key 'article.url_id'")helper returned article 4828048 created: False,fragments A sees: before its commit 0, after 2,articles at url: 1Full suite: 1343 passed, 18 skipped (20 subtests passed).
6. Review fixes (second commit)
6a. Translated URL too long →
None(was a regression in the first commit)Url.find_or_createreturnsNonewhen the path is over 255 chars (url.py:99-101).#translated-from-<src>-to-<target>-<level>suffix adds about 28 chars, so an upload whose path is 228–255 chars getstranslated_url = None.Article(None, ...), and that worked. The first commit of this PR instead called the helper withdict(url=None). That runsurl_id IS NULL ... one_or_none(), which either returns an unrelated article with no url (created=False: no fragments, and the user gets a personal copy of someone else's article) or raisesMultipleResultsFound(500).translated_urlisNone, skip the helper and insert directly, withcreated = True, as master did. Such articles get no race protection, the same as on master.test_an_upload_whose_translated_url_is_too_long_gets_its_own_articletranslates two different uploads with 250-char paths. Each must get its own article, with fragments. Without the fix, the second request got the first one's article.6b.
race_safe: a collision on another unique key re-raises the original errorIntegrityErrorfrom any unique key, and then looks the winner up byfiltersonly.filtersdoesn't cover, no row matches, and.one()raisedNoResultFound, which hid the real constraint..one_or_none(). If it finds nothing, the helper re-raises the originalIntegrityError. The savepoint has already undone only this insert, so the caller's session is as it was before the call.MultipleResultsFoundbehaves as before.test_collision_on_another_unique_key_raises_the_integrity_errorcollides on(bookmark, article)while filtering by anidthat matches nothing. It expects anIntegrityErrorand checks that the caller's pending change survives.Tests after the review fixes
How to verify in prod
All read-only.
How often it happened. API logs (journalctl keeps about 9 days):
The second command counts every
url_idduplicate, not only this endpoint's, so read it next to the first. In FMD, endpointendpoints.article_upload_translate_and_adaptshows its 500s for a longer period.The article from the alert (it was created by the winner, so it should be there once):
No translated article with a doubled set of fragments (there should be none from this path before or after; a doubled set would show up as two fragments with the same
order):After deploy: the two
journalctllines above should stop finding new hits for this endpoint, and no new FMD alert should come withDuplicate entry ... for key 'url_id'fromarticle_upload_translate_and_adapt.Follow-ups (not in this PR)
Source.find_or_createcommits asource(+source_text) for its own LLM text, which no article then uses. That's one orphan row pair per lost race.PersonalCopy.exists_forbefore either inserts. The model declares no unique key on(user_id, article_id), so that would add a second row rather than a 500. I haven't checked prod for such pairs.POST /translate_and_adapt_articleinzeeguu/api/endpoints/article.py(~lines 614–695) callstranslate_and_adapt, then doesadd+commit(690–691) and a separate fragments commit (694–695). It should get its own PR, ideally sharing one "create translated article" function with this endpoint.create_article_fragmentsnow runs in the same transaction as the insert, which reads with the snapshot taken before the LLM call.NewText.find_or_create(commit=False)can therefore miss a paragraph text that another request committed in the meantime. It then inserts a duplicatecontent_hash, and the whole translation is rolled back (500). Before, the fragments ran after a commit, with a fresher snapshot. This needs two different articles containing an identical new paragraph within seconds of each other, so it is rare.Url.find_or_createstill uses the older commit + fullrollback()retry loop. It works for this endpoint, but it would throw away pending work in a caller that has some. It could move ontorace_safein a separate PR.🤖 Generated with Claude Code