Change log rotation to a quota based system

[rablador]

Currently we only keep one .old.log, meaning we have the current log and the previous log. The logs are rotated on every app/process start, which is often on iOS. It would really help debugging thing if we had logs a bit further back in time.

We should therefore improve the current state by keeping as many logs as our filesystem budget allows for, eg.:

• never have more than 5MB of log files of any one type (application or packet tunnel)
• never have any single log file be larger than 500KB

Instead of suffixing log files with an .old.log when they're rotated, all log files should be named $loggingTarget.$currentDate.log, eg. PacketTunnel.2024.03.03-21:32:01.log.

---

This change is 

[linear]

IOS-522 Keep more logs

[acb-mv]

Reviewed 4 of 8 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[buggmagnet]

Reviewed 3 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @rablador)

---

ios/MullvadLogging/LogRotation.swift line 20 at r2 (raw file):

    public struct Options {
        let storageSizeLimit: Int?

nit
I feel that these parameters are optional just because it made it easier to write tests.
I'm not sure how I feel about this. But IMHO they shouldn't be optional.

---

ios/MullvadLogging/LogRotation.swift line 89 at r2 (raw file):

    }

    private static func rotateLogsByDate(logs: [LogData], oldestAllowedDate: Date) throws {

nit
What do you think of the following name ?
private static func deleteLogsOlderThan(_ dateThreshold: Date, in logs[LogData]) throws

---

ios/MullvadLogging/LogRotation.swift line 92 at r2 (raw file):

        let fileManager = FileManager.default

        for log in logs where log.creationDate < oldestAllowedDate {

Cool usage of a where clause in a for loop !

---

ios/MullvadLogging/LogRotation.swift line 100 at r2 (raw file):

        let fileManager = FileManager.default

        // From newest to oldest, delete all logs outside maximum capacity.

This comment is outdated, it should only refer to log size now.

---

ios/MullvadLogging/LogRotation.swift line 102 at r2 (raw file):

        // From newest to oldest, delete all logs outside maximum capacity.
        var fileSizes = UInt64.zero
        for log in logs {

This doesn't look to be doing the right thing

The following test will fail

    func testRotateSize() throws {
        let logPaths = [
            directoryPath.appendingPathComponent("test1.log"),
            directoryPath.appendingPathComponent("test2.log"),
        ]

        try writeDataToDisk(path: logPaths[0], byteSize: 3)
        try writeDataToDisk(path: logPaths[1], byteSize: 10_000)

        try LogRotation.rotateLogs(logDirectory: directoryPath, options: LogRotation.Options(storageSizeLimit: 3999))
        let logFileCount = try fileManager.contentsOfDirectory(atPath: directoryPath.relativePath).count
        XCTAssertEqual(logFileCount, 1)
    }

---

ios/MullvadVPNTests/LogRotationTests.swift line 16 at r2 (raw file):
From the docs:

Override setUp() async throws for asynchronous state preparation with error handling before each test. Do not override setUp(completion:) for synchronous state preparation.

The doc is not exactly great, but the take away is that if we're doing synchronous preparation, we should use setUpWithError instead.

---

ios/MullvadVPNTests/LogRotationTests.swift line 89 at r2 (raw file):

extension LogRotationTests {
    private func writeDataToDisk(path: URL, byteSize: Int) throws {
        let data = Data((0 ..< 1000).map { UInt8($0 & 0xff) })

Did you mean to use the byteSize parameter ?
Also we should probably rename it to fileSize since the size of a byte is always 8 bits.

[rablador]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadLogging/LogRotation.swift line 20 at r2 (raw file):

Previously, buggmagnet wrote…

nit
I feel that these parameters are optional just because it made it easier to write tests.
I'm not sure how I feel about this. But IMHO they shouldn't be optional.

The alternative would be to add fake and unrelated size constraints when testing dates and vice versa.

---

ios/MullvadLogging/LogRotation.swift line 89 at r2 (raw file):

Previously, buggmagnet wrote…

nit
What do you think of the following name ?
private static func deleteLogsOlderThan(_ dateThreshold: Date, in logs[LogData]) throws

Sounds good!

---

ios/MullvadLogging/LogRotation.swift line 100 at r2 (raw file):

Previously, buggmagnet wrote…

This comment is outdated, it should only refer to log size now.

We still want to delete them from newest to oldest so that we don't shave off new newest logs rather than the oldest.

---

ios/MullvadLogging/LogRotation.swift line 102 at r2 (raw file):

Previously, buggmagnet wrote…

This doesn't look to be doing the right thing

The following test will fail

    func testRotateSize() throws {
        let logPaths = [
            directoryPath.appendingPathComponent("test1.log"),
            directoryPath.appendingPathComponent("test2.log"),
        ]

        try writeDataToDisk(path: logPaths[0], byteSize: 3)
        try writeDataToDisk(path: logPaths[1], byteSize: 10_000)

        try LogRotation.rotateLogs(logDirectory: directoryPath, options: LogRotation.Options(storageSizeLimit: 3999))
        let logFileCount = try fileManager.contentsOfDirectory(atPath: directoryPath.relativePath).count
        XCTAssertEqual(logFileCount, 1)
    }

It depends on if log age is relevant. First log is older and should probably be deleted first, then on to the next and so on.

---

ios/MullvadVPNTests/LogRotationTests.swift line 16 at r2 (raw file):

Previously, buggmagnet wrote…

From the docs:

Override setUp() async throws for asynchronous state preparation with error handling before each test. Do not override setUp(completion:) for synchronous state preparation.

The doc is not exactly great, but the take away is that if we're doing synchronous preparation, we should use setUpWithError instead.

Done.

---

ios/MullvadVPNTests/LogRotationTests.swift line 89 at r2 (raw file):

Previously, buggmagnet wrote…

Did you mean to use the byteSize parameter ?
Also we should probably rename it to fileSize since the size of a byte is always 8 bits.

I did...
Size in bytes is what I wanted to convey. Filesize doesn't say anything about the size unit.

[buggmagnet]

Reviewed 4 of 8 files at r1, 1 of 7 files at r2, 3 of 4 files at r3.
Reviewable status: 10 of 11 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadLogging/LogRotation.swift line 20 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

The alternative would be to add fake and unrelated size constraints when testing dates and vice versa.

I think that's an okay tradeoff. We don't want to have a situation where we mistakenly use an API because it made testing easier.
Both the storage size limit, and the oldest allowed date are a mandatory part of it.

Additionally, we should also have the maximum file size for a single item here, as per the specs.

---

ios/MullvadLogging/LogRotation.swift line 100 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

We still want to delete them from newest to oldest so that we don't shave off new newest logs rather than the oldest.

Yes we do, but we delete them by age in a different function

---

ios/MullvadLogging/LogRotation.swift line 102 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

It depends on if log age is relevant. First log is older and should probably be deleted first, then on to the next and so on.

This is not about the age of the log, it's about the implementation being incorrect.
In the test, only the file that is 10_000 bytes should be deleted because we want to filter file sizes over 3999 bytes.
However, both files get deleted.

We want a limit on both

• The size of a single log file
• The total size of all files combined.

i.e. let's say the total limit of logs is 500KB, if we have 300 files at 1KB, that is valid.
If we have 1 file at 200KB and a file at 400KB, the 400KB file should get deleted in the next rotation.

---

ios/MullvadVPNTests/LogRotationTests.swift line 89 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I did...
Size in bytes is what I wanted to convey. Filesize doesn't say anything about the size unit.

Fair enough. Although typically file sizes are always expressed in bytes, it wouldn't make too much sense to express them in another base unit.

[rablador]

Reviewable status: 9 of 11 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

ios/MullvadLogging/LogRotation.swift line 20 at r2 (raw file):

Previously, buggmagnet wrote…

I think that's an okay tradeoff. We don't want to have a situation where we mistakenly use an API because it made testing easier.
Both the storage size limit, and the oldest allowed date are a mandatory part of it.

Additionally, we should also have the maximum file size for a single item here, as per the specs.

Fair enough.
As discussed offline, individual file size limit will be added in another PR.

---

ios/MullvadLogging/LogRotation.swift line 100 at r2 (raw file):

Previously, buggmagnet wrote…

Yes we do, but we delete them by age in a different function

Discussed and resolved offline.

---

ios/MullvadLogging/LogRotation.swift line 102 at r2 (raw file):

Previously, buggmagnet wrote…

This is not about the age of the log, it's about the implementation being incorrect.
In the test, only the file that is 10_000 bytes should be deleted because we want to filter file sizes over 3999 bytes.
However, both files get deleted.

We want a limit on both

• The size of a single log file
• The total size of all files combined.

i.e. let's say the total limit of logs is 500KB, if we have 300 files at 1KB, that is valid.
If we have 1 file at 200KB and a file at 400KB, the 400KB file should get deleted in the next rotation.

Discussed and resolved offline.

---

ios/MullvadVPNTests/LogRotationTests.swift line 89 at r2 (raw file):

Previously, buggmagnet wrote…

Fair enough. Although typically file sizes are always expressed in bytes, it wouldn't make too much sense to express them in another base unit.

You might be right. Let's go with simply fileSize.

[buggmagnet]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.