Remove unused Item.per_page attribute

I feel like this was part of `will_paginate` back before the Rails
community had itself figured out about what belongs in a model?

But yeah, a default per-page value for search results does not belong
here. And I don't think anything references it anymore, because we pass
`per_page` to the `paginate` call in `ItemsController` explicitly! So,
goodbye!

app/models/item.rb

@@ -18,9 +18,6 @@ class Item < ApplicationRecord
   SPECIAL_COLOR_DESCRIPTION_REGEX =
     /This item is only wearable by [a-zA-Z]+ painted ([a-zA-Z]+)\.|WARNING: This [a-zA-Z]+ can be worn by ([a-zA-Z]+) [a-zA-Z]+ ONLY!|If your Neopet is not painted ([a-zA-Z]+), it will not be able to wear this item\./
 
-  cattr_reader :per_page
-  @@per_page = 30
-
   scope :newest, -> {
     order(arel_table[:created_at].desc) if arel_table[:created_at]
   }

app/models/swf_asset.rb

@@ -51,18 +51,65 @@ class SwfAsset < ApplicationRecord
   end
 
   def manifest
-    raise "manifest_url is blank" if manifest_url.blank?
+    @manifest ||= load_manifest
-    @manifest ||= NeopetsMediaArchive.load_json(manifest_url)
   end
 
-  def preload_manifest
+  def preload_manifest(save_changes: true)
-    raise "manifest_url is blank" if manifest_url.blank?
+    load_manifest(return_content: false, save_changes:)
-    NeopetsMediaArchive.preload_file(manifest_url)
+  end
+
+  def load_manifest(return_content: true, save_changes: true)
+    return nil if manifest_url.blank?
+
+    # If we recently tried loading the manifest and got a 4xx HTTP status code
+    # (e.g. a 404, there's a surprising amount of these!), don't try again. But
+    # after enough time passes, if this is called again, we will!
+    #
+    # (We always retry 5xx errors, on the assumption that they probably
+    # represent intermittent failures, whereas 4xx errors are not likely to
+    # succeed just by retrying.)
+    if manifest_loaded_at.present?
+      last_try_was_4xx =(400...500).include?(manifest_status_code)
+      last_try_was_recent = (Time.now - manifest_loaded_at) <= 1.day
+      if last_try_was_4xx and last_try_was_recent
+        Rails.logger.debug "Skipping loading manifest for asset #{id}: " +
+          "last try was status #{manifest_status_code} at #{manifest_loaded_at}"
+        return nil
+      end
+    end
+
+    begin
+      NeopetsMediaArchive.load_file(manifest_url, return_content:) =>
+        {content:, source:}
+    rescue NeopetsMediaArchive::ResponseNotOK => error
+      Rails.logger.warn "Failed to load manifest for asset #{id}: " +
+        error.message
+      self.manifest_loaded_at = Time.now
+      self.manifest_status_code = error.status
+      save! if save_changes
+      return nil
+    end
+
+    if source == "network" || manifest_loaded_at.blank?
+      self.manifest_loaded_at = Time.now
+      self.manifest_status_code = 200
+      save! if save_changes
+    end
+
+    return nil unless return_content
+
+    begin
+      JSON.parse(content)
+    rescue JSON::ParserError => error
+      Rails.logger.warn "Failed to parse manifest for asset #{id}: " +
+        error.message
+      return nil
+    end
   end
 
   MANIFEST_BASE_URL = Addressable::URI.parse("https://images.neopets.com")
   def manifest_asset_urls
-    return {} if manifest_url.nil?
+    return {} unless manifest.present?
 
     begin
       # Organize the asset URLs by file extension, convert them from paths to
@@ -88,7 +135,10 @@ class SwfAsset < ApplicationRecord
         # (There's probably only one of each! I'm just going by the same logic
         # we've seen in the JS library case, that later entries are more likely
         # to be correct.)
-        { png: assets_by_ext[:png].last, svg: assets_by_ext[:svg].last }
+        {
+          png: assets_by_ext.fetch(:png, []).last,
+          svg: assets_by_ext.fetch(:svg, []).last,
+        }
       end
     rescue StandardError => error
       Rails.logger.error "Could not read URLs from manifest: #{error.full_message}"
@@ -259,7 +309,9 @@ class SwfAsset < ApplicationRecord
       swf_assets.map do |swf_asset|
         semaphore.async do
           begin
-            swf_asset.preload_manifest
+            # Don't save changes in this big async situation; we'll do it all
+            # in one batch after, to avoid too much database concurrency!
+            swf_asset.preload_manifest(save_changes: false)
           rescue StandardError => error
             Rails.logger.error "Could not preload manifest for asset " + 
               "#{swf_asset.id} (#{swf_asset.manifest_url}): #{error.message}"
@@ -272,6 +324,10 @@ class SwfAsset < ApplicationRecord
     ensure
       barrier.stop # If something goes wrong, clean up all tasks.
     end
+
+    SwfAsset.transaction do
+      swf_assets.each(&:save!)
+    end
   end
 
   before_save do

app/services/neopets_media_archive.rb

@@ -17,11 +17,6 @@ module NeopetsMediaArchive
 
   ROOT_PATH = Pathname.new(Rails.configuration.neopets_media_archive_root)
 
-  # Load the file from the given `images.neopets.com` URI, as JSON.
-  def self.load_json(uri)
-    JSON.parse(load_file(uri))
-  end
-
   # Load the file from the given `images.neopets.com` URI.
   def self.load_file(uri, return_content: true)
     local_path = local_file_path(uri)
@@ -31,7 +26,7 @@ module NeopetsMediaArchive
       begin
         content = File.read(local_path)
         debug "Loaded source file from filesystem: #{local_path}"
-        return content
+        return {content: content, source: "filesystem"}
       rescue Errno::ENOENT
         # If it doesn't exist, that's fine: just move on and download it.
       end
@@ -42,7 +37,7 @@ module NeopetsMediaArchive
       # we're not ready to use yet.)
       if File.exist?(local_path)
         debug "Source file is already loaded, skipping: #{local_path}"
-        return
+        return {content: nil, source: "filesystem"}
       end
     end
 
@@ -53,7 +48,7 @@ module NeopetsMediaArchive
     File.write(local_path, content)
     info "Wrote source file to filesystem: #{local_path}"
     
-    return_content ? content : nil
+    {content: return_content ? content : nil, source: "network"}
   end
 
   # Load the file from the given `images.neopets.com` URI, but don't return its
@@ -78,10 +73,9 @@ module NeopetsMediaArchive
     # requests in parallel!
     Sync do
       response = INTERNET.get(uri)
-      if response.status == 404
+      if response.status != 200
-        raise NotFound, "origin server returned 404: #{uri}"
+        raise ResponseNotOK.new(response.status),
-      elsif response.status != 200
+          "expected status 200 but got #{response.status} (#{uri})"
-        raise "expected status 200 but got #{response.status} (#{uri})"
       end
       response.body.read
     end
@@ -106,7 +100,13 @@ module NeopetsMediaArchive
     ROOT_PATH + path_within_archive(uri)
   end
 
-  class NotFound < StandardError; end
+  class ResponseNotOK < StandardError
+    attr_reader :status
+    def initialize(status)
+      super
+      @status = status
+    end
+  end
 
   private
 

db/migrate/20240225231346_add_manifest_loaded_at_and_manifest_status_code_to_swf_assets.rb

@@ -0,0 +1,6 @@
+class AddManifestLoadedAtAndManifestStatusCodeToSwfAssets < ActiveRecord::Migration[7.1]
+  def change
+    add_column :swf_assets, :manifest_loaded_at, :datetime
+    add_column :swf_assets, :manifest_status_code, :integer
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_02_21_005949) do
+ActiveRecord::Schema[7.1].define(version: 2024_02_25_231346) do
   create_table "alt_styles", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.integer "species_id", null: false
     t.integer "color_id", null: false
@@ -245,6 +245,8 @@ ActiveRecord::Schema[7.1].define(version: 2024_02_21_005949) do
     t.timestamp "manifest_cached_at"
     t.string "known_glitches", limit: 128, default: ""
     t.string "manifest_url"
+    t.datetime "manifest_loaded_at"
+    t.integer "manifest_status_code"
     t.index ["body_id"], name: "swf_assets_body_id_and_object_id"
     t.index ["type", "remote_id"], name: "swf_assets_type_and_id"
     t.index ["zone_id"], name: "idx_swf_assets_zone_id"