diff --git a/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadAction.tsx b/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadAction.tsx index b96e2c5f1db7..41f088c14a86 100644 --- a/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadAction.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadAction.tsx @@ -103,11 +103,13 @@ export const ExperimentReloadAction = ({ lastRefresh, onClick, progress, + queuedHint, }: { isRefreshing: boolean lastRefresh: string | null onClick: () => void progress?: { completed: number; total: number } + queuedHint?: string }): JSX.Element => { const { autoRefresh, experiment } = useValues(experimentLogic) const { setAutoRefresh, setPageVisibility, stopAutoRefreshInterval } = useActions(experimentLogic) @@ -158,7 +160,7 @@ export const ExperimentReloadAction = ({ size="xsmall" icon={isRefreshing ? : } data-attr="refresh-experiment" - disabledReason={isRefreshing ? 'Loading...' : null} + disabledReason={isRefreshing ? (queuedHint ?? 'Loading...') : null} sideAction={ ended ? undefined diff --git a/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadActionContainer.tsx b/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadActionContainer.tsx index 9317a609ba07..330faa35673f 100644 --- a/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadActionContainer.tsx +++ b/frontend/src/scenes/experiments/ExperimentView/ExperimentReloadActionContainer.tsx @@ -37,7 +37,7 @@ export function ExperimentReloadActionContainer({ function RecalculationReloadAction({ experiment }: { experiment: Experiment }): JSX.Element { const metricsLogic = experimentMetricsLogic({ experiment }) - const { isRecalculating, recalculationProgress, lastRefresh } = useValues(metricsLogic) + const { isRecalculating, recalculationProgress, lastRefresh, queuedRerun } = useValues(metricsLogic) const { triggerRecalculation } = useActions(metricsLogic) const { autoRefresh, currentRefresh } = useValues(experimentLogic) const { refreshExperimentResults, reportExperimentMetricsRefreshed } = useActions(experimentLogic) @@ -47,6 +47,7 @@ function RecalculationReloadAction({ experiment }: { experiment: Experiment }): isRefreshing={isRecalculating} lastRefresh={lastRefresh} progress={recalculationProgress} + queuedHint={queuedRerun ? 'Changes will apply after the current recalculation finishes' : undefined} onClick={() => { reportExperimentMetricsRefreshed(experiment, true, { triggered_by: 'manual', diff --git a/frontend/src/scenes/experiments/experimentMetricsLogic.test.ts b/frontend/src/scenes/experiments/experimentMetricsLogic.test.ts index abd65803062c..c10b79097c43 100644 --- a/frontend/src/scenes/experiments/experimentMetricsLogic.test.ts +++ b/frontend/src/scenes/experiments/experimentMetricsLogic.test.ts @@ -599,6 +599,54 @@ describe('experimentMetricsLogic', () => { }).toDispatchActions(['setCurrentRecalculation']) expect(createMock).toHaveBeenCalled() }) + + describe('queuing', () => { + it('queues instead of posting when a run is active', async () => { + const createMock = jest.fn(() => [201, pendingRecalculation]) + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [ + 200, + inProgressRecalculation, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + mountLogic() + await expectLogic(logic).toDispatchActions(['setCurrentRecalculation']) + expect(logic.values.isRecalculating).toBe(true) + + await expectLogic(logic, () => { + logic.actions.triggerRecalculation('metric_config_change') + }).toDispatchActions(['setQueuedRerun']) + + expect(logic.values.queuedRerun).toBe('metric_config_change') + expect(createMock).not.toHaveBeenCalled() + }) + + it('does not queue a cold_run (cold_run always starts fresh)', async () => { + const createMock = jest.fn(() => [201, pendingRecalculation]) + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [ + 200, + inProgressRecalculation, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + mountLogic() + await expectLogic(logic).toDispatchActions(['setCurrentRecalculation']) + expect(logic.values.isRecalculating).toBe(true) + + await expectLogic(logic, () => { + logic.actions.triggerRecalculation('cold_run') + }).toDispatchActions(['setCurrentRecalculation']) + + expect(logic.values.queuedRerun).toBeNull() + expect(createMock).toHaveBeenCalled() + }) + }) }) describe('loading + progress selectors', () => { @@ -743,6 +791,8 @@ describe('experimentMetricsLogic', () => { // Flush afterMount → 404 → trigger → poll(create), then drive enough ticks to exhaust retries. await jest.advanceTimersByTimeAsync(0) + // A config change queued a rerun while the run was in flight. + logic.actions.setQueuedRerun('metric_config_change') for (let i = 0; i < MAX_POLL_RETRIES + 2; i++) { await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS) } @@ -752,6 +802,9 @@ describe('experimentMetricsLogic', () => { expect(lemonToast.error).toHaveBeenCalledWith( 'Failed to load recalculation results. Please reload to try again.' ) + // The bail clears the queued rerun so it can't strand: the terminal branch (its only consumer) + // never runs on a give-up path. + expect(logic.values.queuedRerun).toBeNull() }) it('on a cold_run, applies partial results mid-flight before the run is terminal', async () => { @@ -805,6 +858,111 @@ describe('experimentMetricsLogic', () => { expect(logic.values.currentRecalculation).toEqual(expect.objectContaining({ status: 'in_progress' })) expect(logic.values.primaryMetricsResults[0]).toEqual(primaryResult) }) + + it('fires the queued rerun when the current run reaches terminal', async () => { + const createMock = jest.fn(() => [201, pendingRecalculation]) + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}], + '/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [ + 200, + inProgressRecalculation, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + jest.useFakeTimers() + mountLogic() + + // Flush the afterMount → 404 → cold-run trigger → poll(create) chain before queuing. + await jest.advanceTimersByTimeAsync(0) + + // A config change arrives while the cold run is active: it queues instead of posting. + logic.actions.triggerRecalculation('metric_config_change') + expect(logic.values.queuedRerun).toBe('metric_config_change') + + // Now let the active run reach terminal; the retrieve mock always answers 'in_progress', so + // repoint it to 'completed' for the tick that follows. + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}], + '/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [ + 200, + completedRecalculation2, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + + await expectLogic(logic, async () => { + await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS) + }).toDispatchActions([ + (a) => a.type === logic.actionTypes.setQueuedRerun && a.payload.trigger === null, + (a) => + a.type === logic.actionTypes.triggerRecalculation && a.payload.trigger === 'metric_config_change', + ]) + + expect(logic.values.queuedRerun).toBeNull() + // The rerun fired as a fresh run (not re-queued), since the prior run was terminal by then. + await jest.advanceTimersByTimeAsync(0) + expect(createMock).toHaveBeenCalledTimes(2) + }) + + it('coalesces multiple queued config changes into a single rerun at the stronger scope', async () => { + const createMock = jest.fn(() => [201, pendingRecalculation]) + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}], + '/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [ + 200, + inProgressRecalculation, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + jest.useFakeTimers() + mountLogic() + + // Flush the afterMount → 404 → cold-run trigger → poll(create) chain before queuing. + await jest.advanceTimersByTimeAsync(0) + + // Two config changes arrive while the cold run is still active: they coalesce to the + // stronger scope instead of posting twice, so exactly one rerun fires later. + logic.actions.triggerRecalculation('metric_config_change') + expect(logic.values.queuedRerun).toBe('metric_config_change') + logic.actions.triggerRecalculation('experiment_config_change') + expect(logic.values.queuedRerun).toBe('experiment_config_change') + + // Still one active run: neither queued change posted a create. + expect(createMock).toHaveBeenCalledTimes(1) + + // Now let the active run reach terminal; the retrieve mock always answers 'in_progress', so + // repoint it to 'completed' for the tick that follows. + useMocks({ + get: { + '/api/projects/:team_id/experiments/:id/metrics_recalculation/latest/': () => [404, {}], + '/api/projects/:team_id/experiments/:id/metrics_recalculation/:recalc_id/': () => [ + 200, + completedRecalculation2, + ], + }, + post: { '/api/projects/:team_id/experiments/:id/metrics_recalculation/': createMock }, + }) + + await expectLogic(logic, async () => { + await jest.advanceTimersByTimeAsync(POLL_INTERVAL_MS) + }).toDispatchActions([ + (a) => a.type === logic.actionTypes.setQueuedRerun && a.payload.trigger === null, + (a) => + a.type === logic.actionTypes.triggerRecalculation && + a.payload.trigger === 'experiment_config_change', + ]) + + expect(logic.values.queuedRerun).toBeNull() + // Exactly one fresh rerun fired, coalesced to the stronger scope. + await jest.advanceTimersByTimeAsync(0) + expect(createMock).toHaveBeenCalledTimes(2) + }) }) describe('liveRowsProgress', () => { @@ -949,4 +1107,23 @@ describe('experimentMetricsLogic', () => { expect(createMock).not.toHaveBeenCalled() }) }) + + describe('queuedRerun reducer', () => { + beforeEach(() => { + mountLogic() + }) + it('coalesces to the stronger scope', () => { + logic.actions.setQueuedRerun('metric_config_change') + expect(logic.values.queuedRerun).toBe('metric_config_change') + logic.actions.setQueuedRerun('experiment_config_change') + expect(logic.values.queuedRerun).toBe('experiment_config_change') + logic.actions.setQueuedRerun('metric_config_change') + expect(logic.values.queuedRerun).toBe('experiment_config_change') + }) + it('clears on null', () => { + logic.actions.setQueuedRerun('metric_config_change') + logic.actions.setQueuedRerun(null) + expect(logic.values.queuedRerun).toBeNull() + }) + }) }) diff --git a/frontend/src/scenes/experiments/experimentMetricsLogic.ts b/frontend/src/scenes/experiments/experimentMetricsLogic.ts index d41d8042041d..5fb416ec24fb 100644 --- a/frontend/src/scenes/experiments/experimentMetricsLogic.ts +++ b/frontend/src/scenes/experiments/experimentMetricsLogic.ts @@ -169,6 +169,7 @@ export interface experimentMetricsLogicValues { nextRetryAt: string | null primaryMetricsResults: CachedNewExperimentQueryResponse[] primaryMetricsResultsErrors: (unknown | null)[] + queuedRerun: ExperimentMetricsRecalculationTriggerEnumApi | null recalculatingMetricUuids: string[] recalculationDisplayState: 'cold' | 'initial' | 'partial' | 'refreshing' | 'resting' recalculationLoading: boolean @@ -248,6 +249,9 @@ export interface experimentMetricsLogicActions { setPrimaryMetricsResultsErrors: (errors: (unknown | null)[]) => { errors: unknown[] } + setQueuedRerun: (trigger: ExperimentMetricsRecalculationTriggerEnumApi | null) => { + trigger: ExperimentMetricsRecalculationTriggerEnumApi | null + } setRecalculatingMetricUuids: (uuids: string[]) => { uuids: string[] } @@ -318,6 +322,7 @@ export const experimentMetricsLogic = kea([ setRecalculationLoading: (loading: boolean) => ({ loading }), // The metrics still showing a stale value while a non-cold recalc refreshes them in place. setRecalculatingMetricUuids: (uuids: string[]) => ({ uuids }), + setQueuedRerun: (trigger: ExperimentMetricsRecalculationTriggerEnumApi | null) => ({ trigger }), }), reducers({ currentRecalculation: [ @@ -334,6 +339,16 @@ export const experimentMetricsLogic = kea([ setCurrentRecalculation: () => false, }, ], + queuedRerun: [ + null as ExperimentMetricsRecalculationTriggerEnumApi | null, + { + /** + * If the state is `experiment_config_change`, it sticks. + */ + setQueuedRerun: (state, { trigger }) => + trigger === null ? null : state === 'experiment_config_change' ? state : trigger, + }, + ], primaryMetricsResults: [ [] as CachedNewExperimentQueryResponse[], { @@ -698,6 +713,17 @@ export const experimentMetricsLogic = kea([ if (!experimentIsLaunched()) { return } + /** + * If we have a recalculation in flight, and the user has changed the experiment or the metrics configuration, + * we schedule a new recalculation with the corresponding trigger. + */ + if ( + (trigger === 'experiment_config_change' || trigger === 'metric_config_change') && + values.isRecalculating + ) { + actions.setQueuedRerun(trigger) + return + } /** * Dim the metrics that already show something (a value or an error) so they read as * "refreshing" until the new result streams in. Cold runs have nothing prior, so nothing to dim. @@ -810,8 +836,14 @@ export const experimentMetricsLogic = kea([ } cache.activeRecalculationId = recalculationId + /** + * if for any reason the recalculation workflow never reaches terminal state, we need to + * stop polling, and clear recalculation state. This includes any queued rerun, that will never + * be fired. + */ if (Date.now() - (cache.recalcStartMs ?? Date.now()) > MAX_POLL_DURATION_MS) { actions.setRecalculatingMetricUuids([]) + actions.setQueuedRerun(null) lemonToast.error('Recalculation appears stuck. Please reload to try again.') return } @@ -830,9 +862,11 @@ export const experimentMetricsLogic = kea([ /** * Retry on transient errors, but give up after MAX_POLL_RETRIES consecutive failures so a * persistently failing endpoint can't poll forever. + * If we reach the retry cap, we also need to clear queued reruns that never fire. */ cache.pollRetryCount = (cache.pollRetryCount ?? 0) + 1 if (cache.pollRetryCount >= MAX_POLL_RETRIES) { + actions.setQueuedRerun(null) lemonToast.error('Failed to load recalculation results. Please reload to try again.') return } @@ -893,6 +927,17 @@ export const experimentMetricsLogic = kea([ `${recalculation.failed_metrics} of ${recalculation.total_metrics} metrics failed to load` ) } + + /** + * The user has changed the experiment or the metrics configuration while a recalculation was in flight. + * We clear the flag an trigger a new recalculation to reflect the changes. Cache resolution is handled by the + * durable execution backend, so we'll try to reuse the cached results. + */ + const queued = values.queuedRerun + if (queued) { + actions.setQueuedRerun(null) + actions.triggerRecalculation(queued) + } }, } }),