From 5dd95e7d9528b8e315cdaed51b3f2aec3e28c040 Mon Sep 17 00:00:00 2001 From: Jeremy Walker Date: Fri, 28 Aug 2026 01:14:25 +0200 Subject: [PATCH] Hand the generated payload to the S3 cache write Retrieve generated the payload for the response and then deferred a Store job that generated it a second time to write it. Every cache miss therefore serialized twice, which matters most while a cache is cold and misses are the common case. Store now has nothing left to do beyond calling S3Cache::Write, so it goes, and Retrieve defers the write itself with the payload it already built. The key is passed with it rather than recomputed in the job. Store derived the key from the record it reloaded through its GlobalID, so a record that changed between the request and the job running would have had the old payload filed under the new version's key, where it would then be read back as current. Capturing key and payload together keeps them the same version. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VDJXJXDiyrvKZBnXMuqGif --- .../concept/cached_content/retrieve.rb | 11 ++++- app/commands/concept/cached_content/store.rb | 21 --------- .../exercise/cached_content/retrieve.rb | 11 ++++- app/commands/exercise/cached_content/store.rb | 22 --------- .../cached_serialized_view/retrieve.rb | 13 ++++- .../solution/cached_serialized_view/store.rb | 21 --------- .../concept/cached_content/retrieve_test.rb | 16 +++++-- .../concept/cached_content/store_test.rb | 19 -------- .../exercise/cached_content/retrieve_test.rb | 20 ++++++-- .../exercise/cached_content/store_test.rb | 32 ------------- .../cached_serialized_view/retrieve_test.rb | 47 +++++++++++++++---- .../cached_serialized_view/store_test.rb | 21 --------- 12 files changed, 95 insertions(+), 159 deletions(-) delete mode 100644 app/commands/concept/cached_content/store.rb delete mode 100644 app/commands/exercise/cached_content/store.rb delete mode 100644 app/commands/solution/cached_serialized_view/store.rb delete mode 100644 test/commands/concept/cached_content/store_test.rb delete mode 100644 test/commands/exercise/cached_content/store_test.rb delete mode 100644 test/commands/solution/cached_serialized_view/store_test.rb diff --git a/app/commands/concept/cached_content/retrieve.rb b/app/commands/concept/cached_content/retrieve.rb index e187102795..0ee64ef68c 100644 --- a/app/commands/concept/cached_content/retrieve.rb +++ b/app/commands/concept/cached_content/retrieve.rb @@ -25,9 +25,16 @@ def call private def cached = S3Cache::Read.(cache_key) + # The content built here is handed straight to the deferred write rather + # than being regenerated by it, which would parse the documents twice. + # + # The key is captured here too, not recomputed in the job. It is derived + # from synced_to_git_sha, and the job reloads the concept through a + # GlobalID, so a sync landing in between would file this content under + # the new sha's key, where it would be read back as that sync's content. def generate! - Concept::CachedContent::Generate.(concept).tap do - Concept::CachedContent::Store.defer(concept) + Concept::CachedContent::Generate.(concept).tap do |content| + S3Cache::Write.defer(cache_key, content) end end diff --git a/app/commands/concept/cached_content/store.rb b/app/commands/concept/cached_content/store.rb deleted file mode 100644 index fc149b6537..0000000000 --- a/app/commands/concept/cached_content/store.rb +++ /dev/null @@ -1,21 +0,0 @@ -# Generates a concept's parsed content and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Concept::CachedContent::Store - include Mandate - - queue_as :background - - initialize_with :concept - - def call - S3Cache::Write.(cache_key, Concept::CachedContent::Generate.(concept)) - end - - private - # Must match Concept::CachedContent::Retrieve#cache_key - def cache_key - uuid = concept.uuid - "concept-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{concept.synced_to_git_sha}.json" - end -end diff --git a/app/commands/exercise/cached_content/retrieve.rb b/app/commands/exercise/cached_content/retrieve.rb index e2cf9585c9..82cabc6503 100644 --- a/app/commands/exercise/cached_content/retrieve.rb +++ b/app/commands/exercise/cached_content/retrieve.rb @@ -27,9 +27,16 @@ def call private def cached = S3Cache::Read.(cache_key) + # The content built here is handed straight to the deferred write rather + # than being regenerated by it, which would parse the documents twice. + # + # The key is captured here too, not recomputed in the job. It is derived + # from the sha we read at, and the job reloads both records through + # GlobalIDs, so a sync landing in between would file this content under + # the new sha's key, where it would be read back as that sync's content. def generate! - Exercise::CachedContent::Generate.(exercise, solution).tap do - Exercise::CachedContent::Store.defer(exercise, solution) + Exercise::CachedContent::Generate.(exercise, solution).tap do |content| + S3Cache::Write.defer(cache_key, content) end end diff --git a/app/commands/exercise/cached_content/store.rb b/app/commands/exercise/cached_content/store.rb deleted file mode 100644 index d3d7923423..0000000000 --- a/app/commands/exercise/cached_content/store.rb +++ /dev/null @@ -1,22 +0,0 @@ -# Generates an exercise's parsed content and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Exercise::CachedContent::Store - include Mandate - - queue_as :background - - initialize_with :exercise, :solution - - def call - S3Cache::Write.(cache_key, Exercise::CachedContent::Generate.(exercise, solution)) - end - - private - # Must match Exercise::CachedContent::Retrieve#cache_key - def cache_key - uuid = exercise.uuid - sha = solution ? solution.git_sha : exercise.git_sha - "exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{sha}.json" - end -end diff --git a/app/commands/solution/cached_serialized_view/retrieve.rb b/app/commands/solution/cached_serialized_view/retrieve.rb index 91ed3b0236..d24785e034 100644 --- a/app/commands/solution/cached_serialized_view/retrieve.rb +++ b/app/commands/solution/cached_serialized_view/retrieve.rb @@ -19,9 +19,18 @@ def call private def cached = S3Cache::Read.(cache_key) + # The payload built here is handed straight to the deferred write + # rather than being regenerated by it: a miss otherwise serializes the + # solution twice, and misses are the common case while the cache fills. + # + # The key is captured here too, not recomputed in the job. It is derived + # from updated_at, and the job reloads the solution through a GlobalID — + # so a solution that changes between request and job would file this + # (now old) payload under the new version's key, where it would be read + # back as current. Capturing both together keeps them the same version. def generate! - Solution::CachedSerializedView::Generate.(solution).tap do - Solution::CachedSerializedView::Store.defer(solution) + Solution::CachedSerializedView::Generate.(solution).tap do |payload| + S3Cache::Write.defer(cache_key, payload) end end diff --git a/app/commands/solution/cached_serialized_view/store.rb b/app/commands/solution/cached_serialized_view/store.rb deleted file mode 100644 index 7ed7b5f5ee..0000000000 --- a/app/commands/solution/cached_serialized_view/store.rb +++ /dev/null @@ -1,21 +0,0 @@ -# Generates the solution-view payload and writes it to the S3 cache. -# Deferred from Retrieve on a cache miss so the write (and the repeat -# generation it needs) happens off the request path. -class Solution::CachedSerializedView::Store - include Mandate - - queue_as :background - - initialize_with :solution - - def call - S3Cache::Write.(cache_key, Solution::CachedSerializedView::Generate.(solution)) - end - - private - # Must match Solution::CachedSerializedView::Retrieve#cache_key - def cache_key - uuid = solution.uuid - "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - end -end diff --git a/test/commands/concept/cached_content/retrieve_test.rb b/test/commands/concept/cached_content/retrieve_test.rb index fbe172121e..4f67171f5b 100644 --- a/test/commands/concept/cached_content/retrieve_test.rb +++ b/test/commands/concept/cached_content/retrieve_test.rb @@ -12,7 +12,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase { about: "

cached about

", introduction: "

cached intro

" }.to_json ) - Concept::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { about: "

cached about

", introduction: "

cached intro

" }, @@ -23,7 +23,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase test "cache miss generates live and defers a write" do concept = create :concept - Concept::CachedContent::Store.expects(:defer).with(concept) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), anything) assert_equal( Concept::CachedContent::Generate.(concept), @@ -31,6 +31,16 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase ) end + test "cache miss parses once, handing the generated content to the write" do + concept = create :concept + generated = { about: "

about

", introduction: "

intro

" } + + Concept::CachedContent::Generate.expects(:call).with(concept).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), generated) + + assert_equal generated, Concept::CachedContent::Retrieve.(concept) + end + test "an old sha's cache entry is not read after the concept syncs" do concept = create :concept upload_to_s3( @@ -39,7 +49,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase ) concept.update!(synced_to_git_sha: "new-sha") - Concept::CachedContent::Store.expects(:defer).with(concept) + S3Cache::Write.expects(:defer).with(cache_key(concept, concept.synced_to_git_sha), anything) refute_equal "

stale

", Concept::CachedContent::Retrieve.(concept)[:about] end diff --git a/test/commands/concept/cached_content/store_test.rb b/test/commands/concept/cached_content/store_test.rb deleted file mode 100644 index e4ac743206..0000000000 --- a/test/commands/concept/cached_content/store_test.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'test_helper' - -class Concept::CachedContent::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded sha-versioned key" do - concept = create :concept - - Concept::CachedContent::Store.(concept) - - uuid = concept.uuid - key = "concept-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{concept.synced_to_git_sha}.json" - - expected = JSON.parse(Concept::CachedContent::Generate.(concept).to_json, symbolize_names: true) - assert_equal expected, S3Cache::Read.(key) - end -end diff --git a/test/commands/exercise/cached_content/retrieve_test.rb b/test/commands/exercise/cached_content/retrieve_test.rb index 899fca337b..9d7c4015a6 100644 --- a/test/commands/exercise/cached_content/retrieve_test.rb +++ b/test/commands/exercise/cached_content/retrieve_test.rb @@ -13,7 +13,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "

cached intro

", instructions: "

cached instructions

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { introduction: "

cached intro

", instructions: "

cached instructions

" }, @@ -24,7 +24,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase test "cache miss generates live and defers a write" do exercise = create :practice_exercise - Exercise::CachedContent::Store.expects(:defer).with(exercise, nil) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), anything) assert_equal( Exercise::CachedContent::Generate.(exercise, nil), @@ -32,6 +32,16 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase ) end + test "cache miss parses once, handing the generated content to the write" do + exercise = create :practice_exercise + generated = { introduction: "

intro

", instructions: "

instructions

" } + + Exercise::CachedContent::Generate.expects(:call).with(exercise, nil).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), generated) + + assert_equal generated, Exercise::CachedContent::Retrieve.(exercise, nil) + end + test "a solution reads the key for the sha it is pinned to" do exercise = create :practice_exercise solution = create :practice_solution, exercise:, git_sha: OLD_GIT_SHA @@ -40,7 +50,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "", instructions: "

pinned

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal "

pinned

", Exercise::CachedContent::Retrieve.(exercise, solution)[:instructions] end @@ -53,7 +63,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase { introduction: "", instructions: "

shared

" }.to_json ) - Exercise::CachedContent::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal "

shared

", Exercise::CachedContent::Retrieve.(exercise, solution)[:instructions] end @@ -66,7 +76,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase ) exercise.update_columns(git_sha: OLD_GIT_SHA) - Exercise::CachedContent::Store.expects(:defer).with(exercise, nil) + S3Cache::Write.expects(:defer).with(cache_key(exercise, exercise.git_sha), anything) refute_equal "

stale

", Exercise::CachedContent::Retrieve.(exercise, nil)[:instructions] end diff --git a/test/commands/exercise/cached_content/store_test.rb b/test/commands/exercise/cached_content/store_test.rb deleted file mode 100644 index 138104682e..0000000000 --- a/test/commands/exercise/cached_content/store_test.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'test_helper' - -class Exercise::CachedContent::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded sha-versioned key" do - exercise = create :practice_exercise - - Exercise::CachedContent::Store.(exercise, nil) - - uuid = exercise.uuid - key = "exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{exercise.git_sha}.json" - - expected = JSON.parse(Exercise::CachedContent::Generate.(exercise, nil).to_json, symbolize_names: true) - assert_equal expected, S3Cache::Read.(key) - end - - test "writes to the solution's pinned sha" do - exercise = create :practice_exercise - solution = create :practice_solution, exercise:, git_sha: OLD_GIT_SHA - - Exercise::CachedContent::Store.(exercise, solution) - - uuid = exercise.uuid - assert S3Cache::Read.("exercise-content/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{OLD_GIT_SHA}.json") - end - - # A real commit in the test repo, so content can still be read at it. - OLD_GIT_SHA = '0b04b8976650d993ecf4603cf7413f3c6b898eff'.freeze -end diff --git a/test/commands/solution/cached_serialized_view/retrieve_test.rb b/test/commands/solution/cached_serialized_view/retrieve_test.rb index de65f5841d..b7bc5e6ec0 100644 --- a/test/commands/solution/cached_serialized_view/retrieve_test.rb +++ b/test/commands/solution/cached_serialized_view/retrieve_test.rb @@ -7,11 +7,10 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase test "cache hit returns the stored payload without deferring a write" do solution = create(:practice_solution, :published) - uuid = solution.uuid - key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - upload_to_s3(Exercism.config.aws_cache_bucket, key, { iterations: [], language: "cached" }.to_json) + upload_to_s3(Exercism.config.aws_cache_bucket, cache_key_for(solution), + { iterations: [], language: "cached" }.to_json) - Solution::CachedSerializedView::Store.expects(:defer).never + S3Cache::Write.expects(:defer).never assert_equal( { iterations: [], language: "cached" }, @@ -23,7 +22,7 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase solution = create(:practice_solution, :published) create(:iteration, solution:) - Solution::CachedSerializedView::Store.expects(:defer).with(solution) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), anything) payload = Solution::CachedSerializedView::Retrieve.(solution) @@ -31,14 +30,38 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase assert_equal solution.track.highlightjs_language, payload[:language] end + test "cache miss serializes once, handing the generated payload to the write" do + solution = create(:practice_solution, :published) + create(:iteration, solution:) + generated = { iterations: [], language: "generated-once" } + + Solution::CachedSerializedView::Generate.expects(:call).with(solution).once.returns(generated) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), generated) + + assert_equal generated, Solution::CachedSerializedView::Retrieve.(solution) + end + + test "write is keyed on the version that was serialized, not a later one" do + solution = create(:practice_solution, :published) + key_at_read_time = cache_key_for(solution) + + S3Cache::Write.expects(:defer).with(key_at_read_time, anything) + Solution::CachedSerializedView::Retrieve.(solution) + + # Had the key been recomputed when the deferred job ran, a solution + # changing in the meantime would have filed this payload under the new + # version's key, where it would be read back as current. + solution.update!(updated_at: solution.updated_at + 1.hour) + refute_equal key_at_read_time, cache_key_for(solution.reload) + end + test "an old version's cache entry is not read after the solution changes" do solution = create(:practice_solution, :published) - uuid = solution.uuid - old_key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - upload_to_s3(Exercism.config.aws_cache_bucket, old_key, { language: "stale" }.to_json) + upload_to_s3(Exercism.config.aws_cache_bucket, cache_key_for(solution), + { language: "stale" }.to_json) solution.update!(updated_at: solution.updated_at + 1.hour) - Solution::CachedSerializedView::Store.expects(:defer).with(solution) + S3Cache::Write.expects(:defer).with(cache_key_for(solution), anything) payload = Solution::CachedSerializedView::Retrieve.(solution) @@ -55,4 +78,10 @@ class Solution::CachedSerializedView::RetrieveTest < ActiveSupport::TestCase Solution::CachedSerializedView::Retrieve.(solution) ) end + + private + def cache_key_for(solution) + uuid = solution.uuid + "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" + end end diff --git a/test/commands/solution/cached_serialized_view/store_test.rb b/test/commands/solution/cached_serialized_view/store_test.rb deleted file mode 100644 index b0ab02c9a7..0000000000 --- a/test/commands/solution/cached_serialized_view/store_test.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'test_helper' - -class Solution::CachedSerializedView::StoreTest < ActiveSupport::TestCase - setup do - setup_s3_cache_bucket! - end - - test "writes the generated payload to the sharded versioned key" do - solution = create(:practice_solution, :published) - create(:iteration, solution:) - - Solution::CachedSerializedView::Store.(solution) - - uuid = solution.uuid - key = "solution-view/#{uuid[0, 2]}/#{uuid[2, 2]}/#{uuid}/#{solution.updated_at.to_i}.json" - stored = S3Cache::Read.(key) - - expected = JSON.parse(Solution::CachedSerializedView::Generate.(solution).to_json, symbolize_names: true) - assert_equal expected, stored - end -end