feat: make sure purge applies to other r2 hosts

.env.example

@@ -8,7 +8,7 @@ R2_ENDPOINT=https://YOUR_ACCOUNT_ID.r2.cloudflarestorage.com
 
 # Public hostname for CDN URLs (used in generated links)
 CDN_HOST=cdn.hackclub.com
-CDN_ASSETS_HOST=cdn.hackclub-assets.com
+CDN_ASSETS_HOST=user-cdn.hackclub-assets.com
 # =============================================================================
 # Hack Club OAuth
 # =============================================================================
@@ -47,6 +47,7 @@ ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT=your_key_derivation_salt
 # Cloudflare (cache purging on delete)
 # =============================================================================
 CLOUDFLARE_ZONE_ID=your_zone_id
+CLOUDFLARE_ASSETS_ZONE_ID=your_assets_zone_id
 CLOUDFLARE_API_TOKEN=your_api_token
 
 # =============================================================================

README.md

@@ -79,6 +79,9 @@ See `.env.example` for the full list. Key variables:
 | `R2_ENDPOINT` | R2 endpoint URL |
 | `CDN_HOST` | Public hostname for CDN URLs |
 | `CDN_ASSETS_HOST` | Public R2 bucket hostname |
+| `CLOUDFLARE_ZONE_ID` | Zone containing `CDN_HOST`, for cache purging on delete |
+| `CLOUDFLARE_ASSETS_ZONE_ID` | Zone containing `CDN_ASSETS_HOST` (a *different* zone) |
+| `CLOUDFLARE_API_TOKEN` | Token with cache-purge permission on both zones |
 | `HACKCLUB_CLIENT_ID` | OAuth client ID from Hack Club Auth |
 | `HACKCLUB_CLIENT_SECRET` | OAuth client secret |
 | `LOCKBOX_MASTER_KEY` | 64-char hex key for encrypting API keys |
@@ -89,7 +92,10 @@ See `.env.example` for the full list. Key variables:
 | Domain | Points to |
 |--------|-----------|
 | `cdn.hackclub.com` | Rails app (Heroku/Fly/etc.) |
-| `cdn.hackclub-assets.com` | R2 bucket (custom domain in R2 settings) |
+| `user-cdn.hackclub-assets.com` | R2 bucket (custom domain in R2 settings) |
+
+The two hostnames are in separate Cloudflare zones. Deleting an upload purges
+the edge cache in both; see `app/jobs/purge_cloudflare_cache_job.rb`.
 
 ## API
 

app/jobs/purge_cloudflare_cache_job.rb

@@ -1,27 +1,83 @@
 # frozen_string_literal: true
 
 class PurgeCloudflareCacheJob < ApplicationJob
+  class ConfigurationError < StandardError; end
+
+  class PurgeFailedError < StandardError; end
+
   queue_as :default
 
+  retry_on Faraday::Error, wait: :polynomially_longer, attempts: 5
+  retry_on PurgeFailedError, wait: :polynomially_longer, attempts: 5
+
   def perform(urls)
-    zone_id = ENV["CLOUDFLARE_ZONE_ID"]
-    api_token = ENV["CLOUDFLARE_API_TOKEN"]
+    urls = Array(urls).map { |url| url.to_s.strip }.reject(&:blank?).uniq
+    return if urls.empty?
 
-    return unless zone_id.present? && api_token.present?
+    api_token = ENV["CLOUDFLARE_API_TOKEN"].presence
+    if api_token.nil?
+      return if Rails.env.local?
 
-    conn = Faraday.new(url: "https://api.cloudflare.com") do |f|
-      f.request :json
-      f.response :json
-      f.adapter :net_http
+      raise ConfigurationError,
+        "CLOUDFLARE_API_TOKEN is not set!! cache purging will fail!"
+    end
+
+    zone_ids = zone_ids_by_host
+
+    urls.group_by { |url| URI.parse(url).host }.each do |host, host_urls|
+      zone_id = zone_ids[host]
+
+      if zone_id.blank?
+        raise ConfigurationError,
+          "No Cloudflare zone id configured for #{host}, so #{host_urls.size} " \
+          "URL(s) cannot be purged (configured hosts: #{zone_ids.keys.join(', ')})"
+      end
+
+      host_urls.each_slice(30) do |slice|
+        purge!(zone_id: zone_id, files: slice, api_token: api_token)
+      end
+    end
+  end
+
+  private
+
+  # Host => zone id. Both zones are required: purging only one leaves the other
+  # serving the deleted file.
+  def zone_ids_by_host
+    mapping = {
+      CDNHost.host => ENV["CLOUDFLARE_ZONE_ID"].presence,
+      CDNHost.assets_host => ENV["CLOUDFLARE_ASSETS_ZONE_ID"].presence
+    }
+
+    configured = mapping.values.compact
+    if mapping.size > 1 && configured.uniq.size < configured.size
+      raise ConfigurationError,
+        "CLOUDFLARE_ZONE_ID and CLOUDFLARE_ASSETS_ZONE_ID are set to the same zone, " \
+        "but #{mapping.keys.join(' and ')} are in different Cloudflare zones"
     end
 
-    response = conn.post("/client/v4/zones/#{zone_id}/purge_cache") do |req|
+    mapping
+  end
+
+  def purge!(zone_id:, files:, api_token:)
+    response = connection.post("/client/v4/zones/#{zone_id}/purge_cache") do |req|
       req.headers["Authorization"] = "Bearer #{api_token}"
-      req.body = { files: Array(urls) }
+      req.body = { files: files }
     end
 
-    unless response.success?
-      Rails.logger.error("Cloudflare cache purge failed: #{response.body}")
+    body = response.body
+    return if response.success? && body.is_a?(Hash) && body["success"]
+
+    raise PurgeFailedError,
+      "Cloudflare cache purge failed for zone #{zone_id} " \
+      "(HTTP #{response.status}, #{files.size} URL(s)): #{body.inspect}"
+  end
+
+  def connection
+    @connection ||= Faraday.new(url: "https://api.cloudflare.com") do |f|
+      f.request :json
+      f.response :json
+      f.adapter :net_http
     end
   end
 end

app/models/cdn_host.rb

@@ -6,13 +6,21 @@
 # "https://https://cdn.hackclub.com".
 module CDNHost
   DEFAULT_HOST = "cdn.hackclub.com"
+  DEFAULT_ASSETS_HOST = "user-cdn.hackclub-assets.com"
 
   module_function
 
   def host
-    raw = ENV["CDN_HOST"].presence || DEFAULT_HOST
-    raw.sub(%r{\Ahttps?://}i, "").sub(%r{/+\z}, "")
+    normalize(ENV["CDN_HOST"].presence || DEFAULT_HOST)
+  end
+
+  def assets_host
+    normalize(ENV["CDN_ASSETS_HOST"].presence || DEFAULT_ASSETS_HOST)
   end
 
   def base_url = "https://#{host}"
+
+  def assets_base_url = "https://#{assets_host}"
+
+  def normalize(raw) = raw.to_s.sub(%r{\Ahttps?://}i, "").sub(%r{/+\z}, "")
 end

app/models/upload.rb

@@ -69,10 +69,12 @@ def human_file_size
     ActiveSupport::NumberHelper.number_to_human_size(byte_size)
   end
 
-  # Direct URL to public R2 bucket
+  # Direct URL to public R2 bucket. The key holds the raw filename, which can
+  # contain spaces or "#", so escape it the same way the route helper escapes
+  # cdn_url - otherwise "#" starts a fragment and both the redirect and the
+  # cache purge silently target the wrong URL.
   def assets_url
-    host = ENV.fetch("CDN_ASSETS_HOST", "cdn.hackclub-assets.com")
-    "https://#{host}/#{blob.key}"
+    "#{CDNHost.assets_base_url}/#{ActionDispatch::Journey::Router::Utils.escape_path(blob.key)}"
   end
 
   # Get CDN URL (uses external uploads controller)
@@ -228,5 +230,7 @@ def purge_blob
     Rails.logger.info("Blob #{blob.key} already deleted from S3, skipping purge")
   end
 
-  def purge_cdn_cache = PurgeCloudflareCacheJob.perform_later(assets_url)
+  # Both hosts cache the file: the assets host caches the bytes and the CDN host
+  # caches the redirect that points at them. Purge both or the delete is partial.
+  def purge_cdn_cache = PurgeCloudflareCacheJob.perform_later([ assets_url, cdn_url ])
 end

test/jobs/purge_cloudflare_cache_job_test.rb

@@ -0,0 +1,199 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class PurgeCloudflareCacheJobTest < ActiveSupport::TestCase
+  ENV_KEYS = %w[
+    CLOUDFLARE_API_TOKEN CLOUDFLARE_ZONE_ID CLOUDFLARE_ASSETS_ZONE_ID
+    CDN_HOST CDN_ASSETS_HOST
+  ].freeze
+
+  CDN_URL = "https://cdn.hackclub.com/an-id/file.png"
+  ASSETS_URL = "https://user-cdn.hackclub-assets.com/an-id/file.png"
+
+  CONFIGURED = {
+    "CLOUDFLARE_API_TOKEN" => "token",
+    "CLOUDFLARE_ZONE_ID" => "cdn-zone",
+    "CLOUDFLARE_ASSETS_ZONE_ID" => "assets-zone"
+  }.freeze
+
+  # Clears every var this job reads so a developer's .env can't change results.
+  def with_env(values)
+    previous = ENV_KEYS.index_with { |key| ENV[key] }
+    ENV_KEYS.each { |key| ENV.delete(key) }
+    values.each { |key, value| ENV[key.to_s] = value }
+    yield
+  ensure
+    ENV_KEYS.each { |key| previous[key].nil? ? ENV.delete(key) : ENV[key] = previous[key] }
+  end
+
+  # The test environment is "local", where a missing token is allowed to no-op.
+  def with_rails_env(name)
+    previous = Rails.env
+    Rails.env = name
+    yield
+  ensure
+    Rails.env = previous
+  end
+
+  # Returns [job, requests] where the job's Faraday connection is stubbed and
+  # every purge call is appended to requests.
+  def stubbed_job(status: 200, body: { "success" => true, "errors" => [] })
+    requests = []
+
+    stubs = Faraday::Adapter::Test::Stubs.new do |stub|
+      stub.post(%r{\A/client/v4/zones/[^/]+/purge_cache\z}) do |env|
+        requests << {
+          zone: env.url.path[%r{/zones/([^/]+)/}, 1],
+          files: JSON.parse(env.body).fetch("files"),
+          authorization: env.request_headers["Authorization"]
+        }
+        [ status, { "Content-Type" => "application/json" }, body.to_json ]
+      end
+    end
+
+    connection = Faraday.new do |f|
+      f.request :json
+      f.response :json
+      f.adapter :test, stubs
+    end
+
+    job = PurgeCloudflareCacheJob.new
+    job.define_singleton_method(:connection) { connection }
+    [ job, requests ]
+  end
+
+  test "purges each host in its own zone" do
+    with_env(CONFIGURED) do
+      job, requests = stubbed_job
+      job.perform([ ASSETS_URL, CDN_URL ])
+
+      assert_equal 2, requests.size
+      assert_equal [ ASSETS_URL ], requests.find { |r| r[:zone] == "assets-zone" }[:files]
+      assert_equal [ CDN_URL ], requests.find { |r| r[:zone] == "cdn-zone" }[:files]
+      assert_equal [ "Bearer token" ], requests.map { |r| r[:authorization] }.uniq
+    end
+  end
+
+  test "respects CDN_HOST and CDN_ASSETS_HOST overrides" do
+    with_env(CONFIGURED.merge(
+      "CDN_HOST" => "cdn.example.com",
+      "CDN_ASSETS_HOST" => "assets.example.net"
+    )) do
+      job, requests = stubbed_job
+      job.perform([ "https://assets.example.net/key", "https://cdn.example.com/an-id/f.png" ])
+
+      assert_equal %w[assets-zone cdn-zone], requests.map { |r| r[:zone] }.sort
+    end
+  end
+
+  test "does nothing when there are no urls" do
+    with_env(CONFIGURED) do
+      job, requests = stubbed_job
+      job.perform([])
+      job.perform(nil)
+
+      assert_empty requests
+    end
+  end
+
+  # Matches the each_slice size in the job; Cloudflare's per-request limit is 30
+  # URLs outside Enterprise.
+  BATCH_SIZE = 30
+
+  test "deduplicates urls and batches them per request" do
+    with_env(CONFIGURED) do
+      job, requests = stubbed_job
+      urls = Array.new(BATCH_SIZE + 5) do |i|
+        "https://user-cdn.hackclub-assets.com/id-#{i}/f.png"
+      end
+      job.perform(urls + [ urls.first ])
+
+      assert_equal 2, requests.size
+      assert_equal BATCH_SIZE, requests.first[:files].size
+      assert_equal 5, requests.second[:files].size
+    end
+  end
+
+  test "raises when the assets zone is not configured" do
+    with_env(CONFIGURED.except("CLOUDFLARE_ASSETS_ZONE_ID")) do
+      job, = stubbed_job
+
+      error = assert_raises(PurgeCloudflareCacheJob::ConfigurationError) do
+        job.perform([ ASSETS_URL ])
+      end
+      assert_match "user-cdn.hackclub-assets.com", error.message
+    end
+  end
+
+  test "raises when a url belongs to neither configured host" do
+    with_env(CONFIGURED) do
+      job, = stubbed_job
+
+      assert_raises(PurgeCloudflareCacheJob::ConfigurationError) do
+        job.perform([ "https://somewhere-else.example.com/key" ])
+      end
+    end
+  end
+
+  test "raises when both zone ids are the same" do
+    with_env(CONFIGURED.merge("CLOUDFLARE_ASSETS_ZONE_ID" => "cdn-zone")) do
+      job, requests = stubbed_job
+
+      error = assert_raises(PurgeCloudflareCacheJob::ConfigurationError) do
+        job.perform([ ASSETS_URL ])
+      end
+      assert_match "same zone", error.message
+      assert_empty requests
+    end
+  end
+
+  test "raises outside local environments when the api token is missing" do
+    with_env(CONFIGURED.except("CLOUDFLARE_API_TOKEN")) do
+      with_rails_env("production") do
+        job, requests = stubbed_job
+
+        assert_raises(PurgeCloudflareCacheJob::ConfigurationError) do
+          job.perform([ ASSETS_URL ])
+        end
+        assert_empty requests
+      end
+    end
+  end
+
+  test "skips quietly in local environments when the api token is missing" do
+    with_env(CONFIGURED.except("CLOUDFLARE_API_TOKEN")) do
+      job, requests = stubbed_job
+
+      assert_nil job.perform([ ASSETS_URL ])
+      assert_empty requests
+    end
+  end
+
+  # Cloudflare answers 200 with success:false for a rejected purge, which the
+  # previous implementation treated as a win.
+  test "raises when cloudflare reports failure in a 200 response" do
+    with_env(CONFIGURED) do
+      job, = stubbed_job(body: {
+        "success" => false,
+        "errors" => [ { "code" => 1012, "message" => "Unable to purge" } ]
+      })
+
+      error = assert_raises(PurgeCloudflareCacheJob::PurgeFailedError) do
+        job.perform([ ASSETS_URL ])
+      end
+      assert_match "1012", error.message
+    end
+  end
+
+  test "raises when cloudflare rejects the request" do
+    with_env(CONFIGURED) do
+      job, = stubbed_job(status: 403, body: { "success" => false })
+
+      error = assert_raises(PurgeCloudflareCacheJob::PurgeFailedError) do
+        job.perform([ ASSETS_URL ])
+      end
+      assert_match "403", error.message
+    end
+  end
+end