diff --git a/app/controllers/shipit/api/stacks_controller.rb b/app/controllers/shipit/api/stacks_controller.rb index a483a5e3a..14052bdbf 100644 --- a/app/controllers/shipit/api/stacks_controller.rb +++ b/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 diff --git a/app/controllers/shipit/stacks_controller.rb b/app/controllers/shipit/stacks_controller.rb index fcef6eed9..1fdccfeb1 100644 --- a/app/controllers/shipit/stacks_controller.rb +++ b/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 diff --git a/app/jobs/shipit/github_sync_job.rb b/app/jobs/shipit/github_sync_job.rb index b5fa8baee..d18541ce9 100644 --- a/app/jobs/shipit/github_sync_job.rb +++ b/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 diff --git a/test/controllers/api/stacks_controller_test.rb b/test/controllers/api/stacks_controller_test.rb index 58cc0e87e..60c8a624a 100644 --- a/test/controllers/api/stacks_controller_test.rb +++ b/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 diff --git a/test/controllers/stacks_controller_test.rb b/test/controllers/stacks_controller_test.rb index 699144762..6d82212c7 100644 --- a/test/controllers/stacks_controller_test.rb +++ b/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 diff --git a/test/jobs/github_sync_job_test.rb b/test/jobs/github_sync_job_test.rb index 77670ebb6..d0a330dd2 100644 --- a/test/jobs/github_sync_job_test.rb +++ b/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