From fb8e257fadd3ee896e99b655c8989a28553a26db Mon Sep 17 00:00:00 2001 From: Aqeel Nazeer Date: Wed, 12 Aug 2026 12:58:56 +0530 Subject: [PATCH] Only cache the deploy spec when the sync changed something GithubSyncJob unconditionally enqueued CacheDeploySpecJob, so every sync triggered a full clone and spec evaluation even when it found nothing new: refresh requests, duplicate webhook deliveries, and eventual-consistency double-syncs all re-computed a spec that could not have changed. The deploy spec is a pure function of the committed files at the newest reachable commit. Skip the enqueue when no commits were appended, the newest reachable commit is unchanged, and a cached spec exists. The commit comparison uses the same expression CacheDeploySpecJob resolves for its checkout (commits.reachable.last), so rewind force-pushes that only detach commits still refresh the cache. Explicit refreshes (UI button and API endpoint) pass force_spec_cache: true through GithubSyncJob, which skips the conditional: refreshing is how a stale or broken cached spec gets fixed when the head has not moved. Threading the flag through the sync job rather than enqueuing CacheDeploySpecJob directly guarantees the spec is computed from the post-sync head; a direct enqueue racing the sync on a different queue could cache the pre-sync head and then swallow the sync's follow-up job via the dedupe lock. The flag survives eventual-consistency retries via params.merge. --- .../shipit/api/stacks_controller.rb | 7 ++- app/controllers/shipit/stacks_controller.rb | 7 ++- app/jobs/shipit/github_sync_job.rb | 14 ++++++ .../controllers/api/stacks_controller_test.rb | 4 +- test/controllers/stacks_controller_test.rb | 2 +- test/jobs/github_sync_job_test.rb | 46 ++++++++++++++++++- 6 files changed, 73 insertions(+), 7 deletions(-) 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