SavedObjects update can cause version conflict errors even when not using optimistic concurrency control

[rudolf]

To support sharing to multiple spaces #27004 the update operation adds a version check to be sure that the namespaces are still the same ones as verified by the pre-flight check:

kibana/src/core/server/saved_objects/service/lib/repository.ts

Line 1035 in d4b2a51

...getExpectedVersionProperties(version, preflightResult),

However, this means even if consumers of the API don't specify a version field, they can still get version conflicts for multi namespace saved objects.

To work around this we should explore removing the preflight check and replacing it with a scripted update. The script should then validate the namespaces before updating the document.

Related: #82719

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[Dosant]

We're working on RFC for improving search and search sessions,
And this saved_objects update issue will help us a lot: simplify code, improve performance, reducing surface for errors

We have this code path where we with each search within a search session update a search-session saved object with information about that search. We could refactor it to make only a partial update and remove logic for handling conflicts and scheduling retries:

kibana/x-pack/plugins/data_enhanced/server/search/session/session_service.ts

Lines 233 to 281 in 573fdce

	private updateOrCreate = async (
	deps: SearchSessionDependencies,
	user: AuthenticatedUser | null,
	sessionId: string,
	attributes: Partial<SearchSessionSavedObjectAttributes>,
	retry: number = 1
	): Promise<SavedObject<SearchSessionSavedObjectAttributes> | undefined> => {
	const retryOnConflict = async (e: any) => {
	this.logger.debug(`Conflict error | ${sessionId}`);
	// Randomize sleep to spread updates out in case of conflicts
	await sleep(100 + Math.random() * 50);
	return await this.updateOrCreate(deps, user, sessionId, attributes, retry + 1);
	};
	this.logger.debug(`updateOrCreate | ${sessionId} | ${retry}`);
	try {
	return (await this.update(
	deps,
	user,
	sessionId,
	attributes
	)) as SavedObject<SearchSessionSavedObjectAttributes>;
	} catch (e) {
	if (SavedObjectsErrorHelpers.isNotFoundError(e)) {
	try {
	this.logger.debug(`Object not found | ${sessionId}`);
	return await this.create(deps, user, sessionId, attributes);
	} catch (createError) {
	if (
	SavedObjectsErrorHelpers.isConflictError(createError) &&
	retry < this.sessionConfig.maxUpdateRetries
	) {
	return await retryOnConflict(createError);
	} else {
	this.logger.error(createError);
	}
	}
	} else if (
	SavedObjectsErrorHelpers.isConflictError(e) &&
	retry < this.sessionConfig.maxUpdateRetries
	) {
	return await retryOnConflict(e);
	} else {
	this.logger.error(e);
	}
	}
	return undefined;
	};

cc @lukasolson, @ppisljar

[rudolf]

@Dosant it looks like search-session is type: 'single' https://github.com/elastic/kibana/blob/main/x-pack/plugins/data_enhanced/server/saved_objects/search_session.ts so I think you're more likely hitting the problem described in #82719

Based on my understanding of search sessions I don't think it makes sense to share across spaces so sharing wouldn't be something we'd do in the future either, correct?

[Dosant]

Based on my understanding of search sessions I don't think it makes sense to share across spaces so sharing wouldn't be something we'd do in the future either, correct?

Yes, this is correct.

so I think you're more likely hitting the problem described in #82719

Ok, indeed looks more like it.

Basically what we want is for a saved object type: 'single' to be able to use savedOjbects.update without a version and assume that the update will go through without potentially throwing a conflict error
WIll #82719 help?

[joshdover]

I ran into this problem again in some optimization work where I'm trying to parellelize two different async 'threads' that update two different fields on the same SO. Worth noting that I'm also wanting to use refresh: false on these calls since there are many subsequent updates (by design) to the same SO within the same request.

I was able to workaround this by using pRetry without any bad side effects. PR: #130906

[pgayvallet]

@rudolf Can we close this given #131371 removed the usage of the preflight check for the getExpectedVersionProperties calls in SOR.update?

[rudolf]

yeah agree #131371 closed this.