Removing a shell does not remove it from the running shells table

[deanmarcussen]

Describe the bug

OC 1.8x introduced RemoveShellContextAsync on the IShellHost and associated UI etc.
When removing a tenant the code does not ever call IRunningShellTable.Remove which means the shell that has been removed, is still matched on by the ModularTenantContainerMiddleware.

This results in a 404

We have some middleware that sits around the modular tenant middleware, which relies on removing the shell from the running shell table, when removing a tenant (we wrote our own implementation of removing, before OC provided one), which no longer works correctly because of this.

Previously we were just disabling the shell, which removes if the IRunningShellTable, so it seems to make sense to have the same functionality/behaviour on remove.

adding IRunningShellTable.Remove(shellSettings) during removing a shell fixes the issue.

[Piedone]

Looks like an issue indeed. Would you like to submit a PR for the fix?

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[deanmarcussen]

I did look at this, the challenge was there where some 150+ comments on the original pr, so dificult to figure out the intent.

The way I see it, the tenant really needs to be disabled first, and have the shell released, so it will release all it's resources, and then given a few minutes to let all the existing requests to complete.

Looking at the code in the ShellHost it feels like ReleaseShellContextAsync would do that, if the settings came in as disabled, at which point it gets removed from the running shells table

[Piedone]

Isn't this not suitable alone, then?

adding IRunningShellTable.Remove(shellSettings) during removing a shell fixes the issue.

[sebastienros]

and then given a few minutes to let all the existing requests to complete

Just to make it clear there is a request counter to know when a shell has no active request anymore, at which point if the shell is supposed to be restarted it is recycled. At that point we could also check if it has to be removed from the tenants table, and do so.

[deanmarcussen]

I reviewed this, and there is a design which in the OC code base only allows removing a disabled tenant, so I have woven that same arrangement into our code base, to disable, delay for a few moments, and then remove.

There is no check on the request counter as Seb mentions above, so it is a matter of waiting for the requests to clear before removing it.