Make config file saving safe against write failures

[andrey-utkin]

Note: this hasn't been tested yet, is a speculative fix to the bug which has many bugreports, including #18620 and #25175 . I may go forward and create tests, for now it's to advance the conversation in this PR.

---

Make config file saving safe against write failures

In case of disk space depletion, writing a new version of config fails, leaving the config file empty.

This patch improves safety of updates to config.php by saving the new version of the config into a randomly-named temporary file in the same directory, and then renaming it to config.php, which renaming is atomic in POSIX-compliant filesystems and shouldn't involve disk space allocation. If writing the new config version is impossible, the current config remains unchanged.

This patch drops the use of file locking as unnecessary. File locking merely established order in which the concurrent independent processes attempted file writing. In the end, the process which was last to update the file "wins" - their changes persist and the changes of previous writers are dropped as there's no conflict detection or change merging mechanism anyway. With the current change, there is still some resulting ordering, and the last writer still wins in the same way. Readers don't need file locking as well, as opening config.php for reading always provides a certain consistent version of the file.

[come-nc]

@andrey-utkin Is writing access to config directory a new requirement then?

Apart from this it looks fine, I was worried about losing permissions setup by the admin but the old code was already doing a chmod anyway.
I am not comfortable with removing the flock, I would put it back around both read and write as before to be extra safe. Unless someone with more filesystem knowledge than me reviews it.

[andrey-utkin]

Is writing access to config directory a new requirement then?

Indeed, the config directory must have write permissions by the user running nextcloud code.
I couldn't find anything specific in the admin docs describing this aspect.
In the instances I run, both all-in-one and an older non-containerized setup from Hetzner, config dir is owned by www-data and is writable.
I guess many setups are already fine, while some, customized, may need to adapt?
I think we're very well positioned for this change since there is an actual config directory in existence and it's meant to be writable.

I am not comfortable with removing the flock, I would put it back around both read and write as before to be extra safe. Unless someone with more filesystem knowledge than me reviews it.

With the file being moved around, file locking won't work the way it used to (I should have written that in the commit message). A process has obtained the lock, but that file was replaces by a new file, and nobody's ever going to contend the lock on the old file handle, so it's become irrelevant. One way around is to define a special lock file, but some admins may not get the idea and remove the file or get worried...

[come-nc]

Indeed, the config directory must have write permissions by the user running nextcloud code. I couldn't find anything specific in the admin docs describing this aspect. In the instances I run, both all-in-one and an older non-containerized setup from Hetzner, config dir is owned by www-data and is writable. I guess many setups are already fine, while some, customized, may need to adapt? I think we're very well positioned for this change since there is an actual config directory in existence and it's meant to be writable.

This is reassuring.
In the case where someone has an instance without write permissions on the config folder, how will Nextcloud fail and signal the issue to the user?

I am not comfortable with removing the flock, I would put it back around both read and write as before to be extra safe. Unless someone with more filesystem knowledge than me reviews it.

With the file being moved around, file locking won't work the way it used to (I should have written that in the commit message). A process has obtained the lock, but that file was replaces by a new file, and nobody's ever going to contend the lock on the old file handle, so it's become irrelevant. One way around is to define a special lock file, but some admins may not get the idea and remove the file or get worried...

I see, flock works on a file resource and rename works with paths and not handles.
Still, the lock on reading would not help in the case an admin opens and modify the config.php file in vim for instance?

[andrey-utkin]

In the case where someone has an instance without write permissions on the config folder, how will Nextcloud fail and signal the issue to the user?

Just like before. By HintException, which is supposed to notify the user.

Still, the lock on reading would not help in the case an admin opens and modify the config.php file in vim for instance?

Seems like Vim by default does some renaming business under the cover (as opposed to echo or cat with redirection), not sure it does "atomic update" but possibly so, see https://unix.stackexchange.com/a/37177

I think it's reasonable to put the responsibility onto the sysadmin and their tooling in case of editing configs by hand on a live system. Like, if sysadmin edits /etc/hosts on a live system, it better doesn't get read while in inconsistent state.

[come-nc]

Ok, looks good to me.

[ArtificialOwl]

can confirm this patch fix the issue when disk is full on a virtual HD on linux

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[come-nc]

/backport to stable24

[come-nc]

/backport to stable23

[nickvergessen]

Since this was merged into master I got my config.php file wiped multiple times. Already my reset script sometimes triggers it, as well as running talk integration tests.

E.g. I set a couple of config values in my reset script via occ, the first bunch worked and then it gives a random write error:

Config value enabled for app admin_audit set to no
Config value enabled for app cloud_federation_api set to no
Config value enabled for app dashboard set to no
Config value enabled for app encryption set to no
Config value enabled for app federation set to no
Config value enabled for app files_external set to no
Config value enabled for app lookup_server_connector set to no
Config value enabled for app oauth2 set to no
Config value enabled for app systemtags set to no
Config value enabled for app testing set to no
Config value enabled for app updatenotification set to no
Config value enabled for app user_ldap set to no
Config value enabled for app user_status set to no
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config
Cannot write into "config" directory!
This can usually be fixed by giving the web server write access to the config directory.

But, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.
See https://docs.nextcloud.com/server/25/go.php?to=admin-config

[nickvergessen]

Reverted temporarily in #34137

[joshtrichards]

Was an alternative implementation ever proposed after this was reverted? I can't find one.