Hand the generated payload to the S3 cache write

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
 

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
 

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
 

test/commands/concept/cached_content/retrieve_test.rb

@@ -12,7 +12,7 @@ class Concept::CachedContent::RetrieveTest < ActiveSupport::TestCase
       { about: "<p>cached about</p>", introduction: "<p>cached intro</p>" }.to_json
     )
 
-    Concept::CachedContent::Store.expects(:defer).never
+    S3Cache::Write.expects(:defer).never
 
     assert_equal(
       { about: "<p>cached about</p>", introduction: "<p>cached intro</p>" },
@@ -23,14 +23,24 @@ 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),
       Concept::CachedContent::Retrieve.(concept)
     )
   end
 
+  test "cache miss parses once, handing the generated content to the write" do
+    concept = create :concept
+    generated = { about: "<p>about</p>", introduction: "<p>intro</p>" }
+
+    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 "<p>stale</p>", Concept::CachedContent::Retrieve.(concept)[:about]
   end

test/commands/exercise/cached_content/retrieve_test.rb

@@ -13,7 +13,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase
       { introduction: "<p>cached intro</p>", instructions: "<p>cached instructions</p>" }.to_json
     )
 
-    Exercise::CachedContent::Store.expects(:defer).never
+    S3Cache::Write.expects(:defer).never
 
     assert_equal(
       { introduction: "<p>cached intro</p>", instructions: "<p>cached instructions</p>" },
@@ -24,14 +24,24 @@ 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),
       Exercise::CachedContent::Retrieve.(exercise, nil)
     )
   end
 
+  test "cache miss parses once, handing the generated content to the write" do
+    exercise = create :practice_exercise
+    generated = { introduction: "<p>intro</p>", instructions: "<p>instructions</p>" }
+
+    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: "<p>pinned</p>" }.to_json
     )
 
-    Exercise::CachedContent::Store.expects(:defer).never
+    S3Cache::Write.expects(:defer).never
 
     assert_equal "<p>pinned</p>", Exercise::CachedContent::Retrieve.(exercise, solution)[:instructions]
   end
@@ -53,7 +63,7 @@ class Exercise::CachedContent::RetrieveTest < ActiveSupport::TestCase
       { introduction: "", instructions: "<p>shared</p>" }.to_json
     )
 
-    Exercise::CachedContent::Store.expects(:defer).never
+    S3Cache::Write.expects(:defer).never
 
     assert_equal "<p>shared</p>", 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 "<p>stale</p>", Exercise::CachedContent::Retrieve.(exercise, nil)[:instructions]
   end

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,22 +22,46 @@ 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)
 
     assert_equal Solution::CachedSerializedView::Generate.(solution), payload
     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