Fixes #5: Prevent repairs from reactivating older revisions

docs/reference/runtime.md

@@ -175,7 +175,9 @@ Use **Repair Scripts** on the Project page to synchronize its published rows
 with the active revision. It restores missing or changed rows and reports the
 number changed. Matching rows are left alone, with no write or change log.
 The Project-level and revision-level **Activate** routes refuse a revision
-that is already active, so use **Repair Scripts** for this operation.
+that is already active, so use **Repair Scripts** for this operation. If
+another activation or a deactivation changes the active revision while a
+repair is in progress, the repair is refused and changes nothing.
 
 Repairs made through a request are recorded in the change log and queue update
 events attributed to the requesting user. Background activation and repair do

netbox_scripts/activation.py

@@ -56,7 +56,7 @@ class ActivationResult(NamedTuple):
     scripts: ScriptSyncResult
 
 
-def activate_revision(revision, *, automatic=False):
+def activate_revision(revision, *, automatic=False, repair=False):
     """
     Make one revision active and publish its Scripts in a single transaction.
 
@@ -66,6 +66,9 @@ def activate_revision(revision, *, automatic=False):
     built from. Raises ActivationError, including for a snapshot no build could have produced,
     and RevisionCorruptError when the stored tree no longer matches its manifest.
 
+    With repair, the revision is republished only while its project still serves it, and
+    ActivationError is raised otherwise.
+
     Return an ActivationResult, or None when an automatic request is no longer current or permitted.
     """
     _validated_records(revision)
@@ -74,6 +77,9 @@ def activate_revision(revision, *, automatic=False):
     written = []
 
     def on_promote(*, project, revision, using):
+        # Checked against the locked row, because the caller read the revision in force before the lock.
+        if repair and project.active_revision_id != revision.pk:
+            raise ActivationError(_('Revision {pk} is not the active revision of its project.').format(pk=revision.pk))
         written.append(
             synchronize_scripts(project=project, revision=revision, records=_validated_records(revision), using=using)
         )

netbox_scripts/migration/cutover.py

@@ -261,7 +261,7 @@ def _activate_project(project):
             return _outcome(project, newest.pk, outcome)
         revision, outcome = candidate, 'activated'
     try:
-        activation.activate_revision(revision)
+        activation.activate_revision(revision, repair=revision.pk == project.active_revision_id)
     except _ACTIVATION_FAILURES as error:
         return _outcome(project, revision.pk, f'could not be activated: {error}')
     return _outcome(project, revision.pk, outcome)

netbox_scripts/tests/migration/test_activation.py

@@ -178,6 +178,16 @@ def test_a_serving_project_moves_to_a_newer_valid_source(self):
         self.assertEqual(project.active_revision_id, latest.pk)
         self.assertEqual(self.outcome(results, project), 'activated')
 
+    def test_the_pass_does_not_revert_an_activation_made_after_its_read(self):
+        stale = self.serving_project()
+        latest = self.staged(stale, {'latest.py': b'VALUE = 2\n'}, RevisionStatusChoices.VALID)
+        activation.activate_revision(latest)
+
+        outcome = cutover._activate_project(stale)
+
+        self.assertEqual(ScriptProject.objects.get(pk=stale.pk).active_revision_id, latest.pk)
+        self.assertTrue(outcome['outcome'].startswith('could not be activated: '))
+
     def test_a_second_run_over_fewer_modules_keeps_the_whole_record(self):
         # record_step assigns, so replacing the list would shrink what verification then checks.
         self.stage_and_validate()

netbox_scripts/tests/test_activation.py

@@ -251,6 +251,18 @@ def valid_with(self, records, marker=1):
 
 
 class ActivateRevisionTestCase(ActivationMixin, TestCase):
+    def overtaken_by(self, change):
+        """Patch verification to run change once on this connection, between a promotion's unlocked and locked reads."""
+        real = store.verify_revision_tree
+        pending = [change]
+
+        def verify(*args, **kwargs):
+            if pending:
+                pending.pop()()
+            return real(*args, **kwargs)
+
+        return mock.patch.object(store, 'verify_revision_tree', side_effect=verify)
+
     def test_activation_publishes_a_row_for_every_recorded_script(self):
         revision = self.valid_with([record(), record(class_name='AuditDevices', position=1)])
         activate_revision(revision)
@@ -275,6 +287,35 @@ def test_reactivating_the_active_revision_repairs_a_lost_row(self):
         activate_revision(revision)
         self.assertEqual(NetBoxScript.objects.get().class_name, 'DeployDevices')
 
+    def test_a_repair_overtaken_by_another_activation_changes_nothing(self):
+        first = self.valid_with([record()], marker=1)
+        activate_revision(first)
+        second = self.valid_with([record(class_name='Later')], marker=2)
+
+        with (
+            self.overtaken_by(lambda: activate_revision(second)),
+            self.assertRaises(ActivationError) as captured,
+        ):
+            activate_revision(first, repair=True)
+
+        self.assertIn('is not the active revision', str(captured.exception))
+        self.assertEqual(ScriptProject.objects.get(pk=self.project.pk).active_revision_id, second.pk)
+        self.assertFalse(NetBoxScript.objects.get(class_name='Later').is_retired)
+        self.assertTrue(NetBoxScript.objects.get(class_name='DeployDevices').is_retired)
+
+    def test_a_repair_overtaken_by_a_deactivation_changes_nothing(self):
+        revision = self.valid_with([record()])
+        activate_revision(revision)
+
+        with (
+            self.overtaken_by(lambda: deactivate_revision(revision)),
+            self.assertRaises(ActivationError),
+        ):
+            activate_revision(revision, repair=True)
+
+        self.assertIsNone(ScriptProject.objects.get(pk=self.project.pk).active_revision_id)
+        self.assertTrue(NetBoxScript.objects.get().is_retired)
+
     def test_activation_imports_nothing(self):
         # Re-importing could disagree with the verdict the revision already carries.
         revision = self.valid_with([record()])

netbox_scripts/tests/views/test_projects.py

@@ -19,6 +19,7 @@
 from netbox_scripts.storage import service
 from netbox_scripts.tests.plugin_testing import ObjectPermissionTestMixin, PluginTestCases
 from netbox_scripts.ui import ScriptProjectPanel, ScriptProjectStatePanel
+from netbox_scripts.views import ScriptProjectRepairView
 from users.models import ObjectPermission
 from utilities.testing import TestCase, create_tags, create_test_user, post_data
 
@@ -801,6 +802,21 @@ def test_posting_with_nothing_wrong_says_so_instead_of_claiming_a_repair(self):
         self.assertIn('already matched its revision', self.message(response))
         self.assertNotIn('Repaired', self.message(response))
 
+    def test_posting_after_another_activation_refuses_instead_of_reverting_it(self):
+        self.grant('view', 'activate')
+        stale = ScriptProject.objects.get(pk=self.project.pk)
+        newer = service.stage_revision(self.project, {'deploy.py': b'VALUE = 2\n'}).revision
+        ScriptProjectRevision.objects.filter(pk=newer.pk).update(
+            status=RevisionStatusChoices.VALID, discovered_scripts=self.revision.discovered_scripts
+        )
+        activation.activate_revision(ScriptProjectRevision.objects.get(pk=newer.pk))
+
+        with mock.patch.object(ScriptProjectRepairView, 'get_object', return_value=stale):
+            response = self.client.post(self.url(), follow=True)
+
+        self.assertEqual(ScriptProject.objects.get(pk=self.project.pk).active_revision_id, newer.pk)
+        self.assertIn('is not the active revision', self.message(response))
+
     def test_the_change_permission_alone_is_not_enough(self):
         self.grant('view', 'change')
         self.assertHttpStatus(self.client.get(self.url()), 403)

netbox_scripts/views/projects.py

@@ -182,7 +182,7 @@ def post(self, request, **kwargs):
         """Republish, reporting how many rows moved rather than a bare success."""
         project = self.get_object(**kwargs)
         try:
-            result = activation.activate_revision(project.active_revision)
+            result = activation.activate_revision(project.active_revision, repair=True)
         except (ActivationError, RevisionCorruptError, StorageError, OSError) as error:
             # Expected refusals: the revision moved on, or its stored tree no longer matches.
             messages.error(request, _('The Scripts could not be repaired: {error}').format(error=error))