Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't disable apps for errors in unrelated code paths #18249

Open
ChristophWurst opened this issue Dec 5, 2019 · 5 comments
Open

Don't disable apps for errors in unrelated code paths #18249

ChristophWurst opened this issue Dec 5, 2019 · 5 comments
Labels
1. to develop Accepted and waiting to be taken care of feature: apps management technical debt

Comments

@ChristophWurst
Copy link
Member

Steps to reproduce

  1. Run Nextcloud
  2. Run into an unhandled exception during app initialization phase
  3. See unrelated apps getting disabled

Expected behaviour

We only disable apps if it's really their fault and the error was triggered there.

Actual behaviour

If apps call some other app's or server code and that causes an exception (database or redis down, for example) that app is wrongly disabled. Leading to other bugs and unwanted behavior.

Server configuration

Nextcloud version:

any


cc @blizzz @nickvergessen @rullzer

@ChristophWurst ChristophWurst added bug 1. to develop Accepted and waiting to be taken care of technical debt 16-feedback labels Dec 5, 2019
@nickvergessen
Copy link
Member

yeah, well its kind of hard to figure out what is temporary, what needs the app disabled and what not.

@klada
Copy link

klada commented Dec 6, 2019

There are some apps which should never be automatically disabled at all, once enabled (such as authentication backends). We just had this happen (user_saml was disabled) and as soon as this happened hundreds of sync clients started going crazy, which obviously causes a lot of workload for user support in bigger environments.

IMHO it would also be fine to prevent auto-disabling of specific apps through a config entry (similar to alwaysEnabled in core/shipped.json), as it would at least prevent the scenario I just described from happening again. Or you could just exclude any app of type authentication from this auto-disabling behavior.

@nickvergessen
Copy link
Member

Also files_accesscontrol should e.g. not be disabled but preferably break things.
Same for external storages I guess, before they remove tons of data from all sync clients?

@rullzer
Copy link
Member

rullzer commented Dec 10, 2019

Lets tackle it in steps.

Lets first make sure that authentication apps do not get auto disabled.

rullzer added a commit that referenced this issue Dec 10, 2019
For #18249

If an app encounters an error during loading of app.php the app is
normally disabled. However. We should make sure that this doesn't happen
for authentication apps (looking at your user_saml).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
rullzer added a commit that referenced this issue Dec 10, 2019
For #18249

If an app encounters an error during loading of app.php the app is
normally disabled. However. We should make sure that this doesn't happen
for authentication apps (looking at your user_saml).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
backportbot-nextcloud bot pushed a commit that referenced this issue Dec 10, 2019
For #18249

If an app encounters an error during loading of app.php the app is
normally disabled. However. We should make sure that this doesn't happen
for authentication apps (looking at your user_saml).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
backportbot-nextcloud bot pushed a commit that referenced this issue Dec 10, 2019
For #18249

If an app encounters an error during loading of app.php the app is
normally disabled. However. We should make sure that this doesn't happen
for authentication apps (looking at your user_saml).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@szaimen

This comment was marked as outdated.

@szaimen szaimen added needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 1. to develop Accepted and waiting to be taken care of labels Jan 9, 2023
@nickvergessen nickvergessen added 1. to develop Accepted and waiting to be taken care of 26-feedback and removed needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop Accepted and waiting to be taken care of feature: apps management technical debt
Projects
None yet
Development

No branches or pull requests

6 participants