Fixes #13: Recover stalled revisions when reselecting Script Files

docs/reference/models/scriptprojectrevision.md

@@ -129,7 +129,10 @@ Later edits or disabling a declaration do not change that revision's verdict.
 
 To use the current declarations with stored source, the storage service offers
 a refresh operation. It creates or returns the revision for the same source
-digest and the current Script File digest, without another upload.
+digest and the current Script File digest, without another upload. Revisions
+for different selections share stored source, so a returned revision still in
+`staging` or `storage_failed` becomes `materialized` once the refresh has
+verified that source.
 
 A canonical builder creates the snapshot. Refresh, validation and activation
 check it before use, including its shape, paths, ordering, uniqueness and digest.
@@ -161,6 +164,7 @@ Storage, validation and activation are separate stages:
 | `staging` | `materialized` | Source was written and verified against its manifest |
 | `staging` | `storage_failed` | A retryable storage write failed |
 | `storage_failed` | `staging` | Re-staging identical content resumes the write |
+| `staging` or `storage_failed` | `materialized` | A Script File refresh verified that the same source is stored |
 | (new) | `invalid` | Content was rejected before anything was stored |
 | `materialized` | `validating` | Validation claimed the revision |
 | `validating` | `valid` or `invalid` | Validation reached a verdict |

netbox_scripts/storage/service.py

@@ -216,6 +216,7 @@ def _refresh_revision_script_files(revision):
     uploaded again. The source revision only needs a digest, so even an INVALID verdict on
     the old configuration stays untouched while its content gets a fresh candidate. Returns
     a StagedRevision whose created flag says whether this configuration was already staged.
+    A returned row whose write never completed is settled as MATERIALIZED.
 
     Raises ValueError for a revision with no digest, since a rejected staging attempt stored
     no content to refresh, RevisionCorruptError when the stored tree no longer matches the
@@ -257,6 +258,12 @@ def _refresh_revision_script_files(revision):
                 'script_file_snapshot': snapshot,
             },
         )
+        if candidate.status in constants.RETRYABLE_REVISION_STATUSES:
+            # Another selection stored this tree, verified above, so the unfinished write needs no retry.
+            ScriptProjectRevision.objects.using(using).filter(
+                pk=candidate.pk, status__in=constants.RETRYABLE_REVISION_STATUSES
+            ).update(status=RevisionStatusChoices.MATERIALIZED, validation_errors=[], last_updated=timezone.now())
+            _refresh_surviving(candidate, using)
         return StagedRevision(candidate, created)
 
 

netbox_scripts/tests/storage/test_service.py

@@ -6,6 +6,7 @@
 from django.db import DEFAULT_DB_ALIAS, IntegrityError, connection, router, transaction
 from django.test import TestCase, override_settings
 from django.test.utils import CaptureQueriesContext
+from rq.timeouts import JobTimeoutException
 
 from netbox_scripts.choices import RevisionStatusChoices
 from netbox_scripts.models import ScriptFile, ScriptProject, ScriptProjectRevision
@@ -671,6 +672,13 @@ def test_an_invalid_staging_attempt_still_freezes_the_configuration(self):
 class RefreshScriptFilesTestCase(StorageServiceMixin, TestCase):
     """refresh_revision_script_files restages stored content under the current configuration."""
 
+    def stored_under_another_selection(self, files):
+        """Return the revision that stores files with hello.py selected, leaving hello.py deselected."""
+        ScriptFile.objects.update_or_create(project=self.project, source_path='hello.py', defaults={'enabled': True})
+        stored = self.materialize(files)
+        ScriptFile.objects.filter(project=self.project, source_path='hello.py').update(enabled=False)
+        return stored
+
     def test_refresh_stages_the_content_under_the_current_configuration(self):
         first = self.materialize()
         script_file = ScriptFile.objects.create(project=self.project, source_path='hello.py')
@@ -712,6 +720,56 @@ def test_refresh_leaves_the_source_verdict_untouched(self):
         self.assertEqual(first.status, RevisionStatusChoices.INVALID)
         self.assertEqual(refreshed.status, RevisionStatusChoices.MATERIALIZED)
 
+    def test_refresh_settles_a_revision_whose_write_did_not_complete(self):
+        cases = (
+            (OSError('read-only file system'), RevisionStatusChoices.STORAGE_FAILED),
+            (JobTimeoutException(), RevisionStatusChoices.STAGING),
+        )
+        for marker, (error, status) in enumerate(cases):
+            with self.subTest(status=status):
+                files = content(marker)
+                with mock.patch(WRITE_TARGET, side_effect=error), self.assertRaises(type(error)):
+                    service.stage_revision(self.project, files)
+                earlier = self.project.revisions.get(status=status)
+                stored = self.stored_under_another_selection(files)
+
+                refreshed, created = service.refresh_revision_script_files(stored)
+
+                self.assertFalse(created)
+                self.assertEqual((refreshed.pk, refreshed.status), (earlier.pk, RevisionStatusChoices.MATERIALIZED))
+                row = ScriptProjectRevision.objects.get(pk=earlier.pk)
+                self.assertEqual((row.status, row.validation_errors), (RevisionStatusChoices.MATERIALIZED, []))
+                self.assertEqual(ScriptProject.objects.get(pk=self.project.pk).source_revision_id, earlier.pk)
+
+    def test_refresh_leaves_a_revision_owned_by_validation_untouched(self):
+        errors = [{'path': 'hello.py', 'code': 'syntax_error', 'message': 'bad syntax'}]
+        for marker, status in enumerate((RevisionStatusChoices.VALIDATING, RevisionStatusChoices.INVALID)):
+            with self.subTest(status=status):
+                files = content(marker)
+                earlier = self.materialize(files)
+                ScriptProjectRevision.objects.filter(pk=earlier.pk).update(status=status, validation_errors=errors)
+                stored = self.stored_under_another_selection(files)
+
+                refreshed = service.refresh_revision_script_files(stored).revision
+
+                self.assertEqual(refreshed.pk, earlier.pk)
+                row = ScriptProjectRevision.objects.get(pk=earlier.pk)
+                self.assertEqual((row.status, row.validation_errors), (status, errors))
+
+    def test_refresh_settles_nothing_when_the_stored_tree_fails_verification(self):
+        with mock.patch(WRITE_TARGET, side_effect=OSError('read-only file system')), self.assertRaises(OSError):
+            service.stage_revision(self.project, GOOD_FILES)
+        earlier = self.project.revisions.get()
+        stored = self.stored_under_another_selection(GOOD_FILES)
+        self.overwrite(stored.digest, 'hello.py', b'tampered')
+
+        with self.assertRaises(RevisionCorruptError):
+            service.refresh_revision_script_files(stored)
+
+        row = ScriptProjectRevision.objects.get(pk=earlier.pk)
+        self.assertEqual(row.status, RevisionStatusChoices.STORAGE_FAILED)
+        self.assertEqual(row.validation_errors[0]['code'], 'storage_write_failed')
+
 
 class ActivationSnapshotTestCase(StorageServiceMixin, TestCase):
     """Activation trusts the script file snapshot only after its return-trip check."""

netbox_scripts/tests/test_reconciliation.py

@@ -17,7 +17,7 @@
     ProjectSourceTypeChoices,
     RevisionStatusChoices,
 )
-from netbox_scripts.jobs import ProjectReconciliationJob, RevisionValidationJob
+from netbox_scripts.jobs import ProjectReconciliationJob, ProjectScriptFileRefreshJob, RevisionValidationJob
 from netbox_scripts.models import (
     NetBoxScript,
     ScriptFile,
@@ -354,3 +354,25 @@ def test_an_unchanged_source_leaves_the_active_revision_alone(self):
         self.assertIn('has not changed', str(job.log_entries))
         self.assertEqual(self.project.active_revision_id, active.pk)
         self.assertEqual(ScriptProjectRevision.objects.filter(project=self.project).count(), 1)
+
+    def test_reselecting_a_file_activates_the_revision_whose_write_failed(self):
+        ScriptFile.objects.create(project=self.project, source_path='audit.py', enabled=True)
+        data_file(self.source, 'scripts/audit.py', SCRIPT.replace(b'Deploy', b'Audit'))
+        self.reconcile()
+        DataFile.objects.filter(path='scripts/deploy.py').delete()
+        data_file(self.source, 'scripts/deploy.py', SCRIPT + b'\nVALUE = 2\n')
+        with mock.patch.object(store, 'write_revision', side_effect=StorageError('the backend refused')):
+            self.assertEqual(self.run_reconciliation().status, JobStatusChoices.STATUS_FAILED)
+        failed = self.project.revisions.get(status=RevisionStatusChoices.STORAGE_FAILED)
+
+        self.project.select_script_files(['deploy.py'])
+        ProjectScriptFileRefreshJob.enqueue(immediate=True, project_id=self.project.pk)
+        # Stores the content the failed write could not.
+        self.reconcile()
+        self.assertFalse(NetBoxScript.objects.get(project=self.project, class_name='Audit').is_executable)
+        self.project.select_script_files(['deploy.py', 'audit.py'])
+        ProjectScriptFileRefreshJob.enqueue(immediate=True, project_id=self.project.pk)
+
+        self.project = ScriptProject.objects.get(pk=self.project.pk)
+        self.assertEqual((self.project.active_revision_id, self.project.source_revision_id), (failed.pk, failed.pk))
+        self.assertTrue(NetBoxScript.objects.get(project=self.project, class_name='Audit').is_executable)