Stop reporting a failure that a later import disproved

lib/spatial_features/has_spatial_features/feature_import.rb

@@ -58,6 +58,19 @@ def update_features!(skip_invalid: false, allow_blank: false, force: false, **op
         end
       end
 
+      # This import produced geometry, so a failure recorded before it no longer describes
+      # the record. `SpatialProcessingJob`'s hooks are the only thing that records one, and
+      # an import run outside a job never reaches them.
+      #
+      # Conditioned on features rather than on the import returning, because `allow_blank`
+      # accepts an import that produced none — clearing there would retire the failure on a
+      # record that still has no geometry. Written directly rather than through
+      # `#clear_feature_update_error_status`, whose `with_lock` refuses a record holding
+      # unsaved changes and would turn this successful import into an `ImportError`.
+      if persisted? && features? && updating_features_failed?
+        QueuedSpatialProcessing.update_cached_status(self, 'update_features!', nil)
+      end
+
       return true
     rescue StandardError => e
       raise e if e.is_a?(EmptyImportError)

lib/spatial_features/has_spatial_features/queued_spatial_processing.rb

@@ -4,20 +4,26 @@ module QueuedSpatialProcessing
     mattr_accessor :priority_offset, default: 0 # Offsets the queued priority of spatial tasks. Lower numbers run with higher priority
 
     class_methods do
-      # Records whose most recent feature import failed. `->>` yields NULL for a missing
-      # key, so a record that has never been imported is not included.
+      # Returns the records whose most recent queued feature update failed.
+      #
+      # @return [ActiveRecord::Relation] empty when the model has no
+      #   `spatial_processing_status_cache` column. `->>` yields NULL for a missing key, so
+      #   a record that has never been imported is excluded.
       def with_failed_feature_updates
+        return none unless column_names.include?('spatial_processing_status_cache')
+
         where("spatial_processing_status_cache->>'update_features!' = 'failure'")
       end
 
-      # Re-import everything whose last attempt failed. Nothing else retries these, so a
-      # record broken by a defect stays broken after the defect is fixed unless something
-      # sweeps it up.
+      # Queues a feature update for every record whose most recent queued update failed.
+      #
+      # Queuing returns immediately, so a caller does not wait on the imports. Each import
+      # then runs through `SpatialProcessingJob`, whose hooks record the outcome, including
+      # for a record whose sources are unchanged and whose import is a no-op.
       #
-      # Queued rather than run inline, so a caller (a deploy migration, a console) doesn't
-      # block on reimporting shapefiles, and so `SpatialProcessingJob`'s callbacks maintain
-      # `spatial_processing_status_cache` — calling `#update_features!` directly bypasses
-      # them and leaves a record flagged as failed even when the retry succeeded.
+      # @param options [Hash] passed to `#delay_update_features!`, so `:priority` sets the
+      #   queue priority and the rest reach `#update_features!`.
+      # @return [void]
       def retry_failed_feature_updates!(**options)
         with_failed_feature_updates.find_each do |record|
           record.delay_update_features!(**options)
@@ -63,6 +69,18 @@ def clear_feature_update_error_status
       end
     end
 
+    # Returns true when the most recent queued feature update failed.
+    #
+    # Reports the state of the queued update rather than the outcome of the last import.
+    # `SpatialProcessingJob`'s hooks are what record a failure, so an import run outside a job
+    # never records one. Such an import does clear a failure recorded before it, if it
+    # produced geometry.
+    #
+    # @return [Boolean] false when the model has no `spatial_processing_status_cache` column,
+    #   when no update has been queued, and when the last queued update succeeded or is
+    #   still running.
+    # @note A record can hold usable features and still answer true, so a caller asking
+    #   whether the geometry is usable wants `#features?` as well.
     def updating_features_failed?
       spatial_processing_status(:update_features!) == :failure
     end

lib/spatial_features/version.rb

@@ -1,3 +1,3 @@
 module SpatialFeatures
-  VERSION = "3.11.1"
+  VERSION = "3.11.2"
 end

spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb

@@ -486,6 +486,71 @@ def test_files
       end
     end
 
+    # Inline imports don't run the job hooks that maintain the status, so without this a
+    # record repaired by an inline import (a nightly rake task, a console) reads as broken
+    # forever — and is picked up again by every later `::retry_failed_feature_updates!`.
+    context 'when a record with a recorded failure imports successfully' do
+      subject do
+        # `title` is a column the import never writes, so it stays dirty across the import.
+        new_dummy_class(:spatial_processing_status_cache => :jsonb, :title => :string) do
+          has_spatial_features :import => { :test_files => :File }
+
+          def test_files
+            [fixture_file_path("shapefile.zip")]
+          end
+        end.create
+      end
+
+      before do
+        SpatialFeatures::QueuedSpatialProcessing.update_cached_status(subject, 'update_features!', 'failure')
+      end
+
+      it 'stops reporting the stale failure' do
+        expect { subject.update_features!(:force => true) }
+          .to change { subject.updating_features_failed? }
+          .from(true).to(false)
+      end
+
+      it 'is no longer selected for retry' do
+        subject.update_features!(:force => true)
+        expect(subject.class.with_failed_feature_updates).not_to include(subject)
+      end
+
+      # `#clear_feature_update_error_status` locks the row, and `lock!` refuses a record
+      # holding unsaved changes. Reaching it from here would report a successful import as a
+      # failure, and would do it only for records that had already failed.
+      it 'succeeds when the record holds unsaved changes' do
+        subject.title = 'unsaved'
+
+        expect { subject.update_features!(:force => true) }.not_to raise_error
+      end
+    end
+
+    # `allow_blank` accepts an import that produced nothing, so the import completes. The
+    # record still has no geometry, and the nightly geometry task passes this option over
+    # every record, so clearing here would retire failures wholesale.
+    context 'when an import completes without producing features' do
+      subject do
+        new_dummy_class(:spatial_processing_status_cache => :jsonb) do
+          has_spatial_features :import => { :test_files => :File }
+
+          def test_files
+            [fixture_file_path("archive_without_any_known_file.zip")]
+          end
+        end.create
+      end
+
+      before do
+        SpatialFeatures::QueuedSpatialProcessing.update_cached_status(subject, 'update_features!', 'failure')
+      end
+
+      it 'keeps reporting the failure' do
+        expect { subject.update_features!(:force => true, :allow_blank => true) }
+          .not_to change { subject.reload.updating_features_failed? }
+          .from(true)
+      end
+    end
+
     context 'when every source file is unreadable' do
       subject do
         new_dummy_class(:spatial_processing_status_cache => :jsonb) do

spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb

@@ -65,6 +65,16 @@ def status!(record, state)
       expect { klass.retry_failed_feature_updates! }.not_to change { Delayed::Job.count }
     end
 
+    # Every other method in this file guards on the column. Without it the scope raises
+    # `PG::UndefinedColumn` on a spatial model that does not carry one.
+    it 'finds nothing on a model with no status column' do
+      plain = new_dummy_class
+      plain.create
+
+      expect { plain.with_failed_feature_updates.to_a }.not_to raise_error
+      expect(plain.with_failed_feature_updates).to be_empty
+    end
+
     it 'passes options through to the queued job' do
       klass.create.tap {|record| status!(record, 'failure') }