save: Save files atomically

[JoeKar]

The target situation shall be, that micro checks if the file to be stored already exists and if so it shall work with a temporary file first, before the target file is overwritten respective the temporary file renamed to the target file.
Possible symlinks pointing to the target file will be resolved, before the save takes place. This shall guarantee that this symlink isn't renamed by the new approach.

TODOs:

• Properly handle all of the irregular file types and character devices etc.pp.
• Simplify the chosen overwriteFile() interface (see: save: Save files atomically #3273 (comment))
• util.EscapePath(b.AbsPath) does not uniquely encode the file path (see: https://github.com/zyedidia/micro/pull/3273/files#r1599137940)
• b.Backup() executed asynchronously (from backupThread) is accessing the line array without locking. (see: https://github.com/zyedidia/micro/pull/3273/files#r1599137940)
• Decouple the backup step from the target file write (see: https://github.com/zyedidia/micro/pull/3273/files#r1600795305)
• Remove broken backup (see: https://github.com/zyedidia/micro/pull/3273/files#r1599076040)
• Rework save & backup trigger (see: save: Save files atomically #3273 (comment))

Fixes #1916
Fixes #3148
Fixes #3196

[dmaluka]

There are legitimate cases when rename fails (or some previous steps fail, e.g. creating the .tmp file) but it is actually perfectly possible to save the file, so we should still try our best to save it, rather than just return an error to the user. For example:

• Micro has no write permissions to the directory where the file is, so it cannot create other files in this directory and cannot change the name of the file. But it has write permissions to the file itself, so it can overwrite the file contents.
• The file itself is a mountpoint (e.g. a bind mountpoint), so technically it is on a different filesystem than other files in the same directory, so rename fails.

So, what to do? If the buffer has been successfully written to the temporary file and then os.Rename() failed, it means we already have a backup copy, so we can now overwrite the user's file as well. If overwrite successfully completes (including fsync!), we can remove the temporary file. If overwrite fails, we should keep the temporary file. And we should probably rename the temporary file to the backup file in ~/.config/micro/backups then? (And if this rename, in turn, fails (e.g. if ~/.config/micro/backups is on a different filesystem), we should try to copy & remove then? hmmm...)

But if it was not os.Rename() but some previous step that failed, e.g. if we failed to create .tmp file due to lack of permission, what then? Then we should probably be able to create a temporary file in ~/.config/micro/backups instead. Actually, such a temporary file should be already there, - it is the backup file.

All this makes me think: as you noted yourself in your TODO, we should try to reuse the backup file if possible; and given the above, it seems we'd better just use the backup, no other temporary file. So the algorithm can be simple and clean:

1. If the backup option is on, synchronize the backup file with the buffer.
2. If the backup option is off (no backup file yet), create the backup file.
3. Rename the backup file to the user's file.
4. If rename failed, overwrite the user's file with the buffer.
5. If overwrite failed, don't remove the backup.

The .tmp approach might have some advantages over the above simplified approach (the reduced likelihood of rename failure, e.g. if the user works a lot file files on a different filesystem than ~/.config/micro), but they seem to be outweighed by the increased likelihood of rename failure in other cases (e.g. lack of permission to the directory), the increased complexity and mess, and the fact that we may leave nasty .tmp files around in the user's directories.

[dmaluka]

BTW looks like vim does more or less the same: https://github.com/vim/vim/blob/master/src/bufwrite.c#L1179

Moreover, if the file is a symlink, it doesn't seem to try to resolve it, it immediately falls backs to overwriting the file from the backup instead of renaming. I guess we can do the same, for simplicity (after all, a symlink is just a corner case, and we need to support the overwrite fallback anyway).

OTOH, I guess it doesn't hurt if we do resolve the symlink. E.g. we can move that to a separate resolveSymlink() function, so the code will not become much more complicated.

P.S. And nano, for that matter, doesn't seem to use rename at all: https://git.savannah.gnu.org/cgit/nano.git/tree/src/files.c#n1748
It just always overwrites the file from the temporary file, even though it is slower and less convenient.

[dmaluka]

P.S. I hope we don't need to do all those scrupulous proactive checks like st.st_dev != st_old.st_dev that vim does, we can just try os.Rename() and fall back to overwrite if it fails. With the exception for symlinks, which we need to check or resolve beforehand, to prevent replacing them with regular files.

BTW... what about FIFOs, sockets, device files, ...?

[JoeKar]

BTW... what about FIFOs, sockets, device files, ...?

micro seems to have serious trouble with such files anyway, it crashed with a fifo, it hung with a fd...
vim "opens" them and tells, that these are no files.

Edit:
I suggest to handle files only in case they're supported by micro. Means, we should prevent loading (and storing) files like ModeDevice + ModeCharDevice, ModeIrregular, etc.pp. because a crash or stuck is unacceptable.

[JoeKar]

If the backup option is off (no backup file yet), create the backup file.

Since the backupThread is async (with possible delay) we can create the backup anyway and don't need to consider the option. For the removal of the backup the option is important then, right?

[dmaluka]

Our goal is only to ensure that the original file is never left in a damaged state (or in the worst case, it is left in a damaged state, but we have a backup so we can restore it), right? If /home is full so we cannot do anything, it is not our problem.

[dmaluka]

...A more tricky problem: when renaming, we should preserve file permissions (chmod) of the original file, and probably also its owner & group (chown). And even worse, probably also its extended attributes (setxattr).

Vim seems to do all that: here, here and here.

And if anything of that fails, we should probably fall back to overwrite, before even trying to rename. (We don't want to silently change user's file attributes, right?)

[JoeKar]

Ok, that's a position. But even then we can only proceed in the moment the backup was successful, otherwise we would take a broken backup as source to overwrite a healthy target.

Our goal is only to ensure that the original file is never left in a damaged state (or in the worst case, it is left in a damaged state, but we have a backup so we can restore it), right?

Do we then really have? In the moment we ignore running /home out of space and accept damaging the backup we've no "plan b" in the moment something goes wrong with the target.

Edit:

And if anything of that fails, we should probably fall back to overwrite, before even trying to rename. (We don't want to silently change user's file attributes, right?)

Yes, then we're safer with the direct overwrite from a different source instead moving the source.

[dmaluka]

We should make sure to avoid this situation. If the backup was unsuccessful, we should not overwrite the target file with it, we should just tell the user that we cannot save the file. Also I think we should remove the broken backup.

[JoeKar]

Ok, I can live with that. It's the simpler approach and we don't need to use temporary files.

[JoeKar]

Still a lot of rework left, but thank your for your time reviewing this! 👍

[JoeKar]

I did a lot of (commit) refactoring, but still I'm not fully happy with it.
The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

[dmaluka]

I did a lot of (commit) refactoring, but still I'm not fully happy with it. The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

IIUC, basically, all these troubles are due to the need to avoid circular dependencies between packages?

If so, it seems we could avoid these troubles by implementing overwrite & backup stuff as a part of the config package. OTOH, that sounds like an abuse of the config package: it is supposed to implement stuff related specifically to configuration, not stuff like buffer backups.

So maybe instead, simply provide separate implementations of overwrite & backup stuff for different kinds of files, in both buffer and config packages (especially since, as I noted in another comment, it seems better to have separate implementations of Overwrite() anyway). Maybe make them implement some common interface (if there is a need for that).

A very rough idea:

type FileWriter interface {
	Overwrite(name string) error
	BackupName(name string) string
	KeepBackup() bool
}

and a common Write() implementation using this interface:

func Write(name string, fwriter FileWriter) error

Or maybe even Write() implementations need to be different for different files.

P.S. Side note: maybe let's name it SafeWrite()?

[JoeKar]

Yes, I know dc701a1 doesn't really belong to the PR in the first place, but I touched some WriteFile's anyway.
It was not a big deal to do this cleanup.

[dmaluka]

First, Backup() is called not only from backupThread(), it is also called from cmd/micro/micro.go during an emergency exit after a crash. So with this change, those emergency backups don't work anymore.

Second, this requestedBackup thing is ugly and unreliable in the first place. When we just set a variable to a value, there is no guarantee that this new value becomes immediately visible to other goroutines (since there are no memory barriers used). The easiest way to make it work reliably would be to use atomics, like we already do for b.fini (though that is still ugly, I'd prefer a channel-based solution instead, unless it's too complicated).

Third, ok, your change prevents starting an async backup after the buffer has been saved, but it still doesn't prevent starting an async backup while a save is in progress, or starting a save while an async backup is in progress?

A sketchy idea I have in my mind how to fix that is to do both saves and periodic backups in the same dedicated goroutine, upon (different) requests from the main goroutine (so that they are automatically serialized, no need for more mutexes or other crap). This goroutine would also notify the main goroutine about a completing of a save or a periodic backup. For saves, the main goroutine would block on waiting for this completion notification. For periodic backups, the main goroutine could just update requestedBackup (or whatever we name it) upon this completion notification.

Or perhaps we could do without a separate goroutine at all, i.e. do periodic backups in the main goroutine instead, on the main event loop, upon timer events? That would easily solve all synchronization problems, but that would mean freezing the UI when writing a backup to a (slow) disk takes a noticeable amount of time... (Although we need to partially freeze it anyway, since we need to lock the buffer, - but that only freezes buffer editing, not the entire UI...)

[JoeKar]

I like the idea with the serialization of the save and backup triggers. As of now this should have the least impact on the UI handling and buffer editing.
Let us try this approach.

[dmaluka]

So I've been testing this, and it is still not really safe or useful. I try to save a file on an almost full tmpfs filesystem, micro says it fails to save it due to no space left on device, the file becomes corrupted (written imcompletely), but before that micro indeed successfully writes it to the backup file in ~/.config/micro/backups, so I should be able at least to recover the file next time I open it in micro? But no, I can't, since after I close the buffer, the backup file in ~/.config/micro/backups disappears (probably because we always call RemoveBackup() in b.Fini()).