1798181 - when compact runs out of disk, partial nstmp is left behind, disk remaining full

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Karl]

Steps to reproduce:

using Thunderbird 102.2.2 (64-bit) on Ubuntu Linux 20.04 (recently updated from previous ESR version)
my main mailbox is accessed via IMAP. I do not use a "deleted" folder but delete marks as deleted, and "Compact" manually occasionally.
I did some cleanup in INBOX (delete messages, move messages to other folders, strip big attachments from messages, resulting in a deleted big version and a remaining stripped version)
THen I chose "Compact" for the INBOX folder. I knew I may have enough free disk space or not.

Actual results:

I watched with df the amount of free space going down - to zero. Okay so I didn't have enough free space, the popup "Compact failed please check you have enough free space, write permission etc.".
But the free space remained at zero.
I made free space and repeated the Compact, successfully. Now I gained the disk space resulting from the successful Compact, but am still missing the disk space gobbled up by the unsuccessful attempt.
Examining the disk directory /PATH/Thunderbird-Profiles/STRING.default/ImapMail/MAILBOXNAME, I see that a nstmp file is sitting there with timestamp of the unsuccessful attempt.

Expected results:

It is very sensible to build the compacted folder next to the original file so that the file isn't corrupted if something happens during this operation.

Apparently the second Compact used a different name because the nstmp file was already there. It is also sensible that Compact does not overwrite existing files, although in my case it would have been preferable to re-use the existing temp file, I admit that in other circumstances this file may contain valuable recovery/forensic data.

However, I would expect that the first Compact deletes the temporary file it created when determining that the operation was unsuccessful, otherwise this is a drain of disk space which is not easily fixed by someone not looking under the hood of the Thunderbird Profile file structure, and this cannot be expected from an average user. If it doesn't want to delete it, or doesn't succeed in doing so, as this may be possible (like in my case, disk full) or not in some other case (e.g. connection to the filesystem was lost), it should at least include in the error message "a temporary file named <PROFILE PATH>/ImapMail/nstmp may remain and can be safely deleted" to enable the user to do the cleanup manually.

I am convinced in previous Thunderbird versions disk space was freed after an unsuccessful Compact, as this is the first time I had to investigate this, and I've had tight disk space before, so this looks like a regression compared to the previous ESR to me.

Comment 1

[Magnus Melin [:mkmelin]]

I guess nsMsgFolderCompactor.cpp should clean up temp files on failures?

Comment 2

[Ben Campbell]

Attachment: Bug 1798181 - Make sure listener is invoked upon write failure for IMAP/News folder compaction. r=mkmelin

Comment 3

[Ben Campbell]

OK... I've done a little (manual) testing by returning error codes from file writes.
The local folder version seems to cope fine and clean up gracefully.

But there's separate compaction code for Offline messages in IMAP/news folders. Of course there is! even though they are doing essentially exactly the same thing as for local folders :-)

And the offline compactor just hangs upon error.
The cleanup is performed in the compactor destructor function. But I made some changes a while back so the compaction listener makes sure to hold a reference to the compactor until it's definitely finished. But, the offline compactor wasn't telling the listener that the compaction failed, and the listener locked the compactor in existance and... deadlock.
So. This patch just makes sure to let the listener know that the compaction failed, then the compactor is released, it cleans up and it all seems to work fine.
Sometimes smart pointers are just way more trouble than they're worth (c.f. nsIMsgDatabase.ForceDBClosed() ecetera...).

Would be nice to have some unit tests to force write errors in this kind of stuff, but not too sure how to approach it. Maybe there's something in the M-C side we could use as an example? Something for a separate bug maybe.

Comment 4

[Wayne Mery (:wsmwk)]

Benc, There is also a small matter of ... Compact should not run out of space - bug 854791.

Karl, was there any other application consuming disk during the compact?

Comment 5

[Wayne Mery (:wsmwk)]

Such a rich history of compact issues. Related to tmp :

• bug 674742 comment 70 and the comments that follow - where removal of the temp file is discussed
• bug 389132
• bug 784872
• bug 784888

Comment 6

[Karl]

during the failed compact operation, no other activity has influenced the free disk space on the relevant volume that I am aware of.
I have a .thunderbird/profiles.ini that redirects the profile to a directory within /M (because mail is the sort of thing I do not want to sit on my drive unencrypted):

/dev/mapper/ubuntu--vg-root 467109504 337042372 110027824  76% /
/dev/mapper/veracrypt1       10485500   9333512   1151988  90% /M

other than mail, I have mainly small documents on that volume and little fluctuation.
Now, the interesting thing is that nstmp is 766.6MiB in size, but INBOX is 616.4MiB.
Unfoirtunately, I do not recall whether and how much I might have further cleaned upo b efore I re-attempted the Compact, so this isn't an indication that Compact didn't tighten as much as it could. Sorry for this imcompleteness of information.
To me it seems more likely that the free space check either works on the wrong volume, or doesn't succeed on Linux, or doesn't work for offline Imap copies in the same way as for local Mbox files (assuming the code introduced in the bug you referred is still in place).

Comment 7

[Ben Campbell]

(In reply to Wayne Mery (:wsmwk) from comment #4)

Benc, There is also a small matter of ... Compact should not run out of space - bug 854791.

Hmm. It does do a check which looks more-or-less sane...
https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgFolderCompactor.cpp#307

current size - expected savings + expectedDBsize
(where expectedDBsize allows for 1KB per message - from https://bugzilla.mozilla.org/show_bug.cgi?id=854791#c8)

(In reply to Karl from comment #6)

To me it seems more likely that the free space check either works on the wrong volume, or doesn't succeed on Linux, or doesn't work for offline Imap copies in the same way as for local Mbox files (assuming the code introduced in the bug you referred is still in place).

The local and IMAP compaction code share the pre-compaction free-space check.
I think checking the wrong volume seems like the most likely culprit... if it's going through a symlink, I could easily see it getting mixed up. I'll investigate.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfdb6cf02ca9
Make sure listener is invoked upon write failure for IMAP/News folder compaction. r=mkmelin

Comment 9

[Ben Campbell]

No dice. The nsIFile GetDiskSpaceAvailable() call seems to do the Right Thing(tm), both for local and imap folders.

I just did some testing with symlinking various folders to a smaller volume (and even symlinking just the mbox file), but in all cases it reported the free space I expected - if there's a symlink which leads to another volume, it correctly reports the free space on that destination volume, as I'd expect.
(this is on Linux).

I think that probably just about wraps it up for this bug. If more information emerges on how to confuse the compaction, I think it justifies another bug.

Comment 10

[Rob Lemley [:rjl]]

Comment on attachment 9301470 [details]
Bug 1798181 - Make sure listener is invoked upon write failure for IMAP/News folder compaction. r=mkmelin

[Triage Comment]
Prerequisite for uplifting bug 1797616 to c-esr102 to avoid a merge conflict. Changes from this bug are overwritten by bug 1797616.

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 102.6.1:
https://hg.mozilla.org/releases/comm-esr102/rev/17e70d5466d7