fix: Improve media_query retry behavior (#2603)

src/card-controller/issues/issue-manager.ts

@@ -187,9 +187,21 @@ export class IssueManager {
 
   private _scheduleRetryIfNeeded(): void {
     if (!this._stateManager.needsRetry()) {
+      // No issue still has a failing problem, so reset the backoff: a future
+      // failure should start again from the base delay.
       this._retryTimer.reset();
       return;
     }
+    if (!this._stateManager.canRetryNow()) {
+      // An issue still has a failing problem but cannot retry right now. Cancel
+      // the pending timer so it does not fire a retry the issue cannot take --
+      // but do not reset(), which is reserved for the resolved case above.
+      // Consequently no retry is scheduled here; scheduling resumes on a later
+      // evaluate() (any condition-state change re-runs this), once the issue
+      // reports it can retry again or has resolved.
+      this._retryTimer.cancel();
+      return;
+    }
     if (this._retryTimer.isRunning()) {
       return;
     }

src/card-controller/issues/issues/media-query.ts

@@ -17,27 +17,78 @@ export class MediaQueryIssue extends AbstractErrorIssue {
 
   private _api: CardIssueManagerAPI;
 
+  // True while a retry query is in flight. The error is retained throughout (so
+  // the issue stays visible), but no further retry is scheduled until this
+  // attempt resolves via a success (reset) or a fresh failure (trigger).
+  private _retrying = false;
+
   constructor(api: CardIssueManagerAPI) {
     super();
     this._api = api;
   }
 
+  // A fresh outcome (success or failure) ends any in-flight retry.
+  public trigger(context: { error: unknown }): void {
+    this._retrying = false;
+    super.trigger(context);
+  }
+
+  public reset(): void {
+    this._retrying = false;
+    super.reset();
+  }
+
+  // There is still a failed query to retry as long as an error is present --
+  // including while a retry is in flight -- so the backoff keeps escalating
+  // across attempts instead of restarting each time one is dispatched.
   public needsRetry(): boolean {
     return this._error !== null;
   }
 
+  // A retry can be dispatched only when one is not already in flight; otherwise
+  // the manager waits for the in-flight attempt to succeed or fail.
+  public canRetryNow(): boolean {
+    return this._error !== null && !this._retrying;
+  }
+
   public retry(): boolean {
     if (this._error === null) {
       return false;
     }
-    this._error = null;
-    void this._api.getViewManager().setViewByParametersWithNewQuery();
+
+    // An attempt is already running (only a forced retry reaches here while in
+    // flight; scheduled ones are gated by canRetryNow). Do not dispatch a second
+    // concurrent query -- the retry the caller wants is already happening.
+    if (this._retrying) {
+      return true;
+    }
+
+    // Mark the attempt in flight and keep the error set: it is cleared only on
+    // confirmed success, or replaced on the next failure. The 'retry' intent
+    // tells the view manager not to clear it up front (see
+    // setViewByParametersWithNewQuery). The error must stay visible until it is
+    // resolved, without any "flicker"/resetting.
+    this._retrying = true;
+    this._runRetryQuery().catch(() => {});
 
     // Exclusive retry. No other issue should attempt to retry until the next
     // evaluation cycle, when we'll know if this was successful.
     return true;
   }
 
+  private async _runRetryQuery(): Promise<void> {
+    try {
+      await this._api
+        .getViewManager()
+        .setViewByParametersWithNewQuery({ intent: 'retry' });
+    } finally {
+      // The attempt has settled. On the normal paths a success (reset) or a
+      // failure (trigger) has already cleared this flag; the query is not
+      // guaranteed to report either, so clear it here as a fallback.
+      this._retrying = false;
+    }
+  }
+
   public getNotification(): Notification | null {
     return this.getIssue()?.notification ?? null;
   }

src/card-controller/issues/state-manager.ts

@@ -114,13 +114,17 @@ export class IssueStateManager implements IssueReadOnlyState {
     return [...this._issues.values()].some((issue) => issue.needsRetry?.());
   }
 
+  public canRetryNow(): boolean {
+    return [...this._issues.values()].some((issue) => this._canRetryNow(issue));
+  }
+
   public retry(key?: IssueKey, force?: boolean): void {
     const issues = key
       ? [this._issues.get(key)].filter(isTruthy)
       : [...this._issues.values()];
 
     for (const issue of issues) {
-      if (!force && !issue.needsRetry?.()) {
+      if (!force && !this._canRetryNow(issue)) {
         continue;
       }
       if (issue.retry?.()) {
@@ -162,6 +166,12 @@ export class IssueStateManager implements IssueReadOnlyState {
   // Private helpers.
   // =========================================================================
 
+  // An issue that does not implement canRetryNow falls back to needsRetry, so
+  // issues with no in-flight concept can be retried whenever they need one.
+  private _canRetryNow(issue: Issue): boolean {
+    return issue.canRetryNow?.() ?? issue.needsRetry?.() ?? false;
+  }
+
   private _logIfNew(issue: Issue): void {
     const description = issue.getIssue();
     if (!description) {

src/card-controller/issues/types.ts

@@ -72,10 +72,16 @@ export interface Issue {
   // depends on transient state (e.g. no current error to show).
   getNotification?(): Notification | null;
 
-  // Whether this issue wants the manager to schedule a retry. Gates
-  // scheduled retries; user-initiated (forced) retries bypass this check.
+  // Whether this issue has an unresolved problem that needs retry. The manager
+  // keeps the backoff counter alive while this is true and clears it once the
+  // problem resolves. Stays true across an in-flight retry (see canRetryNow).
   needsRetry?(): boolean;
 
+  // Whether a retry can run right now. False while one is already in flight, so
+  // the manager waits without resetting the backoff. Forced (user-initiated)
+  // retries bypass it. Defaults to needsRetry() when not implemented.
+  canRetryNow?(): boolean;
+
   // Called by the manager when a retry is due. Returns true to stop the retry
   // loop (exclusive), false to allow subsequent issues to also retry.
   retry?(): boolean;

src/card-controller/view/types.ts

@@ -50,6 +50,12 @@ export interface ViewFactoryOptions {
   // user-facing locks (e.g. an active call ending).
   force?: boolean;
 
+  // Why this view change is being made. 'retry' marks a re-run of a previously
+  // failed query and, unlike a navigation, does not clear the existing error(s)
+  // up front: it persists until this attempt resolves. Defaults to
+  // 'navigation'.
+  intent?: 'navigation' | 'retry';
+
   // Options for the query executor that control how a query is executed and the
   // result selected.
   queryExecutorOptions?: QueryExecutorOptions;

src/card-controller/view/view-manager.ts

@@ -218,10 +218,6 @@ export class ViewManager implements ViewManagerInterface {
         },
       });
       this._api.getIssueManager().reset('view_incompatible');
-      // A new query is about to run, so any stale media_query error from a
-      // previous attempt is no longer meaningful. If this new query also
-      // fails, it will re-trigger below.
-      this._api.getIssueManager().reset('media_query');
     } catch (e) {
       if (!this._view) {
         initialView = this._getFailSafeView(viewFactoryFunc);
@@ -233,6 +229,15 @@ export class ViewManager implements ViewManagerInterface {
       return;
     }
 
+    // A new query is about to run and is allowed to commit, so any stale
+    // media_query error from a previous attempt is no longer meaningful (on
+    // failure it re-triggers below). A retry is the exception: it re-runs the
+    // same failing query, so its error must persist until this attempt resolves
+    // rather than being cleared here and re-appearing on the next failure.
+    if (options?.intent !== 'retry') {
+      this._api.getIssueManager().reset('media_query');
+    }
+
     if (this._view && this._shouldAdoptQueryAndResults(initialView)) {
       initialView.query = this._view.query;
       initialView.queryResults = this._view.queryResults;

tests/card-controller/issues/issue-manager.test.ts

@@ -771,6 +771,57 @@ describe('IssueManager', () => {
     });
   });
 
+  describe('in-flight retry', () => {
+    it('should hold the backoff and not arm a timer while a retry is in flight', () => {
+      vi.spyOn(Math, 'random').mockReturnValue(0.5);
+      const api = createCardAPI();
+      const config = createConfig();
+      vi.mocked(api.getConfigManager().getConfig).mockReturnValue({
+        ...config,
+        view: {
+          ...config.view,
+          issues: { interaction_mode: 'all', retry_seconds: 'auto' },
+        },
+      });
+      const manager = new IssueManager(api);
+
+      const canRetryNow = vi.fn().mockReturnValue(true);
+      const issue = createIssue('media_query', {
+        hasIssue: vi.fn().mockReturnValue(true),
+        needsRetry: vi.fn().mockReturnValue(true),
+        canRetryNow,
+        retry: vi.fn().mockReturnValue(false),
+      });
+      manager.addIssue(issue);
+
+      manager.evaluate();
+
+      // First attempt fires at the base delay, advancing the backoff to
+      // attempt 1.
+      vi.advanceTimersByTime(RETRY_EXPONENTIAL_BASE_SECONDS * 0.75 * 1000);
+      expect(issue.retry).toBeCalledTimes(1);
+
+      // The attempt is now in flight: the problem is still unresolved
+      // (needsRetry) but cannot be retried right now (canRetryNow). The running
+      // timer is cancelled and no further attempt fires, however long we wait.
+      canRetryNow.mockReturnValue(false);
+      manager.evaluate();
+      vi.advanceTimersByTime(RETRY_EXPONENTIAL_MAX_SECONDS * 1000);
+      expect(issue.retry).toBeCalledTimes(1);
+
+      // The attempt fails and becomes retryable again. Because the backoff was
+      // preserved, the next delay is the attempt-1 step (base*2), not base.
+      canRetryNow.mockReturnValue(true);
+      manager.evaluate();
+
+      vi.advanceTimersByTime(RETRY_EXPONENTIAL_BASE_SECONDS * 0.75 * 1000);
+      expect(issue.retry).toBeCalledTimes(1);
+
+      vi.advanceTimersByTime(RETRY_EXPONENTIAL_BASE_SECONDS * 2 * 0.75 * 1000);
+      expect(issue.retry).toBeCalledTimes(2);
+    });
+  });
+
   describe('reset', () => {
     it('should reset a specific issue and re-evaluate', () => {
       const api = createCardAPI();

tests/card-controller/issues/issues/media-query.test.ts

@@ -1,9 +1,9 @@
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, vi } from 'vitest';
 
 import type { CardController } from '../../../../src/card-controller/controller';
 import { MediaQueryIssue } from '../../../../src/card-controller/issues/issues/media-query';
 import type { InternalCallbackActionConfig } from '../../../../src/config/schema/actions/custom/internal';
-import { createCardAPI } from '../../../test-utils';
+import { createCardAPI, flushPromises } from '../../../test-utils';
 
 const createIssue = (): {
   issue: MediaQueryIssue;
@@ -106,26 +106,122 @@ describe('MediaQueryIssue', () => {
     });
   });
 
+  describe('canRetryNow', () => {
+    it('should return false when not triggered', () => {
+      const { issue } = createIssue();
+
+      expect(issue.canRetryNow()).toBe(false);
+    });
+
+    it('should return true when triggered but false while a retry is in flight', () => {
+      const { issue } = createIssue();
+      issue.trigger({ error: new Error('query failed') });
+
+      expect(issue.canRetryNow()).toBe(true);
+
+      issue.retry();
+
+      expect(issue.canRetryNow()).toBe(false);
+    });
+  });
+
   describe('retry', () => {
-    it('should return requery action and clear error and needsRetry', () => {
+    it('should re-run the query with the retry intent and keep the error present', () => {
       const { issue, api } = createIssue();
       issue.trigger({ error: new Error('query failed') });
 
       const result = issue.retry();
 
       expect(result).toEqual(true);
-      expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalled();
-      expect(issue.needsRetry()).toBe(false);
-      expect(issue.hasIssue()).toBe(false);
+      expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledWith({
+        intent: 'retry',
       });
 
-    it('should return null when needsRetry is false', () => {
+      // The error persists across the retry so the issue stays visible and the
+      // problem stays unresolved, but no further retry can be dispatched until
+      // this in-flight attempt resolves.
+      expect(issue.hasIssue()).toBe(true);
+      expect(issue.needsRetry()).toBe(true);
+      expect(issue.canRetryNow()).toBe(false);
+    });
+
+    it('should allow a retry again once a fresh failure is reported via trigger', () => {
+      const { issue } = createIssue();
+      issue.trigger({ error: new Error('query failed') });
+      issue.retry();
+      expect(issue.canRetryNow()).toBe(false);
+
+      // The failure is reported out-of-band via trigger (the normal path), which
+      // ends the in-flight attempt.
+      issue.trigger({ error: new Error('query failed again') });
+
+      expect(issue.canRetryNow()).toBe(true);
+    });
+
+    it('should fully resolve when the in-flight attempt succeeds', () => {
+      const { issue } = createIssue();
+      issue.trigger({ error: new Error('query failed') });
+      issue.retry();
+
+      // A success clears the issue via reset.
+      issue.reset();
+
+      expect(issue.hasIssue()).toBe(false);
+      expect(issue.needsRetry()).toBe(false);
+      expect(issue.canRetryNow()).toBe(false);
+    });
+
+    it('should return null when there is no error to retry', () => {
       const { issue } = createIssue();
 
       const result = issue.retry();
 
       expect(result).toBe(false);
     });
+
+    it('should not dispatch a second query when a retry is already in flight', () => {
+      const { issue, api } = createIssue();
+      issue.trigger({ error: new Error('query failed') });
+
+      issue.retry();
+      // A forced retry (e.g. the user tapping retry) while the first is in
+      // flight must not start a competing query.
+      const result = issue.retry();
+
+      expect(result).toBe(true);
+      expect(api.getViewManager().setViewByParametersWithNewQuery).toBeCalledTimes(1);
+    });
+
+    it('should stop treating the retry as in flight once the query settles without an outcome', async () => {
+      const { issue, api } = createIssue();
+      vi.mocked(
+        api.getViewManager().setViewByParametersWithNewQuery,
+      ).mockResolvedValue();
+      issue.trigger({ error: new Error('query failed') });
+
+      issue.retry();
+      expect(issue.canRetryNow()).toBe(false);
+
+      // The query resolved without reporting a trigger/reset outcome; the
+      // in-flight flag must still clear so a further retry can be dispatched.
+      await flushPromises();
+
+      expect(issue.canRetryNow()).toBe(true);
+    });
+
+    it('should stop treating the retry as in flight when the query rejects', async () => {
+      const { issue, api } = createIssue();
+      vi.mocked(api.getViewManager().setViewByParametersWithNewQuery).mockRejectedValue(
+        new Error('boom'),
+      );
+      issue.trigger({ error: new Error('query failed') });
+
+      issue.retry();
+
+      await flushPromises();
+
+      expect(issue.canRetryNow()).toBe(true);
+    });
   });
 
   it('should clear the issue after reset', () => {

tests/card-controller/issues/state-manager.test.ts

@@ -221,6 +221,30 @@ describe('IssueStateManager', () => {
     });
   });
 
+  describe('canRetryNow', () => {
+    it('should return false when no issue can retry now', () => {
+      expect(createManager().canRetryNow()).toBe(false);
+    });
+
+    it('should return true when an issue reports it can retry now', () => {
+      assert(mockMediaLoad.canRetryNow);
+      vi.mocked(mockMediaLoad.canRetryNow).mockReturnValue(true);
+
+      expect(createManager().canRetryNow()).toBe(true);
+    });
+
+    it('should fall back to needsRetry for an issue that does not implement canRetryNow', () => {
+      const issue: Issue = {
+        key: 'media_query',
+        hasIssue: () => true,
+        getIssue: () => null,
+        needsRetry: () => true,
+      };
+
+      expect(createManager([issue]).canRetryNow()).toBe(true);
+    });
+  });
+
   describe('retry', () => {
     it('should call retry on issues that want retry with non-exclusive result', () => {
       assert(mockMediaLoad.needsRetry);

tests/card-controller/view/view-manager.test.ts

@@ -579,6 +579,31 @@ describe('should handle exceptions', () => {
         .mock.calls.filter(([key]) => key === 'media_query').length,
     ).toBe(2);
   });
+
+  it('should not reset media_query on the retry dispatch, keeping the error visible', async () => {
+    const error = new Error();
+    const viewFactory = mock<ViewFactory>();
+    viewFactory.getViewByParameters.mockReturnValue(createView());
+    const viewQueryExecutor = mock<ViewQueryExecutor>();
+    viewQueryExecutor.getNewQueryModifiers.mockRejectedValue(error);
+
+    const api = createInitializedCardAPI();
+    const manager = new ViewManager(api, {
+      viewFactory,
+      viewQueryExecutor,
+    });
+
+    await manager.setViewByParametersWithNewQuery({ intent: 'retry' });
+
+    // A retry re-runs the same failing query, so the existing error must not be
+    // cleared up front (it would blink away and back); the failure just
+    // re-triggers it.
+    expect(api.getIssueManager().reset).not.toBeCalledWith('media_query');
+    expect(api.getIssueManager().trigger).toBeCalledWith(
+      'media_query',
+      expect.objectContaining({ error }),
+    );
+  });
 });
 
 describe('hasMajorMediaChange', () => {