Stop reporting a failure that a later import disproved

[njakobsen]

The narrowest thing worth doing ahead of #59, which will replace this mechanism entirely.

spatial_processing_status_cache is written only by SpatialProcessingJob's hooks, so an import run inline never touches it. A record whose import failed and was later repaired inline — a nightly rake task, a console — keeps reporting a failure indefinitely, and ::retry_failed_feature_updates! picks the same records up on every run.

A successful import now clears a failure recorded before it.

Why cleared, not set to success

#update_spatial_processing_status writes nil when no job exists, and an inline import has no job. Setting :success would claim a job outcome that never happened; clearing asserts nothing and only stops the record contradicting an import that has just succeeded. It also keeps the value's vocabulary unchanged while #59 is undecided.

The first version of this fix did not work

#update_features! returns from inside its transaction block when the cache key still matches:

return if !force && features_cache_key_matches?(cache_key)

Ruby's return there exits the method, so anything after the block is skipped. The clear was placed after it and so ran only for imports that actually did work — never for an inline caller that passes no force, which is exactly the case it was written for. Models with no features_hash column never short-circuit, which is why testing against ReferralSubmission showed the fix working.

Caught by running it rather than reading it:

force: true  -> status=nil
no force     -> returned=nil status="failure" features=17

Seventeen features present and still flagged failed. The clear now runs on both paths, and the new example fails against the old placement.

Comments reconciled

Three comments in these two files disagreed about what the status column holds — ::with_failed_feature_updates called it import outcome, #updating_features_failed? called it job state, and ::retry_failed_feature_updates! still warned about the behaviour this commit changes. They now say the same thing, and #updating_features_failed? documents the gap that remains: an inline import that fails still records nothing, which needs #59 rather than another patch here.

::with_failed_feature_updates also returns none on a model without the column instead of raising, matching every other method in that file.

What this deliberately does not fix

The inverse gap above, and the reindexing of models whose has_features facet has no Glint reindex_on — Referral and Site opt into reindex_after_updating_features, but SpatialFeaturesDefaults#update_features! checks that flag inside the block that sets it false, so solr_index! is unreachable. That is a Connect Engine bug and belongs in its own issue.

Verification

bundle exec rspec — 269 examples, 0 failures (19 pending, pre-existing). Three new examples covering the clear on the import path, on the no-op path, and that the record stops being selected for retry. Verified red-to-green on each.

3.11.2 — additive, no behaviour change to any existing call.

Refs #59, https://github.com/culturecode/stolo_connect/issues/1962

🤖 Generated with Claude Code

https://claude.ai/code/session_01HLgkg1oN6dkKfLzhKyDUFt

[njakobsen]

If there were errors on the previous run, and we don't do any work on this run, does it really make sense to clear the failures?

[njakobsen]

No, and I've dropped it — b834598.

Clearing there would retire a failure on the strength of a hash comparison rather than a successful import. features_hash records which sources the last successful import used; it is not evidence that the features are still present or intact. Something that deleted features without touching the hash would leave a record that looks healthy by that test and is not.

It also buys nothing, which I should have checked before adding it. The queued path already covers the case: a job whose import short-circuits on the cache key still finishes successfully, so SpatialProcessingJob#success writes the status regardless. Verified:

after queued retry -> status="success"
still selected?     false

So the only thing the early-return branch changed was the inline path, and only for a record whose sources are unchanged since its last success but which has a failure recorded after it — which needs a forced import that failed, or a job that died without touching the data. In every one of those the right answer is to leave the flag alone until something actually does the work.

The remaining clear runs only after an import completes, and both examples still fail without it.

[njakobsen]

We're breaking our rules about having the caller explain the "why". See https://github.com/culturecode/connect_engine/blob/master/.claude/docs/code-style.md

[njakobsen]

Fixed. Dropping the early-return call left a single caller, so the wrapper is gone and the reason sits at the call site:

# This import succeeded, so a failure recorded before it no longer describes the
# record. Only `SpatialProcessingJob`'s hooks write that status, and an import run
# outside a job never reaches them.
clear_feature_update_error_status if persisted? && updating_features_failed?

The private method was also a naming hazard next to the public #clear_feature_update_error_status it delegated to.

[njakobsen]

Parked. The Homalco spatial-import work ships on 3.11.1, which does not depend on anything here — stolo_connect #1964 pins spatial_features (3.11.1) and references none of this behaviour.

Recording why, because the reason is more useful than the patch.

Four bugs in four lines of production code

Every one was the same species: another branch of #update_features! where "correct" means something different.

1. The clear sat after the transaction block, so an import that short-circuited on an unchanged cache key never reached it — missing the inline callers that pass no force, which was the case it was written for.
2. Clearing on that short-circuit was then wrong in principle: it retires a failure on the strength of a hash comparison rather than an import, and features_hash records which sources the last success used, not that the features still exist.
3. #clear_feature_update_error_status locks the row, and lock! refuses a record holding unsaved changes — so a successful import raised ImportError, and returned nil silently under skip_invalid. Only for records that had already failed, which is the population this exists to repair.
4. allow_blank: true accepts an import that produced nothing, so the clear fired on records with no geometry — blanking the engine's error banner. stolo:update_geometry passes exactly that option over every referral nightly, and this branch created the population it would hit, since unreadable files now complete blank-with-warnings instead of raising.

Why they kept coming

They are #59. The column has no defined contract, so correctness cannot be reasoned about — only enumerated, branch by branch, and the enumeration is complete by luck. Each fix satisfied the branches known at the time and was falsified by the next one.

The last version is correct as far as I can establish: 271 examples green, each new one verified to fail against its own defect and nothing else. I would still not merge it. A fifth branch is likelier than not.

All of this disappears under a per-source record of import outcome, because "is this record broken" stops being inferred from the return value of whatever last ran. Returning to #59 next, with fresh eyes on the design rather than another patch on the inference.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HLgkg1oN6dkKfLzhKyDUFt