Skip to content

Fix AutoLogger log deletion and cleanup sorting bugs #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nschimme wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix AutoLogger log deletion and cleanup sorting bugs #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
347ff66
Fix AutoLogger log deletion and cleanup sorting bugs
nschimme Jul 20, 2026
ab8fec3
Fix AutoLogger log deletion and cleanup sorting bugs
nschimme Jul 20, 2026
112fe92
Fix AutoLogger log deletion and cleanup sorting bugs
nschimme Jul 20, 2026
1a05a1d
Fix AutoLogger log deletion and cleanup sorting bugs
nschimme Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/global/utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,12 @@ static_assert(!utils::equals(0.0, 1.0));
static_assert(utils::equals(1.0, 1.0));
static_assert(utils::equals(0.0, 0.0));
static_assert(utils::equals(0.0, -0.0));

QDateTime utils::getFileTime(const QFileInfo &fileInfo)
{
const QDateTime birth = fileInfo.birthTime();
if (birth.isValid() && birth.toMSecsSinceEpoch() > EPOCH_CUTOFF_MS) {
return birth;
}
return fileInfo.lastModified();
}
6 changes: 6 additions & 0 deletions src/global/utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,17 @@
#include <tuple>
#include <type_traits>

#include <QDateTime>
#include <QFileInfo>
#include <QPointer>
#include <queue>

namespace utils {

inline constexpr qint64 EPOCH_CUTOFF_MS = 315532800000LL; // Milliseconds for Jan 1, 1980 00:00:00 UTC

NODISCARD QDateTime getFileTime(const QFileInfo &fileInfo);

// This mainly exists to avoid float-equal warnings,
// but it also checks that the floating point types are the same.
template<concepts::IsFloatingPoint A>
Expand Down
33 changes: 24 additions & 9 deletions src/logger/autologger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,15 @@
#include "../configuration/configuration.h"
#include "../global/TextUtils.h"
#include "../global/random.h"
#include "../global/utils.h"

#include <algorithm>
#include <sstream>
#include <tuple>

#include <QDateTime>
#include <QDebug>
#include <QDir>
#include <QFileInfo>
#include <QMessageBox>
#include <QStringList>
Expand Down Expand Up @@ -116,20 +119,32 @@ void AutoLogger::deleteOldLogs()
return;
}

// Sort files so we can delete the oldest
std::sort(fileInfoList.begin(), fileInfoList.end(), [](const auto &a, const auto &b) {
return a.birthTime() < b.birthTime();
struct FileWithTime
{
QFileInfo info;
QDateTime time;
};

QList<FileWithTime> filesWithTime;
filesWithTime.reserve(fileInfoList.size());
for (const auto &fileInfo : fileInfoList) {
filesWithTime.append({fileInfo, utils::getFileTime(fileInfo)});
}

// Sort files from newest to oldest based on cached time
std::sort(filesWithTime.begin(), filesWithTime.end(), [](const auto &a, const auto &b) {
return a.time > b.time;
});

qint64 totalFileSize = 0, deleteFileSize = 0;
QList<QFileInfo> filesToDelete;
QFileInfoList filesToDelete;
const QDate &today = QDate::currentDate();
for (const auto &fileInfo : fileInfoList) {
totalFileSize += fileInfo.size();
for (const auto &item : filesWithTime) {
totalFileSize += item.info.size();
bool deleteFile = false;
switch (conf.cleanupStrategy) {
case AutoLoggerEnum::DeleteDays:
if (fileInfo.birthTime().date().daysTo(today) >= conf.deleteWhenLogsReachDays) {
if (item.time.date().daysTo(today) >= conf.deleteWhenLogsReachDays) {
deleteFile = true;
}
break;
Expand All @@ -144,8 +159,8 @@ void AutoLogger::deleteOldLogs()
abort();
}
if (deleteFile) {
deleteFileSize += fileInfo.size();
filesToDelete.append(fileInfo);
deleteFileSize += item.info.size();
filesToDelete.append(item.info);
}
}

Expand Down
239 changes: 239 additions & 0 deletions tests/TestGlobal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <tuple>

#include <QDebug>
#include <QTemporaryFile>
#include <QtTest/QtTest>

TestGlobal::TestGlobal() = default;
Expand Down Expand Up @@ -648,4 +649,242 @@ void TestGlobal::weakHandleTest()
test::testWeakHandle();
}

struct MockFileInfo
{
QString name;
QDateTime birthTime;
QDateTime lastModified;
qint64 size;
};

static QDateTime getFileTimeMock(const MockFileInfo &fileInfo)
{
const QDateTime birth = fileInfo.birthTime;
if (birth.isValid() && birth.toMSecsSinceEpoch() > utils::EPOCH_CUTOFF_MS) {
return birth;
}
return fileInfo.lastModified;
}

void TestGlobal::autoLoggerLogicTest()

@sourcery-ai sourcery-ai Bot Jul 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider also covering the DeleteDays cleanup strategy with date-based mocks.

This test currently covers getFileTime and the DeleteSize regression. Since the PR description also calls out incorrect deletion under DeleteDays when birthTime is epoch 0 or invalid, please add a date-based scenario using getFileTimeMock (logs older/newer than deleteWhenLogsReachDays, including epoch-0 and invalid birthTime) to confirm that only truly old logs are removed.

Suggested implementation:

    MockFileInfo m3{ "f3.txt", QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC), QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC), 100 };
    QCOMPARE(getFileTimeMock(m3), QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC)); // Uses birthTime because it is valid and > 1980

    // 2. Verify DeleteDays cleanup logic using getFileTimeMock
    {
        // Simulated "now" and retention window
        const QDateTime now = QDateTime(QDate(2025, 1, 10), QTime(0, 0, 0), Qt::UTC);
        const int deleteWhenLogsReachDays = 3;
        const QDateTime cutoff = now.addDays(-deleteWhenLogsReachDays);

        // fOldBirthValid: valid birthTime, older than cutoff -> should be deleted
        MockFileInfo fOldBirthValid{
            "old_birth_valid.log",
            QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC),   // 9 days old
            QDateTime(QDate(2025, 1, 2), QTime(0, 0, 0), Qt::UTC),
            100
        };

        // fOldEpochBirth: epoch birthTime (<= 1980) but old lastModified -> should be deleted via lastModified fallback
        MockFileInfo fOldEpochBirth{
            "old_epoch_birth.log",
            QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),   // epoch 0 (invalid for age)
            QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC),   // 9 days old, older than cutoff
            100
        };

        // fRecentInvalidBirth: invalid birthTime, recent lastModified -> should NOT be deleted
        MockFileInfo fRecentInvalidBirth{
            "recent_invalid_birth.log",
            QDateTime(),                                            // invalid
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            100
        };

        // fRecentEpochBirth: epoch birthTime, recent lastModified -> should NOT be deleted
        MockFileInfo fRecentEpochBirth{
            "recent_epoch_birth.log",
            QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),   // epoch 0
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            100
        };

        // fRecentValidBirth: valid, recent birthTime -> should NOT be deleted
        MockFileInfo fRecentValidBirth{
            "recent_valid_birth.log",
            now.addDays(-1),                                        // 1 day old, newer than cutoff
            now.addDays(-1),
            100
        };

        const QList<MockFileInfo> files = {
            fOldBirthValid,
            fOldEpochBirth,
            fRecentInvalidBirth,
            fRecentEpochBirth,
            fRecentValidBirth
        };

        QStringList deletedFiles;
        for (const auto &file : files) {
            const QDateTime fileTime = getFileTimeMock(file);
            if (fileTime < cutoff) {
                deletedFiles << file.fileName;
            }
        }

        // Only files that are truly older than the cutoff (based on getFileTimeMock)
        // should be selected for deletion.
        const QStringList expectedDeletedFiles = {
            "old_birth_valid.log",
            "old_epoch_birth.log"
        };

        QCOMPARE(deletedFiles, expectedDeletedFiles);
    }

    // 3. Verify sorting and cumulative size deletion logic

If your DeleteDays cleanup logic is implemented in a helper (e.g. a function or method that already takes a list of files, deleteWhenLogsReachDays, and "now"), you can further improve this test by:

  1. Replacing the manual loop that builds deletedFiles with a direct call to that helper, using the same files, now, and deleteWhenLogsReachDays.
  2. Asserting on its output instead of the locally computed deletedFiles.

Adjust the container type (QList, QVector, etc.) and field name file.fileName if your actual MockFileInfo definition uses different identifiers.

{
// 1. Verify getFileTimeMock and boundary logic
// Case A: birthTime is epoch 0 -> falls back to lastModified
MockFileInfo m1{"f1.txt",
QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),
QDateTime(QDate(2023, 1, 1), QTime(0, 0, 0), Qt::UTC),
100};
QCOMPARE(getFileTimeMock(m1), QDateTime(QDate(2023, 1, 1), QTime(0, 0, 0), Qt::UTC));

// Case B: birthTime is invalid -> falls back to lastModified
MockFileInfo m2{"f2.txt",
QDateTime(),
QDateTime(QDate(2022, 1, 1), QTime(0, 0, 0), Qt::UTC),
100};
QCOMPARE(getFileTimeMock(m2), QDateTime(QDate(2022, 1, 1), QTime(0, 0, 0), Qt::UTC));

// Case C: birthTime is valid and after 1980 -> uses birthTime
MockFileInfo m3{"f3.txt",
QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC),
QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::UTC),
100};
QCOMPARE(getFileTimeMock(m3), QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::UTC));

// Case D (Comment 1 Boundary): birthTime exactly at the epoch cutoff (1980-01-01) -> falls back to lastModified
MockFileInfo mBoundary{"f_boundary.txt",
QDateTime(QDate(1980, 1, 1), QTime(0, 0, 0), Qt::UTC),
QDateTime(QDate(2021, 1, 1), QTime(0, 0, 0), Qt::UTC),
100};
QCOMPARE(getFileTimeMock(mBoundary), QDateTime(QDate(2021, 1, 1), QTime(0, 0, 0), Qt::UTC));

// Case E (Timezone robustness): verify comparison behaves identically across timezones using milliseconds
MockFileInfo mLocal{"f_local.txt",
QDateTime(QDate(2024, 1, 1), QTime(0, 0, 0), Qt::LocalTime),
QDateTime(QDate(2025, 1, 1), QTime(0, 0, 0), Qt::LocalTime),
100};
// Should still resolve to birthTime because 2024-01-01 is past 1980-01-01 in any local timezone
QCOMPARE(getFileTimeMock(mLocal), mLocal.birthTime);

// 2. Verify DeleteDays cleanup logic using getFileTimeMock (Comment 3 & Comment 1 suggestion)
{
// Simulated "now" and retention window
const QDateTime now = QDateTime(QDate(2025, 1, 10), QTime(0, 0, 0), Qt::UTC);
const int deleteWhenLogsReachDays = 3;
const QDateTime cutoff = now.addDays(-deleteWhenLogsReachDays);

// fOldBirthValid: valid birthTime, older than cutoff -> should be deleted
MockFileInfo fOldBirthValid{"old_birth_valid.log",
QDateTime(QDate(2025, 1, 1),
QTime(0, 0, 0),
Qt::UTC), // 9 days old
QDateTime(QDate(2025, 1, 2), QTime(0, 0, 0), Qt::UTC),
100};
Comment on lines +712 to +721

@sourcery-ai sourcery-ai Bot Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Add a DeleteDays boundary case where file age equals the threshold to verify the >= condition

Currently the test only exercises files clearly older (9 days) and newer (1 day) than the 3‑day threshold. Because production uses >= conf.deleteWhenLogsReachDays, please add a case where a file is exactly 3 days old and is deleted, and optionally a 2‑day‑old file that is retained, to validate the boundary behavior and guard against off‑by‑one errors.

Suggested implementation:

    {
        // Simulated "now" and retention window
        const QDateTime now = QDateTime(QDate(2025, 1, 10), QTime(0, 0, 0), Qt::UTC);
        const int deleteWhenLogsReachDays = 3;
        const QDateTime cutoff = now.addDays(-deleteWhenLogsReachDays);

        // fOldBirthValid: valid birthTime, older than cutoff -> should be deleted
        MockFileInfo fOldBirthValid{"old_birth_valid.log",
                                    QDateTime(QDate(2025, 1, 1),
                                              QTime(0, 0, 0),
                                              Qt::UTC), // 9 days old
                                    QDateTime(QDate(2025, 1, 2), QTime(0, 0, 0), Qt::UTC),
                                    100};

        // Boundary case: file exactly at deleteWhenLogsReachDays (3 days old) should be deleted
        MockFileInfo fBoundaryBirthValid{"boundary_birth_valid.log",
                                         QDateTime(QDate(2025, 1, 7),
                                                   QTime(0, 0, 0),
                                                   Qt::UTC), // exactly 3 days old
                                         QDateTime(QDate(2025, 1, 7), QTime(0, 0, 0), Qt::UTC),
                                         100};

        // Boundary case: file just below the threshold (2 days old) should be retained
        MockFileInfo fBelowThresholdBirthValid{"below_threshold_birth_valid.log",
                                               QDateTime(QDate(2025, 1, 8),
                                                         QTime(0, 0, 0),
                                                         Qt::UTC), // 2 days old
                                               QDateTime(QDate(2025, 1, 8), QTime(0, 0, 0), Qt::UTC),
                                               100};
  1. Ensure these new MockFileInfo instances are added to whatever collection or vector of files is passed to the deletion/cleanup logic in this test (e.g. append fBoundaryBirthValid and fBelowThresholdBirthValid alongside fOldBirthValid and any existing mocks).
  2. Extend the test assertions to explicitly verify that boundary_birth_valid.log is deleted (because its age is exactly deleteWhenLogsReachDays and should match the >= condition) and that below_threshold_birth_valid.log is retained (because its age is less than the threshold).
  3. If the test currently asserts on counts (e.g., number of remaining files) rather than specific filenames, update those expectations to reflect the additional boundary-case files.


// fOldEpochBirth: epoch birthTime (<= 1980) but old lastModified -> should be deleted via lastModified fallback
MockFileInfo fOldEpochBirth{"old_epoch_birth.log",
QDateTime(QDate(1970, 1, 1),
QTime(0, 0, 0),
Qt::UTC), // epoch 0 (invalid for age)
QDateTime(QDate(2025, 1, 1),
QTime(0, 0, 0),
Qt::UTC), // 9 days old, older than cutoff
100};

// Boundary case: file exactly at deleteWhenLogsReachDays (3 days old) should be deleted
MockFileInfo fBoundaryBirthValid{"boundary_birth_valid.log",
QDateTime(QDate(2025, 1, 7),
QTime(0, 0, 0),
Qt::UTC), // exactly 3 days old (10 - 7 = 3)
QDateTime(QDate(2025, 1, 7), QTime(0, 0, 0), Qt::UTC),
100};

// Boundary case: file just below the threshold (2 days old) should be retained
MockFileInfo fBelowThresholdBirthValid{"below_threshold_birth_valid.log",
QDateTime(QDate(2025, 1, 8),
QTime(0, 0, 0),
Qt::UTC), // 2 days old (10 - 8 = 2)
QDateTime(QDate(2025, 1, 8), QTime(0, 0, 0), Qt::UTC),
100};

// fRecentInvalidBirth: invalid birthTime, recent lastModified -> should NOT be deleted
MockFileInfo fRecentInvalidBirth{"recent_invalid_birth.log",
QDateTime(), // invalid
now.addDays(-1), // 1 day old, newer than cutoff
100};

// fRecentEpochBirth: epoch birthTime, recent lastModified -> should NOT be deleted
MockFileInfo fRecentEpochBirth{"recent_epoch_birth.log",
QDateTime(QDate(1970, 1, 1),
QTime(0, 0, 0),
Qt::UTC), // epoch 0
now.addDays(-1), // 1 day old, newer than cutoff
100};

// fRecentValidBirth: valid, recent birthTime -> should NOT be deleted
MockFileInfo fRecentValidBirth{"recent_valid_birth.log",
now.addDays(-1), // 1 day old, newer than cutoff
now.addDays(-1),
100};

const QList<MockFileInfo> files = {fOldBirthValid,
fOldEpochBirth,
fBoundaryBirthValid,
fBelowThresholdBirthValid,
fRecentInvalidBirth,
fRecentEpochBirth,
fRecentValidBirth};

QStringList deletedFiles;
for (const auto &file : files) {
const QDateTime fileTime = getFileTimeMock(file);
if (fileTime.date().daysTo(now.date()) >= deleteWhenLogsReachDays) {
deletedFiles << file.name;
}
}

const QStringList expectedDeletedFiles = {"old_birth_valid.log",
"old_epoch_birth.log",
"boundary_birth_valid.log"};

QCOMPARE(deletedFiles, expectedDeletedFiles);
}

// 3. Verify sorting and cumulative size deletion logic (Comment 2)
QList<MockFileInfo> fileInfoList = {
{"file1.txt",
QDateTime(QDate(1970, 1, 1), QTime(0, 0, 0), Qt::UTC),
QDateTime(QDate(2023, 5, 10), QTime(0, 0, 0), Qt::UTC),
60}, // oldest after fallback
{"file2.txt",
QDateTime(QDate(2024, 5, 10), QTime(0, 0, 0), Qt::UTC),
QDateTime(QDate(2024, 5, 10), QTime(0, 0, 0), Qt::UTC),
50}, // middle
{"file3.txt",
QDateTime(),
QDateTime(QDate(2025, 5, 10), QTime(0, 0, 0), Qt::UTC),
30} // newest after fallback
};

// Sort newest to oldest
std::sort(fileInfoList.begin(), fileInfoList.end(), [](const auto &a, const auto &b) {
return getFileTimeMock(a) > getFileTimeMock(b);
});

// Verify sorted order
QCOMPARE(fileInfoList[0].name, QString("file3.txt")); // 2025
QCOMPARE(fileInfoList[1].name, QString("file2.txt")); // 2024
QCOMPARE(fileInfoList[2].name, QString("file1.txt")); // 2023

// Helper to apply cumulative size-based deletion with a given threshold
auto collectFilesToDelete = [&](qint64 deleteWhenLogsReachBytes) {
qint64 totalFileSize = 0;
QList<MockFileInfo> toDelete;

for (const auto &fileInfo : fileInfoList) {
totalFileSize += fileInfo.size;
if (totalFileSize >= deleteWhenLogsReachBytes) {
toDelete.append(fileInfo);
}
}

return toDelete;
};

// Scenario 1: limit is 100 bytes, only the oldest file should be deleted
{
const qint64 deleteWhenLogsReachBytes = 100;
const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

// Expected:
// file3.txt kept (total 30, limit 100)
// file2.txt kept (total 80, limit 100)
// file1.txt deleted (total 140, limit 100)
QCOMPARE(filesToDelete.size(), 1);
QCOMPARE(filesToDelete[0].name, QString("file1.txt"));
}

// Scenario 2: cumulative size hits the limit exactly (verifies >= behavior)
{
const qint64 deleteWhenLogsReachBytes = 140; // total size of all three files
const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

// Expected:
// file3.txt kept (total 30, limit 140)
// file2.txt kept (total 80, limit 140)
// file1.txt deleted when total reaches exactly 140
QCOMPARE(filesToDelete.size(), 1);
QCOMPARE(filesToDelete[0].name, QString("file1.txt"));
}

// Scenario 3: low limit so that multiple files must be deleted
{
const qint64 deleteWhenLogsReachBytes = 60;
const QList<MockFileInfo> filesToDelete = collectFilesToDelete(deleteWhenLogsReachBytes);

// Totals:
// file3.txt: total 30 (< 60) -> kept
// file2.txt: total 80 (>= 60) -> deleted
// file1.txt: total 140 (>= 60) -> deleted
QCOMPARE(filesToDelete.size(), 2);
QCOMPARE(filesToDelete[0].name, QString("file2.txt"));
QCOMPARE(filesToDelete[1].name, QString("file1.txt"));
}

// 4. Verify actual production utils::getFileTime function on a real file (Overall Comment 2)
{
QTemporaryFile tempFile;
QVERIFY(tempFile.open());
tempFile.write("test log data");
tempFile.flush();

QFileInfo realFileInfo(tempFile.fileName());
QDateTime realFileTime = utils::getFileTime(realFileInfo);

// The real file was just created, so its file time must be valid and very recent (at least > 2020)
QVERIFY(realFileTime.isValid());
QVERIFY(realFileTime.toMSecsSinceEpoch()
> QDateTime(QDate(2020, 1, 1), QTime(0, 0, 0), Qt::UTC).toMSecsSinceEpoch());
}
}

QTEST_MAIN(TestGlobal)
1 change: 1 addition & 0 deletions tests/TestGlobal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ private Q_SLOTS:
static void toNumberTest();
static void unquoteTest();
static void weakHandleTest();
static void autoLoggerLogicTest();
};
Loading