From 776a2d29997f1201ecab66436da1d7279fbbb932 Mon Sep 17 00:00:00 2001 From: Nicholas Jakobsen Date: Mon, 3 Aug 2026 18:03:31 -0700 Subject: [PATCH] fix: Stop reporting a failure that a later import disproved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `spatial_processing_status_cache` records a failure only through `SpatialProcessingJob`'s hooks, so an import run outside a job never records one and never clears one. A record whose import failed and was later repaired inline — by a nightly rake task, or from a console — kept reporting a failure indefinitely, and `::retry_failed_feature_updates!` picked the same records up on every run. An import that produces geometry now clears a failure recorded before it. Cleared rather than set to success, matching what `#update_spatial_processing_status` writes when no job exists: it asserts nothing about a job, it only stops the record contradicting an import that just produced features. Three cases deliberately leave the status alone. An import that short-circuits on an unchanged cache key returns before doing anything, and clearing there would retire a failure on the strength of a hash comparison rather than an import; the queued path already covers it, since a job whose import short-circuits still finishes and its `success` hook writes the status. An import under `allow_blank` completes having produced nothing, and the nightly geometry task passes that option over every record, so clearing there would retire failures on records that still have no geometry. And the status is written directly rather than through `#clear_feature_update_error_status`, whose `with_lock` refuses a record holding unsaved changes — reaching it from here turned a successful import into an `ImportError`, and silently returned nil under `skip_invalid`. `::with_failed_feature_updates` returns none on a model without the column rather than raising `PG::UndefinedColumn`, matching every other method in that file. Two comments describing the status column contradicted each other and are reconciled: `::with_failed_feature_updates` described it as import outcome while `::retry_failed_feature_updates!` warned about the behaviour this commit changes. Refs https://github.com/culturecode/spatial_features/issues/59, https://github.com/culturecode/stolo_connect/issues/1962 Co-Authored-By: Claude Opus 5 (1M context) --- .../has_spatial_features/feature_import.rb | 13 ++++ .../queued_spatial_processing.rb | 36 +++++++--- lib/spatial_features/version.rb | 2 +- .../feature_import_spec.rb | 65 +++++++++++++++++++ .../queued_spatial_processing_spec.rb | 10 +++ 5 files changed, 116 insertions(+), 10 deletions(-) diff --git a/lib/spatial_features/has_spatial_features/feature_import.rb b/lib/spatial_features/has_spatial_features/feature_import.rb index 85ee814..2efa762 100644 --- a/lib/spatial_features/has_spatial_features/feature_import.rb +++ b/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) diff --git a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb b/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb index 4fc2a30..0f3f72c 100644 --- a/lib/spatial_features/has_spatial_features/queued_spatial_processing.rb +++ b/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 diff --git a/lib/spatial_features/version.rb b/lib/spatial_features/version.rb index 0d9b80a..fbfc61f 100644 --- a/lib/spatial_features/version.rb +++ b/lib/spatial_features/version.rb @@ -1,3 +1,3 @@ module SpatialFeatures - VERSION = "3.11.1" + VERSION = "3.11.2" end diff --git a/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb b/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb index 867f007..abac410 100644 --- a/spec/lib/spatial_features/has_spatial_features/feature_import_spec.rb +++ b/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 diff --git a/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb b/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb index 1e1398c..0b5428a 100644 --- a/spec/lib/spatial_features/has_spatial_features/queued_spatial_processing_spec.rb +++ b/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') }