Feature/logfile size control#5
Feature/logfile size control#5saksham-chawla wants to merge 2 commits intocodex/replay-pr-44-basefrom
Conversation
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested changes address edge-case handling, filename compatibility, and backend coupling concerns.
Status: Changes Suggested | Risk: Medium | Readiness: 6/10 (Needs Polish)
Issues Identified & Suggestions
- Guard zero log-file count to avoid backend rotation errors:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp - Preserve unsuffixed primary log filename or gate new suffix scheme:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp - Pass backend settings object to reduce positional-arg coupling:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
Review Details
📁 1 files reviewed | 💬 3 comments
👍 / 👎 individual comments to help improve reviews for you
| config.IsCircularFileLogging(), | ||
| config.IsOverwriteLogOnFull(), | ||
| config.GetMaxLogFileSizeBytes(), | ||
| config.GetNoOfLogFiles(), |
There was a problem hiding this comment.
[Logic] When config.GetNoOfLogFiles() returns 0, the condition > 1 is false so no suffix is appended (behaves like 1 file), but the value 0 is still forwarded to FileOutputBackend at line 94. Depending on how the backend uses this value (e.g., modulo for rotation index, loop bounds), this could cause division-by-zero or no-rotation bugs.
Add a guard or clamp:
const auto no_of_log_files = std::max(config.GetNoOfLogFiles(), static_cast<decltype(config.GetNoOfLogFiles())>(1));Then use no_of_log_files in both the if check and the FileOutputBackend constructor.
Context for Agents
When `config.GetNoOfLogFiles()` returns `0`, the condition `> 1` is false so no suffix is appended (behaves like 1 file), but the value `0` is still forwarded to `FileOutputBackend` at line 94. Depending on how the backend uses this value (e.g., modulo for rotation index, loop bounds), this could cause division-by-zero or no-rotation bugs.
Add a guard or clamp:
```cpp
const auto no_of_log_files = std::max(config.GetNoOfLogFiles(), static_cast<decltype(config.GetNoOfLogFiles())>(1));
```
Then use `no_of_log_files` in both the `if` check and the `FileOutputBackend` constructor.
File: score/mw/log/detail/file_recorder/file_recorder_factory.cpp
Line: 94Chain of Thought
- Validation Decision: accepted
- Confidence: 0.65
- Likelihood Score: 3/5
- Likelihood Reason: The value 0 for "number of log files" is a plausible edge case from misconfiguration. Whether it causes an actual bug (division-by-zero, empty loop) depends on the FileOutputBackend implementation, which we cannot inspect. The inconsistency between the
> 1guard and passing raw 0 to the backend is a real code smell that could surface as a bug. - Suggested Action: keep
- Tool Calls Used: 24
Reasoning
Analysis:
-
New code introduced by this PR: The base version didn't pass
GetNoOfLogFiles()toFileOutputBackendat all — the entire rotation/log-file-count feature is new. So this issue is squarely introduced by this PR. -
The logical inconsistency is real: The code checks
config.GetNoOfLogFiles() > 1to decide whether to suffix the filename (line 45), treating values of 0 and 1 identically for naming purposes. However, it then passes the raw value (potentially 0) toFileOutputBackendat line 94. This creates an asymmetry: the filename logic treats 0 as "single file mode" but the backend receives the literal 0. -
Cannot verify downstream impact: The
FileOutputBackendclass (referenced inscore/mw/log/detail/text_recorder/file_output_backend.h) is not available in the changed files, and the file couldn't be located in the repo (likely in a different repo or not cached). We cannot confirm whether the backend performs modulo operations, loop iterations, or other computations where 0 would be problematic. -
The comment is actionable: It provides a concrete fix — use
std::maxto clamp the value to at least 1, and use the clamped value in both places. This is a defensive programming improvement that's straightforward to implement. -
The speculative caveat: The comment explicitly says "Depending on how the backend uses this value" which acknowledges uncertainty. However, clamping the value to ≥1 is a reasonable defensive measure since "0 log files" is semantically meaningless and the clamping would ensure consistency with the filename logic's implicit treatment of 0 as 1.
Overall: The comment identifies a genuine inconsistency in newly-introduced code, the concern about edge cases with 0 is reasonable (though unverified), and the fix is concrete and actionable.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Lookup
- Filename:
file_output_backend.cpp
No files found matching 'file_output_backend.cpp'
3. File Lookup
- Filename:
file_output_backend.h
No files found matching 'file_output_backend.h'
4. File Lookup
- Filename:
configuration.h - Max Results:
10
No files found matching 'configuration.h'
5. File Lookup
- Filename:
file_output_backend.h - Max Results:
10
No files found matching 'file_output_backend.h'
6. File Lookup
- Filename:
FileOutputBackend - Max Results:
10
No files found matching 'FileOutputBackend'
7. File Lookup
- Filename:
file_output_backend - Max Results:
10
No files found matching 'file_output_backend'
8. File Lookup
- Filename:
Configuration - Max Results:
10
No files found matching 'Configuration'
9. File Lookup
- Filename:
configuration.h - Max Results:
20
No files found matching 'configuration.h'
10. File Snapshot (HEAD)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
11. File Snapshot (BASE)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| const std::string app_id{std::string{config.GetAppId().data(), config.GetAppId().size()}}; | ||
| const std::string file_extension{".dlt"}; | ||
| std::string file_name{file_path_base + "/" + app_id + file_extension}; | ||
| if (config.GetNoOfLogFiles() > 1) { |
There was a problem hiding this comment.
[Maintainability] When more than one log file is configured the factory now renames the primary log from <path>/<app>.dlt to <path>/<app>_1.dlt. That file name is an externally observable contract (collectors, uploaders, cleanup scripts expect the unsuffixed name) and this change introduces a silent backward-incompatible storage path without any migration or feature flag. Deployments that already set NoOfLogFiles() > 1 will suddenly stop writing to the previously documented location, and rolling back will leave stale _1 files behind. Please preserve the existing base name for the active file and let rotation logic manage suffixed files, or gate the new naming scheme behind an explicit opt-in so external tooling has a migration path.
Context for Agents
When more than one log file is configured the factory now renames the primary log from `<path>/<app>.dlt` to `<path>/<app>_1.dlt`. That file name is an externally observable contract (collectors, uploaders, cleanup scripts expect the unsuffixed name) and this change introduces a silent backward-incompatible storage path without any migration or feature flag. Deployments that already set `NoOfLogFiles() > 1` will suddenly stop writing to the previously documented location, and rolling back will leave stale `_1` files behind. Please preserve the existing base name for the active file and let rotation logic manage suffixed files, or gate the new naming scheme behind an explicit opt-in so external tooling has a migration path.
File: score/mw/log/detail/file_recorder/file_recorder_factory.cpp
Line: 45Chain of Thought
- Validation Decision: accepted
- Confidence: 0.00
- Likelihood Score: 0/5
- Suggested Action: keep
- Tool Calls Used: 17
Reasoning
Failed to parse validation response
Evidence Summary
1. File Snapshot (BASE)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Lookup
- Filename:
configuration.h - Max Results:
10
No files found matching 'configuration.h'
4. File Lookup
- Filename:
file_output_backend.h - Max Results:
10
No files found matching 'file_output_backend.h'
5. File Lookup
- Filename:
file_recorder_factory.h - Max Results:
5
Found 1 file(s) matching 'file_recorder_factory.h':
- score/mw/log/detail/file_recorder/file_recorder_factory.h
6. File Lookup
- Filename:
file_output_backend.cpp - Max Results:
5
No files found matching 'file_output_backend.cpp'
7. File Snapshot (HEAD)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
8. File Snapshot (BASE)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.h
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
9. File Lookup
- Filename:
Configuration.h - Max Results:
10
No files found matching 'Configuration.h'
10. File Lookup
- Filename:
configuration.cpp - Max Results:
10
No files found matching 'configuration.cpp'
11. File Lookup
- Filename:
log_mode.h - Max Results:
10
No files found matching 'log_mode.h'
12. File Lookup
- Filename:
configuration.hpp - Max Results:
10
No files found matching 'configuration.hpp'
13. File Lookup
- Filename:
configuration.h - Max Results:
10
No files found matching 'configuration.h'
|
|
||
| return std::make_unique<FileOutputBackend>(std::move(message_builder), | ||
| descriptor, | ||
| file_name, |
There was a problem hiding this comment.
[Maintainability] The factory now forwards six backend-specific knobs (IsCircularFileLogging, IsOverwriteLogOnFull, max size, number of files, truncate policy, plus the file name) as a long positional parameter list. This tightly couples CreateFileLoggingBackend to FileOutputBackend internals and guarantees that every future backend policy change will require touching this factory (and risks misordered or forgotten arguments because all types are plain bools/ints). To keep the boundary clean, pass a single backend settings object (or the configuration reference) so the backend owns interpretation of its policies and future changes remain localized.
Context for Agents
The factory now forwards six backend-specific knobs (`IsCircularFileLogging`, `IsOverwriteLogOnFull`, max size, number of files, truncate policy, plus the file name) as a long positional parameter list. This tightly couples `CreateFileLoggingBackend` to `FileOutputBackend` internals and guarantees that every future backend policy change will require touching this factory (and risks misordered or forgotten arguments because all types are plain bools/ints). To keep the boundary clean, pass a single backend settings object (or the configuration reference) so the backend owns interpretation of its policies and future changes remain localized.
File: score/mw/log/detail/file_recorder/file_recorder_factory.cpp
Line: 87Chain of Thought
- Validation Decision: accepted
- Confidence: 0.72
- Likelihood Score: 3/5
- Likelihood Reason: The long positional parameter list with multiple same-typed arguments (3 bools, 2 ints) is a real maintenance risk. Misordering is plausible during future modifications, though currently the code is likely correct. The maintainability concern will grow with each additional backend policy.
- Suggested Action: keep
- Tool Calls Used: 8
Reasoning
The comment accurately identifies that this PR expands the FileOutputBackend constructor call from 5 to 11 positional parameters, adding 6 new backend-specific knobs (file_name, IsCircularFileLogging, IsOverwriteLogOnFull, MaxLogFileSizeBytes, NoOfLogFiles, IsTruncateOnRotation). This is a real change introduced by this PR.
The concern about misordered arguments is legitimate in C++ — three bool parameters (IsCircularFileLogging, IsOverwriteLogOnFull, IsTruncateOnRotation) and two numeric parameters could be swapped without compiler warnings.
The suggestion is actionable: pass a settings struct or the Configuration reference to FileOutputBackend instead of individual parameters. The factory already has const Configuration& config available.
However, this is primarily a design/maintainability suggestion rather than a bug. The current code will work correctly if parameter ordering matches the constructor. The comment is valid but is a moderate-priority refactoring suggestion rather than a critical issue.
Evidence Summary
1. File Snapshot (BASE)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Lookup
- Filename:
file_output_backend.h
No files found matching 'file_output_backend.h'
3. File Snapshot (HEAD)
- File:
score/mw/log/detail/file_recorder/file_recorder_factory.cpp
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Lookup
- Filename:
file_output_backend.cpp
No files found matching 'file_output_backend.cpp'
5. File Lookup
- Filename:
file_output_backend.hpp
No files found matching 'file_output_backend.hpp'
2 participants
This pull request is related to new feature for managing log file sizes to prevent them from growing indefinitely and consuming excessive disk space. A log file size policy has been implemented to control the maximum size of log files and to handle log rotation.
References
eclipse-score#88
Feature/logfile size control
This pull request introduces a new feature to manage log file sizes, preventing them from growing indefinitely and consuming excessive disk space. A log file size policy has been implemented within the
FileRecorderFactoryto control the maximum size of log files and to handle log rotation, including support for multiple log files and circular logging.Architecture Diagram:
flowchart TD subgraph Configuration_Layer C["Configuration"] end subgraph Service_Layer FRF["FileRecorderFactory (+22/-4 lines)"] FOB["FileOutputBackend"] end subgraph OS_Layer F["Fcntl"] U["Unistd"] end C -->|provides config| FRF FRF -->|creates with config| FOB FOB -->|uses| F FOB -->|uses| UThis summary was automatically generated by @propel-code-bot