[data.search.bsearch] Forward request abortSignal to search strategy #170041

lukasolson · 2023-10-27T15:17:55Z

Summary

Attempt to resurrect #169041.

Previously flaky tests:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

…lastic#169041) ## Summary Creates an `abortSignal` from the request disconnected event that is forwarded to the search strategy. In practice this means that when a bsearch call is disconnected (either due to client disconnect or server timeout) the corresponding call to ES is also cancelled. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts

elasticmachine · 2023-10-29T06:13:12Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

lukasolson · 2023-10-30T02:01:30Z

@elasticmachine merge upstream

src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts

drewdaemon

Discovery owns search.

davismcphee

I left some feedback in the comments, but overall the changes seem good. My only other question is are we able to add functional test coverage for these changes, especially around the scenario that led to the crashes in CI? Maybe through API integration tests or similar?

src/plugins/data/server/search/search_service.ts

src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.test.ts

…to bsearch/abort-again

lukasolson · 2023-11-03T23:48:16Z

especially around the scenario that led to the crashes in CI?

Added in 7fc746a

kibana-ci · 2023-11-08T23:56:05Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a0c3b53

Failed CI Steps

FTR Configs #41

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lukasolson

lukasolson · 2023-11-09T00:04:43Z

Should be ready for another look.

davismcphee

Thanks for all the work on this, and especially all the tests! The integration test looks like it does a good job covering the issue we were running into. For good measure I started a flaky test run on some of the FTR configs that were failing last time and the config for the new integration test: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3966.

I'm going to approve now though since it LGTM, and I think we should be good to merge as long as the test runs pass 👍

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts

…lastic#170041) ## Summary Attempt to resurrect elastic#169041. Previously flaky tests: * group1: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3797 * group2: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3798 * group3: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3799 * group4: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3800 ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary Resolves #168957. It turns out the underlying issue was resolved in #170041 (unhandled errors when deleting not being handled). However this still left it up to consumers of `pollSearch` to be 100% sure they weren't leaking unhandled promise rejections. This adds code directly to `pollSearch` that will handle rejections if they aren't handled in the calling code. It also adds tests to consumers of `pollSearch` to make sure they don't barf in the case that the `cancel` function errors. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

## Summary Resolves elastic#168957. It turns out the underlying issue was resolved in elastic#170041 (unhandled errors when deleting not being handled). However this still left it up to consumers of `pollSearch` to be 100% sure they weren't leaking unhandled promise rejections. This adds code directly to `pollSearch` that will handle rejections if they aren't handled in the calling code. It also adds tests to consumers of `pollSearch` to make sure they don't barf in the case that the `cancel` function errors. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

lukasolson and others added 2 commits October 27, 2023 08:13

Return result of cancel

d31c579

lukasolson self-assigned this Oct 27, 2023

lukasolson added 14 commits October 27, 2023 13:15

Fix other cancel

2b13f44

Update search service cancel

d8395bb

Swallow cancellation errors in search interceptor

ba6d8e2

Swallow errors in delete route

312ae34

Clean up changes

ab50c10

Swallow delete errors

a5cd1b3

More async fixes

39d6136

Remove unnecessary changes

22795ca

Use async/await again

f3f0839

Change one little thing

4450a9b

Another test

0a84ff1

another thingy

79697de

Re-add .then

04eccf7

Consistent handling in search strategies

0a13dbe

lukasolson commented Oct 29, 2023

View reviewed changes

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts Outdated Show resolved Hide resolved

lukasolson added 2 commits October 28, 2023 23:11

Swallow errors during cancelSessionSearches

7835584

Merge branch 'main' into bsearch/abort-again

bc865c7

lukasolson added Feature:Search Querying infrastructure in Kibana Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.11.0 v8.12.0 v8.11.1 labels Oct 29, 2023

lukasolson marked this pull request as ready for review October 29, 2023 06:13

lukasolson requested review from a team as code owners October 29, 2023 06:13

lukasolson added the release_note:skip Skip the PR/issue when compiling release notes label Oct 30, 2023

kertal reviewed Oct 30, 2023

View reviewed changes

src/plugins/data/server/search/strategies/eql_search/eql_search_strategy.ts Show resolved Hide resolved

drewdaemon approved these changes Oct 30, 2023

View reviewed changes

davismcphee reviewed Nov 1, 2023

View reviewed changes

lukasolson added 6 commits November 2, 2023 12:23

Merge branch 'main' into bsearch/abort-again

8623348

Review feedback

aa3817f

Merge branch 'bsearch/abort-again' of github.com:lukasolson/kibana in…

ab69247

…to bsearch/abort-again

Merge branch 'main' into bsearch/abort-again

c0de1e0

Add changes to other search strategies

34aa4dc

Add API integration test

7fc746a

lukasolson removed v8.11.0 v8.11.1 labels Nov 7, 2023

lukasolson added 5 commits November 7, 2023 13:32

Fix undefined check

901983a

Merge branch 'main' into bsearch/abort-again

50eb538

Clean up cancel methods

b9fcdc3

Fix overriding of keep_alive and wait_for_completion_timeout

cce4cd1

Merge branch 'main' into bsearch/abort-again

a0c3b53

lukasolson requested review from kertal and davismcphee November 9, 2023 00:04

davismcphee approved these changes Nov 10, 2023

View reviewed changes

src/plugins/data/server/search/strategies/ese_search/ese_search_strategy.ts Show resolved Hide resolved

lukasolson merged commit b84881a into elastic:main Nov 10, 2023
29 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Nov 10, 2023

logeekal mentioned this pull request Nov 16, 2023

DO NOT MERGE - Timeline New UI + Modal + Unified components #171385

Closed

lukasolson mentioned this pull request Apr 25, 2024

[data.search] Fix unhandled promise rejections #181785

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.search.bsearch] Forward request abortSignal to search strategy #170041

[data.search.bsearch] Forward request abortSignal to search strategy #170041

lukasolson commented Oct 27, 2023 •

edited

Loading

elasticmachine commented Oct 29, 2023

lukasolson commented Oct 30, 2023

drewdaemon left a comment

davismcphee left a comment

lukasolson commented Nov 3, 2023

kibana-ci commented Nov 8, 2023

lukasolson commented Nov 9, 2023

davismcphee left a comment

[data.search.bsearch] Forward request abortSignal to search strategy #170041

[data.search.bsearch] Forward request abortSignal to search strategy #170041

Conversation

lukasolson commented Oct 27, 2023 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Oct 29, 2023

lukasolson commented Oct 30, 2023

drewdaemon left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

lukasolson commented Nov 3, 2023

kibana-ci commented Nov 8, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

lukasolson commented Nov 9, 2023

davismcphee left a comment

Choose a reason for hiding this comment

lukasolson commented Oct 27, 2023 •

edited

Loading