Only cache the deploy spec when the sync changed something

app/controllers/shipit/api/stacks_controller.rb

@@ -69,7 +69,12 @@ def destroy
       def refresh
         RefreshStatusesJob.perform_later(stack_id: stack.id)
         RefreshCheckRunsJob.perform_later(stack_id: stack.id)
-        GithubSyncJob.perform_later(stack_id: stack.id)
+        # force_spec_cache: explicit refreshes always recompute the cached deploy
+        # spec, even when the head hasn't moved: refreshing is how a stale or
+        # broken cached spec is fixed. Threading it through the sync job (rather
+        # than enqueuing CacheDeploySpecJob directly) guarantees the spec is
+        # computed from the post-sync head.
+        GithubSyncJob.perform_later(stack_id: stack.id, force_spec_cache: true)
         render_resource(stack, status: :accepted)
       end
 

app/controllers/shipit/stacks_controller.rb

@@ -94,7 +94,12 @@ def statistics
     def refresh
       RefreshStatusesJob.perform_later(stack_id: @stack.id)
       RefreshCheckRunsJob.perform_later(stack_id: @stack.id)
-      GithubSyncJob.perform_later(stack_id: @stack.id)
+      # force_spec_cache: explicit refreshes always recompute the cached deploy
+      # spec, even when the head hasn't moved: refreshing is how a stale or
+      # broken cached spec is fixed. Threading it through the sync job (rather
+      # than enqueuing CacheDeploySpecJob directly) guarantees the spec is
+      # computed from the post-sync head.
+      GithubSyncJob.perform_later(stack_id: @stack.id, force_spec_cache: true)
       flash[:success] = 'Refresh scheduled'
       redirect_to(request.referer.presence || stack_path(@stack))
     end

app/jobs/shipit/github_sync_job.rb

@@ -19,6 +19,8 @@ def perform(params)
       @stack = Stack.find(params[:stack_id])
       expected_head_sha = params[:expected_head_sha]
       retry_count = params[:retry_count] || 0
+      head_before_sync = spec_cache_target
+      appended_commits = []
 
       handle_github_errors do
         new_commits, shared_parent = fetch_missing_commits { stack.github_commits }
@@ -38,6 +40,11 @@ def perform(params)
           stack.lock_reverted_commits! if appended_commits.any?(&:revert?)
         end
       end
+      sync_changed_nothing = appended_commits.empty? &&
+                             spec_cache_target == head_before_sync &&
+                             stack.cached_deploy_spec.present?
+      return if sync_changed_nothing && !params[:force_spec_cache]
+
       CacheDeploySpecJob.perform_later(stack)
     end
 
@@ -63,6 +70,13 @@ def fetch_missing_commits(&block)
 
     protected
 
+    # The commit CacheDeploySpecJob would check out: the newest reachable one.
+    # If it didn't change during the sync (no appends, no detaches), the cached
+    # spec is still accurate and doesn't need to be recomputed.
+    def spec_cache_target
+      stack.commits.reachable.last&.sha
+    end
+
     def handle_github_errors
       yield
     rescue Octokit::NotFound

test/controllers/api/stacks_controller_test.rb

@@ -260,8 +260,8 @@ class StacksControllerTest < ApiControllerTestCase
         assert_json 'message', 'This operation requires the `write:stack` permission'
       end
 
-      test "#refresh queues a GithubSyncJob" do
-        assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id]) do
+      test "#refresh queues a GithubSyncJob with force_spec_cache" do
+        assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, force_spec_cache: true]) do
           post :refresh, params: { id: @stack.to_param }
         end
         assert_response :accepted

test/controllers/stacks_controller_test.rb

@@ -204,7 +204,7 @@ class StacksControllerTest < ActionController::TestCase
 
       assert_enqueued_with(job: RefreshStatusesJob, args: [stack_id: @stack.id]) do
         assert_enqueued_with(job: RefreshCheckRunsJob, args: [stack_id: @stack.id]) do
-          assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id]) do
+          assert_enqueued_with(job: GithubSyncJob, args: [stack_id: @stack.id, force_spec_cache: true]) do
             post :refresh, params: { id: @stack.to_param }
           end
         end

test/jobs/github_sync_job_test.rb

@@ -16,10 +16,50 @@ class GithubSyncJobTest < ActiveSupport::TestCase
       @job.perform(stack_id: @stack.id)
     end
 
-    test "#perform finally enqueue a CacheDeploySpecJob" do
+    test "#perform does not enqueue a CacheDeploySpecJob when the sync found nothing new" do
+      Stack.any_instance.stubs(:github_commits).returns(@github_commits)
+      @job.stubs(:fetch_missing_commits).yields.returns([[], nil])
+
+      assert_no_enqueued_jobs(only: CacheDeploySpecJob) do
+        @job.perform(stack_id: @stack.id)
+      end
+    end
+
+    test "#perform enqueues a CacheDeploySpecJob when the cached spec is missing" do
+      @stack.update!(cached_deploy_spec: nil)
+      Stack.any_instance.stubs(:github_commits).returns(@github_commits)
+      @job.stubs(:fetch_missing_commits).yields.returns([[], nil])
+
+      assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
+        @job.perform(stack_id: @stack.id)
+      end
+    end
+
+    test "#perform enqueues a CacheDeploySpecJob when nothing changed but force_spec_cache is set" do
       Stack.any_instance.stubs(:github_commits).returns(@github_commits)
       @job.stubs(:fetch_missing_commits).yields.returns([[], nil])
 
+      assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
+        @job.perform(stack_id: @stack.id, force_spec_cache: true)
+      end
+    end
+
+    test "#perform preserves force_spec_cache across eventual-consistency retries" do
+      expected_sha = "abcd1234"
+      Stack.any_instance.expects(:github_commits).returns(@github_commits)
+      @job.expects(:fetch_missing_commits).yields.returns([[], nil])
+      @job.expects(:commit_exists?).with(expected_sha).returns(false)
+
+      expected_args = { stack_id: @stack.id, expected_head_sha: expected_sha, force_spec_cache: true, retry_count: 1 }
+      assert_enqueued_with(job: GithubSyncJob, args: [expected_args]) do
+        @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha, force_spec_cache: true)
+      end
+    end
+
+    test "#perform enqueues a CacheDeploySpecJob when commits are detached without new commits" do
+      Stack.any_instance.stubs(:github_commits).returns(@github_commits)
+      @job.stubs(:fetch_missing_commits).yields.returns([[], shipit_commits(:third)])
+
       assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
         @job.perform(stack_id: @stack.id)
       end
@@ -155,7 +195,9 @@ class GithubSyncJobTest < ActiveSupport::TestCase
       @job.expects(:fetch_missing_commits).yields.returns([[], nil])
       @job.expects(:commit_exists?).with(expected_sha).returns(true)
 
-      assert_enqueued_with(job: CacheDeploySpecJob, args: [@stack]) do
+      # No retry is scheduled, and since the sync found nothing new,
+      # no spec re-cache is needed either.
+      assert_no_enqueued_jobs(only: [GithubSyncJob, CacheDeploySpecJob]) do
         @job.perform(stack_id: @stack.id, expected_head_sha: expected_sha)
       end
     end