Atomic saving

[lazka]

Originally reported by: Daniel Plachotich (Bitbucket: danpla, GitHub: danpla)

---

Resizing files "in place" is pretty dangerous way, especially for the large ones like audio. Both memory mapping and ordinary IO can fail during the process for many reasons and leave a corrupt file.

The only safe approach is to use temporary file. This is already used in some players and tagging libraries, for example, in Foobar2000 [1][2][3].

The algorithm is pretty simple:

• Rename the original file (for example, by appending ".orig" to the end)
• Create a new one with the name of original and write to it:
  • New tags
  • Copy data from the original file
• On success, remove the original

---

• Bitbucket: https://bitbucket.org/lazka/mutagen/issue/241

[lazka]

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):

---

On unix it's usually done like this: copy, change, fsync, rename.

I don't think mutagen needs any change for this to work or am I missing something?

Having example code (or a public helper function if this is complicated enough/platform depended) in the docs for this seems like a good idea though.

[lazka]

Original comment by Daniel Plachotich (Bitbucket: danpla, GitHub: danpla):

---

Copying file is not the solution for several reasons.

• This will require x2 more IO calls. This is especially painful for SSD drives and takes x2 times than needed.
• If the header has enough padding, the new one can be written directly without temporary files or anything else. The padding sizes are handled internally, so manual copy-change by the user will be completely redundant most of the times. By the same reason, there is no way to make a helper function or any meaningful example code.
• The mutagen (as well as all programs that use it) will still write files in the dangerous way.
• Even if you could add an example, it would be too complicated: for example, its good idea to check the free space in the same path as a file (as a result — in the same partition/drive) and, if it is not enough, try to fall back to the system default temporary path, and finally raise an exception if there is no free space anywhere. shutil.disk_usage was introduced in 3.3, so you will also need to write something like this. The resulting code would be quite complex, and I almost sure no one will copy-paste it in their programs — almost all will use "dangerous" mutagen API directly as before.

As you see, there is no way to do this without changes in mutagen. I don't see any reasons to afraid this changes — nothing will be broken. I'm sure that both developers and end users of programs will be happy, because data safety is paramount.

[lazka]

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):

---

Copying file is not the solution for several reasons.

I'm confused; your proposed algorithm copies the file. What am I missing?

This will require x2 more IO calls. This is especially painful for SSD drives and takes x2 times than needed.

...

If the header has enough padding, the new one can be written directly without temporary files or anything else. The padding sizes are handled internally, so manual copy-change by the user will be completely redundant most of the times. By the same reason, there is no way to make a helper function or any meaningful example code.

If you want to prevent data loss on disk errors/power loss you need a temporary file. You never know if a write() succeeds and how much is written until the error.

The mutagen (as well as all programs that use it) will still write files in the dangerous way.

I wouldn't want every application using mutagen to get slow because of this so this is expected.

Even if you could add an example, it would be too complicated: for example, its good idea to check the free space in the same path as a file (as a result — in the same partition/drive) and, if it is not enough, try to fall back to the system default temporary path, and finally raise an exception if there is no free space anywhere. shutil.disk_usage was introduced in 3.3, so you will also need to write something like this. The resulting code would be quite complex, and I almost sure no one will copy-paste it in their programs — almost all will use "dangerous" mutagen API directly as before.

rename doesn't work across mount points.

[lazka]

Original comment by Daniel Plachotich (Bitbucket: danpla, GitHub: danpla):

---

If I clearly understand your previous post, you suggested firs make a copy and only then insert the new tags. Inserting require shift the whole data, so there are x2 more IO operations than actually needed: (1) the copying itself and (2) the shifting. In the my algorithm, I suggest to create a new empty temporary file, write new headers and then copy data from the original. Simply saying, it uses only copying, in a one pass; shifting is not needed.

If you want to prevent data loss on disk errors/power loss you need a temporary file. You never know if a write() succeeds and how much is written until the error.

Only the tags can be damaged in this way, but the audio data itself will be untouched. More or less easy, the corrupt tags can be simply removed.

I wouldn't want every application using mutagen to get slow because of this so this is expected.

As I said before, the amount of IO operations will be more or less the same as in regular shift using read()-write() or in a one copy operation, so I don't think that there will be any noticeable performance loss.

rename doesn't work across mount points.

Yes, I know — I actually meant that temporary directory is one possible example solution when there is no space on the current partition and it's requires moving (not renaming, of course) the file when the mount points are different. But instead it can be better to just raise an exception.

Probably the links I posted in the previous post are better explain the idea:

Callback interface for write-tags-to-temp-file-and-swap scheme, used for ID3v2 tag updates and such where entire file needs to be rewritten. As a speed optimization, file content can be copied to a temporary file in the same directory as the file being updated, and then source file can be swapped for the newly created file with updated tags. This also gives better security against data loss on crash compared to rewriting the file in place and using memory or generic temporary file APIs to store content being rewritten.

---

If your FLAC files have enough padding for the metadata then they are modified in place. This is fast and doesn't cause any fragmentation or unnecessary disk writes. If they do not have the required space for the tags then foobar will have to re-create the files. This is done by writing the required headers and metadata+padding blocks into new temporary files followed by copying the encoded audio data from the original files into the temp files. Upon success the original files are destroyed and the temp files are renamed to replace them.

---

foobar2000 does not cause file corruption, at least on files that are valid to begin with. If files have enough padding writing new tags involves just writing the few changed bytes. If that can't be done and temporary files need to be involved the original file won't be touched unless the temp file was written succesfully. Once the temporary file is done the two files are simply swapped in place and the original file is removed on success. If the operating system reports errors on any operation foobar's console will report them to the user.

[lazka]

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):

---

If I clearly understand your previous post, you suggested firs make a copy and only then insert the new tags. Inserting require shift the whole data, so there are x2 more IO operations than actually needed: (1) the copying itself and (2) the shifting. In the my algorithm, I suggest to create a new empty temporary file, write new headers and then copy data from the original. Simply saying, it uses only copying, in a one pass; shifting is not needed.

OK, now I get what you mean. Note that with the recent padding changes it is unlikely that we do need to resize.

FileType.save() currently takes a filename argument to save to another file but it is deprecated (as it's confusing that only tags get saved..). We could allow passing a non-existing/empty file path to FileType and make it copy the file + changed tags there... but this change probably isn't easy because most code assumes it saves to a valid file.

a = mutagen.mp4.MP4("bla.mp4")
# changes...
a.save("bla.mp4.temp")
fsync()
os.rename("bla.mp4.temp", "bla.mp4")

What do you think?

Only the tags can be damaged in this way, but the audio data itself will be untouched. More or less easy, the corrupt tags can be simply removed.

This depends on the file type and parser error handling.

As I said before, the amount of IO operations will be more or less the same as in regular shift using read()-write() or in a one copy operation, so I don't think that there will be any noticeable performance loss.

We currently only replace a few kb without resizing in the common case.. so this will be 100x slower. Just saying that the default behavior as it is now can't change in mutagen.

[lazka]

Original comment by Daniel Plachotich (Bitbucket: danpla, GitHub: danpla):

---

As I mentioned before, mutagen will behave as now in case of suitable amount of padding, i.e. tags will written directly without resizing.

I think such code is not a solution: it makes impossible to use advantage of an existing padding in most of cases, because save() will have to copy contents of the original file in all cases. It's much more simpler to do all the stuff inside the save(). By the way, fsync (os.fsync?) is not needed here, since both files are closed at this point:
http://stackoverflow.com/a/7127162; fsync requires an opened file handle.

Actually, the solution I proposed is just a "safe" alternative to internally used insert_bytes()/delete_bytes(); it's very simple to implement.

[lazka]

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):

---

As I mentioned before, mutagen will behave as now in case of suitable amount of padding, i.e. tags will written directly without resizing.

What if writing fails in the middle of this?

By the way, fsync (os.fsync?) is not needed here, since both files are closed at this point

flush + os.fsync is needed before close and rename to ensure the data gets written to disk.

[lazka]

Original comment by Daniel Plachotich (Bitbucket: danpla, GitHub: danpla):

---

What if writing fails in the middle of this?

Like any other software (and Mutagen in particular) does — nothing. The audio is not corrupted, so this is not a big issue. But it may be useful to add a global option (disabled by default) so that temporary file will be used in all cases.

flush + os.fsync is needed before close and rename to ensure the data gets written to disk.

Closing a file does this things automatically. flush and os.fsync manually called when file doesn't closed, but a data needs to be immediately written on disk (to be available for other processes, etc.).

[lazka]

Original comment by Christoph Reiter (Bitbucket: lazka, GitHub: lazka):

---

Like any other software (and Mutagen in particular) does — nothing. The audio is not corrupted, so this is not a big issue. But it may be useful to add a global option (disabled by default) so that temporary file will be used in all cases.

The audio may not be corrupted but the file is (in case of mp4 the audio will too be corrupted as we need to update the offsets). I don't see any benefit in a data integrity feature which only works in the rare case of a resize.

Closing a file does this things automatically. flush and os.fsync manually called when file doesn't closed, but a data needs to be immediately written on disk (to be available for other processes, etc.).

man close says otherwise:

"A successful close does not guarantee that the data has been success‐
fully saved to disk, as the kernel defers writes. It is not common for
a filesystem to flush the buffers when the stream is closed. If you
need to be sure that the data is physically stored, use fsync(2). (It
will depend on the disk hardware at this point.)"

[lazka]

Original comment by Daniel Plachotich (Bitbucket: danpla, GitHub: danpla):

---

This improvement is primarily for audio integrity.

Padding in ID3 and FLAC added especially for this purpose, i.e. don't rewrite file if possible. If padding in MP4 doesn't allow do this without the risk of audio corruption, it's not an issue to use temp each time for it.

My apologies — Python does only flush on close, but not fsync.

Although I don't sure it really needed. For example, neither cp from coreutils nor shutil.copy use it:

• http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c
• https://hg.python.org/cpython/file/tip/Lib/shutil.py

[Lanchon]

FYI:

this link discusses the create/write/fsync/rename idiom:
http://stackoverflow.com/questions/2333872/atomic-writing-to-file-with-python

regarding this code:

f = open(tmpFile, 'w')
f.write(text)
# make sure that all data is on disk
# see http://stackoverflow.com/questions/7433057/is-rename-without-fsync-safe
f.flush()
os.fsync(f.fileno()) 
f.close()

os.rename(tmpFile, myFile)

it says:

If successful, the renaming will be an atomic operation (this is a POSIX requirement). On Windows, if dst already exists, OSError will be raised even if it is a file; there may be no way to implement an atomic rename when dst names an existing file

also see this old project:
https://github.com/sashka/atomicfile
https://github.com/sashka/atomicfile/search?q=windows&type=Issues

and this notes from microsoft:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365241%28v=vs.85%29.aspx
https://msdn.microsoft.com/en-us/library/windows/desktop/hh802690(v=vs.85).aspx#applications_updating_a_single_file_with_document-like_data

[lazka]

Thanks.

I've implemented something similar in quodlibet: https://github.com/quodlibet/quodlibet/blob/master/quodlibet/quodlibet/util/atomic.py

[lazka]

Proposed API:

def replace(self, filename=None):
    """Replaces the file with a copy including all tag changes. In case it
    fails or gets interrupted the file will be unchanged. If it returns
    successfully the file will be replaced and all file system changes will be
    flushed to disk.

    This will prevent corrupt files due to bugs in mutagen or if the system
    stops working due to a power failure, crashes etc.

    This operation can fail in cases where save() would succeed.
    This operation can be significantly slower than save().

    Args:
        filename (fspath): The file to save to or None to use the file
            loaded from
    Raises:
        MutagenError: in case of an error
    """

    raise NotImplementedError

[Lanchon]

Replaces the file with a copy

all file system changes will be flushed to disk.

these are implementation details and shouldn't be in the API. atomicity is the key property here. implementation strategy and durability should not be specified.

def replace(self, filename=None)

IMHO this is the wrong API. no file system in real use that i know of is fully transactional. this API provides and abstraction that is not well supported underneath. in the end, it will create a copy of the file in all supported OSes; this is why i think it is the wrong API.

a more useful API would be saveAs(self, file=None) that saves pending changes to a copy of the file somewhere else. API help would include a note that, if 'file' parameter is missing, it will save to a temp file and then replace the original as atomically as possible. (and stating that this saving mode is safer and slower than the usual save is OK; but it should be noted that the usual save is safe enough for normal use.)

This operation can be significantly slower than save().

yes, the saveAs() API could be added. but the important issue here is that it doesn't fix the problem with the current implementation of save()! furthermore, you leave it to the app programmer to decide what to do, and they know little or nothing about risks. what do you expect them to do? wouldn't all of them either choose replace()/saveAs() or ask you a question personally?

plus the 'safe save' is only needed when the ID3v2 tag is added or resized, because loosing the tag is not as bad a loosing the payload, and the tag is so small that a partial save is really unlikely. and with replace/saveAs you force a copy on each save, when 99% of the time it is not needed.

what is really needed is a safe 'payload scroll' only when resizing the ID3v2 tag. IMO, the normal save should be upgraded to behave in a safe enough manner, and that is it. this entails copying the file but only when a payload move is needed. more free space is needed by the new save? too bad. i rather have a reasonably safe save that complains the disk is full than a dangerous save. if the disk is full, users will have to deal with that very soon, so let them deal with it now.

so to recap, my proposal is:

1. fix save(): its current implementation is unsafe and unfit for all uses. most existing client programs will never upgrade to replace() anyway, so replace won't fix the problem. most newer clients will choose replace() and your library will become slow and hated.
2. optionally (and at a later time) add saveAs() API. include with it a proper implementation of "save to temp and rename" when no target file is given.

[Lanchon]

and comparatively the save/replace solution has problems:

1. save() is unfit for all uses given that it is unsafe. it should be fixed or removed from the API.
2. replace() is unfit for most uses given that its implementation is terribly inefficient most of the time.

keep in mind that the shortcoming of the current save implementation is not a 'designed feature'. it is not a calculated compromise of performance and safety; it is a plain-old bug in the implementation, arising from not considering certain failure modes. its behavior is not something anyone would want to keep, but a bug that should be fixed.

fortunately the save() API is the right API for the fixed implementation (a safe enough save), so you can fix this without impacting clients.

[lazka]

Thanks for your input.

Replaces the file with a copy
all file system changes will be flushed to disk.

these are implementation details and shouldn't be in the API. atomicity is the key property here. implementation strategy and durability should not be specified.

I didn't want to be too specific as in reality the drive/file system can lie about flushing and on Windows we don't know what's happening exactly. We could say it does an atomic replace if the OS/hardware supports it, if not it will at least reduce the probability of data corruption as far as possible (?)

def replace(self, filename=None)

IMHO this is the wrong API. no file system in real use that i know of is fully transactional. this API provides and abstraction that is not well supported underneath. in the end, it will create a copy of the file in all supported OSes; this is why i think it is the wrong API.

Why is it wrong?

a more useful API would be saveAs(self, file=None) that saves pending changes to a copy of the file somewhere else. API help would include a note that, if 'file' parameter is missing, it will save to a temp file and then replace the original as atomically as possible. (and stating that this saving mode is safer and slower than the usual save is OK; but it should be noted that the usual save is safe enough for normal use.)

There might be a misunderstanding here. I planed exactly what you propose with replace().
The filename argument allows to save tags to another file than loaded from, likeID3().replace("foo.mp3"). Imo temp file creation should be an implementation detail as the only logical place to put it in is the same directory. Or can you think of a use case providing your own temp file?

This operation can be significantly slower than save().

yes, the saveAs() API could be added. but the important issue here is that it doesn't fix the problem with the current implementation of save()! furthermore, you leave it to the app programmer to decide what to do, and they know little or nothing about risks. what do you expect them to do? wouldn't all of them either choose replace()/saveAs() or ask you a question personally?

Good question. I'll have to think about that.

plus the 'safe save' is only needed when the ID3v2 tag is added or resized, because loosing the tag is not as bad a loosing the payload, and the tag is so small that a partial save is really unlikely. and with replace/saveAs you force a copy on each save, when 99% of the time it is not needed.

I don't like multiple shades of "safe". If we somehow leave the file in an invalid but playable state, the next tagger might get tricked into corrupting the file, or some players wont accept the file. And I really don't want to think at every operation in the code about what would happen if there was a crash right after. And is there any guarantee that file operations get flushed to disk in the order I execute them?

what is really needed is a safe 'payload scroll' only when resizing the ID3v2 tag. IMO, the normal save should be upgraded to behave in a safe enough manner, and that is it. this entails copying the file but only when a payload move is needed. more free space is needed by the new save? too bad. i rather have a reasonably safe save that complains the disk is full than a dangerous save. if the disk is full, users will have to deal with that very soon, so let them deal with it now.

I'm not sure, if we talk probability here, how often the "safe enough" approach would result in a recoverable file (from the user POV, not starting up the HEX editor and cleaning things up by hand). (Don't forget all the other tagging formats..).

I agree that free space, permissions, and path limits reached by the temp file should just be ignored/error out.

so to recap, my proposal is:

1. fix save(): its current implementation is unsafe and unfit for all uses. most existing client programs will never upgrade to replace() anyway, so replace won't fix the problem. most newer clients will choose replace() and your library will become slow and hated.

I think you are exaggerating a bit. What amount of programs do atomic replace for your documents? If your kernel crashes a lot you have lots of other problems too. I at least hope that kernel bugs like the one you reported are rare.

1. optionally (and at a later time) add saveAs() API. include with it a proper implementation of "save to temp and rename" when no target file is given.

Regarding the naming. I prefer replace() as it highlights the the file gets replaced as a whole, while when I read saveAs() I think of saving to another location (like the "save as" menu entry in many file menus), and while that is a step of the process, it's just an implementation detail.

[Lanchon]

well, what can i say, my view is different.

IMHO save() is broken because it causes a very high probability of loosing data, compared to any other saves the users normally do. to worsen things, the data lost is not the data being edited (the audio), and this is the worst scenario and something users won't understand or be lenient with. save() has to be fixed because it is broken and has not use case as it is. there is no point in introducing another API and leaving a broken one up. and expecting existing clients to change is not realistic.

replace() is not the solution, as it causes massive performance loss all the time, when most of the time it is not needed.

the probability that a 1KB structure gets partially written is abysmally low compared to the probability that a 100MB file gets partially written; maybe 100.000 times smaller. (i listen to dj sets mostly, so my individual files are typically over 100MB.) this means that around 100.000 corruption incidents of moved tracks would be observed before 1 tag-only corruption happens (if both operations are equally likely to be executed). but of course, before 100.000 corruptions happen your library would be eliminated from any successful app.

i guess 100.000 times safer than the current implementation can be called 'safe enough', given that you are not currently bombarded by thousands of corruption reports.

Regarding the naming. I prefer replace() as it highlights the the file gets replaced as a whole, while when I read saveAs() I think of saving to another location

this is exactly what saveAs() does: saves a new file. i proposed that omitting the argument causes save as temp file and replacement of the original. ok, maybe this is not the best interface. so instead: saveAs() must be called with a file argument, and the file must not exist. saveAtomically(), saveViaCopy(), atomicSave(), saveSafely(), safeSave() are all possible names of "saveAs() to temp and then atomically replace". the docs should state that although atomicSave() is safer, save() should be is safe enough for all uses. (implementation of atomicSave() has a low priority IMHO because it will almost never be used. fixing save() has a high priority.)

replace() is a bad name, and the semantics are not at all clear. why would you call replace("xxx.mp3") to save a new xxx.mp3 file? and if you want to save to a new file, why would the provided API silently replace any existing file instead of complaining that it exists? and most importantly: if the file exists, would replace() replace it atomically? if not, how would you document that? if yes, then there is a 3rd filename involved that is not being specified!

my semantics:
save() is save in-place. reasonably safe and reasonably performant. (creates a temp copy only if a dangerous operation is needed.)
saveAs() is save to new file. no ugly replacement semantics: the method just returns an error if the file exists.
saveAtomically() is an ultra-safe, ultra slow implementation of save in-place. included for completeness and because it is easy to implement once you have saveAs().

What amount of programs do atomic replace for your documents?

all of them i suppose. the problem is that some apps didn't do an fsync so data losses happened all the same when people changed file systems. you can read about it: https://lwn.net/Articles/322823/

the point is that data loss happens a lot. if your library is used that is; if nobody uses it, then yes, whatever you do here is just fine :). but the idea is that a library must be robust just in case it becomes successful. this is why IMHO save() has to be fixed.

i know that adding code to implement a decision to copy or not before starting the save process to the existing code base can be painful. but you don't need to do that. you can start saving in-place and only create a temp when you need to scroll big data. after the scroll, you immediately replace atomically, and then continue to save in-place. this is an easy change (only the scroll operation has to be modified) and yet it adds a lot of reliability.

this highlights that saveAs()/saveAtomically() are not necessarily needed to solve (and are not the solution to) save()'s issues. saveAs()/saveAtomically(), if desired, can be added at a later stage.

(of course, in a perfect world you would detect scroll before starting save() and use saveAs() instead.)

[lazka]

well, what can i say, my view is different.

Yeah, but I appreciate your input. I will ask some of the larger mutagen users and see what semantics they would expect or what API they want.

[catap]

@lazka any news about that?