Skip to content

Comments

[EsAggs] Freeze cached responses to avoid response mutation#222160

Merged
ppisljar merged 21 commits intoelastic:mainfrom
ppisljar:search/freeze_response
Jun 27, 2025
Merged

[EsAggs] Freeze cached responses to avoid response mutation#222160
ppisljar merged 21 commits intoelastic:mainfrom
ppisljar:search/freeze_response

Conversation

@ppisljar
Copy link
Contributor

@ppisljar ppisljar commented Jun 2, 2025 •
edited by kibanamachine
Loading

Summary

resolves #221886
also fixes 4 other bugs (other bucket, maps (2x), vega where we are modifying the response

also adds deep freeze on the search service cache object, so we can catch this problems earlier.

story: search service caches responses from elasticsearch, and next time we are to make the same request we get the response from cache. But if anyone who gets access to this object modifies it, the cached version gets modified, which might break the processing for the consumer that retrieves the cached version after it has been altered.

in order to prevent it we should:

  • not modify this object that is stored on the cache
  • clone/copy before modification (functions should not have side effects)
  • freezing the object so it can never be modified without noticing.

@ppisljar ppisljar added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Jun 2, 2025
@ppisljar ppisljar marked this pull request as ready for review June 9, 2025 11:58
@ppisljar ppisljar requested review from a team as code owners June 9, 2025 11:58
@elasticmachine
Copy link
Contributor

elasticmachine commented Jun 9, 2025

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@markov00 markov00 requested a review from Copilot June 9, 2025 13:19
Copilot AI reviewed Jun 9, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures search responses and data structures are not mutated by introducing immutable operations and deep freezing cached responses, and fixes related bugs in maps, other buckets aggregation, and Vega data handling.

  • Replace in-place reverse() calls with non-mutating clones before reversing.
  • Add structuredClone before modifying feature collections, aggregation responses, and Vega raw responses.
  • Introduce deepFreeze on search interceptor responses to catch unintended mutations early.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
x-pack/platform/plugins/shared/maps/.../es_search_source.tsx Use slice().reverse() instead of mutating reverse()
x-pack/platform/plugins/shared/maps/.../geojson_source_data.tsx Clone GeoJSON before assigning feature IDs
src/platform/plugins/shared/data/tsconfig.json Add @kbn/std for deepFreeze and structuredClone support
src/platform/plugins/shared/data/public/.../search_interceptor.ts Deep-freeze search responses
src/platform/plugins/shared/data/common/.../_terms_other_bucket_helper.ts Clone both responses before merging "other" buckets
src/platform/plugins/shared/data/common/.../agg_configs.ts Use structuredClone instead of cloneDeep
src/platform/plugins/private/vis_types/vega/.../search_api.ts Clone raw Vega responses before exposing

thomasneirynck
thomasneirynck reviewed Jun 9, 2025
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a real potential to introduce a severe performance hit. Especially when doing this for every response (imagine deep-cloning arbitrary table results for Discover).

What's the context of this fix? Why does it require cloning?

@dej611 dej611 mentioned this pull request Jun 10, 2025
Merged
@elasticmachine
Copy link
Contributor

elasticmachine commented Jun 10, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.0MB 3.0MB +16.0B
visTypeVega 2.1MB 2.1MB +17.0B
total +33.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 402.2KB 402.3KB +56.0B

History

@lukasolson
Copy link
Contributor

lukasolson commented Jun 11, 2025

I wonder if we can get nearly the same benefit without the performance hit if we use a type-based compile-time approach instead of runtime. We could mark rawResponse as Readonly (or DeepReadonly if necessary). Then the type checks should fail for each of these instances where we're modifying the rawResponse somehow.

@markov00 markov00 changed the title [EsAggs] Freeze cached responses [EsAggs] Freeze cached responses to avoid response mutation Jun 26, 2025
@markov00 markov00 added backport:version Backport to applied version labels v9.1.0 v8.19.0 release_note:skip Skip the PR/issue when compiling release notes bug Fixes for quality problems that affect the customer experience and removed release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Jun 26, 2025
@elasticmachine
Copy link
Contributor

elasticmachine commented Jun 26, 2025

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
kibanaUtils 412 414 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.1MB 3.1MB +16.0B
visTypeVega 2.1MB 2.1MB +17.0B
total +33.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 402.8KB 402.8KB +31.0B
kibanaUtils 55.7KB 55.8KB +54.0B
total +85.0B
Unknown metric groups

API count

id before after diff
kibanaUtils 605 607 +2

History

nreese
nreese approved these changes Jun 26, 2025
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kibana-presentation changes LGTM
code review only

@ppisljar ppisljar merged commit 180f90a into elastic:main Jun 27, 2025
23 checks passed
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 27, 2025

Starting backport for target branches: 8.19, 9.1

https://github.com/elastic/kibana/actions/runs/15926901120

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 27, 2025
@ppisljar
freeze response (elastic#222160)
c57d6a4 
(cherry picked from commit 180f90a)
@kibanamachine kibanamachine mentioned this pull request Jun 27, 2025
Closed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 27, 2025
@ppisljar
freeze response (elastic#222160)
7f51b34 
(cherry picked from commit 180f90a)
@kibanamachine kibanamachine mentioned this pull request Jun 27, 2025
Merged
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 27, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.19
✅ 9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine mentioned this pull request Jun 27, 2025
Merged
kibanamachine added a commit that referenced this pull request Jul 1, 2025
@kibanamachine @ppisljar @markov00
[9.1] [EsAggs] Freeze cached responses to avoid response mutation (#2…
47863fa 
…22160) (#225634)

# Backport

This will backport the following commits from `main` to `9.1`:
- [freeze response
(#222160)](#222160)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Peter
Pisljar","email":"peter.pisljar@elastic.co"},"sourceCommit":{"committedDate":"2025-06-27T13:00:07Z","message":"freeze
response
(#222160)","sha":"180f90a65cdd3edd4f9a215533aee1a0477436df","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:Visualizations","release_note:skip","backport:version","v9.1.0","v8.19.0","v9.2.0"],"title":"[EsAggs]
Freeze cached responses to avoid response
mutation","number":222160,"url":"https://github.com/elastic/kibana/pull/222160","mergeCommit":{"message":"freeze
response
(#222160)","sha":"180f90a65cdd3edd4f9a215533aee1a0477436df"}},"sourceBranch":"main","suggestedTargetBranches":["9.1","8.19"],"targetPullRequestStates":[{"branch":"9.1","label":"v9.1.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/222160","number":222160,"mergeCommit":{"message":"freeze
response
(#222160)","sha":"180f90a65cdd3edd4f9a215533aee1a0477436df"}}]}]
BACKPORT-->

Co-authored-by: Peter Pisljar <peter.pisljar@elastic.co>
Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 1, 2025
@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 1, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @ppisljar

4 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 2, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @ppisljar

@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 3, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @ppisljar

@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 4, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @ppisljar

@kibanamachine
Copy link
Contributor

kibanamachine commented Jul 7, 2025

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @ppisljar

@ppisljar ppisljar mentioned this pull request Jul 8, 2025
Merged
ppisljar added a commit that referenced this pull request Jul 8, 2025
@ppisljar
[8.19] [EsAggs] Freeze cached responses to avoid response mutation (#…
8a66504 
…222160) (#226918)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukasolson lukasolson lukasolson left review comments

Copilot code review Copilot Copilot left review comments

@nreese nreese nreese approved these changes

@dej611 dej611 dej611 approved these changes

@Dosant Dosant Dosant approved these changes

+1 more reviewer

@thomasneirynck thomasneirynck thomasneirynck left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.19.0 v9.1.0 v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Multiple layers with identical time shifted aggs are ignored in visualization

9 participants

@ppisljar @elasticmachine @lukasolson @kibanamachine @nreese @dej611 @thomasneirynck @Dosant @markov00