Allow ext storage Local to go unavailable

[PVince81]

• Resolves: Detect external storage of type Local as unavailable when path is unmounted on system #39706

Summary

Whenever an external storage of type Local points at a non-existing directory, process this as a StorageNotAvailable instead of returning 404.

This makes desktop clients ignore the folder instead of deleting it when it becomes unavailable.

The code change was limited to external storages to avoid issues during setup and with the default home storage.

TODO

• actually test with desktop client to confirm behavior: Allow ext storage Local to go unavailable #39707 (comment)
• properly convert the exception so that it appears in Webdav response

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes -> N/A
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[PVince81]

I've tested with desktop client 3.9.0 and it did not delete the unavailable folder.
After restoring the folder, syncing continued.
Also, while the folder was unavailable, syncing of other folders worked fine.

However the desktop client complained repeatedly about the folder causing an error.
Either the response needs to be tweaked or the client needs adjustment.
The PROPFIND response when unavailable looks like this:

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\ServiceUnavailable</s:exception>
  <s:message>Storage is temporarily not available</s:message>
</d:error>

I'm guessing something is missing to have the exception name be properly propagated into the XML as StorageNotAvailableException.

• find out how to properly convert the exception

[PVince81]

from what I see, other storages tend to throw that exception in regular methods, not in the constructor.
I purposefully chose to throw earlier as I didn't want to do the additional stat('') for any kind of exception happening in the methods.

Currently throwing the exception will trigger a FailedStorage in constructStorage.
I'm guessing that the Webdav layer doesn't know how to deal with FailedStorage yet.

[PVince81]

Found it, it seems that StorageNotAvailableException is converted to ServiceNotAvailableException... why not preserve it or convert to Webdav ? https://github.com/nextcloud/server/blob/bugfix/39706/local-ext-storage-unavailable-mode/apps/dav/lib/Connector/Sabre/ObjectTree.php#L161

I think I remembered it wrong and this distinction was never implemented on the client side, or a different approach was used ? Searching for StorageNotAvailableException in the desktop client code did not show results. @tobiasKaminsky

Or is the distinction done whether the 503 is returned on the root or on a subdir ?

In any case it would be good to have errors be less flashy when a storage is not mounted, but still let the user know somehow, maybe only once the first time ?

[max-nextcloud]

I'm currently investigating a scenario where metadata (tags, shares, etc) were lost after the main data directory which was on an NFS went offline.

Would such a check also make sense for the data directory itself?

[icewind1991]

Would such a check also make sense for the data directory itself?

It should already do this by checking for the existing of the .ocdata file in the root of the data directory.

[szaimen]

@PVince81 is it fine when I merge this or do you want to address the missing point first? Would also be okay to do this in follow-up imho :)