1204047 - After sync_projects, approved translations get unapproved

Status: RESOLVED FIXED

(Webtools Graveyard :: Pontoon, defect)

Description

[Matjaz Horvat [:mathjazz]]

You either approve unapproved string or submit approved string. After sync_projects, the string gets unapproved. But not always and not all strings. So far we were only able to reproduce this for .lang files, more specifically in Mozilla.org project.

Happened to me in /privacy/principles.lang:
https://bugzilla.mozilla.org/show_bug.cgi?id=1203057#c1

Now cs is reporting the same problem in the same file and also in /firefox/tracking-protection-tour.lang
https://groups.google.com/forum/#!topic/mozilla.dev.l10n/DPde63fb66g

They left them unapproved for us to debug.

Comment 1

[Osmose [:osmose, :mkelly]]

So I'm not 100% sure the reason behind this, but one possible cause that I'm able to replicate is a race condition in sync_projects:

- sync_projects loops through all the entities in a project and builds a ChangeSet for the changes it needs to make.
- Entity "Blah" is checked the the sync sees no changes to it, so it marks "Blah" for updating from VCS.
- Before the ChangeSet is executed, but after "Blah" has been added to the changeset, a user submits an approved translation.
- sync_projects executes the ChangeSet, which marks the approved translation as unapproved because it doesn't match what is in VCS.

Without anything better to go on, I think trying to eliminate this race condition and checking to see if we get any more reports of unapproved strings once it gets deployed is the way to go.

Comment 2

[Matjaz Horvat [:mathjazz]]

I wouldn't bet on that, given how many many times the bug occured lately, but it's worth a try.

It would be interesting to check under what conditions sync_projects even unapproves approved strings.

Comment 3

[Matjaz Horvat [:mathjazz]]

Just happened again, this time here:
https://pontoon.mozilla.org/sl/mozillaorg/mozorg/404.lang/

Indeed, I was translating during the sync cycle.

Comment 4

[Osmose [:osmose, :mkelly]]

(In reply to Matjaz Horvat [:mathjazz] from comment #2)
> I wouldn't bet on that, given how many many times the bug occured lately,
> but it's worth a try.
> 
> It would be interesting to check under what conditions sync_projects even
> unapproves approved strings.

Only place it unapproves strings is when it's unapproving other strings after updating from VCS.

Comment 5

[Osmose [:osmose, :mkelly]]

I talked at length with ErikRose about some ideas to handle the race condition, and we settle on an idea using queues:

- When users submit translations, instead of directly updating the database, we add an entry in a queue (like RabbitMQ or something) for updating the database with that translation. Meanwhile locally the user sees the updated translation.

- A worker pool takes jobs from the queue and updates the database with the new translations.

- When it's time to sync, the cronjob that currently runs the job instead adds an entry to the queue for sync.

- When a worker takes the sync job, it somehow notifies the other workers to stop processing new translations until the sync is done. If we are able to parallelize the project syncs like we plan to eventually, we could even mark only the project currently being synced as closed for new translations.

- Once sync is complete, workers start accepting new translations again. Because the translations did not land mid-sync, the race condition is avoided, but the translations still eventually apply.

There's a few upsides and downsides here:

- Upside: Requests to submit translations become possibly faster because the user isn't waiting on the database before they get a response.

- Upside: Utilizes a queuing system that we were already planning on adding anyway.

- Downside: If a user submits a translation and then refreshes the page before a worker gets to their job, the translation won't appear to have updated. With some extra JS work, we can make this less likely by locally caching translations in something like localStorage, but that may be overkill.

Thoughts?

Comment 6

[Osmose [:osmose, :mkelly]]

mathjazz and I agree on attempting the solution above, but after spending some time researching and implementing it I've decided to find another solution, as I ran into issues with the queue setup that I wanted to use (RabbitMQ and Celery) and being able to control the queue of tasks as described above.

Instead, my solution is to only unapprove translations that occur before sync starts. This introduces the possibility of having multiple approved translations for a single entity. To avoid that, we can wrap the sync in a transaction (which it should be wrapped in anyway) and, as the last step before committing the transaction, search for all translations for entities with multiple approvals, and unapprove all but the translations with the latest approval date. This effectively means that an approval outside of sync that happens during sync will override an approval coming from the sync itself such that, at the next sync, the proper translation will be committed to VCS successfully.

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/pontoon

https://github.com/mozilla/pontoon/commit/27e7182f51ea5cf7be273f9e0f8ca47bd1d56c62
Fix bug 1204047: Avoid race conditions from translations during sync.

When updating from VCS, only unapprove translations that existed before sync
started. This might cause multiple approved translations, so remove duplicate
approvals before committing the sync transaction.

Comment 8

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Happened again :( Two strings I've translated changed state to Suggested instead of being pushed to the repository. This time in https://pontoon.mozilla.org/cs/mozillaorg/teach/smarton/tracking.lang/

Comment 9

[Matjaz Horvat [:mathjazz]]

Michal, we're tracking this here now, it's a different bug but with same consequences:
https://bugzilla.mozilla.org/show_bug.cgi?id=1214670