From 56081f49fafb6b2f9b9511d659884fc199f8ffb1 Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Fri, 23 Oct 2026 23:18:30 +0200 Subject: [PATCH] fix(storage): Settle a reused revision whose write failed A Script Files refresh that resolves to a revision left in Staging or Storage failed now marks it Materialized once the same source verifies as stored, so the revision is accepted and validated. Fixes #13 --- .../reference/models/scriptprojectrevision.md | 6 +- netbox_scripts/storage/service.py | 7 +++ netbox_scripts/tests/storage/test_service.py | 58 +++++++++++++++++++ netbox_scripts/tests/test_reconciliation.py | 24 +++++++- 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/docs/reference/models/scriptprojectrevision.md b/docs/reference/models/scriptprojectrevision.md index 0ae27cd..5d38c41 100644 --- a/docs/reference/models/scriptprojectrevision.md +++ b/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 | diff --git a/netbox_scripts/storage/service.py b/netbox_scripts/storage/service.py index 775e752..a8db0cb 100644 --- a/netbox_scripts/storage/service.py +++ b/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) diff --git a/netbox_scripts/tests/storage/test_service.py b/netbox_scripts/tests/storage/test_service.py index 5849a6e..3598ec3 100644 --- a/netbox_scripts/tests/storage/test_service.py +++ b/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.""" diff --git a/netbox_scripts/tests/test_reconciliation.py b/netbox_scripts/tests/test_reconciliation.py index 2698233..be61039 100644 --- a/netbox_scripts/tests/test_reconciliation.py +++ b/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)