[bug] Files may get reordered by the rotation job

[gmasclet]

We're using rotating-file-stream to handle the rotation of our log files.

We've recently noticed that the filenames are sometimes out of order in the history file, meaning that the rotation may delete another file than the oldest one.

Unless I'm wrong, in the history() method, the files are sorted by their ctime. See the last line of the following extract:

rotating-file-stream/index.ts

Lines 500 to 518 in 73165e2

	for(const file of files) {
	if(file) {
	try {
	const stats = await this.fsStat(file);
	if(stats.isFile()) {
	res.push({
	name: file,
	size: stats.size,
	time: stats.ctime.getTime()
	});
	} else this.emit("warning", new Error(`File '${file}' contained in history is not a regular file`));
	} catch(e) {
	if(e.code !== "ENOENT") throw e;
	}
	}
	}
	res.sort((a, b) => a.time - b.time);

And from what I see in the Node documentation on https://nodejs.org/api/fs.html#class-fsstats:

ctime "Change Time": Time when file status was last changed (inode data modification). Changed by the chmod(2), chown(2), link(2), mknod(2), rename(2), unlink(2), utimes(2), read(2), and write(2) system calls.

So, it seems that in some cases, the ctime of the file may be externally modified, which causes the history file to be reordered during the next rotation job.

May I suggest we instead keep the history file order, disregarding the file ctime?

[iccicci]

Hi @gmasclet ,

Thank you for reporting.

In the page you are referring, the previous line to the one you highlighted:

mtime "Modified Time": Time when file data last modified. Changed by the mknod(2), utimes(2), and write(2) system calls.

It seems checking mtime rather than ctime would be the right way to achieve the target desired by the design.
I'm goign to apply this fix (which also is much more cheap than your proposal).

Do you think it could be enough to solve your problem?

Thank you

[gmasclet]

Hi @iccicci,

Thanks for your responsiveness 🙂

I think that checking mtime rather than ctime is a clear improvement: looks like there are too many things that can affect ctime. Yet, I fear that there can still be some edge cases not covered, for instance an admin opening an archived log file and saving it inadvertently. I know it seems a bit unlikely, but it's not impossible.

My proposal would simply be to remove the res.sort((a, b) => a.time - b.time); statement. This way, the history file is never reordered and behaves as a FIFO queue:

• The latest file is always added to the end of the history file.
• The removed file is always the first one, that is the oldest added in the history.
• External tampering with the files has no effect on this order.

Let me know what you think about that.

[iccicci]

That would break this.

When options.maxFiles or options.maxSize are enabled for first time, an history file can be created with one rotated filename (as returned by filename generator function) at each line.

That would make required also the history file to be created with correct sort, in case somebody wants to enable the feature after some log files are already created.

Since every system has its own requirements, probably your request could be accomplished with a new configuration option.

My plan:

• apply the ctime -> mtime fix soon;
• implement the new option to achive the logic you are proposing, but this may require some more days.

Does it work for you?
Thank you

[iccicci]

Released v3.2.5 with mtime fix.

[gmasclet]

Thanks a lot @iccicci!

The mtime fix clearly mitigates the issue, so we're going to bump to v3.2.5 to benefit from it.

---

When options.maxFiles or options.maxSize are enabled for first time, an history file can be created with one rotated filename (as returned by filename generator function) at each line.

I wasn't aware about that rule, this explains why the "sort by time" statement can not be simply removed.

I'm definitely interested if you can implement a new option so that the history file is never reordered once created.