Skip FailedStorage in background scan

[PVince81]

Description

The background job that scans storages must skip failed storages to
avoid potential exceptions, especially when the failed storage comes
from a shared storage where the source is not accessible.

This fix adds not only a check for FailedStorage in the background scanner but also hardens the FailedStorage class with a new FailedScanner class to prevent trying to scan into the failed storage.
Note that the latter hardening is not needed directly for the fix thanks to the extra check in the background job but could be useful to catch other code paths that might reach the scanner part.

Additionally this also skips unavailable external storages earlier.

Related Issue

Fixes #26055
Read from #26055 (comment)

Motivation and Context

How Has This Been Tested?

See steps

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

Backports:

• stable9.1: I suggest to only backport the check in Utils/Scanner.php but not the new FailedScanner class

Please review @jvillafanez @DeepDiver1975

[mention-bot]

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @icewind1991 and @Xenopathic to be potential reviewers.

[jvillafanez]

The "FailedScanner" class doesn't follow the expected return values from the "IScanner" class. We need either adjust the "FailedScanner" class to return proper values according the interface, or change the documentation in the "IScanner" class.

We also should check if returning "false" is being handled correctly by the system because, again, the interface doesn't say a boolean can be returned in those methods.

[PVince81]

@jvillafanez I've adjusted the return values to be null after seeing that it's the regular scanner does. I've also updated the IScanner PHPDoc to state that null can be a value too.

[jvillafanez]

Any other "FailedThing" we should provide, such as propagator, watcher, etc? Last time I checked this, the storage was pretty coupled with all these components.

[PVince81]

@jvillafanez good point. Or maybe we just throw directly if someone calls getWatcher or getPropagator. I'll check, maybe that happens already.

These FS APIs could really use a good refactor.

[PVince81]

Tests passed but unpublished

[PVince81]

@jvillafanez I added the methods getUpdater, getPropagator and getWatcher but made them throw StorageNotAvailableException directly.

Retested with some unavailable storage scenarios like fed share, ext storage, still works fine with:

• occ files:scan
• cron.php bkg scan
• Webdav access

[jvillafanez]

I don't think it's a good idea to throw that exception there and return an instance in the other methods. I think it's more natural to either return "null" or "false", or return a "FailedWhatever", than throwing an exception.

Suppose that I get an storage instance (which I don't know and I don't care if it's a FailedStorage), and I request to get the watcher. A "StorageNotAvailable" exception thrown there is completely unexpected, specially if other methods such as the "getCache" returns an instance fine. In addition, the "StorageNotAvailable" is suppose to be thrown when you're accessing the backend, but those method don't access to the backend.

[PVince81]

Yeah, you're right... consistency is key.
We could return "null" instead but in some places in the code it might also come as a surprised as there are usually no null checks. So I guess having a FailedSomething is the way to go ? (even though I'm not too happy to have so many such classes)

[jvillafanez]

We have to patch one thing or another: either all the places where this is used accept "null" or we need to make sure all the methods return an instance.

[PVince81]

You know what... I think I'm going to remove these bullshit classes and restrict the fix to only checking in the background job and be done with it. There are more important things to work on.

[PVince81]

I have now simplified the PR to the minimum required to make it work and fix the issue.

Please review.

[jvillafanez]

Maybe a log to know what storages are being skipped. Other than that 👍

[PVince81]

stable9.1: #26977

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.