BUG: Workspace creation not thread-safe

[bwaidelich]

Observation

The following exception occurs when trying to log in to the Neos backend:

Exception #1651153651 in line 101 of Packages/Libraries/neos/eventstore-doctrineadapter/src/DoctrineEventStore.php: Expected version: 0, actual version: 2

Analysis

The contentGraph projection failed and stopped with status ERROR (see cr_default_subscriptions table) and the following error_message:

An exception occurred while executing a query: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '<workspace-name>' for key 'cr_default_p_graph_workspace.PRIMARY'

And there are multiple WorkspaceWasCreated events for the same workspace name, which should never happen 🤯
SQL query to debug that:

SELECT JSON_UNQUOTE(JSON_EXTRACT(payload, '$.workspaceName')) workspace, COUNT(*) count FROM cr_default_events WHERE type = 'WorkspaceWasCreated' GROUP BY JSON_EXTRACT(payload, '$.workspaceName') ORDER BY count DESC

(count should be 1 for every workspace)

Bug

We're using the red model to enforce invariants, but there's a race condition between the check and the event publication:

neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php

Lines 123 to 154 in 97a4585

	$this->requireWorkspaceToNotExist($command->workspaceName, $commandHandlingDependencies);
	$baseWorkspace = $commandHandlingDependencies->findWorkspaceByName($command->baseWorkspaceName);
	if ($baseWorkspace === null) {
	throw new BaseWorkspaceDoesNotExist(sprintf(
	'The workspace %s (base workspace of %s) does not exist',
	$command->baseWorkspaceName->value,
	$command->workspaceName->value
	), 1513890708);
	}
	$sourceContentStreamVersion = $commandHandlingDependencies->getContentStreamVersion($baseWorkspace->currentContentStreamId);
	$this->requireContentStreamToNotBeClosed($baseWorkspace->currentContentStreamId, $commandHandlingDependencies);
	$this->requireContentStreamToNotExistYet($command->newContentStreamId, $commandHandlingDependencies);
	// When the workspace is created, we first have to fork the content stream
	yield $this->forkContentStream(
	$command->newContentStreamId,
	$baseWorkspace->currentContentStreamId,
	$sourceContentStreamVersion,
	sprintf('Create workspace %s with base %s', $command->workspaceName->value, $baseWorkspace->workspaceName->value)
	);
	yield new EventsToPublish(
	WorkspaceEventStreamName::fromWorkspaceName($command->workspaceName)->getEventStreamName(),
	Events::with(
	new WorkspaceWasCreated(
	$command->workspaceName,
	$command->baseWorkspaceName,
	$command->newContentStreamId,
	)
	),
	ExpectedVersion::ANY(),
	);

Fix

An easy fix would be to replace ExpectedVersion::ANY() with ExpectedVersion::NO_STREAM().
This would mean that a workspace can't ever be re-created once it was deleted (via WorkspaceWasRemoved).
But maybe that's the behavior we want to have since re-using a previously existing workspace sounds dangerous..

Alternatively we'll have to add a version and removed column to the workspace read model so that we can enforce constraints using optimistic locking (like we do with content streams)

Work around

1. do a database backup!
2. delete invalid WorkspaceWasCreated events from the cr_default_events table
3. run the ./flow subscription:reactivate contentGraph command to re-activate and catch up the contentGraph projection

Related: #5058

[kitsunet]

I could see that as possible fix. Already checked, the Neos.Neos Workspacervice auto appends suffix numbers to the name to avoid duplicates, so this woudl be safe (eg. my cmueller user being deleted and recreated later). That was one possible issue I could see. For now that seems like a good fix to me.

[mhsdesign]

i was already under the impression that we had the NO_STREAM check for workspace creations but i opened the discussion that we cant as the workspace module would crash when creating the same workspace twice.

Because @kitsunet it only appends suffixes to KNOWN workspaces as the workspace does not exist currently in the table (hard removed) it does not know it exists.

See #5456 (comment)

An alternative fix could be to indeed use NO_STREAM and then in the workspace service go through all WORKSPACE_WAS_CREATED events to find all workspace names that existed once. That would be a little more expensive than asking a projection though (because the event store is not guaranteed to be optimised for reading)

Alternatively we could use the NO_STREAM flag and catch that exception in a simple retry loop. As there is only ONE reason for that exception for being thrown. I think i like this the most - though that could go wrong too and is slow (needs an recursion prevention)

[bwaidelich]

see #5488 for reference

[mhsdesign]

I dont think the version column will fix this and allow to reuse the stream still. Even when we introduce a version column we would need to resort to NO_STREAM.

Process 1 Create workspace (p1)
Process 2 Create workspace (p2)

p1: check if workspace and version exists in table -> no
p1: emit workspace was created event
p2: check if workspace and version exists in table -> no
p2: emit workspace was created event
p1: do catchup but projection error because of duplicated events
p2: no catchup necessary

now using NO_STREAM will tell p2 to not emit the workspace creation and an error will be thrown but the system should be okay after that.

Now in case we would want to reuse workspace streams (e.g. delete workspace and recreate a new one with the SAME name) we cant use NO_STREAM.

On creation we could do the following:

1. Handle createWorkspace command
2. Get last event on the workspace stream
3. If last event does not exist -> stream is empty and no workspace exists. Emit WorkspaceWasCreated with NO_STREAM
4. If last event exists, assert that it MUST be a WorkspaceWasDeleted event - otherwise the workspace still exists which is invalid - use the next version after the deleted event as expected version to emit WorkspaceWasCreated

This works because we dont ask the projection for anything and sorely rely on the evenstream.

[bwaidelich]

I dont think the version column will fix this and allow to reuse the stream still

Right, that's why I added

needs version check (and deleted flag?) for workspace creation

to the todo list

This works because we dont ask the projection for anything and sorely rely on the evenstream

Yes, that's an option (even though we solved it differently for content streams and added the soft deletion).

However, the NO_STREAM approach would be much simpler and I have the feeling that we should use the workspace name rather like an identifier (i.e. there should be no way to re-create it).

[kitsunet]

I am fine with treating it like an identifier that can never come back. However I think we might safe us (and devs) some trouble if we then adjusted our fn (username) => workspacename such that it always appends a hash or timestamp or something. I think otherwise people sooner or later will want their "pure" workspace name back and cannot do that. If it's always postfixed that might be nicer.

Just a thought.

Oh also means we need to make damn sure you cannot delete the live workspace... 😆

[bwaidelich]

we might safe us (and devs) some trouble if we [...] adjusted our [...] workspacename such that it always appends a hash or timestamp or something

Yes, a timestamp makes sense IMO. But I think we need to tweak WorkspaceService::createPersonalWorkspaceForUserIfMissing() in general to make it thread safe.

Also related to what @mhsdesign wrote in Slack:

• explict workspace creation screen
  • the user will see that the login worked and see neos fast
  • if the creation takes longer - because the fork might be expensive for a big site - we can offer a progress bar and the user will not terminate the connection which might be a cause for inconsistencies?
  • feature: in a later step we can also offer to view live in read only