Fix AutoLogger log deletion and cleanup sorting bugs - #243
Open
nschimme wants to merge 4 commits into
Open
Conversation
- Implement a robust `getFileTime()` helper that falls back to using `lastModified()` when `birthTime()` is invalid or returns epoch 0 (1970-01-01). This resolves log loss when running inside sandboxed environments like Flatpak, where creation times on FUSE mounts/document portals are not supported or are set to 0. - Reverse log cleanup sorting from oldest-first to newest-first. This corrects a bug under the `DeleteSize` strategy where cumulative log sizes starting from the oldest files were checked, which mistakenly kept the oldest logs and purged all the newest logs. - Add unit tests to `TestGlobal` verifying correct behavior of file time fallback logic and newest-first cumulative size cleanup.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideIntroduces a robust file timestamp helper and updates AutoLogger cleanup to use it and sort logs newest-to-oldest, plus unit tests that validate both age-based and size-based deletion behavior. Sequence diagram for updated AutoLogger log cleanup using utils::getFileTimesequenceDiagram
participant AutoLogger
participant utils
participant QFileInfo
AutoLogger->>AutoLogger: deleteOldLogs()
AutoLogger->>AutoLogger: collect QFileInfo list for log files
AutoLogger->>AutoLogger: sort files newest_to_oldest
loop sort comparator
AutoLogger->>utils: getFileTime(fileInfo)
alt birthTime_valid_and_recent
utils->>QFileInfo: birthTime()
utils-->>AutoLogger: QDateTime birth
else birthTime_invalid_or_too_old
utils->>QFileInfo: lastModified()
utils-->>AutoLogger: QDateTime lastModified
end
end
Note over AutoLogger: qint64 totalFileSize = 0
loop for each fileInfo (newest_to_oldest)
AutoLogger->>utils: getFileTime(fileInfo)
utils-->>AutoLogger: QDateTime fileTime
AutoLogger->>AutoLogger: [conf.cleanupStrategy == DeleteDays]
AutoLogger->>AutoLogger: check fileTime.date().daysTo(today)
AutoLogger->>AutoLogger: update totalFileSize and decide deletion
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In getFileTime(), epochCutoff is created as a UTC QDateTime while birthTime()/lastModified() are typically in LocalTime, so it would be safer to normalize the time specs (e.g., compare using birth.toUTC() and lastModified.toUTC() or construct epochCutoff with the same timeSpec) to avoid subtle timezone-based misclassifications.
- The autoLoggerLogicTest uses a MockFileInfo and getFileTimeMock rather than QFileInfo/getFileTime directly, which risks the test diverging from the real implementation over time; consider wiring the test to the actual getFileTime and QFileInfo (or a thin wrapper) so changes to the production helper are covered automatically.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In getFileTime(), epochCutoff is created as a UTC QDateTime while birthTime()/lastModified() are typically in LocalTime, so it would be safer to normalize the time specs (e.g., compare using birth.toUTC() and lastModified.toUTC() or construct epochCutoff with the same timeSpec) to avoid subtle timezone-based misclassifications.
- The autoLoggerLogicTest uses a MockFileInfo and getFileTimeMock rather than QFileInfo/getFileTime directly, which risks the test diverging from the real implementation over time; consider wiring the test to the actual getFileTime and QFileInfo (or a thin wrapper) so changes to the production helper are covered automatically.
## Individual Comments
### Comment 1
<location path="tests/TestGlobal.cpp" line_range="670-678" />
<code_context>
+
+void TestGlobal::autoLoggerLogicTest()
+{
+ // 1. Verify getFileTimeMock logic
+ 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)); // Falls back to lastModified because birth is epoch 0 (<= 1980)
+
+ 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)); // Falls back because birth is invalid
+
+ 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 sorting and cumulative size deletion logic
</code_context>
<issue_to_address>
**suggestion (testing):** Add a boundary test for birthTime exactly at the epoch cutoff (1980-01-01).
Since we only test invalid, epoch-0, and clearly post-1980 values, please add a case with `birthTime == 1980-01-01T00:00:00Z` to lock in the strict `>` behavior (i.e., that it still falls back to `lastModified`). This boundary is easy to regress if the comparison changes.
</issue_to_address>
### Comment 2
<location path="tests/TestGlobal.cpp" line_range="697-719" />
<code_context>
+ QCOMPARE(fileInfoList[1].name, QString("file2.txt")); // 2024
+ QCOMPARE(fileInfoList[2].name, QString("file1.txt")); // 2023
+
+ // Apply cumulative size check (limit is 100 bytes)
+ qint64 totalFileSize = 0;
+ QList<MockFileInfo> filesToDelete;
+ qint64 deleteWhenLogsReachBytes = 100;
+
+ for (const auto &fileInfo : fileInfoList) {
+ totalFileSize += fileInfo.size;
+ bool deleteFile = false;
+ if (totalFileSize >= deleteWhenLogsReachBytes) {
+ deleteFile = true;
+ }
+ if (deleteFile) {
+ filesToDelete.append(fileInfo);
+ }
+ }
+
+ // 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"));
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend size-based deletion tests to cover equality and multiple deletions scenarios.
To better cover `DeleteSize`, please also add a case where the cumulative size hits the limit exactly (verifying `>=` behavior) and another where it exceeds the limit enough that multiple files must be deleted. This will guard the threshold logic and ensure the loop deletes all required files.
```suggestion
// Helper to apply cumulative size-based deletion with a given threshold
auto collectFilesToDelete = [&](qint64 deleteWhenLogsReachBytes) {
qint64 totalFileSize = 0;
QList<MockFileInfo> filesToDelete;
for (const auto &fileInfo : fileInfoList) {
totalFileSize += fileInfo.size;
if (totalFileSize >= deleteWhenLogsReachBytes) {
filesToDelete.append(fileInfo);
}
}
return filesToDelete;
};
// 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"));
}
}
```
</issue_to_address>
### Comment 3
<location path="tests/TestGlobal.cpp" line_range="668" />
<code_context>
+ return fileInfo.lastModified;
+}
+
+void TestGlobal::autoLoggerLogicTest()
+{
+ // 1. Verify getFileTimeMock logic
</code_context>
<issue_to_address>
**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:
```cpp
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+670
to
+678
| // 1. Verify getFileTimeMock logic | ||
| 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)); // Falls back to lastModified because birth is epoch 0 (<= 1980) | ||
|
|
||
| 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)); // Falls back because birth is invalid | ||
|
|
||
| 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 |
There was a problem hiding this comment.
suggestion (testing): Add a boundary test for birthTime exactly at the epoch cutoff (1980-01-01).
Since we only test invalid, epoch-0, and clearly post-1980 values, please add a case with birthTime == 1980-01-01T00:00:00Z to lock in the strict > behavior (i.e., that it still falls back to lastModified). This boundary is easy to regress if the comparison changes.
Comment on lines
+697
to
+719
| // Apply cumulative size check (limit is 100 bytes) | ||
| qint64 totalFileSize = 0; | ||
| QList<MockFileInfo> filesToDelete; | ||
| qint64 deleteWhenLogsReachBytes = 100; | ||
|
|
||
| for (const auto &fileInfo : fileInfoList) { | ||
| totalFileSize += fileInfo.size; | ||
| bool deleteFile = false; | ||
| if (totalFileSize >= deleteWhenLogsReachBytes) { | ||
| deleteFile = true; | ||
| } | ||
| if (deleteFile) { | ||
| filesToDelete.append(fileInfo); | ||
| } | ||
| } | ||
|
|
||
| // 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")); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Extend size-based deletion tests to cover equality and multiple deletions scenarios.
To better cover DeleteSize, please also add a case where the cumulative size hits the limit exactly (verifying >= behavior) and another where it exceeds the limit enough that multiple files must be deleted. This will guard the threshold logic and ensure the loop deletes all required files.
Suggested change
| // Apply cumulative size check (limit is 100 bytes) | |
| qint64 totalFileSize = 0; | |
| QList<MockFileInfo> filesToDelete; | |
| qint64 deleteWhenLogsReachBytes = 100; | |
| for (const auto &fileInfo : fileInfoList) { | |
| totalFileSize += fileInfo.size; | |
| bool deleteFile = false; | |
| if (totalFileSize >= deleteWhenLogsReachBytes) { | |
| deleteFile = true; | |
| } | |
| if (deleteFile) { | |
| filesToDelete.append(fileInfo); | |
| } | |
| } | |
| // 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")); | |
| } | |
| // Helper to apply cumulative size-based deletion with a given threshold | |
| auto collectFilesToDelete = [&](qint64 deleteWhenLogsReachBytes) { | |
| qint64 totalFileSize = 0; | |
| QList<MockFileInfo> filesToDelete; | |
| for (const auto &fileInfo : fileInfoList) { | |
| totalFileSize += fileInfo.size; | |
| if (totalFileSize >= deleteWhenLogsReachBytes) { | |
| filesToDelete.append(fileInfo); | |
| } | |
| } | |
| return filesToDelete; | |
| }; | |
| // 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")); | |
| } | |
| } |
| return fileInfo.lastModified; | ||
| } | ||
|
|
||
| void TestGlobal::autoLoggerLogicTest() |
There was a problem hiding this comment.
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:
- Replacing the manual loop that builds
deletedFileswith a direct call to that helper, using the samefiles,now, anddeleteWhenLogsReachDays. - 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.
- Implement a robust `getFileTime()` helper that falls back to using `lastModified()` when `birthTime()` is invalid or returns epoch 0 (1970-01-01). This resolves log loss when running inside sandboxed environments like Flatpak, where creation times on FUSE mounts/document portals are not supported or are set to 0. - Reverse log cleanup sorting from oldest-first to newest-first. This corrects a bug under the `DeleteSize` strategy where cumulative log sizes starting from the oldest files were checked, which mistakenly kept the oldest logs and purged all the newest logs. - Add unit tests to `TestGlobal` verifying correct behavior of file time fallback logic and newest-first cumulative size cleanup. Format all changed files with clang-format.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #243 +/- ##
==========================================
+ Coverage 25.08% 25.23% +0.14%
==========================================
Files 528 528
Lines 44211 44305 +94
Branches 4793 4800 +7
==========================================
+ Hits 11092 11180 +88
- Misses 33119 33125 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Implement a robust `utils::getFileTime()` helper in `src/global/utils.cpp` that falls back to using `lastModified()` when `birthTime()` is invalid or returns epoch 0 (1970-01-01), with timezone-independent UTC millisecond comparisons. This resolves log loss when running inside sandboxed environments like Flatpak. - Reverse log cleanup sorting from oldest-first to newest-first. This corrects a bug under the `DeleteSize` strategy where cumulative log sizes starting from the oldest files were checked, which mistakenly kept the oldest logs and purged all the newest logs. - Add comprehensive, boundary-locked unit tests in `TestGlobal` covering the timezone check, boundary tests for the 1980-01-01 epoch cutoff, extensive size-based deletion scenarios, and date-based `DeleteDays` mocks. Run all tests successfully. - Format all files with clang-format.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The 1980 epoch cutoff logic is duplicated between
utils::getFileTimeandgetFileTimeMock; consider exposing the cutoff as a shared constant or helper to keep tests aligned with production behavior as this logic evolves. - In
deleteOldLogs,utils::getFileTimeis called multiple times per file (in the sort comparator and again in the DeleteDays check); consider caching the computed timestamp perQFileInfoto avoid redundant work and keep the cleanup logic simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The 1980 epoch cutoff logic is duplicated between `utils::getFileTime` and `getFileTimeMock`; consider exposing the cutoff as a shared constant or helper to keep tests aligned with production behavior as this logic evolves.
- In `deleteOldLogs`, `utils::getFileTime` is called multiple times per file (in the sort comparator and again in the DeleteDays check); consider caching the computed timestamp per `QFileInfo` to avoid redundant work and keep the cleanup logic simpler.
## Individual Comments
### Comment 1
<location path="tests/TestGlobal.cpp" line_range="715-724" />
<code_context>
+ const int deleteWhenLogsReachDays = 3;
</code_context>
<issue_to_address>
**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:
```cpp
{
// 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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+715
to
+724
| 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}; |
There was a problem hiding this comment.
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};
- 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
fBoundaryBirthValidandfBelowThresholdBirthValidalongsidefOldBirthValidand any existing mocks). - Extend the test assertions to explicitly verify that
boundary_birth_valid.logis deleted (because its age is exactlydeleteWhenLogsReachDaysand should match the>=condition) and thatbelow_threshold_birth_valid.logis retained (because its age is less than the threshold). - 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.
- Implement a robust `utils::getFileTime()` helper in `src/global/utils.cpp` that falls back to using `lastModified()` when `birthTime()` is invalid or returns epoch 0 (1970-01-01), with timezone-independent UTC millisecond comparisons and a shared constant `EPOCH_CUTOFF_MS`. This resolves log loss when running inside sandboxed environments like Flatpak. - Optimize and cache resolved file times in `deleteOldLogs` using a `FileWithTime` structure to avoid redundant work and file system queries during log cleanup. - Reverse log cleanup sorting from oldest-first to newest-first. This corrects a bug under the `DeleteSize` strategy where cumulative log sizes starting from the oldest files were checked, which mistakenly kept the oldest logs and purged all the newest logs. - Add comprehensive, boundary-locked unit tests in `TestGlobal` covering timezone checks, boundary tests for the 1980-01-01 epoch cutoff, extensive size-based deletion scenarios, exactly-at-boundary `DeleteDays` deletion (3-days-old), and 2-day-old retention. Run all tests successfully. - Format all files with clang-format.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves a critical log loss issue when running MMapper in sandboxed environments (like Flatpak) or unsupported filesystems where
QFileInfo::birthTime()returns epoch 0. Under the defaultDeleteDaysstrategy, this caused all logs to be mistakenly treated as decades-old and deleted on connection. We introduce a robustgetFileTime()helper with a fallback tolastModified(). We also reverse log sorting to newest-to-oldest, correcting a bug in theDeleteSizestrategy where newest logs were pruned instead of oldest ones. Finally, we add comprehensive unit tests to cover both bugs.PR created automatically by Jules for task 13694242729865111320 started by @nschimme
Summary by Sourcery
Fix AutoLogger log retention to use a robust file timestamp helper and correct log ordering for cleanup strategies.
Enhancements:
Tests: