FEATURE: Command to delete stale workspaces

[sachera]

Basic Idea

Each Neos account has its own workspace and each workspace its own content stream. These content streams result in a potentially large number of entries in the content repositories hierarchyrelation table, which in turn, will make the fork operation slower.

Core Observation: In a lot of cases only a small amount of these accounts is actually actively used, i.e. have unpublished changes.

The basic idea of this PR is to remove the unnecessary lines from the hierarchyrelation table by deleting "stale" workspaces:

• Workspaces can be deleted if they are "stale".
  • A workspace is considered stale iff it is a user workspace, up to date, not used as a base workspace for another workspace, and not used within a given time interval.
• This will reduce the time a fork operation requires and therefore speed up the publish process.
• When a user logs in again, a new content stream is forked from the base workspace and associated to the workspace. (Behavior did not change)

Implementation Details

The feature is exposed through commands in the WorkspaceCommandController:

./flow workspace:removeStale: removes all stale user workspaces. As written above a workspace is considered stale iff:
    - it is a user workspace
    - contains no pending changes
    - has no other workspace depending on it
    - and the user the workspace belongs to did not log in for a given amount of time (by default 7 days).

The command is meant to be used in a cron job or similar to minimize the amount of active workspaces without the need to manually delete workspaces.

Related

pull request #5718 (superseded by this PR)

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the correct branch:
  • If it's a bugfix, use the lowest maintained branch which has the bug
  • If it's a non-breaking feature, use the branch of the next version (might be either minor or major)
  • If it's a breaking feature it should typically go into the next major version
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[skurfuerst]

Hey Andreas, awesome change!

two nitpicks, and a question:

does the re-creation of a workspace automatically work already? (If you know, a short pointer to this would be nice <3 )

All the best,
Sebastian

[mhsdesign]

Thanks for getting back with this change.

What do you say regarding my previous note:

There is one slight difference to removing and recreating a workspace than to reactivate and deactivate it. The additional neos workspace metadata like title, description, and roles are lost. So in case we hold that information dear we can possibly implement an activation or deactivation on Neos side.

I see we now use $contentRepositoryInstance->handle(DeleteWorkspace::create($workspace)); directly from the outside CommandController and thus leave the Neos MetaData intact. The Neos MetaData though - if kept intact - deserves a new state at Neos\Neos\Domain\Model\WorkspaceClassification possibly like PERSONAL_STALE, or as flag in WorkspaceMetadata. That way when requesting a workspace in createPersonalWorkspaceForUserIfMissing() we better know to expect the workspace not being there and that we have to recreate it.

That also means the WorkspaceService should encapsulate removing a personal workspace and marking it stale. Im not sure what the best api is jet - e.g. if CommandController or WorkspaceService should determine which workspaces to delete, but probably the latter.

(NEW) if the user doesnt write to the workspace for some time we can remove the users workspace

Also i see that you implemented a way to find users that have not logged in and wrote tests :D juhu!. But we can also look into the event store to date the last change of this workspace. Im not sure which is better and as we operate in Neos.Neos both are legal. Looking into the eventstore is obviously a bit more expensive but this is not relevant here. Also using the event store might allow to extend this feature for arbitrary workspaces where we dont have any information like the user login time.
Ill need to think if looking into the eventstore would even tell us the right information;)

[sachera]

Hey Andreas, awesome change!

two nitpicks, and a question:

does the re-creation of a workspace automatically work already? (If you know, a short pointer to this would be nice <3 )

All the best, Sebastian

Regarding your questions: Yes it does in Neos\Neos\Ui\Controller\BackendController::indexAction createPersonalWorkspaceForUserIfMissing is called in line 160. I do not see any case the workspace is required which is not handled by this.

[skurfuerst]

The Neos MetaData though - if kept intact - deserves a new state at Neos\Neos\Domain\Model\WorkspaceClassification possibly like PERSONAL_STALE, or as flag in WorkspaceMetadata. That way when requesting a workspace in createPersonalWorkspaceForUserIfMissing() we better know to expect the workspace not being there and that we have to recreate it.

I am not sure if we gain anything from this state ,or if we rather should make the system "self healing" as it is now.

Also i see that you implemented a way to find users that have not logged in and wrote tests :D juhu!. But we can also look into the event store to date the last change of this workspace. Im not sure which is better and as we operate in Neos.Neos both are legal. Looking into the eventstore is obviously a bit more expensive but this is not relevant here. Also using the event store might allow to extend this feature for arbitrary workspaces where we dont have any information like the user login time.

I would suggest to keep it as it is right now; we can always adjust to the 2nd version later.

Regarding the logic move from CommandController to service, I agree :)

All the best,
Sebastian

[sachera]

What do you say regarding my previous note:

There is one slight difference to removing and recreating a workspace than to reactivate and deactivate it. The additional neos workspace metadata like title, description, and roles are lost. So in case we hold that information dear we can possibly implement an activation or deactivation on Neos side.

I see we now use $contentRepositoryInstance->handle(DeleteWorkspace::create($workspace)); directly from the outside CommandController and thus leave the Neos MetaData intact. The Neos MetaData though - if kept intact - deserves a new state at Neos\Neos\Domain\Model\WorkspaceClassification possibly like PERSONAL_STALE, or as flag in WorkspaceMetadata. That way when requesting a workspace in createPersonalWorkspaceForUserIfMissing() we better know to expect the workspace not being there and that we have to recreate it.

I tried not to touch the core with this changes at all and instead rely as much as possible on what is already there. Adding a new state like PERSONAL_STALE without need seems to contradict this. I could however add tests to ensure a workspace deleted due to being stale can be restored with createPersonalWorkspaceForUserIfMissing(). This is currently just assumed implicitly.

That also means the WorkspaceService should encapsulate removing a personal workspace and marking it stale. Im not sure what the best api is jet - e.g. if CommandController or WorkspaceService should determine which workspaces to delete, but probably the latter.

Agreed, The code was moved and adjusted slightly (I decided against using SplObjectStorage, since in hindsight it seemed to me it made the code more complicated, not less as I initially hoped it would).

(NEW) if the user doesnt write to the workspace for some time we can remove the users workspace

Also i see that you implemented a way to find users that have not logged in and wrote tests :D juhu!. But we can also look into the event store to date the last change of this workspace. Im not sure which is better and as we operate in Neos.Neos both are legal. Looking into the eventstore is obviously a bit more expensive but this is not relevant here. Also using the event store might allow to extend this feature for arbitrary workspaces where we dont have any information like the user login time. Ill need to think if looking into the eventstore would even tell us the right information;)

I looked into it but were inconclusive if the dates in the eventstore were better. If i remember correctly (it was a few months ago) they did not update on user login, even if the backend was opened. This would mean the personal workspace of a user logging in regularly e.g. to review changes but not making changes on it would be considered stale. This would result in regular deletion and recreation (on login) of this users personal workspace. I am not sure if this scenario is even slightly realistic. But in the end I had the impression I was unable to tell what the implications of using the eventstore data would be, thus decided against using it.

[mhsdesign]

i have put my suggestions in a dedicated review pr -
sachera#1 - targeting your fork. Feel free to read through the commits and cherry pick them as you see fit or merge them into your branch as is.

Regarding

Regarding your questions: Yes it does in Neos\Neos\Ui\Controller\BackendController::indexAction createPersonalWorkspaceForUserIfMissing is called in line 160. I do not see any case the workspace is required which is not handled by this.

I know of the requireWorkspace call in the method createPersonalWorkspaceForUserIfMissing which will fail. This is shown in my last commit which provides a failing test.

To fix that ill get to the new metadata state, you wrote:

I tried not to touch the core with this changes at all and instead rely as much as possible on what is already there. Adding a new state like PERSONAL_STALE without need seems to contradict this.

Neos.Neos is core but its not the ESCR core if you know:) It should not be too invasive to add a new state. We only have to test that the workspace module still functions if a user logs in targeting that workspace module and NOT going to the content module first:)

I hope my adjustments sachera#1 make sense:)

[sachera]

i have put my suggestions in a dedicated review pr - sachera#1 - targeting your fork. Feel free to read through the commits and cherry pick them as you see fit or merge them into your branch as is.

Thank you. The changes look good and I merged them. Maybe I will change some tests to allow ordering of expectations and actual data to differ since I do not consider any specific ordering to be guaranteed by the API and therefore don't want to fix it in the tests.

Regarding

Regarding your questions: Yes it does in Neos\Neos\Ui\Controller\BackendController::indexAction createPersonalWorkspaceForUserIfMissing is called in line 160. I do not see any case the workspace is required which is not handled by this.

I know of the requireWorkspace call in the method createPersonalWorkspaceForUserIfMissing which will fail. This is shown in my last commit which provides a failing test.

Good catch. I would have expected it to behave differently but I see why it doesn't. Unfortunately fixing this seems to be more than just recreating the workspace and reusing the metadata: the information which workspace is the base workspace is lost when deleting it.

When the test is adjusted to use a root workspace named "live" (instead of "some_root_workspace") it is easy to fix for the case the personal workspace is directly based on the root.

Looking at this I also assume the information the stale (deleted) user workspace is based on another is lost when deleting it. Any thoughts on how to tackle this?

To fix that ill get to the new metadata state, you wrote:

I tried not to touch the core with this changes at all and instead rely as much as possible on what is already there. Adding a new state like PERSONAL_STALE without need seems to contradict this.

Neos.Neos is core but its not the ESCR core if you know:) It should not be too invasive to add a new state. We only have to test that the workspace module still functions if a user logs in targeting that workspace module and NOT going to the content module first:)

At this point i fail to see the gain a new state PERSONAL_STALE has. Is there another case in which the metadata could exist but the workspace itself does not? Is this meant to allow for such cases in the future (vs. implicitly assuming this can only happen for stale workspaces)? Otherwise just checking if a workspace exists for a given name seems to be good enough and less invasive.

Also when looking into createPersonalWorkspaceForUserIfMissing I wondered how getPersonalWorkspaceForUser should handle a stale workspace. I would argue it should create the workspace if the metadata is still there, since the workspace should behave as if it exists outside of the WorkspaceService. But I would like to hear other opinions on this.

[Sebobo]

I would suggest a different naming for the command. The command sounds it removes any stale workspace, but the comment correctly explains its only certain user workspaces. Though a note on their automatic recreation would also be helpful here in the comment.

If we hopefully soonish can extend the removal to other types of workspaces with potential different rules, we have a naming clash. Or what did you think about future future extensions?