Created attachment 188 [details]
Patch from Ed to correct the most likely silent write failures on disk full

(In reply to comment #2)
Gale said (and it seems to be the trend on -dev) "Errors should be
aggregated I think, certainly not one message box per log error."

I resolved this by trapping the first error, not retrying (seems to be the trend on -dev), popping a dialog telling the user of the specific error, rolling back all successful Write()s in the current operation, doing a complete recovery of memory allocations and cleaning up any leftover orphans (files & folders); then a second dialog appears saying "could not (save or whatever)". Only the single Log entry occurs and there is no cascade.

(In reply to comment #2)

Issues with the patch: 

* AudioIO.cpp: Putting up wxMessageBox from AudioIO will cause thread issues, right? Also, bad form to present method name and tech details to users, except possibly in the log. E.g., "AudioIO::FillBuffers mCaptureTracks[i]->Append failed 1" -- a user can only be nonplussed by that.

* SimpleBlockFile.*: mbHadWriteFailure is used only outside the class. 

* DirManager.cpp: Why remove the default parameter values in the declaration of RecursivelyEnumerate? Note that in DirManager::CleanTempDir, RecursivelyEnumerate is called without specifying those values. Does that even compile for you? You did the same thing on other methods. And there are lots of changes that look like they're just spaces added or removed. In general, why make any change if it "ain't broken"? Just makes more code to review, unnecessarily.

* DirManager.cpp: Similar comment about text in messages to user: "file write error DEBUG" isn't even a sentence.

* DirManager.cpp: Weird to throw an XMLFileWriterException in a non-XMLFileWriter class. Plus, your messages cite wxWidgets methods -- again, totally perplexing to a user.

* It looks like you did not address the commented TODO places "out of disk space". 


That's enough problems in specifics, as much as I'm willing to review at this point.


I think this patch needs a *lot* of work. Also, it adds a lot more text to user, and that would require a lot of translation work. Overall, I don't think it's worthwhile before 2.0. 


Your patch makes *lots* of changes unrelated to this bug, and lots of similar code in many places. I suggest that we address only the topic of this bug, because it's fairly significant. 

And I think there must be a better locus of where to discover and back out of its occurrence. We should at least start with the "out of disk space" situations already noted in the code. And check all the wxASSERT calls in Sequence. 


Also, rather than putting removal code in lots of places, did you look into just doing an internal Undo upon failure?


Finally, fwiw, the "Error" entries in the log are generated by wxWidgets, not our code. I'll look into debugging at least where this happens per the Steps. 


Thanks for the effort!

(In reply to comment #3)

In commit r11447, I modified WriteSimpleBlockFile to check the results of all its calls to wxFFile::Write(), and return false if any fails. They're now noticed with a wxASSERT() in SimpleBlockFile constructor. Unfortunately, the current architecture does not make it easy to alert user to this via dialog, but at least we now check for it and catch it in debug.

Ed, if you have comments on my questions in comment #2, I'm open to them. In particular, the 'throw' to get around the architecture issues, might have merit. Other than that, I think further work on this should wait until post-2.0.

Patch rejected, per comments by Vaughan.

Raised to P2, because it appears as a listed limitation on the Wikipedia Audacity page, so is having a significant impact on the perception of Audacity.

Wording of release note changed as edits (cut and paste) need not take much space, but an effect applied to the whole track does.

We do actually provide an estimate of recording time left available (based on free disk space) in the Status Bar message - but I'm guessing that many folk don't notice that or pay much attention to it.

Maybe in addition we could, on Audacity start-up, flash up a message dialog in "large friendly letters" also showing how much recording time is available (and maybe making a reference there to the Status Bar space/time monitor).

(In reply to Peter Sampson from comment #7)
But we don't tell users when they have actually run out, and that is the problem.

(In reply to James Crook from comment #8)
Indeed, but telling them when they have "actually run out" is probably a bit too late.

Ideally we should warn them as disk space is getting critically low (10%, 5%? - less?), telling them how much recording time remains, so they can halt the recording if appropriate/possible.  

If they choose not to stop recording then Audacity should periodically remind them with the same warning message with the new space/time remaining.

If they are about to totally run out of space Audacity should stop recording of its own accord and tell the user why it did so.  We should not wait until all available space has gone as not all OSs cope well with a disk full situation.

The same space/time warning message is the one I would like to see shown on startup (with an option, a preference, to be able to turn off the startup message).

The running out of space messages should be mandatory imo and should not be capable of being turned off.

The more I look at this the more I think that this bug really deserves P1 status.

(In reply to Peter Sampson from comment #9)
There is a message of low temporary space on launching Audacity (100 MB, IIRC). But you can then save an empty project to another drive with almost no space and get no warning there is little space there.  

There is an out of disk space warning in the status bar when you run out during recording.    

Just the same as stopping recording when out of space, Audacity should not go ahead with an edit that it can see in advance it does not have the space for, which will result in damage to the audio (not immediately apparent when zoomed out). 

I think P2 will have to do for now.

See http://wiki.audacityteam.org/wiki/Proposal:_improvements_to_Audacity_to_prevent_disk_over-run_issues

We now have 36 FIXME: TRAP_ERR comments in the Audacity source code.  If these are fixed then probably we have caught all can't write to disk issues.  We need to write the code once to report the error and abort recording or abort whatever other disk-writing activity is in progress.  Using RAII idiom is important for many of these.

This bug report set me off on a very large project to handle disk failures, identifying not only recording but other places where some possibility (though not as great a probability) of disk space exhaustion exists, and checking that incomplete side effects get properly rolled back between detection of errors and reports to the user.

I also considered where exceptions should be thrown for failures to read.  Perhaps to test these fixes you could do lengthy operations on projects stored on external devices, and disconnect cables before they finish.  For drawing or playback, just substitute silence, but abort other operations with a user visible message.

The project is complete here:

https://github.com/audacity/audacity/commit/d417acd5c66f642db6efb205c8c640cfb7b106f9

Belatedly, I call fix made.  I think I have done enough to address what's in the bug title and in the steps to reproduce.

That is, no more silent failures resulting in projects in a corrupt state.  (Whether from recording sound, or generating it, or applying an effect that increases the size needed for undo history beyond capacity.)

I count only 25 TRAP_ERR comments left.  I will not remove more of them now.

The ideas linked in comment 12, for estimating and anticipating disk exhaustions before they happen, is not in the scope of my work.  If that is a good idea, make it an enhancement request.

(In reply to Paul L from comment #15)
Testing on W10 audacity-win-rf1aa916-2.2.0-alpha-23-jul-17
and on macOS Sierra 10.12.5 00f8658 27Jul17

Testing with a USB clip drive with limited space
1) Recording
2) Effects like Amplift
3) Dulicates/align/Mix&Render
4) Export as WAV

all failed cleanly with the error message appearing, clearly identifying the problem drive.

I'm not sure why this was ever marked as a regression because AFAIK Audacity has never in previous releases trapped this error - and nor has it failed cleanly.

A big round of applause to Paul for finally fixing this   :-))


I don't think we need to raise a bug for tracking the earler trapping of such events - the proposal linked to in Comment #12 should suffice - see:
http://wiki.audacityteam.org/wiki/Proposal:_Earlier_Disk_Over-run_Warnings

I emphasize again that the hard part is making sure that after the message is raised in the middle of a failed operation, the project and the disk reverts completely to its status quo ante (except, in case of recording, keeping as much new recording as possible), and that "nothing funny happens" with continued use of Audacity thereafter.  

Making this guarantee was the hard programming work requiring review of hundreds of things in the code.

Proving the negative, "nothing funny," is impossible, but I take it you are satisfied there was no grossly obvious slapstick going on.

(In reply to Paul L from comment #17)
>Proving the negative, "nothing funny," is impossible, 
>but I take it you are satisfied there was no grossly 
>obvious slapstick going on

You take it correctly - the recordings failed with the partial projects left intact.

The only "problem child" was the Export (a WAV export) - and theat left an alleged WAV file behind - but it was not playable - easy for the user to clean up though.


So yes I am "satisfied" - and extremely pleased that once we get 2.2.0 out the door we can remove that "drawback" that has long ben a black mark against us on the Wikipedia Audacity page.

I also just tested Importing a file larger than could be accomadated on the USB clip drive - also failed cleanly.  No import made, no residue visible.

Please open bugs, P3 or P4, if you observe that export fails to remove incomplete results.

There are various exporting command (multiple by tracks or by labels, etc.) and some might have a bug and others not.

(In reply to Paul L from comment #20)
Actually I realized I was testing a fringe case where was almost no soace at all - when I freed up a little more space it produce a nice partila WAV file that was playable - a good clean, graceful, fail/error.

Testing for the main case as described looks good.
Fringe cases look at least "acceptable" to me.
Anything else could be logged as "enhancements".

Now tested on all platforms, so closing as fixed.