Skip to content

Commit bac7ef4

Browse files
fix(nimbus): prevent kinto preview sync from clobbering concurrent user state transitions (#15619)
Because * The kinto preview sync task hydrated experiments in memory across slow Kinto network calls, then called experiment.save(), clobbering any concurrent write — most visibly a Request Launch on a Preview experiment, which appeared to succeed in the HTMX response but reverted on refresh * All three sections (push, expire, unpublish) had the same lost-update shape This commit * Replaces experiment.save() with conditional QuerySet.update() in all three sections, gating each write on the status the task observed and writing only the columns it owns * Adds regression tests for the push and expiry races Fixes #15618
1 parent 6b66976 commit bac7ef4

2 files changed

Lines changed: 95 additions & 12 deletions

File tree

experimenter/experimenter/kinto/tasks.py

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -505,10 +505,22 @@ def nimbus_synchronize_preview_experiments_in_kinto():
505505
kinto_client.create_record(data)
506506

507507
with transaction.atomic():
508-
experiment.published_dto = data
509-
experiment.published_date = timezone.now()
510-
experiment.save()
508+
updated = NimbusExperiment.objects.filter(
509+
pk=experiment.pk,
510+
status=NimbusExperiment.Status.PREVIEW,
511+
).update(
512+
published_dto=data,
513+
published_date=timezone.now(),
514+
)
515+
516+
if not updated:
517+
logger.info(
518+
f"{experiment.slug} left Preview during push; "
519+
f"next sync will reconcile Kinto"
520+
)
521+
continue
511522

523+
experiment.refresh_from_db()
512524
generate_nimbus_changelog(
513525
experiment,
514526
get_kinto_user(),
@@ -525,11 +537,20 @@ def nimbus_synchronize_preview_experiments_in_kinto():
525537

526538
for experiment in expired_experiments:
527539
with transaction.atomic():
528-
experiment.status = NimbusExperiment.Status.DRAFT
529-
experiment.publish_status = NimbusExperiment.PublishStatus.IDLE
530-
experiment.published_date = None
531-
experiment.save()
540+
updated = NimbusExperiment.objects.filter(
541+
pk=experiment.pk,
542+
status=NimbusExperiment.Status.PREVIEW,
543+
).update(
544+
status=NimbusExperiment.Status.DRAFT,
545+
publish_status=NimbusExperiment.PublishStatus.IDLE,
546+
published_date=None,
547+
)
548+
549+
if not updated:
550+
logger.info(f"{experiment.slug} left Preview before expiry; skipping")
551+
continue
532552

553+
experiment.refresh_from_db()
533554
generate_nimbus_changelog(
534555
experiment,
535556
get_kinto_user(),
@@ -545,12 +566,15 @@ def nimbus_synchronize_preview_experiments_in_kinto():
545566
kinto_client.delete_record(experiment.slug)
546567

547568
with transaction.atomic():
548-
experiment.refresh_from_db()
549-
if experiment.status == NimbusExperiment.Status.DRAFT:
550-
experiment.published_date = None
551-
experiment.published_dto = None
552-
experiment.save()
569+
NimbusExperiment.objects.filter(
570+
pk=experiment.pk,
571+
status=NimbusExperiment.Status.DRAFT,
572+
).update(
573+
published_date=None,
574+
published_dto=None,
575+
)
553576

577+
experiment.refresh_from_db()
554578
generate_nimbus_changelog(
555579
experiment,
556580
get_kinto_user(),

experimenter/experimenter/kinto/tests/test_tasks.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1666,6 +1666,65 @@ def test_reraises_exception(self):
16661666
with self.assertRaises(Exception):
16671667
tasks.nimbus_synchronize_preview_experiments_in_kinto()
16681668

1669+
def test_does_not_revert_user_transition_during_concurrent_preview_push(self):
1670+
experiment = NimbusExperimentFactory.create_with_lifecycle(
1671+
NimbusExperimentFactory.Lifecycles.PREVIEW,
1672+
published_date=None,
1673+
application=NimbusExperiment.Application.DESKTOP,
1674+
)
1675+
self.setup_kinto_get_main_records([])
1676+
1677+
def simulate_user_request_launch(*args, **kwargs):
1678+
NimbusExperiment.objects.filter(pk=experiment.pk).update(
1679+
status=NimbusExperiment.Status.DRAFT,
1680+
status_next=NimbusExperiment.Status.LIVE,
1681+
publish_status=NimbusExperiment.PublishStatus.REVIEW,
1682+
)
1683+
1684+
self.mock_kinto_client.create_record.side_effect = simulate_user_request_launch
1685+
1686+
tasks.nimbus_synchronize_preview_experiments_in_kinto()
1687+
1688+
experiment.refresh_from_db()
1689+
self.assertEqual(experiment.status, NimbusExperiment.Status.DRAFT)
1690+
self.assertEqual(experiment.status_next, NimbusExperiment.Status.LIVE)
1691+
self.assertEqual(experiment.publish_status, NimbusExperiment.PublishStatus.REVIEW)
1692+
self.assertFalse(
1693+
experiment.changes.filter(
1694+
message=NimbusChangeLog.Messages.PUSHED_TO_PREVIEW,
1695+
changed_by__email=settings.KINTO_DEFAULT_CHANGELOG_USER,
1696+
).exists()
1697+
)
1698+
1699+
def test_does_not_expire_experiment_no_longer_in_preview(self):
1700+
experiment = NimbusExperimentFactory.create_with_lifecycle(
1701+
NimbusExperimentFactory.Lifecycles.PREVIEW,
1702+
published_date=timezone.now(),
1703+
application=NimbusExperiment.Application.DESKTOP,
1704+
)
1705+
1706+
NimbusExperiment.objects.filter(id=experiment.id).update(
1707+
_updated_date_time=timezone.now() - timezone.timedelta(days=31),
1708+
status=NimbusExperiment.Status.DRAFT,
1709+
status_next=NimbusExperiment.Status.LIVE,
1710+
publish_status=NimbusExperiment.PublishStatus.REVIEW,
1711+
)
1712+
1713+
self.setup_kinto_get_main_records([experiment.slug])
1714+
1715+
tasks.nimbus_synchronize_preview_experiments_in_kinto()
1716+
1717+
experiment.refresh_from_db()
1718+
self.assertEqual(experiment.status, NimbusExperiment.Status.DRAFT)
1719+
self.assertEqual(experiment.status_next, NimbusExperiment.Status.LIVE)
1720+
self.assertEqual(experiment.publish_status, NimbusExperiment.PublishStatus.REVIEW)
1721+
self.assertFalse(
1722+
experiment.changes.filter(
1723+
message=NimbusChangeLog.Messages.EXPIRED_FROM_PREVIEW,
1724+
changed_by__email=settings.KINTO_DEFAULT_CHANGELOG_USER,
1725+
).exists()
1726+
)
1727+
16691728

16701729
class TestNimbusSendEmails(MockKintoClientMixin, TestCase):
16711730
def setUp(self):

0 commit comments

Comments
 (0)