From 038c1485b3276e040a8fb9cf044cb7b7a1fc75fe Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Sun, 19 Jul 2026 12:04:59 +0530 Subject: [PATCH 1/3] Fix `updating` lock leak in `DashboardStateSync` Wrap handleURLChange and gotoNewState in try/finally so the updating lock is released on early returns and throws. Previously a bare return (unloaded default URL params or explore state) left the lock stuck, permanently freezing URL and session-storage sync until a page reload. --- .../loaders/DashboardStateSync.ts | 201 +++++++++--------- 1 file changed, 103 insertions(+), 98 deletions(-) diff --git a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts index 37057cc8fffb..373cc8fcf66a 100644 --- a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts +++ b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts @@ -214,89 +214,92 @@ export class DashboardStateSync { if (this.updating || !this.initialized) return; this.updating = true; - const { data: validSpecData } = get(this.dataLoader.validSpecQuery); - const metricsViewSpec = validSpecData?.metricsView ?? {}; - const exploreSpec = validSpecData?.explore ?? {}; - const { data: rillDefaultExploreURLParams } = get( - this.rillDefaultExploreURLParams, - ); + try { + const { data: validSpecData } = get(this.dataLoader.validSpecQuery); + const metricsViewSpec = validSpecData?.metricsView ?? {}; + const exploreSpec = validSpecData?.explore ?? {}; + const { data: rillDefaultExploreURLParams } = get( + this.rillDefaultExploreURLParams, + ); - // Type-safety - if (!rillDefaultExploreURLParams) return; + // Type-safety + if (!rillDefaultExploreURLParams) return; - const partialExplore = this.dataLoader.getExploreStateFromURLParams( - urlSearchParams, - type, - ); + const partialExplore = this.dataLoader.getExploreStateFromURLParams( + urlSearchParams, + type, + ); - // This can be undefined when one of the queries has not loaded yet. - // Rest of the code can be indeterminate when queries have not loaded. - // This shouldn't ideally happen. - if (!partialExplore) return; + // This can be undefined when one of the queries has not loaded yet. + // Rest of the code can be indeterminate when queries have not loaded. + // This shouldn't ideally happen. + if (!partialExplore) return; - const pageState = get(page); + const pageState = get(page); - if (metricsViewSpec.timeDimension && !import.meta.env.VITEST) { - // Resolve start/end by making a network call. - [ - partialExplore.selectedTimeRange, - // partialExplore.selectedComparisonTimeRange, - ] = await resolveTimeRanges( - this.client, - exploreSpec, + if (metricsViewSpec.timeDimension && !import.meta.env.VITEST) { + // Resolve start/end by making a network call. [ partialExplore.selectedTimeRange, // partialExplore.selectedComparisonTimeRange, - ], - partialExplore.selectedTimezone, - undefined, - partialExplore.selectedTimeDimension, + ] = await resolveTimeRanges( + this.client, + exploreSpec, + [ + partialExplore.selectedTimeRange, + // partialExplore.selectedComparisonTimeRange, + ], + partialExplore.selectedTimezone, + undefined, + partialExplore.selectedTimeDimension, + ); + } + + // Merge the partial state from url into the store + metricsExplorerStore.mergePartialExplorerEntity( + this.exploreName, + partialExplore, ); - } - - // Merge the partial state from url into the store - metricsExplorerStore.mergePartialExplorerEntity( - this.exploreName, - partialExplore, - ); - // Get time controls state after explore state is updated. - const timeControlsState = get(this.timeControlStore); - // Get the updated URL, this could be different from the page url if we added extra state. - // The extra state could come from session storage, home bookmark or yaml defaults - const redirectUrl = this.getUrlForExploreState(partialExplore); - - // Get the full updated state and save to session storage - const updatedExploreState = - get(metricsExplorerStore).entities[this.exploreName]; - updateExploreSessionStore( - this.exploreName, - this.extraPrefix, - updatedExploreState, - exploreSpec, - metricsViewSpec, - timeControlsState, - ); - if (!this.dataLoader.disableMostRecentDashboardState) { - // Update "most recent explore state" with updated state from url - saveMostRecentPartialExploreState( + // Get time controls state after explore state is updated. + const timeControlsState = get(this.timeControlStore); + // Get the updated URL, this could be different from the page url if we added extra state. + // The extra state could come from session storage, home bookmark or yaml defaults + const redirectUrl = this.getUrlForExploreState(partialExplore); + + // Get the full updated state and save to session storage + const updatedExploreState = + get(metricsExplorerStore).entities[this.exploreName]; + updateExploreSessionStore( this.exploreName, this.extraPrefix, updatedExploreState, + exploreSpec, + metricsViewSpec, + timeControlsState, ); + if (!this.dataLoader.disableMostRecentDashboardState) { + // Update "most recent explore state" with updated state from url + saveMostRecentPartialExploreState( + this.exploreName, + this.extraPrefix, + updatedExploreState, + ); + } + + // If the url doesn't need to be changed further then we can skip the goto + if (redirectUrl.search === pageState.url.search) { + return; + } + + // using `replaceState` directly messes up the navigation entries, + // `from` and `to` have the old url before being replaced in `afterNavigate` calls leading to incorrect handling. + await goto(redirectUrl, { + replaceState: true, + state: pageState.state, + }); + } finally { + this.updating = false; } - - this.updating = false; - // If the url doesn't need to be changed further then we can skip the goto - if (redirectUrl.search === pageState.url.search) { - return; - } - - // using `replaceState` directly messes up the navigation entries, - // `from` and `to` have the old url before being replaced in `afterNavigate` calls leading to incorrect handling. - return goto(redirectUrl, { - replaceState: true, - state: pageState.state, - }); } /** @@ -311,44 +314,46 @@ export class DashboardStateSync { if (this.updating) return; this.updating = true; - const { data: validSpecData } = get(this.dataLoader.validSpecQuery); - const exploreSpec = validSpecData?.explore ?? {}; - const metricsViewSpec = validSpecData?.metricsView ?? {}; - const timeControlsState = get(this.timeControlStore); + try { + const { data: validSpecData } = get(this.dataLoader.validSpecQuery); + const exploreSpec = validSpecData?.explore ?? {}; + const metricsViewSpec = validSpecData?.metricsView ?? {}; + const timeControlsState = get(this.timeControlStore); - const pageState = get(page); + const pageState = get(page); - // Get the new url params for the updated state - const newUrl = this.getUrlForExploreState(exploreState); + // Get the new url params for the updated state + const newUrl = this.getUrlForExploreState(exploreState); - // Update the session storage with the new explore state. - updateExploreSessionStore( - this.exploreName, - this.extraPrefix, - exploreState, - exploreSpec, - metricsViewSpec, - timeControlsState, - ); - if (!this.dataLoader.disableMostRecentDashboardState) { - // Update "most recent explore state" with updated state. - // Since we do not update the state per action we do it here as blanket update. - saveMostRecentPartialExploreState( + // Update the session storage with the new explore state. + updateExploreSessionStore( this.exploreName, this.extraPrefix, exploreState, + exploreSpec, + metricsViewSpec, + timeControlsState, ); - } + if (!this.dataLoader.disableMostRecentDashboardState) { + // Update "most recent explore state" with updated state. + // Since we do not update the state per action we do it here as blanket update. + saveMostRecentPartialExploreState( + this.exploreName, + this.extraPrefix, + exploreState, + ); + } + + // If the state didnt result in a new url then skip goto. + // This avoids adding redundant urls to the history. + if (newUrl.search === pageState.url.search) { + return; + } - // If the state didnt result in a new url then skip goto. - // This avoids adding redundant urls to the history. - if (newUrl.search === pageState.url.search) { + // dashboard changed so we should update the url + await goto(newUrl); + } finally { this.updating = false; - return; } - - // dashboard changed so we should update the url - await goto(newUrl); - this.updating = false; } } From 133cc169c9e9e021470adcc5540a40ef99c676fe Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Mon, 20 Jul 2026 11:31:54 +0530 Subject: [PATCH 2/3] Take the `updating` lock after guard clauses in `handleURLChange` --- .../loaders/DashboardStateSync.ts | 40 ++++++++++--------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts index 373cc8fcf66a..2aaa9742b6e8 100644 --- a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts +++ b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts @@ -212,31 +212,33 @@ export class DashboardStateSync { // Since we call this in afterNavigation, there could be a scenario where navigation completes but data for init isnt loaded yet. // Init already incorporates the url into the state so we can skip this processing. if (this.updating || !this.initialized) return; - this.updating = true; - try { - const { data: validSpecData } = get(this.dataLoader.validSpecQuery); - const metricsViewSpec = validSpecData?.metricsView ?? {}; - const exploreSpec = validSpecData?.explore ?? {}; - const { data: rillDefaultExploreURLParams } = get( - this.rillDefaultExploreURLParams, - ); + const { data: validSpecData } = get(this.dataLoader.validSpecQuery); + const metricsViewSpec = validSpecData?.metricsView ?? {}; + const exploreSpec = validSpecData?.explore ?? {}; + const { data: rillDefaultExploreURLParams } = get( + this.rillDefaultExploreURLParams, + ); - // Type-safety - if (!rillDefaultExploreURLParams) return; + // Type-safety + if (!rillDefaultExploreURLParams) return; - const partialExplore = this.dataLoader.getExploreStateFromURLParams( - urlSearchParams, - type, - ); + const partialExplore = this.dataLoader.getExploreStateFromURLParams( + urlSearchParams, + type, + ); - // This can be undefined when one of the queries has not loaded yet. - // Rest of the code can be indeterminate when queries have not loaded. - // This shouldn't ideally happen. - if (!partialExplore) return; + // This can be undefined when one of the queries has not loaded yet. + // Rest of the code can be indeterminate when queries have not loaded. + // This shouldn't ideally happen. + if (!partialExplore) return; - const pageState = get(page); + const pageState = get(page); + // Take the lock only once the guards have passed; + // the finally ensures a throw below cannot leave it stuck. + this.updating = true; + try { if (metricsViewSpec.timeDimension && !import.meta.env.VITEST) { // Resolve start/end by making a network call. [ From 651c2c502ed7271f0d416d03e4b08db2df15665d Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Mon, 20 Jul 2026 11:46:36 +0530 Subject: [PATCH 3/3] Release the `updating` lock before the redirect goto in `handleURLChange` Holding the lock through the awaited goto made gotoNewState drop any state change that landed while the navigation was in flight, so the URL silently missed those updates. Restore the original release point (before the goto) and keep the try/finally only around the state-merge section for the throw paths. --- .../loaders/DashboardStateSync.ts | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts index 2aaa9742b6e8..7b1c3e3e1240 100644 --- a/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts +++ b/web-common/src/features/dashboards/state-managers/loaders/DashboardStateSync.ts @@ -238,6 +238,7 @@ export class DashboardStateSync { // Take the lock only once the guards have passed; // the finally ensures a throw below cannot leave it stuck. this.updating = true; + let redirectUrl: URL; try { if (metricsViewSpec.timeDimension && !import.meta.env.VITEST) { // Resolve start/end by making a network call. @@ -266,7 +267,7 @@ export class DashboardStateSync { const timeControlsState = get(this.timeControlStore); // Get the updated URL, this could be different from the page url if we added extra state. // The extra state could come from session storage, home bookmark or yaml defaults - const redirectUrl = this.getUrlForExploreState(partialExplore); + redirectUrl = this.getUrlForExploreState(partialExplore); // Get the full updated state and save to session storage const updatedExploreState = @@ -287,21 +288,23 @@ export class DashboardStateSync { updatedExploreState, ); } - - // If the url doesn't need to be changed further then we can skip the goto - if (redirectUrl.search === pageState.url.search) { - return; - } - - // using `replaceState` directly messes up the navigation entries, - // `from` and `to` have the old url before being replaced in `afterNavigate` calls leading to incorrect handling. - await goto(redirectUrl, { - replaceState: true, - state: pageState.state, - }); } finally { + // Release before the goto below: state changes made while the navigation is in flight + // must still be picked up by gotoNewState. this.updating = false; } + + // If the url doesn't need to be changed further then we can skip the goto + if (redirectUrl.search === pageState.url.search) { + return; + } + + // using `replaceState` directly messes up the navigation entries, + // `from` and `to` have the old url before being replaced in `afterNavigate` calls leading to incorrect handling. + return goto(redirectUrl, { + replaceState: true, + state: pageState.state, + }); } /**