diff --git a/src/card-controller/issues/issue-manager.ts b/src/card-controller/issues/issue-manager.ts index a0912394..e546805a 100644 --- a/src/card-controller/issues/issue-manager.ts +++ b/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; } diff --git a/src/card-controller/issues/issues/media-query.ts b/src/card-controller/issues/issues/media-query.ts index fab05783..f415d437 100644 --- a/src/card-controller/issues/issues/media-query.ts +++ b/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 { + 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; } diff --git a/src/card-controller/issues/state-manager.ts b/src/card-controller/issues/state-manager.ts index 2afc3020..ea18c0ab 100644 --- a/src/card-controller/issues/state-manager.ts +++ b/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) { diff --git a/src/card-controller/issues/types.ts b/src/card-controller/issues/types.ts index 3606299a..454ee032 100644 --- a/src/card-controller/issues/types.ts +++ b/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; diff --git a/src/card-controller/view/types.ts b/src/card-controller/view/types.ts index e3a8d5d4..9c448055 100644 --- a/src/card-controller/view/types.ts +++ b/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; diff --git a/src/card-controller/view/view-manager.ts b/src/card-controller/view/view-manager.ts index 36a5db85..bb9d6efe 100644 --- a/src/card-controller/view/view-manager.ts +++ b/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; diff --git a/tests/card-controller/issues/issue-manager.test.ts b/tests/card-controller/issues/issue-manager.test.ts index f0146f90..c78dbfe5 100644 --- a/tests/card-controller/issues/issue-manager.test.ts +++ b/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(); diff --git a/tests/card-controller/issues/issues/media-query.test.ts b/tests/card-controller/issues/issues/media-query.test.ts index bbb97775..92e19d7a 100644 --- a/tests/card-controller/issues/issues/media-query.test.ts +++ b/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', + }); + + // 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 return null when needsRetry is 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', () => { diff --git a/tests/card-controller/issues/state-manager.test.ts b/tests/card-controller/issues/state-manager.test.ts index cd7a6d44..57d33069 100644 --- a/tests/card-controller/issues/state-manager.test.ts +++ b/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); diff --git a/tests/card-controller/view/view-manager.test.ts b/tests/card-controller/view/view-manager.test.ts index c3bc91d5..696bddb2 100644 --- a/tests/card-controller/view/view-manager.test.ts +++ b/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.getViewByParameters.mockReturnValue(createView()); + const viewQueryExecutor = mock(); + 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', () => {