Skip to content

Feature/logfile size control #5

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

Closed
saksham-chawla wants to merge 2 commits into from
+22 −4
Closed

Feature/logfile size control #5

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e209909
Adding changes related to logfile size feature
atarekra Jan 26, 2026
f9f4719
Merge branch 'main' into feature/logfile-size-control
atarekra Jan 27, 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
26 changes: 22 additions & 4 deletions score/mw/log/detail/file_recorder/file_recorder_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,25 @@ std::unique_ptr<Backend> FileRecorderFactory::CreateFileLoggingBackend(
const Configuration& config,
score::cpp::pmr::memory_resource* memory_resource) noexcept
{
const std::string file_name{std::string(config.GetLogFilePath().data(), config.GetLogFilePath().size()) + "/" +
std::string{config.GetAppId().data(), config.GetAppId().size()} + ".dlt"};
const std::string file_path_base{std::string(config.GetLogFilePath().data(), config.GetLogFilePath().size())};
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) {
Copy link
Copy Markdown

@propel-code-bot propel-code-bot Bot Mar 18, 2026

Choose a reason for hiding this comment

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

Recommended

[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: 45
Chain 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'

Fix in Cursor

file_name = file_path_base + "/" + app_id + "_1" + file_extension;
}

auto open_flags = score::os::Fcntl::Open::kReadWrite | score::os::Fcntl::Open::kCreate | score::os::Fcntl::Open::kCloseOnExec;
if (!config.IsCircularFileLogging())
{
open_flags |= score::os::Fcntl::Open::kAppend;
}


// NOLINTBEGIN(score-banned-function): FileLoggingBackend is disabled in production. Argumentation: Ticket-75726
const auto descriptor_result = fcntl_->open(
file_name.data(),
score::os::Fcntl::Open::kWriteOnly | score::os::Fcntl::Open::kCreate | score::os::Fcntl::Open::kCloseOnExec,
open_flags,
score::os::Stat::Mode::kReadUser | score::os::Stat::Mode::kWriteUser | score::os::Stat::Mode::kReadGroup |
score::os::Stat::Mode::kReadOthers);
// NOLINTEND(score-banned-function): see above for detailed explanation
Expand Down Expand Up @@ -72,9 +84,15 @@ std::unique_ptr<Backend> FileRecorderFactory::CreateFileLoggingBackend(

return std::make_unique<FileOutputBackend>(std::move(message_builder),
descriptor,
file_name,
Copy link
Copy Markdown

@propel-code-bot propel-code-bot Bot Mar 18, 2026

Choose a reason for hiding this comment

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

Recommended

[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: 87
Chain 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&amp; 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'

Fix in Cursor

std::move(allocator),
score::os::Fcntl::Default(memory_resource),
score::os::Unistd::Default(memory_resource));
score::os::Unistd::Default(memory_resource),
config.IsCircularFileLogging(),
config.IsOverwriteLogOnFull(),
config.GetMaxLogFileSizeBytes(),
config.GetNoOfLogFiles(),
Copy link
Copy Markdown

@propel-code-bot propel-code-bot Bot Mar 18, 2026

Choose a reason for hiding this comment

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

Important

[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: 94
Chain 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 &gt; 1 guard 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:

  1. New code introduced by this PR: The base version didn't pass GetNoOfLogFiles() to FileOutputBackend at all — the entire rotation/log-file-count feature is new. So this issue is squarely introduced by this PR.

  2. The logical inconsistency is real: The code checks config.GetNoOfLogFiles() &gt; 1 to 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) to FileOutputBackend at line 94. This creates an asymmetry: the filename logic treats 0 as "single file mode" but the backend receives the literal 0.

  3. Cannot verify downstream impact: The FileOutputBackend class (referenced in score/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.

  4. The comment is actionable: It provides a concrete fix — use std::max to 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.

  5. 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:  *
...

Fix in Cursor

config.IsTruncateOnRotation());
}

} // namespace detail
Expand Down