[Entity Store] Reset state error after successful task run

[orouz]

closes #261551

we were setting the error to undefined but applying it using mergeAttributes: true which meant existing fields will not be set to undefined

added an updateWith(current => ...) method which lets the caller update the state object manually with mergeAttributes: false

[romulets]

This depends I believe. If it's a requested aborted and we have processed 0 pages, this warrants be called an error.

If one or more pages have been processed, we can ignore.

But I believe the issue should not be treated like this. I think we should clear the error if a successful run has happened. The timeout is the easy one to solve.

But what if there is a cluster upgrade and elasticsearch is unavailable for one or two iterations? Then we are able to resume on the third iteration and the error never gets cleaned.

I believe the best solution is: clear the error SO once we have successful run

[uri-weisman]

I believe the best solution is: clear the error SO once we have successful run

I tend to agree. In addition, I would persist also aborted requests and clear them after a successful run of the Kibana task.

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: 7dac6af

Failed CI Steps

• Jest Tests #11
• Jest Tests #11
• affected Scout: [ security / entity_store ] plugin
• affected Scout: [ security / entity_store ] plugin

Test Failures

• [job] [logs] affected Scout: [ security / entity_store ] plugin / local-stateful-classic - Entity Store Logs Extraction with pagination (entity pages + maxLogsPerPage) - Should extract host with entity pagination (docsLimit 5, wide log slices)
• [job] [logs] affected Scout: [ security / entity_store ] plugin / local-stateful-classic - Entity Store stop/start API tests - Should stop and start the extract entity task after install
• [job] [logs] Jest Tests #11 / SelectedFilters should render properly
• [job] [logs] Jest Tests #11 / SelectedFilters should render properly

Metrics [docs]

✅ unchanged

History

• 💔 Build #427516 failed 1c6a640

[uri-weisman]

Approved, even though we have a minor race condition.
Let's handle it in a follow-up PR.

[uri-weisman]

We have a potential race condition here, any concurrent change to the SO between findOrThrow() and update() can be lost (a call to the entity store management api updates the SO).
I consider it as minor, but something we should be aware of.
The right way to overcome this race would be by optimistic concurrency, adding the SO version to the update call, a conflict will be thrown in case there were an additional update between our read-write operation.

Example:

  public async get(
    id: string,
    spaceId?: string
  ): Promise<{ id: string; attributes: RuleSavedObjectAttributes; version?: string }> {
    const namespace = spaceIdToNamespace(this.spaces, spaceId);
    const doc = await this.client.get<RuleSavedObjectAttributes>(
      RULE_SAVED_OBJECT_TYPE,
      id,
      namespace ? { namespace } : undefined
    );
    return { id: doc.id, attributes: doc.attributes, version: doc.version };
  }

  public async update({
    id,
    attrs,
    version,
  }: {
    id: string;
    attrs: RuleSavedObjectAttributes;
    version?: string;
  }): Promise<void> {
    await this.client.update<RuleSavedObjectAttributes>(RULE_SAVED_OBJECT_TYPE, id, attrs, {
      ...(version ? { version } : {}),
      mergeAttributes: false,
    });
  }

[orouz]

this is the same for our regular update not just the newly added updateWith

doc.version seems like a nice addition we should add anyway, even if this specifically is hardly an issue

[kibanamachine]

Starting backport for target branches: 9.4

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

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	9.4

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

Questions ?

Please refer to the Backport tool documentation