1890135 - when all the messages were deleted inside Inbox: Compact does not work for C-C TB POP3. Deleting all messages and compacting a folder does not shrink the mbox folder file.

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[ISHIKAWA, Chiaki]

I am testing locally built DEBUG version of C-C TB.
I was testing the C-C TB's behavior in the face of I/O errors.
Well, I wanted to check if the messages that are in the folders contain the
intended body. I compare the message body text against the original.
As I move the message by filters or moves, mbox folders tend to have old messages that were deleted and not shown.
My verification program reads all the messages, even deleted ones, in folders including Inbox, and
I do not want to check the deleted messages. I wanted to COUNT the number of messages, and check if they are the same as the locally sent messages. There are original messages with different sizes. They are checked gainst the messages in the folders. By making sure that there are the same numbers as the number of messages sent, I can confirm that C-C TB does not lose messages during I/O error handling (to some extent). My I/O error is an easy one.

Thus I wanted to run the verification program AFTER compaction removes all the deleted messages from the folders.

THEN I REALIZED THE MBOX FOLDERS DO NOT SHRINK AND HAVE ALL THE DELETED MESSAGES IN IT EVEN AFTER COMPACTION.
Inbox, for example.

I think this is a serious regression.

Repeat-by:
Download e-mail messages to Inbox.
Delete or move all the messages and making Inbox logically empty.
Now compact Inbox
C-C TB prints out some space saved, etc.

Expected: Inbox shrinks to the minimum size.

BUT it does not shrink the file at all.

There are warning messages on the tty window where C-C TB was invoked.
Basically, some file size number did not seem to be as expected and compaction does not take place.

[Parent 256558, Main Thread] WARNING: temp file not of expected size in compact: 'tempFileRightSize', file /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgFolderCompactor.cpp:547
[Parent 256558, Main Thread] WARNING: compact failed: 'msfRenameSucceeded', file /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgFolderCompactor.cpp:604

When "temp file not of expected size in compact, the compaction is not attempted.
https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgFolderCompactor.cpp#551

C-C TB should shrink the file size, AND
if it fails, I think it ought to report the failure via GUI.

I will investigate a bit more.

This bug is different from other compact-related bugzilla entries.
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=compact&list_id=16976712

Some of the latest ones are concerned with

• It used to be that C-C TB compacts all the folders, but now it does not. Which is awkward IMHO.
  Can we revert to the old behavior?
• There is a bugzilla mentioning IMAP. This is POP3.
• There is a bugzilla concerning Trash. I think ALL THE FOLDERS suffer from this bug.

I am not sure if the compact works if I delete a single message as opposed to all the messages.

EDIT: Fixed typoes.

Comment 1

[Ben Campbell]

Attachment: Bug 1890135 - Fix folder compaction to not fail on zero-size mbox files. r=#thunderbird-reviewers

Comment 2

[Ben Campbell]

Want to try this patch and see if it helps?
I think the problem is sanity check I put in to ensure that the mbox file was always larger than the messages it contained (because of the overhead of message separator lines ("From "-lines) and the extra characters added due to "From "-escaping in the message body ("From ..." => ">From ...").

BUT.
If you delete all the messages, the resultant mbox will be a zero-length file, which is the same as the size of the messages it contains, not larger.

Simple fix if this is the problem. It just reinforces the old truism that the three biggest problems in software development are naming things and off-by-one errors.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Ben Campbell from comment #2)

Want to try this patch and see if it helps?
I think the problem is sanity check I put in to ensure that the mbox file was always larger than the messages it contained (because of the overhead of message separator lines ("From "-lines) and the extra characters added due to "From "-escaping in the message body ("From ..." => ">From ...").

BUT.
If you delete all the messages, the resultant mbox will be a zero-length file, which is the same as the size of the messages it contains, not larger.

It seems the bug fix works.
Now the folder is truncated to size 0.
Also, it seems that the deletion of messages here and there (not the whole messages) works. It seems they are handled in a different path?
Anyway, it seems to work. Good.

Simple fix if this is the problem. It just reinforces the old truism that
the three biggest problems in software development are naming things and off-by-one errors.
Right on. I think I know the other one. :-)

Comment 4

[ISHIKAWA, Chiaki]

You might want to change the code to look like
((uint64_t) filesize > m_totalMsgSize) || (filesize == 0 && m_totalMsgSize ==0))
if the check is to be effective, though, IMHO.

Comment 5

[ISHIKAWA, Chiaki]

Hmm...
I find another case where C-C TB won't compact Inbox after a successful download of 600+ messages
and about 3/4 of them were moved to other folders.
I tried hitting compact afew times, but nothing happened.
And after I deleted the first message, it compacted the file.
It reported about 20KB saving (of the first message), but actually the file shrunk much more than that (about 450 messages had been deleted already). So there is something wrong in the counting and that it did not offer to compact the file until I deleted a single message. Hmm...
Maybe I have to file another bug on this.

Comment 6

[Ben Campbell]

(In reply to ISHIKAWA, Chiaki from comment #4)

You might want to change the code to look like
((uint64_t) filesize > m_totalMsgSize) || (filesize == 0 && m_totalMsgSize ==0))
if the check is to be effective, though, IMHO.

It's just a quick sanity check - I'll keep it simple for now. And I can think of other icky corner cases...
I'm just about to write up a bug for my complete compaction rewrite, so this old code will hopefully be blown away soon.

(In reply to ISHIKAWA, Chiaki from comment #5)

Hmm...
I find another case where C-C TB won't compact Inbox after a successful download of 600+ messages
and about 3/4 of them were moved to other folders.
I tried hitting compact afew times, but nothing happened.
And after I deleted the first message, it compacted the file.
It reported about 20KB saving (of the first message), but actually the file shrunk much more than that (about 450 messages had been deleted already). So there is something wrong in the counting and that it did not offer to compact the file until I deleted a single message. Hmm...
Maybe I have to file another bug on this.

Hmm... I'd say it's a separate problem. Sounds like it's not even getting as far as starting the compaction. My guess would be that the fault is in the message-moving code, forgetting to update the expunge count for the folder as messages are deleted from it.
The compaction doesn't run if the expunge count is zero.
By manually deleting a message the expunge count gets set to something non-zero, and so compaction starts working again.

Comment 7

[ISHIKAWA, Chiaki]

I think the second problem is reproducible.
As you say, expunge count is not properly set? Hard to believe, but it is true.
I am going to check if this happens without my buffered write patch applied locally and if so, I will file a bug.

There are so many tiny bugs that get in the way of testing my buffered write patch in the face of I/O errors.
Oh well.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c09532b365ba
Fix folder compaction to not fail on zero-size mbox files. r=mkmelin

Comment 9

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #7)

I think the second problem is reproducible.
As you say, expunge count is not properly set? Hard to believe, but it is true.
I am going to check if this happens without my buffered write patch applied locally and if so, I will file a bug.

There are so many tiny bugs that get in the way of testing my buffered write patch in the face of I/O errors.
Oh well.

I have downloaded a daily ASAN build for testing, and this happens with that build.
I have filed Bug 1890253.

Comment 10

[Wayne Mery (:wsmwk)]

guestimating this started with the compact rewrite