feat(logging): add log rotation and transition default server posture to stdout/stderr - #2864
Conversation
fb6670c to
422c09f
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for tackling the unbounded server log growth. The overall direction makes sense, but I found a few issues that should be addressed before merge.
1. Startup values bypass validation
The new limits are validated when changed through the runtime config API, but values loaded from config.json or supplied through the CLI are used directly during startup.
This is particularly risky because the getters return signed int values, which are then assigned to size_t fields in LogRotationConfig.
Examples:
log_max_file_size_mb = 0disables rotation entirely.- A negative value can become a very large
size_t. - A negative
log_max_filesvalue can result in an extremely large rotation loop. - Values above the documented limits are accepted during startup.
Could we validate the fully merged startup configuration before constructing the logging sink and add CLI range validators for:
log_max_file_size_mb:1..2048log_max_files:0..100
2. Reconfiguration loses the original logging mode
reconfigure_application_logging() always resolves targets using LoggingMode::direct_server.
This means a server initially configured as embedded_tray_server silently changes behavior after any runtime logging update, including a simple log-level change. In particular, it may enable the console target and disable or change the file target even though the tray process was initially configured differently.
Could the active LoggingMode be retained and reused during reconfiguration, or could reconfiguration update the existing targets without resolving them again as a direct server?
3. auto and the documented defaults are inconsistent
The current behavior appears contradictory:
defaults.jsonuses"log_file": "auto".- The configuration reference says the default is
"disabled". - The getting-started documentation says direct runs write a log file unless running under systemd or launchd.
- The implementation treats
"auto"exactly like"disabled"for both direct and tray servers.
For the tray application this is especially problematic: console logging is disabled, and auto also disables file logging, leaving only the in-memory log stream. Persistent logs are therefore lost after a crash or restart.
I suggest defining the modes explicitly, for example:
disabled: never create a fileenabled: always create the default file- custom path: write to that path
auto: console-only for direct server runs, file logging for the embedded tray server
Alternatively, remove auto and consistently use disabled, but then the documentation and unused systemd/launchd detection should be updated accordingly.
4. The size cap is not strictly enforced
Rotation happens before writing the formatted record. A single record larger than the configured limit will therefore create a new active file that already exceeds the limit.
There is also a Windows accounting issue: the stream is opened in text mode, while current_size_ assumes a one-byte newline. CRLF translation can make the real file larger than the tracked size.
Using binary mode would make the byte accounting deterministic. Oversized individual records also need an explicit policy, such as truncation, splitting, or dropping with a warning.
5. The integration test does not exercise normal rotation or retention
test_log_rotation_and_retention_cap() appends more than 1 MB directly to the log file and then triggers logging reconfiguration. This primarily tests rotation of a pre-existing oversized file when a new sink is constructed, rather than normal size-triggered rotation through RotatingFileSink::log().
The test also does not verify the stated retention behavior:
.2is never created or checked.- The absence or removal of
.3is not checked. max_files = 0is not covered.- Invalid startup CLI/config values are not covered.
- Tray-mode reconfiguration is not covered.
- Oversized individual records are not covered.
I also could not find this test being explicitly executed by the CI workflows, and its filename does not match normal pytest discovery.
The feature is valuable, but these issues affect the central guarantees of bounded and predictable logging, so I think they should be resolved before merge.
422c09f to
597748d
Compare
|
Thanks for the feedback! I've addressed all review comments:
|
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. Most of the original points are addressed, but I still see a few issues before approval:
- The advertised size bound is still not strict: a pre-existing oversized log is only renamed to
.1, so e.g. a 10 GB legacy log remains 10 GB after startup. A single oversized log record can also exceed the configured limit. - The integration test still mainly exercises startup rotation of pre-created oversized files, not size-triggered rotation through
RotatingFileSink::log()while the server is running. - CLI limits are validated, but invalid values loaded from
config.jsonare still accepted and effectively clamped later rather than rejected consistently. autobehavior now looks correct, butgetting-started.mdstill says direct runs write a persistent log file by default, which no longer matches the implementation.
CI is green and the mode retention / tray behavior / binary accounting fixes look good. I’d address the remaining size-bound + runtime-rotation test issues before merge.
|
Thanks for the thorough review! All 4 points have been addressed in the latest commit (
|
a85d3a5 to
a08ee9f
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. CI and the test coverage look good now.
I still see two things before approval:
- The PR still claims a strict ~60 MB log footprint, but oversized single records and pre-existing oversized logs can exceed that limit and are kept as oversized backups. Either enforce the bound or relax the wording so it matches the actual behavior.
RuntimeConfignow validates the entire merged config on startup, not just the new logging settings. That can make unrelated existing backend/path config fail at startup. I’d keep the new startup validation scoped to the logging limits unless the broader behavior change is intentional and covered separately.
Other than that, the previous issues look addressed.
a08ee9f to
fead687
Compare
|
Thanks for the follow-up review! Both points have been addressed in the latest update:
|
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. The scoped startup validation looks good now.
I still see a few things before approval:
As raised before, the PR description still claims a strict ~60 MB upper bound, but oversized single records and legacy logs can still exceed that. The wording should match the actual steady-state behavior.
getting-started.md seems to have regressed during the rebase and again says direct runs write a persistent log file by default, which no longer matches auto behavior.
Custom log paths should reject directories explicitly. Right now an existing directory can reach the rotation path and be renamed to .1, which is potentially destructive
|
Thanks for pointing these out! All 3 items are addressed in commit 91698bd:
|
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. The previous three points look addressed now.
I still see a couple of things before approval:
- CLI log overrides are persisted to config.json before RuntimeConfig validation runs. An invalid directory passed via --log-file can therefore fail startup but still leave the persisted config in a broken state. I’d validate first and only save the overrides after validation succeeds.
- Rotation filesystem errors are currently ignored. If moving the active log to .1 fails, the code can still reopen the active file with trunc, which risks losing the existing log instead of preserving it.
- The runtime integration test starts the real server and generates traffic, but it doesn’t actually assert that a rotated .1 file was created. Adding that check would make the end-to-end coverage me
|
Thanks for the thoughtful follow-up review! All three items have been addressed in commit a0fd6a5:
All unit tests, integration tests, and the |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. The previous three points look addressed now.
I still see a couple of things before approval:
- Rotation still treats the backup paths (
.1,.2, etc.) as removable/movable entries without checking that they are regular files. An existing directory at one of those paths could therefore be removed or shifted during rotation. - The new rename-failure test doesn’t actually verify that the rename failed. It can still pass if the rename succeeds, so I’d make that failure case deterministic and assert the expected fallback path.
- The runtime test is much better now, but the pre-seeded log is so close to the limit that startup logging itself may create
.1before the HTTP traffic runs. It would be good to make sure the test proves the live requests triggered the rotation.
Other than that, the earlier issues look resolved and CI is looking good.
|
Thanks for the follow-up review! All three items have been addressed in commit 1aec740:
All unit tests, integration tests, and the |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. The previous points are improved, but I still see a few things before approval:
log_max_filesis not a strict retention cap yet. If higher-numbered backups already exist, for example after reducing the setting from 5 to 2,.3–.5are never removed. The current retention test also doesn’t catch this because.3is never created first.- Config validation narrows JSON integers with
get<int>()before checking the configured bounds. Very large integer values should be range-checked as 64-bit values first, then converted. - A non-regular backup path such as a directory at
.1now stays safe, but it also prevents rotation indefinitely. The sink then keeps appending past the configured size limit, which can reintroduce the unbounded-growth problem this PR is meant to solve. - Related to that, the new rename-failure test covers an existing destination conflict, but
fs::rename()is never actually attempted in that case. It still doesn’t exercise the real rename-error fallback path.
The earlier fixes look good and CI is looking healthy. I’d address these remaining edge cases before approval.
|
@superm1 please provide an additional gating review of this PR. I know you spent a lot of time on the existing logging system. |
|
I'll wait to review until after it's rebased and takes into account current pending feedback. |
1aec740 to
f73735b
Compare
|
Thanks for the detailed follow-up review! All items have been addressed and squashed into a clean commit on the branch: 1. Strict Retention Pruning for Higher-Numbered Backups 2. 64-Bit Integer Range Validation 3. Backup Slot Fallback for Blocked Destinations 4. Deterministic Filesystem Rename Error Verification 5. Clean Ephemeral CLI Overrides & Rebase |
f73735b to
ad79955
Compare
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks for the updates. Most of the previous issues look resolved now, but I still see two blockers before approval:
- If all backup slots are blocked by non-regular entries, rotation still falls back to appending to the oversized active log indefinitely. That reintroduces the unbounded-growth case this PR is meant to prevent. The current
test_all_backup_slots_blocked_fallbackactually codifies this behavior, so I’d define a bounded fallback policy instead. - Logging CLI overrides are only used for the initial setup. A later runtime logging change, even just
log_level, rebuilds the rotation config from persistentRuntimeConfigvalues and can silently drop--log-file,--log-max-size-mb, and--log-max-files. CLI precedence should survive reconfiguration.
One smaller coverage issue: the real rename-error test is skipped when running as root, which is how the current Linux C++ CI runs, so that failure path is not actually exercised there.
The retention pruning and 64-bit validation fixes look good now, and the relevant CI checks are green.
ad79955 to
19a1298
Compare
|
Thanks for the follow-up review! All 3 items have been resolved in commit 19a1298: 1. Bounded Fallback Policy when Backup Slots are Blocked 2. CLI Precedence Survives Runtime Reconfiguration 3. Deterministic OS Rename Error Test All 35 C++ unit tests in |
fl0rianr
left a comment
There was a problem hiding this comment.
Thanks, the previous blockers look resolved now. I'm still not fully happy with it, only two points remaining:
- Backup pruning currently treats any regular file named . as a Lemonade backup and deletes it when the suffix is above log_max_files. That can remove unrelated files such as date-based backups. I’d only manage the suffix range Lemonade can actually create, e.g. .1 through .100.
- LemonadeServer.exe uses the shared CLI parser, so it accepts --log-file, --log-max-size-mb, and --log-max-files, but the tray startup path never applies those values as RuntimeConfig overrides. The flags are therefore silently ignored for the tray app.
19a1298 to
02e6933
Compare
|
Thanks for the follow-up review! Both items have been resolved in commit 02e6933: 1. Bounded Backup Pruning Range ( 2. Tray App CLI Logging Overrides |
… to stdout/stderr Introduce thread-safe log rotation via RotatingFileSink and transition direct lemond executions to console-first logging with optional rotating file logging. - Implement RotatingFileSink with size-triggered rotation, max_files backup retention, excess backup pruning, backup slot fallback, and in-place truncation for max_files=0. - Add configuration settings log_max_file_size_mb (1..2048) and log_max_files (0..100) with 64-bit validation across CLI, RuntimeConfig, and config.json. - Support --log-file, --log-max-size-mb, and --log-max-files CLI overrides as in-memory runtime parameters without mutating persistent config. - Add comprehensive C++ unit test suite (test_log_rotation) and Python integration suite (test/server_log_rotation.py).
02e6933 to
fb74ea2
Compare
Previously, Lemonade Server (
lemond) unconditionally appended to a single, un-rotated log file in runtime directories (e.g./run/user/<uid>/mounted on RAMtmpfson systemd Linux distributions). Under heavy or long-running workloads, this file grew unbounded, risking host RAM and disk space exhaustion.To resolve this,
lemondnow defaults to standard output streams (stdout/stderr), allowing OS service managers (systemd,launchd), container runtimes (Docker/Kubernetes), and CI runners to capture and rotate logs natively. When file logging is explicitly enabled (--log-file enabledor custom target path), a thread-safeRotatingFileSinkcaps active file size and manages historic log backup retention.Summary
log_file = "auto"): Out-of-the-box directlemondserver runs stream tostdout/stderr, creating zero silent log files on disk or RAMtmpfs, while embedded tray runs retain file logging (lemonade-server.log).RotatingFileSink: Explicitly enabling file logging caps active log size at 10 MB and retains up to 5 historic backups (lemonade-server.log.1...5), bounding steady-state log disk usage to ~60 MB under normal record sizes..3–.10iflog_max_filesis lowered to 2, or all backups iflog_max_files = 0). If slot.1is occupied by a directory, the sink safely falls back to subsequent available slots (.2...N). If all backup slots are blocked by non-regular entries, the sink applies a bounded fallback policy by truncatingactive_login-place on the rotation boundary to strictly prevent unbounded file growth.--log-file,--log-max-size-mb(1..2048), and--log-max-files(0..100) options tolemondCLI andconfig.jsonwith 64-bit integer range validation. CLI options act as in-memory runtime overrides whose precedence survives runtime reconfiguration (e.g.log_levelchanges) without mutating persistentconfig.jsonon disk..github/actions/capture-server-logs/action.ymlwildcard patterns (lemonade-server.log*) to archive active and rotated log backups across Windows, Linux, and macOS.test/cpp/test_log_rotation.cpp(35 C++ assertions) andtest/server_log_rotation.py(6 integration tests) verifying runtime rotation, retention capping, legacy pruning, slot fallback, OS rename error recovery, bounded fallback, and limit/directory rejection.Scope
Testing
Testing details:
./build/test_log_rotation(35/35 passed)../build/test_cli_runtime_override(31/31 passed).python3 test/server_log_rotation.py(6/6 passed in 19.3s).python3 docs/tools/gen_backend_boilerplate.py --check(0 drift).Documentation
docs/dev/getting-started.mdanddocs/guide/configuration/README.md).Breaking Changes
AI-assisted contribution