fix(trafficrouting): do not block abort/progressDeadline when Istio delays DestinationRule switch. Fixes #4626

[ChanghwanK]

Fixes #4626

Summary

When subset-level Istio traffic routing delays the DestinationRule switch because a traffic-receiving ReplicaSet is not fully available (behavior introduced by #4612), the resulting error propagated as fatal and short-circuited rolloutCanary() before it ever reached syncRolloutStatusCanary(). As a result, a rollout whose canary pods never become available could not time out, turn Degraded, or auto-abort: the controller retried the same reconcile forever (rate-limited requeue) and the rollout stayed Progressing indefinitely, requiring manual intervention (and in the worst observed case, a controller restart) to recover.

The core of the bug is a circular blocking condition: the condition that should trigger the progress-deadline abort (an unavailable ReplicaSet) is exactly the condition that blocked the code path that evaluates the abort.

Root cause

Call path (all line refs at current master):

1. rollout/trafficrouting/istio/istio.go UpdateHash() intentionally skips the DestinationRule switch and returns a plain error when shouldDelayDestinationRuleUpdate() is true. This is an expected, transient condition (logged at Info level), but it is indistinguishable from a fatal error to callers.
2. rollout/trafficrouting.go reconcileTrafficRouting() propagates it unchanged.
3. rollout/canary.go:67-69 rolloutCanary() returns early, skipping everything after it: experiments, analysis runs, ReplicaSet scaling/cleanup, and critically syncRolloutStatusCanary() → persistRolloutStatus(), which is where evaluateProgressDeadlineAbort(), Progressing-condition timeout evaluation, and requeueStuckRollout() live.

Since the delay error recurs on every reconcile as long as the ReplicaSet stays unavailable (e.g. crashlooping pods), the abort/timeout evaluation is blocked permanently, not transiently. Note that abort is also the natural exit for this loop: shouldDelayDestinationRuleUpdate() already exempts the canary ReplicaSet during an abort, so an abort would make the delay condition disappear and let the system converge — it just could never fire.

Fix

Three small pieces:

1. rollout/trafficrouting/istio/istio.go: wrap the delay error with a sentinel (ErrDestinationRuleUpdateDelayed, %w) so callers can distinguish this expected condition via errors.Is. The error message is unchanged. All other errors (API failures etc.) are untouched and keep the existing propagate-and-requeue behavior.
2. rollout/trafficrouting.go reconcileTrafficRouting(): when the sentinel is detected after UpdateHash(), skip SetWeight() for this round (preserving the ordering guarantee from fix(trafficrouting): ensure DestinationRule is updated before SetWeight on rollback #4612: never set weight while the DestinationRule switch is pending) but return nil so the rest of the reconcile still runs — abort/progressDeadline evaluation, conditions/phase updates, ReplicaSet cleanup, and the deadline-based requeue.
3. rollout/canary.go completedCurrentCanaryStep() + a trafficRoutingDelayed flag on rolloutContext: while the switch is delayed, the current step is not treated as completed, so currentStepIndex cannot advance while the desired traffic weight has not actually been applied. This keeps the progression backpressure that the fatal error used to provide.

Behavior before/after while the DestinationRule switch is delayed:

	before	after
SetWeight() called	no	no (unchanged, #4612 guarantee kept)
step index advances	no (reconcile aborted)	no (explicit guard)
progressDeadline / abort evaluated	never	every reconcile
Progressing → TimedOut, phase Degraded	never	on deadline expiry
ReplicaSet cleanup, analysis reconcile	never	every reconcile
fatal traffic-routing errors (API failures etc.)	propagate + requeue	propagate + requeue (unchanged)

Testing

• New: TestCanaryProgressDeadlineAbortNotBlockedByDelayedDestinationRuleSwitch reproduces the reported symptom end-to-end at the controller level: a canary rollout with progressDeadlineAbort: true, stuck past its deadline with a partially-available canary ReplicaSet, must still auto-abort while UpdateHash reports the delay. It fails on master (no abort patch is produced) and passes with this fix.
• New: TestReconcileTrafficRoutingUpdateHashDelayedErr verifies the delayed error is absorbed, SetWeight is not called (unstubbed on the mock, so a regression panics), the step is not completed (currentStepIndex does not advance, no RolloutStepCompleted event).
• Updated: TestRollbackDestinationRuleBeforeSetWeight (the fix(trafficrouting): ensure DestinationRule is updated before SetWeight on rollback #4612 scenario test) still asserts SetWeight is not called on delay, and now also asserts reconciliation continues (stale canary RS cleanup proceeds in the same sync instead of the sync erroring out).
• Updated: TestUpdateHashNoReadyReplicaSets additionally pins the errors.Is contract on the sentinel.
• Each new/updated assertion was verified to fail with the fix reverted (and with the step-completion guard disabled), so they genuinely pin the behavior.
• go build ./..., full go test ./... (0 failures), and golangci-lint run ./rollout/... (0 issues) pass locally.

Production context

This was hit three times in production (2026-05-07, 2026-05-29, 2026-06-23) on v1.8.2; details posted in #4626. The third recurrence affected 8 rollouts simultaneously and required a controller restart to recover. In all cases the trigger was canary pods that could never become available, and the rollouts stayed Progressing with no abort, no Degraded phase, and no status updates until manual intervention.

---

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• My builds are green. Try syncing with master if they are not.
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• I have run all tests locally (including the flaky ones) and they pass on my workstation — full unit suite passes locally; the Istio E2E suite (TestIstioSuite) was not run locally and relies on CI.
• I have used LLM/AI/Agent tools for this PR but I am responsible for all code of this PR
• I understand what the code does and WHY/HOW it works in several scenarios
• I know if my code is just adding new functionality or changing old functionality for existing users — this changes error-handling behavior for the Istio subset-level delayed-switch path only; all other error paths are unchanged.
• My organization is added to USERS.md.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.15%. Comparing base (f2c5c2b) to head (d7026be).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4823      +/-   ##
==========================================
- Coverage   85.17%   85.15%   -0.03%     
==========================================
  Files         166      166              
  Lines       19453    19459       +6     
==========================================
+ Hits        16570    16571       +1     
- Misses       2030     2033       +3     
- Partials      853      855       +2     

Flag	Coverage Δ
e2e	53.01% <100.00%> (-0.01%)	⬇️
unit-tests	81.64% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Published E2E Test Results

  4 files    4 suites   4h 8m 28s ⏱️
149 tests 134 ✅  7 💤  8 ❌
608 runs  568 ✅ 28 💤 12 ❌

For more details on these failures, see this check.

Results for commit d7026be.

♻️ This comment has been updated with latest results.

[github-actions]

Published Unit Test Results

2 627 tests   2 627 ✅  3m 29s ⏱️
  131 suites      0 💤
    1 files        0 ❌

Results for commit d7026be.

♻️ This comment has been updated with latest results.

[ChanghwanK]

Hi @zachaller / @kostis-codefresh, would appreciate a look at this when you have bandwidth.

This fixes a bug we've hit three times in production (2026-05-07, 2026-05-29, 2026-06-23 on v1.8.2) where canary rollouts get permanently stuck Progressing with no abort, no Degraded phase, and no status update, requiring manual intervention (once even a controller restart) to recover. Details in #4626.

The fix is small and scoped (sentinel error to distinguish the transient Istio DestinationRule delay from fatal errors), all CI is green, and it includes a new test that reproduces the exact stuck-forever scenario end-to-end and fails on master without the fix.

Happy to address any review feedback quickly.

[kostis-codefresh]

While reading the PR description and associated issue I had the feeling that we had fixed this same issue in the past.

I took a look at GitHub history and found the following

• #2507 (issue, 2023) first issue in this area
• #3602 (PR) first change in UpdateHash
• #4128 (issue, 2025-02) same issue as #4626 (progressDeadline doesn't take effect)
• #4287 (PR, closed unmerged) first PR that addresses the issue. but it was never merged
• #4299 (PR) first PR that was actually merged
• #4390 (issue) issue in the same area
• #4560 PR fixing #4390
• #4612 yet another Istio related PR

There I also added a comment where I said I feel we let Istio semantics "bleed" to the rest of the code.

I see that this PR is doing the same. We introduce a new flag trafficRoutingDelayed which in theory is universal but in practice is only used by Istio and not any of the other traffic providers.

@ChanghwanK First of all many thanks for the time and effort you put into this. Did your research turn up any of the issues/PRs above? I am asking because this area of the code has been changes a few times already, and I want to make sure any proposed solution is also taking into account all the previous issues.

@zachaller I think we need to make a hard choice. This is the second PR where we change generic traffic routing code because of Istio semantics. We can either continue this route (doing small Istio related fixes) or accept the fact that maybe the existing API is not enough for what Istio needs and we need a proper redesign. But right now I feel there is a contant stream of issues/PRs where the main theme is "Istio is not ready for X, Argo Rollouts things that everything is fine".

It seems to me that several Argo Rollouts functions have very simple semantics (either X happened, or there was an error) and we need to change the API to also include (X did not happen yet, because Istio/traffic provider was not ready, not an error yet). But instead of doing in a case-by-case basis we need to bake this into the API itself. Thoughts?

[ChanghwanK]

@kostis-codefresh Thank you for the thorough archaeology, this context is exactly what I needed.

Did my research turn up these issues/PRs? Partially, to be honest. I root-caused the bug to #4612 (which made the delay error fatal on the generic path) and traced the delay mechanism itself back to #3602. I had not found #4128 / #4287 / #4299 before your comment, so I went through them now. Here is how I see the history fitting together:

• Rollout stuck indefinitely when new pods fail to come up – abort and progressDeadline NOT triggering #4128 is the same symptom as progressDeadlineSeconds/progressDeadlineAbort silently non-functional on canary rollouts with Istio traffic routing when canary ReplicaSet is unavailable #4626 (progressDeadline never fires, rollout stuck Progressing).
• fix: progress deadline stuck with Istio delays #4287 took essentially the same approach as this PR (keep reconciling while the DestinationRule switch is delayed), but was closed in favor of fix: abort scenario where canary/stable service is not provided #4299.
• fix: abort scenario where canary/stable service is not provided #4299 fixed the deadlock inside istio.go by exempting the delay condition during an abort. That exemption is what my PR description calls "the designed exit for this loop" (istio.go:369-371); I referenced the code without knowing which PR introduced it.
• fix(trafficrouting): ensure DestinationRule is updated before SetWeight on rollback #4612 then made the delay error fatal in rolloutCanary(), which blocks reconciliation before evaluateProgressDeadlineAbort() can ever run.

So I believe #4299 and this PR are complementary rather than competing: #4299 makes the delay condition disappear once an abort is in progress, while this PR restores the path that declares the abort in the first place. #4612 unintentionally cut that path, which is why the symptom from #4128 resurfaced as #4626 (we hit it three times in production on v1.8.2). In other words, this PR does not replace any of the previous fixes; it makes the exit installed by #4299 reachable again.

On the "Istio semantics bleed" point: agreed, and the current shape of this PR is guilty of it. rollout/trafficrouting.go imports the istio package just for the sentinel. I can fix that within this PR by moving the sentinel to a provider-neutral location (e.g. the shared traffic routing package), with istio wrapping it. Generic code would then only know "the provider intentionally deferred applying the desired routing state", which any provider could signal, and the istio import goes away. Happy to push that change if you agree with the direction.

On the API redesign: I agree that "X did not happen yet, provider not ready" deserves a first-class representation in the TrafficRoutingReconciler interface rather than case-by-case error conventions, exactly as you suggested back on #4612. That said, it touches all provider implementations and deserves its own design discussion, so my suggestion would be to treat it as a follow-up: merge this PR as a targeted fix for the production-impacting bug (rollouts stuck un-abortable indefinitely), and I would be glad to participate in (or take a first stab at) the interface redesign afterwards, whichever direction @zachaller decides.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ChanghwanK]

Following up here, it's been about 2.5 weeks since my last reply and I know review bandwidth is limited, so no pressure on timeline.

Just wanted to keep this visible since it's still an active production issue for us (rollouts stuck un-abortable, requiring manual intervention to recover).

@zachaller whenever you get a chance, I'd appreciate your call on the two paths from my July 22 comment: merge this as a scoped fix now (I can go ahead and move the sentinel out of the istio package to address @kostis-codefresh's "semantics bleed" concern), or hold for the broader interface redesign first. Happy to start on either.

[zachaller]

Root cause

rolloutCanary() treats every traffic routing error as fatal. Status sync sits on the far side of that early return — that's where evaluateProgressDeadlineAbort() lives, along with the Progressing timeout and the stuck-rollout requeue.

If that's right, any provider that errors persistently gets stuck forever. Not just this case:

• A misconfigured nginx ingress failing SetWeight every reconcile.
• ALB at end-of-rollout, where trafficrouting.go:349 converts an unverified weight into a fatal error.

This PR fixes one instance of a class.

The alternative: #4962

To make this concrete, I put up #4962. It absorbs all traffic routing errors at the rolloutCanary() boundary: skip the stages that assume traffic shifted, always run status sync.

Two guards preserve what the fatal error used to protect:

• The step is never completed while routing wasn't applied.
• Stable is never promoted while routing wasn't applied or the weight is unverified — the canary twin of blue-green's areTargetsVerified(), which I believe also covers the ALB end-of-rollout case.

No sentinel. No errors.Is. No provider knowledge in generic code — generic code stops caring why routing wasn't applied.

@ChanghwanK — would value your eyes on #4962 too. It builds directly on your analysis and reuses your test approach.

[zachaller]

#4963 is maybe a more complete refactor as well