Feature: Log File Size Management and Rotation#9
Feature: Log File Size Management and Rotation#9saksham-chawla wants to merge 13 commits intocodex/replay-pr-83-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.
Action required: core log-rotation changes risk leaks, stalled writes, and misconfiguration, with additional build/test hygiene concerns.
Status: Action Required | Risk: High | Readiness: 4/10 (Needs Significant Work)
Issues Identified & Suggestions
- Fix writer leak, rotation failures, append mode, lint scope, noexcept allocations:
non_blocking_writer.* - Correct console defaults, BUILD exposure, and temp test files:
recorder_factory_stub.cpp,BUILD,target_config_reader_test.cpp
Review Details
📁 19 files reviewed | 💬 11 comments
👍 / 👎 individual comments to help improve reviews for you
|
|
||
| score::cpp::pmr::unique_ptr<score::os::Unistd> unistd_; | ||
| score::cpp::pmr::unique_ptr<score::os::Fcntl> fcntl_; | ||
|
|
There was a problem hiding this comment.
[Reliability] Resource leak: NonBlockingWriter acquires file descriptors via fcntl_->open() in the constructor and in RotateLogFile(), but there is no destructor defined to close file_handle_. When a NonBlockingWriter object is destroyed, the open file descriptor leaks.
Add a destructor that closes the handle:
NonBlockingWriter::~NonBlockingWriter() noexcept
{
if (file_handle_ != -1)
{
unistd_->close(file_handle_);
}
}Also declare it in the header.
Context for Agents
**Resource leak**: `NonBlockingWriter` acquires file descriptors via `fcntl_->open()` in the constructor and in `RotateLogFile()`, but there is no destructor defined to close `file_handle_`. When a `NonBlockingWriter` object is destroyed, the open file descriptor leaks.
Add a destructor that closes the handle:
```cpp
NonBlockingWriter::~NonBlockingWriter() noexcept
{
if (file_handle_ != -1)
{
unistd_->close(file_handle_);
}
}
```
Also declare it in the header.
File: score/mw/log/detail/text_recorder/non_blocking_writer.h
Line: 87Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: Every time a NonBlockingWriter object is destroyed with an open file_handle_ (i.e., the normal case), the file descriptor leaks. This is inevitable in production.
- Suggested Action: keep
- Tool Calls Used: 8
Reasoning
The comment is accurate and identifies a real resource leak introduced by this PR.
BASE version: NonBlockingWriter received a file_handle as a constructor parameter (const std::int32_t fileHandle). The class did not own the file descriptor — it was passed in from outside, and the caller was responsible for its lifecycle. No destructor was needed for this purpose.
HEAD version (this PR): The PR fundamentally changes the ownership model:
- A new constructor (lines 50-116 in .cpp) takes a
file_pathand opens the file itself viafcntl_->open(), storing the result infile_handle_. RotateLogFile()(line 366-476 in .cpp) closes old handles and opens new ones, properly managing FDs during rotation.- A second constructor (lines 118-169) still accepts a
file_descriptorfrom outside, butNonBlockingWriterstill becomes the effective owner (it sets flags on it, rotates it, etc.)
The missing destructor: There is NO destructor declared in the header or defined in the .cpp. When a NonBlockingWriter is destroyed (e.g., when its owner SlotDrainer is destroyed, which happens when FileOutputBackend is destroyed), the file_handle_ is never closed.
SlotDrainer::~SlotDrainer() only calls Flush() — it does not close the file descriptor. Since NonBlockingWriter is a member of SlotDrainer, its implicit destructor runs next, but the implicit destructor does nothing with the raw int32_t file_handle_.
While kCloseOnExec is set on the file descriptor, that only closes it on exec() calls, not on normal object destruction.
This is a new issue introduced by this PR — the base version didn't own file descriptors, so no destructor was needed. The PR makes NonBlockingWriter the owner of file descriptors without adding cleanup logic.
The suggested fix is correct and actionable: add a destructor that closes file_handle_ if it's valid (not -1).
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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/text_recorder/non_blocking_writer.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: *
...
3. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
4. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
5. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/slot_drainer.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: *
...
6. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/file_output_backend.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: *
...
7. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/slot_drainer.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: *
...
8. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/file_output_backend.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: *
...
| } | ||
|
|
||
| auto open_flags = | ||
| score::os::Fcntl::Open::kReadWrite | score::os::Fcntl::Open::kCreate | score::os::Fcntl::Open::kCloseOnExec; |
There was a problem hiding this comment.
[Logic] Opening the log file here uses kReadWrite|kCreate but not O_APPEND, and the seek-to-end happens only in the circular logging branch. When circular logging is disabled, restarting the app will write from offset 0 and overwrite existing logs. Add kAppend to open_flags or always lseek(..., SEEK_END) for existing files regardless of circular logging.
Context for Agents
Opening the log file here uses kReadWrite|kCreate but not O_APPEND, and the seek-to-end happens only in the circular logging branch. When circular logging is disabled, restarting the app will write from offset 0 and overwrite existing logs. Add kAppend to open_flags or always lseek(..., SEEK_END) for existing files regardless of circular logging.
File: score/mw/log/detail/text_recorder/non_blocking_writer.cpp
Line: 85Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: Every app restart with circular_file_logging disabled and an existing log file will trigger this bug. The file will be opened at offset 0 with no append/truncate, leading to partial overwrite and a corrupted log file.
- Suggested Action: keep
- Tool Calls Used: 8
Reasoning
The comment correctly identifies a real bug introduced by this PR:
-
New code: The file-opening logic in the first NonBlockingWriter constructor is entirely new in this PR (the base version only accepted a pre-opened file descriptor).
-
Missing kAppend: Line 84-85 sets
open_flags = kReadWrite | kCreate | kCloseOnExec— no kAppend flag. -
Conditional seek: The
lseek(file_handle_, 0, SEEK_END)at lines 107-115 is gated bycircular_file_logging_ && max_log_file_size_bytes_ > 0. When circular logging is disabled, no seek-to-end occurs. -
Non-circular write path: When
!circular_file_logging_, the InternalFlush method (lines 265-277) performs a simplewrite()from the current file position, which starts at 0. -
Resulting bug: If the log file already exists from a previous run, opening it without O_APPEND or O_TRUNC preserves old content, but writes start from offset 0. This overwrites the beginning of the old file while potentially leaving stale data at the end, resulting in a corrupted log file.
The comment provides concrete, actionable fixes: add kAppend to open_flags or always lseek to SEEK_END for existing files regardless of circular logging.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.cpp - Lines:
50-116
50: NonBlockingWriter::NonBlockingWriter(const std::string& file_path,
51: std::size_t max_chunk_size,
52: score::cpp::pmr::unique_ptr<score::os::Unistd> unistd,
53: score::cpp::pmr::unique_ptr<score::os::Fcntl> fcntl,
54: const bool circular_file_logging,
55: const bool overwrite_log_on_full,
56: const std::size_t max_log_file_size_bytes,
57: const std::size_t no_of_log_files,
58: const bool truncate_on_rotation) noexcept
59: : unistd_{std::move(unistd)},
...
2. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/file_output_backend.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 Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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 Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
5. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/slot_drainer.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: *
...
6. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/file_output_backend.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: *
...
7. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/console_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: *
...
| score::os::FcntlImpl::Default(memory_resource), | ||
| score::os::Unistd::Default(memory_resource)); | ||
| score::os::Unistd::Default(memory_resource), | ||
| config.IsCircularFileLogging(), |
There was a problem hiding this comment.
[Logic] This function, which creates a backend for console logging (STDOUT_FILENO), is now reading file-specific rotation settings from the configuration (config.IsCircularFileLogging(), config.GetMaxLogFileSizeBytes()). These settings are not applicable to console output.
This could be misleading or cause unexpected behavior if NonBlockingWriter changes. Please pass disabled/default values for file rotation settings, similar to how it's done in console_recorder_factory.cpp.
| config.IsCircularFileLogging(), | |
| return std::make_unique<FileOutputBackend>(std::move(message_builder), | |
| STDOUT_FILENO, | |
| std::string{}, | |
| std::move(allocator), | |
| score::os::FcntlImpl::Default(memory_resource), | |
| score::os::Unistd::Default(memory_resource), | |
| false, // circular_file_logging | |
| false, // overwrite_log_on_full | |
| 0, // max_log_file_size_bytes | |
| 1, // no_of_log_files | |
| false); // truncate_on_rotation |
Context for Agents
This function, which creates a backend for console logging (`STDOUT_FILENO`), is now reading file-specific rotation settings from the configuration (`config.IsCircularFileLogging()`, `config.GetMaxLogFileSizeBytes()`). These settings are not applicable to console output.
This could be misleading or cause unexpected behavior if `NonBlockingWriter` changes. Please pass disabled/default values for file rotation settings, similar to how it's done in `console_recorder_factory.cpp`.
```suggestion
return std::make_unique<FileOutputBackend>(std::move(message_builder),
STDOUT_FILENO,
std::string{},
std::move(allocator),
score::os::FcntlImpl::Default(memory_resource),
score::os::Unistd::Default(memory_resource),
false, // circular_file_logging
false, // overwrite_log_on_full
0, // max_log_file_size_bytes
1, // no_of_log_files
false); // truncate_on_rotation
```
File: score/mw/log/detail/recorder_factory_stub.cpp
Line: 49Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 4/5
- Likelihood Reason: The inconsistency is concrete and present in the code. If the configuration happens to have non-default values for circular file logging or max log file size, those would be passed to a console backend where they're not applicable. While this may not cause a runtime crash today (the NonBlockingWriter may simply ignore these for STDOUT), it's misleading and could cause unexpected behavior if the implementation changes.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment correctly identifies that recorder_factory_stub.cpp::CreateConsoleLoggingBackend uses config.IsCircularFileLogging() (line 49) and config.GetMaxLogFileSizeBytes() (line 51) when creating a FileOutputBackend for console output (STDOUT_FILENO). These are file-rotation-specific settings that should not be applied to console output.
The evidence is clear:
- The
console_recorder_factory.cpp(same PR) correctly passes disabled/default values (false,false,0,1,false) for the same type of console backend. - Both functions create a
FileOutputBackendwithSTDOUT_FILENO, meaning they're both for console logging. - The inconsistency was introduced by this PR (the old code in BASE didn't have these parameters at all).
The suggestion provides a concrete, correct fix that aligns with how console_recorder_factory.cpp handles the same scenario. The comment is specific, actionable, and addresses a real issue introduced by this PR.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/file_output_backend.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: *
...
2. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/console_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 Snapshot (HEAD)
- File:
score/mw/log/detail/recorder_factory_stub.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: *
...
|
|
||
| if (!overwrite_log_on_full_) | ||
| { | ||
| // CASE: Circular logging, NO overwrite. |
There was a problem hiding this comment.
[Reliability] RotateLogFile() is marked noexcept but constructs multiple std::string objects via concatenation (e.g., file_path_ + "/" + file_name_ + ... and the console_warning strings). If a std::bad_alloc is thrown during any of these allocations, std::terminate() will be called. In a logging context, memory pressure is exactly when this code path may be exercised.
Either:
- Remove
noexceptfromRotateLogFileand let the caller handle the exception, or - Use fixed-size stack buffers (e.g.,
std::array<char, N>) withsnprintffor path construction, avoiding heap allocation entirely.
Context for Agents
`RotateLogFile()` is marked `noexcept` but constructs multiple `std::string` objects via concatenation (e.g., `file_path_ + "/" + file_name_ + ...` and the `console_warning` strings). If a `std::bad_alloc` is thrown during any of these allocations, `std::terminate()` will be called. In a logging context, memory pressure is exactly when this code path may be exercised.
Either:
1. Remove `noexcept` from `RotateLogFile` and let the caller handle the exception, or
2. Use fixed-size stack buffers (e.g., `std::array<char, N>`) with `snprintf` for path construction, avoiding heap allocation entirely.
File: score/mw/log/detail/text_recorder/non_blocking_writer.cpp
Line: 283Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 3/5
- Likelihood Reason: In typical desktop environments, std::bad_alloc is rare due to memory overcommit. However, this appears to be automotive/embedded software (QNX, AUTOSAR), where memory constraints are stricter. The comment's point about memory pressure being exactly when logging rotation is exercised is valid. The likelihood of hitting this in a constrained environment is plausible.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment is technically accurate and well-reasoned:
-
Accuracy:
RotateLogFile()is indeed markednoexcept(line 366) and contains at least 7std::stringheap-allocating operations (concatenations and constructions). If any of these throwstd::bad_alloc,std::terminate()will be called per C++ standard. -
Introduced by this PR:
RotateLogFile()is entirely new code added by this PR. It did not exist in the BASE commit. -
Actionable: The comment provides two concrete alternatives:
- Remove
noexceptfromRotateLogFileand let the caller handle exceptions - Use fixed-size stack buffers with
snprintfto avoid heap allocation
- Remove
-
Valid concern in context: The codebase targets QNX and references AUTOSAR guidelines, suggesting embedded/automotive use where memory constraints are real. The observation that "memory pressure is exactly when this code path may be exercised" is astute — log rotation happens when files get large, which could correlate with high system resource usage.
-
Broader scope: The same issue also affects
InternalFlush()(alsonoexcept) which has similarconsole_warningstring constructions at lines 286 and 334, though the comment specifically targetsRotateLogFile()which has the most instances.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
2. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.cpp - Lines:
366-476
366: void NonBlockingWriter::RotateLogFile() noexcept
367: {
368: if (no_of_log_files_ <= 1)
369: {
370: return;
371: }
372: if (file_handle_ != -1)
373: {
374: unistd_->close(file_handle_);
375: file_handle_ = -1; // Invalidate handle immediately after closing.
...
| if (file_handle_ == -1) | ||
| { | ||
| if (rotation_failed_) | ||
| { | ||
| return 0; // A rotation attempt already failed, do nothing. | ||
| } | ||
| RotateLogFile(); | ||
| if (file_handle_ == -1) | ||
| { | ||
| return 0; // Rotation failed, do nothing. | ||
| } |
There was a problem hiding this comment.
[Reliability] When internalFlush() cannot open/rotate the file (e.g., directory not writable) the new logic sets rotation_failed_ = true and simply returns 0 (lines 203‑214, and similarly at lines 231, 245, 288). FlushIntoFile() interprets that as a successful-but-incomplete write (flush_output.has_value() is true, but number_of_flushed_bytes_ never advances), and therefore keeps returning Result::kWouldBlock. Because the slot drainer treats kPartiallyProcessed as a transient condition, the slot is never released and all subsequent log slots remain blocked forever – effectively stalling the whole logging subsystem without propagating an error.
As a first step, return score::cpp::make_unexpected(...) in these failure paths instead of 0, so the caller sees a real error. Note: TryFlushSlots() currently also doesn't release slots on errors, so a full fix will additionally require the slot drainer to drop (or skip) the stuck slot on a permanent failure, allowing subsequent log entries to proceed.
Context for Agents
When `internalFlush()` cannot open/rotate the file (e.g., directory not writable) the new logic sets `rotation_failed_ = true` and simply returns `0` (lines 203‑214, and similarly at lines 231, 245, 288). `FlushIntoFile()` interprets that as a successful-but-incomplete write (`flush_output.has_value()` is true, but `number_of_flushed_bytes_` never advances), and therefore keeps returning `Result::kWouldBlock`. Because the slot drainer treats `kPartiallyProcessed` as a transient condition, the slot is never released and all subsequent log slots remain blocked forever – effectively stalling the whole logging subsystem without propagating an error.
As a first step, return `score::cpp::make_unexpected(...)` in these failure paths instead of `0`, so the caller sees a real error. Note: `TryFlushSlots()` currently also doesn't release slots on errors, so a full fix will additionally require the slot drainer to drop (or skip) the stuck slot on a permanent failure, allowing subsequent log entries to proceed.
File: score/mw/log/detail/text_recorder/non_blocking_writer.cpp
Line: 213Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: A rotation failure (e.g., unwritable directory, permission issue, disk full) is a realistic production scenario. When it occurs, the logging subsystem stalls permanently because the slot is never released. This will surface for any deployment where log rotation is enabled and a transient or permanent filesystem error occurs.
- Suggested Action: modify
- Tool Calls Used: 5
Reasoning
The comment accurately identifies a real bug introduced by this PR. Here is the detailed trace:
-
New code at lines 203-214: When
file_handle_ == -1androtation_failed_ == true,InternalFlush()returns0— a validexpected<ssize_t>value, not an error. -
FlushIntoFile() (lines 178-199): Sees
flush_output.has_value() == true(since0is a valid value). Sincenumber_of_flushed_bytes_was never incremented,number_of_flushed_bytes_ != buffer_size, sobuffer_flushed_stayskWouldBlock. ReturnskWouldBlock. -
TryFlushSpans() (slot_drainer.cpp:66-86):
status.value() != kDoneis true, so returnskPartiallyProcessed. -
TryFlushSlots() (slot_drainer.cpp:105-132): Returns early without reaching
allocator_->ReleaseSlot()at line 120. The slot is never freed. -
Flush() (line 140-144): Ignores the result via
std::ignore.
On every subsequent Flush() call, the same sequence repeats — InternalFlush returns 0, the slot remains stuck, and no progress is made. Eventually the CircularAllocator runs out of slots, permanently blocking all logging.
This pattern is entirely new, introduced by the rotation_failed_ + return 0 logic in this PR. The same issue exists at multiple return 0 sites in the new code (lines 207, 212, 231, 245, 288, 295).
The proposed fix (returning make_unexpected) is the correct first step. It would cause FlushIntoFile to propagate an error, making the permanent failure visible to the slot drainer. While TryFlushSlots also doesn't release slots on errors currently, propagating an error is semantically correct and enables proper error handling to be added at the caller level.
One minor nuance: the comment's suggestion alone won't fully fix the stall because TryFlushSlots doesn't release slots on error either — but it correctly identifies the root cause and the first necessary fix step.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/slot_drainer.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/text_recorder/non_blocking_writer.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: *
...
3. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/slot_drainer.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: *
...
4. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
5. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.cpp - Lines:
171-200
171: void NonBlockingWriter::SetSpan(const score::cpp::span<const std::uint8_t>& buffer) noexcept
172: {
173: buffer_flushed_ = Result::kWouldBlock;
174: number_of_flushed_bytes_ = 0U;
175: buffer_ = buffer;
176: }
177:
178: score::cpp::expected<NonBlockingWriter::Result, score::mw::log::detail::Error> NonBlockingWriter::FlushIntoFile() noexcept
179: {
180: const auto buffer_size = static_cast<uint64_t>(buffer_.size());
...
| auto open_flags = | ||
| score::os::Fcntl::Open::kReadWrite | score::os::Fcntl::Open::kCreate | score::os::Fcntl::Open::kCloseOnExec; | ||
|
|
||
| // NOLINTBEGIN(score-banned-function): FileLoggingBackend is disabled in production. Argumentation: Ticket-75726 |
There was a problem hiding this comment.
[Reliability] The NOLINTBEGIN(score-banned-function) on line 87 has no corresponding NOLINTEND(score-banned-function). While this is a lint annotation issue, the practical effect is that the score-banned-function check is suppressed for the entire remainder of the file (~390 lines), silently hiding any future banned function usage in InternalFlush, RotateLogFile, and any code added after this point. This is a real suppression scope bug that defeats the purpose of the lint rule.
Context for Agents
[**Bug**] The `NOLINTBEGIN(score-banned-function)` on line 87 has no corresponding `NOLINTEND(score-banned-function)`. While this is a lint annotation issue, the practical effect is that the `score-banned-function` check is **suppressed for the entire remainder of the file** (~390 lines), silently hiding any future banned function usage in `InternalFlush`, `RotateLogFile`, and any code added after this point. This is a real suppression scope bug that defeats the purpose of the lint rule.
File: score/mw/log/detail/text_recorder/non_blocking_writer.cpp
Line: 87Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: The missing NOLINTEND is a factual observation. Every clang-tidy run will silently skip the score-banned-function check for ~395 lines of new code. This is inevitable unless the annotation is fixed.
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment is factually correct and identifies a real bug introduced by this PR:
-
BASE had properly paired NOLINT: In the base version, lines 95 and 99 had matching
NOLINTBEGIN(score-banned-function)/NOLINTEND(score-banned-function)that narrowly scoped the suppression to a singleunistd_->write()call. -
HEAD has unpaired NOLINTBEGIN: In the new code at line 87,
// NOLINTBEGIN(score-banned-function)is introduced with no correspondingNOLINTEND(score-banned-function)anywhere in the file. I verified all 482 lines of the HEAD file. -
Scope of impact: From line 87 to 482 (end of file), all
score-banned-functionviolations will be silently suppressed. This covers theInternalFlush()function (lines 201-364) with numerousunistd_->write(),unistd_->lseek()calls, and the entireRotateLogFile()function (lines 366-476) which hasunistd_->close(),unistd_->access(),fcntl_->open(),fcntl_->fcntl(),unistd_->write(),unistd_->lseek()calls. -
Introduced by this PR: The old code had proper scoping. This PR replaced it with an unmatched NOLINTBEGIN, defeating the purpose of the lint rule for all new code added.
-
Actionable: The fix is straightforward — add
// NOLINTEND(score-banned-function)after thefcntl_->open()call at line 92 (or after the constructor body that the annotation was meant to cover).
Evidence Summary
1. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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/text_recorder/non_blocking_writer.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 Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.cpp - Lines:
85-120
85: score::os::Fcntl::Open::kReadWrite | score::os::Fcntl::Open::kCreate | score::os::Fcntl::Open::kCloseOnExec;
86:
87: // NOLINTBEGIN(score-banned-function): FileLoggingBackend is disabled in production. Argumentation: Ticket-75726
88: const auto descriptor_result = fcntl_->open(
89: initial_file_path.data(),
90: open_flags,
91: score::os::Stat::Mode::kReadUser | score::os::Stat::Mode::kWriteUser | score::os::Stat::Mode::kReadGroup |
92: score::os::Stat::Mode::kReadOthers);
93:
94: if (descriptor_result.has_value())
...
| cc_library( | ||
| name = "os", | ||
| hdrs = glob(["*.h"]), | ||
| visibility = ["//visibility:public"], | ||
| ) | ||
|
|
There was a problem hiding this comment.
[Maintainability] The new cc_library named "os" with hdrs = glob(["*.h"]) is overly broad — it exposes every header in score/os/ with //visibility:public. This bypasses the fine-grained target structure (:fcntl, :unistd, etc.) already in this BUILD file, allowing any target to depend on //score/os and implicitly get all headers without declaring specific deps.
This defeats the purpose of per-header cc_library targets and can mask missing deps in downstream targets. Consider either:
- Listing only the specific headers needed by the new code, or
- Using the existing fine-grained targets (
:fcntl,:unistd) in the downstreamdepsinstead of this catch-all.
Context for Agents
The new `cc_library` named `"os"` with `hdrs = glob(["*.h"])` is overly broad — it exposes every header in `score/os/` with `//visibility:public`. This bypasses the fine-grained target structure (`:fcntl`, `:unistd`, etc.) already in this BUILD file, allowing any target to depend on `//score/os` and implicitly get all headers without declaring specific deps.
This defeats the purpose of per-header `cc_library` targets and can mask missing `deps` in downstream targets. Consider either:
1. Listing only the specific headers needed by the new code, or
2. Using the existing fine-grained targets (`:fcntl`, `:unistd`) in the downstream `deps` instead of this catch-all.
File: score/os/BUILD
Line: 915Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 5/5
- Likelihood Reason: The catch-all "os" target is newly introduced by this PR and is already consumed by a downstream target (non_blocking_writer) instead of using the proper fine-grained
:stattarget. This will mask missing deps and potentially cause linker issues since the catch-all has no implementation deps. - Suggested Action: keep
- Tool Calls Used: 9
Reasoning
The comment correctly identifies a real and significant issue introduced by this PR:
-
Newly introduced: The
cc_library(name = "os", hdrs = glob(["*.h"]))target did not exist in BASE — it is 100% introduced by this PR. -
Already misused: In
score/mw/log/detail/text_recorder/BUILD, thenon_blocking_writertarget adds@score_baselibs//score/osas a dep. Looking atnon_blocking_writer.cpp, it includesscore/os/stat.hbut doesn't declare@score_baselibs//score/os:stat. The catch-all is being used as a shortcut. -
Defeats fine-grained structure: The
score/os/BUILDfile has ~20+ fine-grained targets (:acl,:dirent,:stat,:fcntl,:unistd, etc.), each with propersrcs,deps(:errno,:object_seam, etc.). The catch-all target bypasses all of this. -
Missing implementation deps: The catch-all target has NO
depsand NOsrcs— it's a header-only library that exposes all headers without linking any implementations. The fine-grained targets like:stathave.cppfiles and proper dependency chains. Using the catch-all means you get headers without the implementation, which is fundamentally broken from a build perspective. -
Actionable: The comment provides two concrete solutions: (a) list only specific headers needed, or (b) use existing fine-grained targets. The correct fix here would be to remove the catch-all
"os"target and add@score_baselibs//score/os:stattonon_blocking_writer's deps instead of@score_baselibs//score/os.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/os/BUILD - Lines:
900-930
900: "//visibility:public", # platform_only
901: ],
902: deps = [
903: ":object_seam",
904: ":sigevent_error",
905: "@score_baselibs//score/mw/log:frontend",
906: "@score_baselibs//score/result",
907: ],
908: )
909:
...
2. File Snapshot (HEAD)
- File:
score/os/BUILD - Lines:
1-100
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 Snapshot (BASE)
- File:
score/os/BUILD - Lines:
900-920
900: "//visibility:public", # platform_only
901: ],
902: deps = [
903: ":object_seam",
904: ":sigevent_error",
905: "@score_baselibs//score/mw/log:frontend",
906: "@score_baselibs//score/result",
907: ],
908: )
909:
...
4. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/BUILD
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: #
...
5. File Snapshot (HEAD)
- File:
score/os/BUILD - Lines:
100-300
100: "@score_baselibs//score/bitmanipulation:bitmask_operators",
101: ],
102: )
103:
104: cc_library(
105: name = "getopt",
106: srcs = [
107: "getopt.cpp",
108: "getopt_impl.cpp",
109: ],
...
6. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
7. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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/text_recorder/BUILD
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: #
...
| uint64_t current_file_position_; | ||
| std::size_t no_of_log_files_; | ||
| bool truncate_on_rotation_; | ||
| std::size_t current_log_file_index_{0}; |
There was a problem hiding this comment.
[Maintainability] The in-class initializer for current_log_file_index_ is 0, but both constructors in non_blocking_writer.cpp initialize it to 1. This is confusing and could lead to subtle bugs if a constructor is added that doesn't explicitly initialize it.
Please make the initialization consistent. Either remove the in-class initializer or change it to 1.
Context for Agents
The in-class initializer for `current_log_file_index_` is `0`, but both constructors in `non_blocking_writer.cpp` initialize it to `1`. This is confusing and could lead to subtle bugs if a constructor is added that doesn't explicitly initialize it.
Please make the initialization consistent. Either remove the in-class initializer or change it to `1`.
File: score/mw/log/detail/text_recorder/non_blocking_writer.h
Line: 100Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 4/5
- Likelihood Reason: The inconsistency is confirmed: in-class default is 0, both constructors set it to 1. The in-class default is dead code that could silently become live if a new constructor is added without explicitly initializing this member, leading to an off-by-one-style bug in file rotation logic.
- Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment is factually accurate:
-
In-class initializer: Line 100 of the header declares
std::size_t current_log_file_index_{0};with a default of0. -
Constructor 1 (lines 50-71 of .cpp): The member initializer list explicitly sets
current_log_file_index_{1}(line 71), overriding the in-class default. -
Constructor 2 (lines 118-141 of .cpp): Similarly sets
current_log_file_index_{1}(line 141).
The in-class default of 0 is never used since both constructors override it. Looking at RotateLogFile() (line 366+), the index cycles from 1 to no_of_log_files_ (line 378-382: current_log_file_index_++; if (current_log_file_index_ > no_of_log_files_) { current_log_file_index_ = 1; }). So 1 is the correct starting value, and 0 would be incorrect/unused.
This is all new code introduced by this PR, so the issue is squarely within scope. The suggestion is actionable: change the in-class default to 1 or remove it. This would prevent a future constructor from accidentally starting with index 0, which would be semantically wrong for the 1-based rotation scheme.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
2. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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: *
...
| } | ||
|
|
||
| NonBlockingWriter::NonBlockingWriter(const int32_t fileHandle, | ||
| NonBlockingWriter::NonBlockingWriter(const std::string& file_path, |
There was a problem hiding this comment.
[Maintainability] The two constructors for NonBlockingWriter (one taking a file path, the other a file descriptor) contain a significant amount of duplicated code for parsing the file path, setting file control flags, and determining the initial file size.
To improve maintainability, consider refactoring this common logic into a private init() method that both constructors can call.
Context for Agents
The two constructors for `NonBlockingWriter` (one taking a file path, the other a file descriptor) contain a significant amount of duplicated code for parsing the file path, setting file control flags, and determining the initial file size.
To improve maintainability, consider refactoring this common logic into a private `init()` method that both constructors can call.
File: score/mw/log/detail/text_recorder/non_blocking_writer.cpp
Line: 50Chain of Thought
- Validation Decision: accepted
- Confidence: 0.92
- Likelihood Score: 4/5
- Likelihood Reason: The code duplication is real and newly introduced by this PR. Three distinct blocks of logic (path parsing ~4 lines, fcntl flag setting ~8 lines, lseek file size determination ~9 lines) are duplicated almost identically across the two constructors. This creates a concrete maintenance risk: a bug fix or behavior change in one constructor could easily be missed in the other.
- Suggested Action: keep
- Tool Calls Used: 3
Reasoning
The comment correctly identifies significant code duplication between two newly-introduced constructors in this PR.
Evidence of duplication (all newly introduced, not pre-existing):
-
Path parsing (Constructor 1 lines 73-76, Constructor 2 lines 143-146): Both constructors identically create a
Pathobject and extractfile_path_,file_extension_, andfile_name_. -
fcntl flag setting (Constructor 1 lines 97-104, Constructor 2 lines 151-158): Both get the current file status flags and set
kNonBlocking | kCloseOnExec. The logic is identical. -
lseek file size determination (Constructor 1 lines 107-115, Constructor 2 lines 160-168): Both check
circular_file_logging_ && max_log_file_size_bytes_ > 0 && file_handle_ != -1and uselseek(file_handle_, 0, SEEK_END)to determine the initial file size. Identical code.
Not pre-existing: In BASE, there was only a single simple constructor (lines 45-56) with an empty body. All the duplicated logic is entirely new in this PR.
Actionability: The comment clearly suggests extracting the common logic into a private init() method, which is a concrete, standard C++ refactoring pattern. The member initializer lists can stay in each constructor, while the body logic (path parsing, flag setting, file size determination) can be moved to a shared method.
Minor nuance: The two constructors do have slight differences in the body—Constructor 1 opens the file via fcntl_->open() and Constructor 2 strips a _1 suffix from the filename—but the shared code (~21 lines) significantly outweighs the differences, and a well-designed init() method could accommodate both paths.
Evidence Summary
1. File Snapshot (BASE)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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/text_recorder/non_blocking_writer.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: *
...
3. File Snapshot (HEAD)
- File:
score/mw/log/detail/text_recorder/non_blocking_writer.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 temp_file_path = "new_config.json"; | ||
| std::ofstream temp_file(temp_file_path); | ||
| temp_file << new_config_content; | ||
| temp_file.close(); | ||
|
|
||
| SetConfigurationFiles({temp_file_path}); | ||
|
|
||
| const auto config_result = GetReader().ReadConfig(); | ||
| ASSERT_TRUE(config_result.has_value()); | ||
| const auto& config = config_result.value(); | ||
|
|
There was a problem hiding this comment.
[Testing] In the test ConfigReaderShallParseFileRotationStructure, the temporary file is created with a fixed path "new_config.json" in the current working directory. If this test runs in parallel with itself or other tests that use the same path, it will race on the file. Additionally, std::remove cleanup at the end won't run if an ASSERT_* fails earlier.
Consider using a unique temp file path (e.g., via mktemp or a test-specific directory) and cleaning up in TearDown() to guarantee cleanup.
Context for Agents
In the test `ConfigReaderShallParseFileRotationStructure`, the temporary file is created with a fixed path `"new_config.json"` in the current working directory. If this test runs in parallel with itself or other tests that use the same path, it will race on the file. Additionally, `std::remove` cleanup at the end won't run if an `ASSERT_*` fails earlier.
Consider using a unique temp file path (e.g., via `mktemp` or a test-specific directory) and cleaning up in `TearDown()` to guarantee cleanup.
File: score/mw/log/configuration/target_config_reader_test.cpp
Line: 548Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: The parallel execution race is unlikely in practice (same binary usually runs sequentially), but the cleanup-on-ASSERT-failure issue is plausible - if the config parsing ever regresses, the temp file will be left behind. This is a real code quality issue in newly introduced code.
- Suggested Action: keep
- Tool Calls Used: 4
Reasoning
The comment identifies two real issues in the newly added test ConfigReaderShallParseFileRotationStructure:
-
Fixed file path: The test writes to
"new_config.json"in CWD (line 538). All other tests in this fixture use pre-existing test data files viaget_path()which resolves toscore/mw/log/configuration/test/data/. This is a new anti-pattern introduced by this PR. -
Cleanup not guaranteed: Line 546 uses
ASSERT_TRUE(config_result.has_value())which aborts the test body on failure. Thestd::removeon line 556 would then not execute, leaving the temp file behind. The fixture'sTearDown()on line 78 is empty and doesn't handle this.
Both issues are newly introduced (this test didn't exist in BASE). The suggestions are concrete and actionable: use a unique temp file path and move cleanup to TearDown().
The parallel execution concern is somewhat theoretical (GTest runs tests sequentially by default within a binary), but the cleanup-on-failure concern is a genuine code quality issue that would leave stale files if the assertion fails.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/log/configuration/target_config_reader_test.cpp - Lines:
1-100
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 (BASE)
- File:
score/mw/log/configuration/target_config_reader_test.cpp - Lines:
1-100
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 Snapshot (HEAD)
- File:
score/mw/log/configuration/target_config_reader_test.cpp - Lines:
100-150
100: {
101: return get_path("syntax_error.json");
102: };
103: const std::string kInvalidAppConfigFile()
104: {
105: return get_path("invalid_app_config.json");
106: };
107: const std::string kInvalidConfigFilePath()
108: {
109: return get_path("___nonexistent___.json");
...
2 participants
This pull request introduces a 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.
The following new settings are available in the logging.json configuration file under the logFileSizePolicy object:
When this policy is enabled, the logger will check the current file size before writing a new message. If the new message causes the file to exceed maxFileSizeInBytes, a log rotation is performed
References
eclipse-score/logging#88
Demonstration diagram for the functionality
The implementation also integrates the file-size checks and rotation workflow into the logging backend (notably the NonBlockingWriter) and updates configuration parsing and backend initialization to propagate the new policy settings through the logging stack.
Architecture Diagram:
flowchart TD subgraph Configuration_Layer A["logging.json (+10 lines)"] -- reads config --> B["TargetConfigReader (+89 lines)"] B -- updates settings --> C["LogConfig (+65 lines)"] end subgraph Logging_Core_Layer C -- provides config --> D["RecorderFactory"] D -- creates backend --> E["FileBackend (+21 lines)"] E -- creates drainer --> F["SlotDrainer (+19 lines)"] F -- uses writer --> G["NonBlockingWriter (+279 lines)"] end subgraph OS_Interaction_Layer G -- performs OS calls --> H["OS_Unistd"] G -- performs OS calls --> I["OS_Fcntl"] H -- writes/rotates --> J[("LogFiles")] I -- manages file flags --> J endThis summary was automatically generated by @propel-code-bot