[Security Solution][Detections] Await promises to ensure promise rejection does not crash kibana

[marshallmain]

Summary

In modern versions of node js, an unhandled promise rejection causes the process to exit. We were creating promises that could reject without .catch and Kibana would crash if the promise rejected.

To reproduce the promise rejection on master:

1. Create 2 indices, one with @timestamp mapped and one with event.ingested mapped.

PUT /test-1
{
  "mappings": {
    "properties": {
      "@timestamp": {
        "type": "date"
      }
    }
  }
}

PUT /test-2
{
  "mappings": {
    "properties": {
      "event.ingested": {
        "type": "date"
      }
    }
  }
}

2. Create a KQL rule that searches both indices and add event.ingested as the timestamp override. Save the rule.
3. Edit the KQL rule and remove test-2 from the indices searched but leave test-1. Activate the rule. The rule will attempt to search test-1 using event.ingested to sort on, however, the search will return an error causing the promise to reject and Kibana will crash when the rule runs for the second time. Not sure yet why it doesn't crash on the first rule run despite the promise rejecting - possibly a race, if it can reach the point where the promise is awaited before the promise rejects then it won't be unhandled.

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 whitelisted 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

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[dhurley14]

It would be great if we could add some unit tests exposing the failing functionality that then pass / is handled with these changes.

In general, the code in search_after_bulk_create has become a bit unruly. This code is making it more difficult to write unit tests for and could use some TLC before we begin merging into a unified architecture with alerting. While you're in here I think this code could utilize some general cleanup. It seems to me we generally want to take the search results from the two searches, merge them, and then do a bulk create all while checking a few conditions to determine when to exit and stop searching. I think it would be great if we could streamline the code and take the outputs from some pure functions and make them inputs to the others and in a way that would make it easier to test. This would in turn help us remove the let hasSortId and let hasBackupSortId which will help us with maintainability going forward.

[marshallmain]

@elasticmachine merge upstream

[marshallmain]

I don't think this behavior is a good candidate for unit testing since a unit test for this case would be extremely dependent on the function implementation rather than the desired function outputs. The unit test would have to mock specific external API calls inside the function in the right order. In addition, since the bug is a race condition we'd have to be very careful when mocking the calls as to not make a flaky test.

If we want to add tooling to detect errors like this one, something like https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-floating-promises.md would probably be effective.

I agree that this code needs to be simplified, however I believe this bug fix needs to go into 7.11 as it can crash Kibana and we shouldn't be doing significant refactoring for 7.11 at this stage.

[dhurley14]

Fair enough. I think a small refactor from the promises being executed in series to running in parallel with Promise.all might be a good update in general. I think you might have mentioned this before?

[marshallmain]

Running the queries in parallel would be an improvement, however in making that change we would also want to handle the error cases for each query independently - right now a promise rejection from either query falls through to the catch at the end. I think it would be safer to bundle that refactoring together in 7.12+.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 1a33924

Metrics [docs]

✅ unchanged

History

• 💛 Build #99327 was flaky 9f61c08
• 💔 Build #99326 failed ea75eda

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