Only cache the deploy spec when the sync changed something

[aqeelvn]

Stacked on #1494 (which stacks on #1493).

What

• GithubSyncJob now skips the CacheDeploySpecJob enqueue when the sync changed nothing: no commits appended, newest reachable commit unchanged, and a cached spec already present.
• The explicit refresh endpoints (stacks#refresh, UI and API) enqueue CacheDeploySpecJob directly and unconditionally.

Why

Every sync ended with an unconditional spec re-cache — a full clone + checkout + shipit.yml evaluation (~65s, disk, memory) — even when the sync was a no-op. No-op syncs are common: refresh button/API hits, duplicate webhook deliveries, and GitHub eventual-consistency double-syncs. The deploy spec is a pure function of the committed files at the stack's newest reachable commit; if that commit didn't change, recomputing the spec is guaranteed wasted work.

This cuts enqueue volume to be proportional to actual head movement instead of webhook/API chatter, relieving the deploys queue that spec-cache jobs share with real deploys.

Correctness details

• Rewind force-pushes (branch reset to an older commit) append nothing but detach commits — the head still changes. The skip condition compares commits.reachable.last&.sha before/after the sync — the exact commit CacheDeploySpecJob would check out — so these still refresh the cache. (Deliberately not Stack#head, which reads commits.reachable.first = the oldest reachable commit under Rails' implicit ordering.)
• Belt and braces: the condition also checks appended_commits.any?, so appends enqueue even if the head comparison were somehow confounded.
• New / cleared stacks: cached_deploy_spec.blank? forces the enqueue.
• Human escape hatch preserved (rev 2): people use Refresh precisely to fix a stale or broken cached spec when the head hasn't moved. Both refresh endpoints now pass force_spec_cache: true through GithubSyncJob, which skips the conditional — threading it through the sync (rather than enqueuing CacheDeploySpecJob directly, as the first revision did) guarantees the spec is computed from the post-sync head and avoids the cross-queue race where a direct enqueue could cache the pre-sync head and then swallow the sync's follow-up via the dedupe lock. The flag survives eventual-consistency retries.
• Inaccessible stacks (404 → mark_as_inaccessible!) no longer enqueue a cache job they'd immediately no-op on — when a cached spec is already present. A blank-spec inaccessible stack still enqueues and the job early-returns; harmless.

Tests

• No-op sync → no CacheDeploySpecJob
• Blank cached spec → enqueued
• Detach-only sync (rewind) → enqueued
• New commits (existing test) → enqueued
• EC-retry "processes normally" test updated: no retry and no re-cache when nothing changed
• Both refresh controller actions → always enqueue

[timothysmith0609]

The core idea is right and the correctness work behind it is careful. I specifically checked the reachable.last vs Stack#head distinction — has_many :commits carries no order and scope :reachable is a bare where(detached: false), so spec_cache_target and CacheDeploySpecJob really do resolve to the same commit. Good catch, and the rewind-force-push reasoning follows from it. Hoisting appended_commits = [] above the block also correctly captures the inner assignment.

One thing I'd like addressed before merge, because it defeats the escape hatch this PR is built around.

🔴 The refresh path races itself, and can leave the spec stale

stacks#refresh now fires GithubSyncJob (queue default) and CacheDeploySpecJob (queue deploys) concurrently. Different queues, no ordering guarantee. If the direct job wins the race:

1. CacheDeploySpecJob caches the spec at the pre-sync head, and holds the dedupe lock for its ~65s runtime (per #1494).
2. GithubSyncJob finds new commits, sees head movement, enqueues a second CacheDeploySpecJob.
3. That one hits the held lock and is dropped by on_duplicate :drop.

Net result: someone pressed Refresh precisely because the spec was stale or broken, and it stays stale — which is the exact scenario the "human escape hatch preserved" section is protecting. The failure is also invisible: the dropped job reports success.

Threading a force: true (or refresh: true) param through GithubSyncJob and letting the existing enqueue site honour it would run exactly one job, after the sync, and sidesteps the race entirely. It also avoids the two-full-clones case when the head has moved.

🟡 Related: this interacts with #1494 to remove the self-healing property

Flagged in more detail on #1494, but restating the half that belongs here. The skip condition is only as good as the guarantee that something eventually recomputes the spec. With #1494 holding the dedupe lock for the whole runtime, a CacheDeploySpecJob enqueued for a newer head while an older run is in flight is silently dropped — and this PR removes the unconditional re-cache that used to catch it on the next webhook. The spec then stays stale until the next head movement.

cached_deploy_spec.blank? is the right instinct for the "is the cache valid" check; it's just too weak a predicate. Comparing against a persisted "spec computed from sha X" column would make the skip condition exact, make the whole flow self-healing, and remove the need for the direct-enqueue-on-refresh hack above. That's migration-first so it can't be this PR, but it may be worth doing before the two behaviours ship together.

🟢 Description inaccuracy

Inaccessible stacks (404 → mark_as_inaccessible!) no longer enqueue a cache job they'd immediately no-op on.

Only when cached_deploy_spec is present. A blank-spec inaccessible stack still enqueues, and the job early-returns on inaccessible?. Harmless, but the bullet overstates it.

🟢 Test coverage

The four new cases cover the branch conditions well. The one gap is the interaction above — worth an integration-shaped test that a refresh on a stack whose head has moved ends with the spec at the new head, since that's the property users actually rely on.

Not blocking on anything but the refresh race; the rest is fine as follow-ups.

[aqeelvn]

Addressed in the rebased fb8e257f:

• Refresh race fixed with your suggested design: the controllers no longer enqueue CacheDeploySpecJob directly — both refresh endpoints pass force_spec_cache: true through GithubSyncJob, which skips the conditional. Exactly one cache job, strictly after the sync, computed from the post-sync head. The flag survives EC retries via params.merge (test added).
• Description updated: the force-param design replaces the direct-enqueue section, and the inaccessible-stacks bullet no longer overstates.
• Tests: force-with-no-changes enqueues; force survives retries; both controller assertions updated.