Skip to content

Commit d2c3246

Browse files
[9.2] [Background search] Fix race condition with sending to background & immediately canceling (elastic#238151) (elastic#240354)
# Backport This will backport the following commits from `main` to `9.2`: - [[Background search] Fix race condition with sending to background & immediately canceling (elastic#238151)](elastic#238151) <!--- Backport version: 9.6.6 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Lukas Olson","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-10-23T19:57:23Z","message":"[Background search] Fix race condition with sending to background & immediately canceling (elastic#238151)\n\n## Summary\n\nPartially addresses https://github.com/elastic/kibana/issues/237771.\n\nPrevents async search requests from being canceled during the state\nwhere the background search saved object is being created (loading) &\nthe user cancels before it is finished being saved.\n\nEnsures that even after cancelling, we still poll after the saved object\nis created to ensure the search is tracked by the saved object.\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [ ] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n- [ ] If a plugin configuration key changed, check if it needs to be\nallowlisted in the cloud and added to the [docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n- [ ] This was checked for breaking HTTP API changes, and any breaking\nchanges have been approved by the breaking-change committee. The\n`release_note:breaking` label should be applied in these situations.\n- [ ] [Flaky Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\nused on any tests changed\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n- [ ] Review the [backport\nguidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)\nand apply applicable `backport:*` labels.\n\n### Identify risks\n\nDoes this PR introduce any risks? For example, consider risks like hard\nto test bugs, performance regression, potential of data loss.\n\nDescribe the risk, its severity, and mitigation for each identified\nrisk. Invite stakeholders and evaluate how to proceed before merging.\n\n- [ ] [See some risk\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)\n- [ ] ...","sha":"4fbfabc446304902debea05d06ce8ac6fcf53e6a","branchLabelMapping":{"^v9.3.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Project:AsyncSearch","Team:DataDiscovery","backport:version","v9.2.0","v9.3.0"],"title":"[Background search] Fix race condition with sending to background & immediately canceling","number":238151,"url":"https://github.com/elastic/kibana/pull/238151","mergeCommit":{"message":"[Background search] Fix race condition with sending to background & immediately canceling (elastic#238151)\n\n## Summary\n\nPartially addresses https://github.com/elastic/kibana/issues/237771.\n\nPrevents async search requests from being canceled during the state\nwhere the background search saved object is being created (loading) &\nthe user cancels before it is finished being saved.\n\nEnsures that even after cancelling, we still poll after the saved object\nis created to ensure the search is tracked by the saved object.\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [ ] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n- [ ] If a plugin configuration key changed, check if it needs to be\nallowlisted in the cloud and added to the [docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n- [ ] This was checked for breaking HTTP API changes, and any breaking\nchanges have been approved by the breaking-change committee. The\n`release_note:breaking` label should be applied in these situations.\n- [ ] [Flaky Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\nused on any tests changed\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n- [ ] Review the [backport\nguidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)\nand apply applicable `backport:*` labels.\n\n### Identify risks\n\nDoes this PR introduce any risks? For example, consider risks like hard\nto test bugs, performance regression, potential of data loss.\n\nDescribe the risk, its severity, and mitigation for each identified\nrisk. Invite stakeholders and evaluate how to proceed before merging.\n\n- [ ] [See some risk\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)\n- [ ] ...","sha":"4fbfabc446304902debea05d06ce8ac6fcf53e6a"}},"sourceBranch":"main","suggestedTargetBranches":["9.2"],"targetPullRequestStates":[{"branch":"9.2","label":"v9.2.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.3.0","branchLabelMappingKey":"^v9.3.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/238151","number":238151,"mergeCommit":{"message":"[Background search] Fix race condition with sending to background & immediately canceling (elastic#238151)\n\n## Summary\n\nPartially addresses https://github.com/elastic/kibana/issues/237771.\n\nPrevents async search requests from being canceled during the state\nwhere the background search saved object is being created (loading) &\nthe user cancels before it is finished being saved.\n\nEnsures that even after cancelling, we still poll after the saved object\nis created to ensure the search is tracked by the saved object.\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [ ] Any text added follows [EUI's writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\nsentence case text and includes [i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas added for features that require explanation or tutorials\n- [ ] [Unit or functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere updated or added to match the most common scenarios\n- [ ] If a plugin configuration key changed, check if it needs to be\nallowlisted in the cloud and added to the [docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n- [ ] This was checked for breaking HTTP API changes, and any breaking\nchanges have been approved by the breaking-change committee. The\n`release_note:breaking` label should be applied in these situations.\n- [ ] [Flaky Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\nused on any tests changed\n- [ ] The PR description includes the appropriate Release Notes section,\nand the correct `release_note:*` label is applied per the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n- [ ] Review the [backport\nguidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing)\nand apply applicable `backport:*` labels.\n\n### Identify risks\n\nDoes this PR introduce any risks? For example, consider risks like hard\nto test bugs, performance regression, potential of data loss.\n\nDescribe the risk, its severity, and mitigation for each identified\nrisk. Invite stakeholders and evaluate how to proceed before merging.\n\n- [ ] [See some risk\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)\n- [ ] ...","sha":"4fbfabc446304902debea05d06ce8ac6fcf53e6a"}}]}] BACKPORT--> Co-authored-by: Lukas Olson <[email protected]>
1 parent f70a494 commit d2c3246

File tree

6 files changed

+62
-12
lines changed

6 files changed

+62
-12
lines changed

src/platform/plugins/shared/data/common/search/poll_search.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ export const pollSearch = <Response extends IKibanaSearchResponse>(
6262
return from(search()).pipe(
6363
expand(() => {
6464
const elapsedTime = Date.now() - startTime;
65-
return timer(getPollInterval(elapsedTime)).pipe(switchMap(search));
65+
return timer(getPollInterval(elapsedTime)).pipe(switchMap(() => search()));
6666
}),
6767
tap((response) => {
6868
if (isAbortResponse(response)) {

src/platform/plugins/shared/data/public/search/search_interceptor/search_interceptor.ts

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,9 @@ export class SearchInterceptor {
292292
) {
293293
const { sessionId, strategy } = options;
294294

295-
const search = () => {
295+
const search = ({
296+
abortSignal = searchAbortController.getSignal(),
297+
}: Pick<ISearchOptions, 'abortSignal'> = {}) => {
296298
const [{ isSearchStored }, afterPoll] = searchTracker?.beforePoll() ?? [
297299
{ isSearchStored: false },
298300
() => {},
@@ -302,7 +304,7 @@ export class SearchInterceptor {
302304
{
303305
...options,
304306
...this.deps.session.getSearchOptions(sessionId),
305-
abortSignal: searchAbortController.getSignal(),
307+
abortSignal,
306308
isSearchStored,
307309
}
308310
)
@@ -319,9 +321,9 @@ export class SearchInterceptor {
319321
const searchTracker = this.deps.session.isCurrentSession(sessionId)
320322
? this.deps.session.trackSearch({
321323
abort: () => searchAbortController.abort(),
322-
poll: async () => {
324+
poll: async (abortSignal) => {
323325
if (id) {
324-
await search();
326+
await search({ abortSignal });
325327
}
326328
},
327329
})
@@ -330,7 +332,8 @@ export class SearchInterceptor {
330332
// track if this search's session will be send to background
331333
// if yes, then we don't need to cancel this search when it is aborted
332334
let isSavedToBackground =
333-
this.deps.session.isCurrentSession(sessionId) && this.deps.session.isStored();
335+
this.deps.session.isCurrentSession(sessionId) &&
336+
(this.deps.session.isSaving() || this.deps.session.isStored());
334337
const savedToBackgroundSub =
335338
this.deps.session.isCurrentSession(sessionId) &&
336339
this.deps.session.state$
@@ -415,8 +418,11 @@ export class SearchInterceptor {
415418
})
416419
);
417420
} else {
418-
searchTracker?.error();
419-
cancel();
421+
// Don't error out the search or cancel if it is being saved to the background
422+
if (!isSavedToBackground) {
423+
searchTracker?.error();
424+
cancel();
425+
}
420426
return throwError(e);
421427
}
422428
}),

src/platform/plugins/shared/data/public/search/session/mocks.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ export function getSessionServiceMock(
5252
})),
5353
destroy: jest.fn(),
5454
cancel: jest.fn(),
55+
isSaving: jest.fn(),
5556
isStored: jest.fn(),
5657
isRestore: jest.fn(),
5758
save: jest.fn(),

src/platform/plugins/shared/data/public/search/session/search_session_state.test.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,5 +132,16 @@ describe('Session state container', () => {
132132
state.transitions.start({ appName });
133133
expect(state.selectors.getState()).toBe(SearchSessionState.None);
134134
});
135+
136+
test('save sets isSaving: true and store sets isSaving: false', () => {
137+
state.transitions.start({ appName });
138+
expect(state.get().isSaving).toBe(false);
139+
140+
state.transitions.save();
141+
expect(state.get().isSaving).toBe(true);
142+
143+
state.transitions.store(mockSavedObject);
144+
expect(state.get().isSaving).toBe(false);
145+
});
135146
});
136147
});

src/platform/plugins/shared/data/public/search/session/search_session_state.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ export interface SessionStateInternal<SearchDescriptor = unknown, SearchMeta ext
9292
*/
9393
appName?: string;
9494

95+
/**
96+
* Is the session in the process of being saved?
97+
*/
98+
isSaving: boolean;
99+
95100
/**
96101
* Has the session already been stored (i.e. "sent to background")?
97102
*/
@@ -160,6 +165,7 @@ const createSessionDefaultState: <
160165
isRestore: false,
161166
isCanceled: false,
162167
isContinued: false,
168+
isSaving: false,
163169
isStarted: false,
164170
trackedSearches: [],
165171
});
@@ -172,6 +178,7 @@ export interface SessionPureTransitions<
172178
start: (state: S) => ({ appName }: { appName: string }) => S;
173179
restore: (state: S) => (sessionId: string) => S;
174180
clear: (state: S) => () => S;
181+
save: (state: S) => () => S;
175182
store: (state: S) => (searchSessionSavedObject: SearchSessionSavedObject) => S;
176183
trackSearch: (state: S) => (search: SearchDescriptor, meta?: SearchMeta) => S;
177184
removeSearch: (state: S) => (search: SearchDescriptor) => S;
@@ -201,12 +208,22 @@ export const sessionPureTransitions: SessionPureTransitions = {
201208
isStored: true,
202209
}),
203210
clear: (state) => () => createSessionDefaultState(),
204-
store: (state) => (searchSessionSavedObject: SearchSessionSavedObject) => {
211+
save: (state) => () => {
212+
if (!state.sessionId) throw new Error("Can't save session. Missing sessionId");
213+
if (state.isStored || state.isRestore)
214+
throw new Error('Can\'t save because current session is already stored"');
215+
return {
216+
...state,
217+
isSaving: true,
218+
};
219+
},
220+
store: (state) => (searchSessionSavedObject?: SearchSessionSavedObject) => {
205221
if (!state.sessionId) throw new Error("Can't store session. Missing sessionId");
206222
if (state.isStored || state.isRestore)
207223
throw new Error('Can\'t store because current session is already stored"');
208224
return {
209225
...state,
226+
isSaving: false,
210227
isStored: true,
211228
searchSessionSavedObject,
212229
};
@@ -357,6 +374,8 @@ export const sessionPureSelectors: SessionPureSelectors = {
357374
return pendingSearches.length > 0
358375
? SearchSessionState.BackgroundLoading
359376
: SearchSessionState.Restored;
377+
case state.isSaving:
378+
return SearchSessionState.BackgroundLoading;
360379
case state.isStored:
361380
return pendingSearches.length > 0
362381
? SearchSessionState.BackgroundLoading

src/platform/plugins/shared/data/public/search/session/session_service.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,11 @@ interface TrackSearchDescriptor {
7070
abort: () => void;
7171

7272
/**
73-
* Keep polling the search to keep it alive
73+
* Used for polling after running in background (to ensure the search makes it into the background search saved
74+
* object) and also to keep the search alive while other search requests in the session are still in progress
75+
* @param abortSignal - signal that can be used to cancel the polling - otherwise the `searchAbortController.getSignal()` is used
7476
*/
75-
poll: () => Promise<void>;
77+
poll: (abortSignal?: AbortSignal) => Promise<void>;
7678

7779
/**
7880
* Notify search that session is being saved, could be used to restart the search with different params
@@ -447,6 +449,15 @@ export class SessionService {
447449
);
448450
}
449451

452+
/**
453+
* Is current session in process of saving
454+
*/
455+
public isSaving(
456+
state: SessionStateContainer<TrackSearchDescriptor, TrackSearchMeta> = this.state
457+
) {
458+
return state.get().isSaving;
459+
}
460+
450461
/**
451462
* Is current session already saved as SO (send to background)
452463
*/
@@ -600,6 +611,8 @@ export class SessionService {
600611
appendStartTime: currentSessionInfoProvider.appendSessionStartTimeToName,
601612
});
602613

614+
this.state.transitions.save();
615+
603616
const searchSessionSavedObject = await this.sessionsClient.create({
604617
name: formattedName,
605618
appId: currentSessionApp,
@@ -622,7 +635,7 @@ export class SessionService {
622635

623636
const extendSearchesPromise = Promise.all(
624637
searchesToExtend.map((s) =>
625-
s.searchDescriptor.poll().catch((e) => {
638+
s.searchDescriptor.poll(new AbortController().signal).catch((e) => {
626639
// eslint-disable-next-line no-console
627640
console.warn('Failed to extend search after session was saved', e);
628641
})

0 commit comments

Comments
 (0)